1041328 - Crash in CryptoKey::PrivateKeyFromPkcs8() when trying to import invalid key data

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

When trying to import a key with a valid header but for example without a public value (for DH), PK11_ImportPrivateKeyInfoAndReturnKey() fails without calling PR_SetError(). This leads to a MOZ_CRASH() in GetXPCOMFromNSSError() due to using MapSECStatus().

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

The right thing would of course be to fix NSS but I wonder whether we should in the meantime just check whether |(rv == SECFailure)| in CryptoKey::PrivateKeyFromPkcs8()? Being able to crash Firefox so easily is surely nothing we'd want even though it's "just" DOS and not exploitable.

Comment 2

[Richard Barnes [:rbarnes]]

We should do both.  Let's use this bug for the PrivateKeyFromPkcs8 change (as a short-term fix).  Please open another bug for the NSS fix.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1041328-Fix-crash-in-CryptoKey-PrivateKeyFromPkc.patch

Comment 4

[Richard Barnes [:rbarnes]]

Comment on attachment 8462524 [details] [diff] [review]
0001-Bug-1041328-Fix-crash-in-CryptoKey-PrivateKeyFromPkc.patch

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

r=me with these addressed

::: dom/crypto/CryptoKey.cpp
@@ +304,2 @@
>  
> +  if (rv == SECFailure) {

Does MapSECStatus really not map SECFailure to an nsresult such that NS_FAILED() == true?

::: dom/crypto/test/test-vectors.js
@@ +435,5 @@
>        "c635518c7dac47e9"
>      )
>    },
> +
> +  broken: {

It would be nice for this to have a clearer name, like broken_pkcs8.

::: dom/crypto/test/tests.js
@@ +1814,5 @@
> +    var that = this;
> +    var alg = {name: "RSA-OAEP", hash: "SHA-1"};
> +
> +    crypto.subtle.importKey("pkcs8", tv.broken.pkcs8, alg, false, ["decrypt"])
> +      .then(error(that), complete(that));

It seems like you're conflating multiple failure reasons here, by providing both an invalid PKCS#8 structure and a bad key/algorithm combination.  Please either change the test vector to be an RSA key, or change the algorithm here to be DH.

Comment 5

[Richard Barnes [:rbarnes]]

(In reply to Richard Barnes [:rbarnes] from comment #4)
> Comment on attachment 8462524 [details] [diff] [review]
> 0001-Bug-1041328-Fix-crash-in-CryptoKey-PrivateKeyFromPkc.patch
> 
> Review of attachment 8462524 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with these addressed
> 
> ::: dom/crypto/CryptoKey.cpp
> @@ +304,2 @@
> >  
> > +  if (rv == SECFailure) {
> 
> Does MapSECStatus really not map SECFailure to an nsresult such that
> NS_FAILED() == true?

Oh, never mind.  Seeing Comment 1 jogged my memory on this.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Richard Barnes [:rbarnes] from comment #4)
> > +
> > +  broken: {
> 
> It would be nice for this to have a clearer name, like broken_pkcs8.

Good point.

> > +    var alg = {name: "RSA-OAEP", hash: "SHA-1"};
> > +
> > +    crypto.subtle.importKey("pkcs8", tv.broken.pkcs8, alg, false, ["decrypt"])
> > +      .then(error(that), complete(that));
> 
> It seems like you're conflating multiple failure reasons here, by providing
> both an invalid PKCS#8 structure and a bad key/algorithm combination. 
> Please either change the test vector to be an RSA key, or change the
> algorithm here to be DH.

Yeah, the problem is that we don't support DH at the moment so we wouldn't crash with "DH" as the algorithm because we don't get that far.

PK11_ImportPrivateKeyInfoAndReturnKey() OTOH only fails for DH as easily. I'll try to create an RSA key with invalid ASN.1 encoding for the private key, that should work.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> PK11_ImportPrivateKeyInfoAndReturnKey() OTOH only fails for DH as easily.
> I'll try to create an RSA key with invalid ASN.1 encoding for the private
> key, that should work.

Argh, no that doesn't work because SEC_ASN1Decode() correctly calls PORT_SetError(). I will leave it as is for now and leave a comment in the DH bug to s/RSA-OAEP/DH in the crash test.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2c987637616

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2c987637616