1203200 - UBSan: Multiple 'index out of bounds for type' errors in pk11db.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Tyson Smith [:tsmith]]

These were found by building NSS with UBSan and running the test suite.

pk11db.c:198:5: runtime error: index 30 out of bounds for type 'unsigned char [6]'
pk11db.c:198:5: runtime error: index 30 out of bounds for type 'unsigned char [6]'
pk11db.c:203:5: runtime error: index 32 out of bounds for type 'unsigned char [6]'
pk11db.c:203:5: runtime error: index 32 out of bounds for type 'unsigned char [6]'
pk11db.c:205:28: runtime error: index 34 out of bounds for type 'unsigned char [6]'

pk11db.c:198:5: runtime error: index 30 out of bounds for type 'unsigned char [6]'
pk11db.c:198:5: runtime error: index 30 out of bounds for type 'unsigned char [6]'
pk11db.c:203:5: runtime error: index 32 out of bounds for type 'unsigned char [6]'
pk11db.c:203:5: runtime error: index 32 out of bounds for type 'unsigned char [6]'
pk11db.c:205:28: runtime error: index 34 out of bounds for type 'unsigned char [6]'
pk11db.c:198:5: runtime error: index 30 out of bounds for type 'unsigned char [6]'
pk11db.c:198:5: runtime error: index 30 out of bounds for type 'unsigned char [6]'
pk11db.c:203:5: runtime error: index 32 out of bounds for type 'unsigned char [6]'
pk11db.c:203:5: runtime error: index 32 out of bounds for type 'unsigned char [6]'
pk11db.c:205:28: runtime error: index 34 out of bounds for type 'unsigned char [6]'

pk11db.c:198:5: runtime error: index 11 out of bounds for type 'unsigned char [6]'
pk11db.c:198:5: runtime error: index 11 out of bounds for type 'unsigned char [6]'
pk11db.c:200:28: runtime error: index 13 out of bounds for type 'unsigned char [6]'
pk11db.c:203:5: runtime error: index 102 out of bounds for type 'unsigned char [6]'
pk11db.c:203:5: runtime error: index 102 out of bounds for type 'unsigned char [6]'

Comment 1

[Tyson Smith [:tsmith]]

pk11db.c:203:5: runtime error: index 32 out of bounds for type 'unsigned char [6]'
    #0 0x7f353f22df9c in lgdb_EncodeData /home/user/code/san_nss/nss/lib/softoken/legacydb/pk11db.c:203:5
    #1 0x7f353f228cc4 in legacy_AddSecmodDB /home/user/code/san_nss/nss/lib/softoken/legacydb/pk11db.c:700:10
    #2 0x7f353f222462 in legacy_ReadSecmodDB /home/user/code/san_nss/nss/lib/softoken/legacydb/pk11db.c:618:2
    #3 0x7f354012491d in sftkdbCall_ReadSecmodDB /home/user/code/san_nss/nss/lib/softoken/lgglue.c:349:12
    #4 0x7f354015bc7c in NSC_ModuleDBFunc /home/user/code/san_nss/nss/lib/softoken/pkcs11.c:2801:10
    #5 0x7f35457117d9 in SECMOD_GetModuleSpecList /home/user/code/san_nss/nss/lib/pk11wrap/pk11pars.c:928:9
    #6 0x7f35457166e3 in SECMOD_LoadModule /home/user/code/san_nss/nss/lib/pk11wrap/pk11pars.c:1045:19
    #7 0x7f3545563454 in nss_InitModules /home/user/code/san_nss/nss/lib/nss/nssinit.c:435:25
    #8 0x7f354555e403 in nss_Init /home/user/code/san_nss/nss/lib/nss/nssinit.c:638:7
    #9 0x7f354555f0b8 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 0x7f3543cfaec4 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 pk11db.c:203:5 in

Comment 2

[Franziskus Kiefer [:franziskus]]

Tyson, do you have the certificate that causes this so I can run certutil directly with it and don't have to run the entire test suite?

