1221839 - UBSan: left shift of 128 by 24 places cannot be represented in type 'int' pkcs11.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Tyson Smith [:tsmith]]

This was found by building NSS with UBSan and running the test suite.

pkcs11.c:3735:40: runtime error: left shift of 128 by 24 places cannot be represented in type 'int'
    #0 0x7f5a2f4686f8 in NSC_OpenSession /home/user/code/san_nss/nss/lib/softoken/pkcs11.c:3735:40
    #1 0x7f5a2f41b14a in FC_OpenSession /home/user/code/san_nss/nss/lib/softoken/fipstokn.c:632:12
    #2 0x7f5a34a88154 in PK11_InitToken /home/user/code/san_nss/nss/lib/pk11wrap/pk11slot.c:1175:8
    #3 0x7f5a34a8db6b in PK11_InitSlot /home/user/code/san_nss/nss/lib/pk11wrap/pk11slot.c:1390:7
    #4 0x7f5a349af7c9 in secmod_LoadPKCS11Module /home/user/code/san_nss/nss/lib/pk11wrap/pk11load.c:537:6
    #5 0x7f5a34a16512 in SECMOD_LoadModule /home/user/code/san_nss/nss/lib/pk11wrap/pk11pars.c:1027:10
    #6 0x7f5a34a16a0c in SECMOD_LoadModule /home/user/code/san_nss/nss/lib/pk11wrap/pk11pars.c:1062:11
    #7 0x7f5a34863454 in nss_InitModules /home/user/code/san_nss/nss/lib/nss/nssinit.c:435:25
    #8 0x7f5a3485e403 in nss_Init /home/user/code/san_nss/nss/lib/nss/nssinit.c:638:7
    #9 0x7f5a3485f0b8 in NSS_Initialize /home/user/code/san_nss/nss/lib/nss/nssinit.c:812:12
    #10 0x507b1e in certutil_main /home/user/code/san_nss/nss/cmd/certutil/certutil.c:2942:18
    #11 0x4fb75f in main /home/user/code/san_nss/nss/cmd/certutil/certutil.c:3634:14
    #12 0x7f5a32ffaec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #13 0x41dc55 in _start (/home/user/code/san_nss/nss/cmd/certutil/Linux3.16_x86_64_clang-3.7_glibc_PTH_64_DBG.OBJ/certutil+0x41dc55)

SUMMARY: AddressSanitizer: undefined-behavior pkcs11.c:3735:40 in

Comment 1

[Franziskus Kiefer [:franziskus]]

This is probably not a problem on most systems (where int is big enough), but can produce an overflow on others. In this case however, we get 0 as result of the shift and thus don't use the slot->index in the sessionID calculation, which doesn't look like a problem to me.

> do {
>      sessionID = (PR_ATOMIC_INCREMENT(&slot->sessionIDCount) & 0xffffff)
>                   | (slot->index << 24);
> } while (sessionID == CK_INVALID_HANDLE);

Comment 2

[Tyson Smith [:tsmith]]

The command leading to the failure was:
modutil -dbdir /home/user/code/san_nss/tests_results/security/workvm.7/fips -fips true

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: check shift value against int size

we should probably check this to avoid the overflow. I'm however still not sure if we actually need this.

Comment 4

[Robert Relyea]

Comment on attachment 8684257 [details] [diff] [review]
check shift value against int size

Review of attachment 8684257 [details] [diff] [review]:
-----------------------------------------------------------------

Actually we can just cast the slot->index to a CK_SESSION_HANDLE before we shift it.. We do need to encode this properly because softoken will pull the index out of the CK_SESSION_HANDLE on subsequent calls. In PKCS #11 session implies slot, so the slot info is not repeated.


((CK_SESSION_HANDLE)slot->index) << 24

CK_SESSION_HANDLE IS at least 32 bits.

bob

::: lib/softoken/pkcs11.c
@@ +3736,5 @@
>          do {
> +            sid = 0;
> +            if (intSize > 24) {
> +                sid = slot->index << 24;
> +            }

We should change slot->index to a PRUint32 instead. The code pretty much assumes it's at least a 32 bit value

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: making slot->index at least 32bit

> We should change slot->index to a PRUint32 instead. The code pretty much assumes it's at least a 32 bit value

ok, so how about this: using CK_ULONG for slot->index instead, then pkcs11.c:237

> slot->index = (nscSlotCount[index] & 0x7f) | ((index << 7) & 0x80);

should be fine as well (nscSlotCount is CK_ULONG and could overflow slot->index if CK_ULONG is more than 32 bit). I also added some unsigned to other outputs that get fed into slot->index.

Comment 6

[Robert Relyea]

Comment on attachment 8692925 [details] [diff] [review]
making slot->index at least 32bit

Review of attachment 8692925 [details] [diff] [review]:
-----------------------------------------------------------------

PRUInt32 would have been marginally preferable in the slot case, but switching that is borderline bikesheading, so the existing patch is fine. The only downside of CK_ULONG is that it can be 64 bits sometimes, but that's not a big issue here.

Comment 7

[Robert Relyea]

adding checkin needed since I don't know if Franziskus has NSS checkin privilege yet or now.

Comment 8

[Franziskus Kiefer [:franziskus]]

yes I do, thanks for the review.

https://hg.mozilla.org/projects/nss/rev/c746e2c15216