1211543 - UBSan: null pointer passed as argument in SECKEY_DestroyPrivateKeyInfo

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Tyson Smith [:tsmith] (PTO)]

Attachment: test_case.der

seckey.c:1567:18: runtime error: null pointer passed as argument 1, which is declared to never be null

/usr/include/string.h:66:62: note: nonnull attribute specified here
    #0 0x7f827a29b98b in SECKEY_DestroyPrivateKeyInfo /home/user/code/fuzz_nss/nss/lib/cryptohi/seckey.c:1567:6
    #1 0x7f827a3546ea in PK11_ImportDERPrivateKeyInfoAndReturnKey /home/user/code/fuzz_nss/nss/lib/pk11wrap/pk11pk12.c:249:5
    #2 0x4e112c in fuzz /home/user/code/fuzz_nss/nss/cmd/checkcert/checkcert.c:22:9
    #3 0x4e147c in main /home/user/code/fuzz_nss/nss/cmd/checkcert/checkcert.c:60:5
    #4 0x7f8278f1dec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #5 0x41b8f5 in _start (/home/user/Desktop/prv/checkcert+0x41b8f5)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Reduced the test case a little:

30 0A [SEQUENCE(10)]
  02 01 30 [INT(len=1) = 48]
  30 05 [SEQUENCE(5)]
    06 01 30 [OID(len=1) = 1.8]
    04 00 [OCTET STRING(len=0)]

Basically we have a valid SECKEY_PrivateKeyInfoTemplate, but with an invalid OID that leads to us bailing out here:

http://hg.mozilla.org/mozilla-central/annotate/d1c5a7c5b433/security/nss/lib/pk11wrap/pk11pk12.c#l482

That's before |pki->privateKey| was set, so |pki->privateKey.data| is still NULL.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Correction:

This isn't related to the invalid OID. All we need is a valid SECKEY_PrivateKeyInfoTemplate construct with an empty OCTET STRING. An OCTET STRING is required but we do accept one with length zero too. This is how we end up with |pki->privateKey.data == NULL|.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

After trying to parse the PrivateKeyInfoTemplate, should we check whether |pki->privateKey.data == NULL| and bail out, freeing the arena? Or should the ASN.1 parser reject input with zero-length, required OCTET STRINGs?

(Also, I believe this isn't security-sensitive.)

Martin, Eric, thoughts?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #3)
> After trying to parse the PrivateKeyInfoTemplate, should we check whether
> |pki->privateKey.data == NULL| and bail out, freeing the arena? Or should
> the ASN.1 parser reject input with zero-length, required OCTET STRINGs?

Looking at this again after a few months, I think that a zero-length octet string is indeed valid ASN.1 and the template parses correctly as the item is given. It's just nothing. So I think the right thing to do is to check whether |pki->privateKey.data == NULL|.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1211543-Don-t-call-SECKEY_DestroyPrivateKeyInfo-.patch

Comment 6

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8744256 [details] [diff] [review]
0001-Bug-1211543-Don-t-call-SECKEY_DestroyPrivateKeyInfo-.patch

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

lgtm

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/9a0b1bf3bd49