Comment 3

[Tyson Smith [:tsmith]]

The command leading to the failure was:
certutil -N -d /home/user/code/san_nss/tests_results/security/workvm.7/CA -f ../tests.pw

Comment 4

[Franziskus Kiefer [:franziskus]]

It's a bit difficult to reproduce this as I don't usually use this part of the code unless I manually force NSS to do so. The Problem stems from the piece of code below (that I don't really understand). We end up with offset=32 that doesn't exist in encoded->names, which is a char* of length 6. But the code actually breaks earlier already.

>    offset = 0;
>    LGDB_PUTSHORT(encoded->names,len);
>    offset += sizeof(unsigned short);
>    PORT_Memcpy(&encoded->names[offset],commonName,len);
>    offset += len;

First we add the length (len) of commonName and commonName itself to encoded->names. This breaks already since commonName is (in tests) "NSS Internal PKCS #11 Module", which is memcopied into encoded->names[2] and thus overflows it.

>    LGDB_PUTSHORT(&encoded->names[offset],len2);
>    offset += sizeof(unsigned short);
>    if (len2) PORT_Memcpy(&encoded->names[offset],dllName,len2);
>    offset += len2;

>    LGDB_PUTSHORT(&encoded->names[offset],len3);
>    offset += sizeof(unsigned short);
>    if (len3) PORT_Memcpy(&encoded->names[offset],param,len3);
>    offset += len3;

Bob: maybe you could help out here as you wrote parts of the code and I'm not sure if there's a simple fix for this. My only idea would be to change the definition of lgdbData and make the names field work, but maybe I just don't get what's supposed to happen here.

Comment 5

[Franziskus Kiefer [:franziskus]]

so I had another look at this and think I know now what's going on here.

The 3 strings commonName, dllName, and param are written into encoded (a lgdbData struct) into the names field, which is size 6. In [1] space for all three strings and their lengths is allocated beginning at encoded->names. From [2] onwards encoded->names is filled with the three strings and their lengths, which overruns 6 (the size defined in lgdbData), but we're still fine because we allocated the space in[1] to handle this.

So while this seems to work ok, there are multiple problems here:
* lgdbData->names shouldn't be misused in this way, having pointers to strings in lgdbData would make more sense
* all lgdbData fields are defined as unsigned char but is mostly filled with unsigned short, which is also used to compute the offset. This only works if unsigned short and unsigned char would have the same size.

I don't think this is actually a security risk, but the code really needs to be rewritten.

[1] https://dxr.mozilla.org/mozilla-central/rev/5b2baa5e9356644a7ed0b73e422eaff62e159ffb/security/nss/lib/softoken/legacydb/pk11db.c#142
[2] https://dxr.mozilla.org/mozilla-central/rev/5b2baa5e9356644a7ed0b73e422eaff62e159ffb/security/nss/lib/softoken/legacydb/pk11db.c#191

Comment 6

[Andrew McCreight [:mccr8]]

Changing to sec-other per the last comment.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1203200-Fix-misleading-OOB-error-in-lgdb_EncodeD.patch

With a UBSan build we're hitting this a lot just when running ssl_gtests. As Franziskus said the method deserves being rewritten so that it doesn't just overrun the struct but for now it seems safer to just fix the warnings if the code is correct.

Comment 8

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8796922 [details] [diff] [review]
0001-Bug-1203200-Fix-misleading-OOB-error-in-lgdb_EncodeD.patch

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

That's all to make ubsan happy?

::: lib/softoken/legacydb/pk11db.c
@@ +202,1 @@
>      if (len2)

if you're at it, please at braces

@@ +208,1 @@
>      if (len3)

same here

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #8)
> That's all to make ubsan happy?

Yup :)

> ::: lib/softoken/legacydb/pk11db.c
> @@ +202,1 @@
> >      if (len2)
> 
> if you're at it, please at braces

Good idea, will do.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/36324d0f1ad7