935618 - (CVE-2014-1498) nsConvertToActualKeyGenParams uses the union in a SECKEYPublicKey without checking its type

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

562           char *certstr = PL_strndup(value, value_len);
563           if (certstr) {
564             keyPairInfo->ecPopCert = CERT_ConvertAndDecodeCertificate(certstr);
565             PL_strfree(certstr);
566 
567             if (keyPairInfo->ecPopCert)
568             {
569               keyPairInfo->ecPopPubKey = CERT_ExtractPublicKey(keyPairInfo->ecPopCert);
570             }
571           }
572         }
573       }
574     }
575 
576     // first try to use the params of the provided CA cert
577     if (keyPairInfo->ecPopPubKey)
578     {
579       returnParams = SECITEM_DupItem(&keyPairInfo->ecPopPubKey->u.ec.DEREncodedParams);
580     }

nsConvertToActualKeyGenParams decodes a base-64 encoded certificate, extracts its public key, and then assumes it's an elliptic curve key without actually checking. I'm almost certain this is exploitable (dereferencing attacker-controlled memory, etc.). It crashes Nightly but doesn't seem to crash my local debug build.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: fix

This checks that we actually have an ec certificate before treating it like one.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: test

This tests with an rsa certificate. I got the test to fail on an asan build without the fix patch, whereas it passed with the fix patch.

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 828347 [details] [diff] [review]
test

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

Good, r+ with nit addressed.

::: security/manager/ssl/tests/mochitest/bugs/test_generateCRMFRequest.html
@@ +56,5 @@
>        // which gets interpreted as an empty string.
>        is(e.toString(), "Error: error:invalid key generation argument:", "expected exception");
>      }
>  
> +    try {

Add a comment indicating that you are using a rsa cert as an ec to ensure correct handling of incorrect cert type.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: test v2

Thanks, Camilo. Carrying over r+.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 828346 [details] [diff] [review]
fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Fairly easily, I think. I'm not certain it's exploitable, but it really looks like it.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The fix itself paints a bulls-eye on the problem.

Which older supported branches are affected by this flaw?

Anything with window.crypto.generateCRMFRequest (which might be disabled on b2g?)

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch should apply to any branch.

How likely is this patch to cause regressions; how much testing does it need?

There's almost no way this could cause regressions.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

This needs a security rating before approval. Can you suggest one?

I assume this affects ESR24, 25, and more current.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Al Billings [:abillings] from comment #6)
> This needs a security rating before approval. Can you suggest one?

I'm having a hard time rating this. It's at least a DOS, because any malicious page can crash the browser. I initially thought this was an arbitrary memory write, but that doesn't appear to be the case. The bug basically interprets attacker-controlled memory as a {pointer, length} tuple, allocates memory of that length and copies memory from the pointer to the new memory. This copied data then later gets reinterpreted as parameters for generating a key, but I'm not familiar enough with that code to know if it is too trusting with the data it receives.
So, this could be just a crash, or it could be a vector to exploit a bug in the key generation code.

> I assume this affects ESR24, 25, and more current.

Anything since 2006: https://github.com/jrmuizel/mozilla-cvs-history/commit/881f80f012aab8c621ca7826e57d18b878a2587b

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d34124c78a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5d6e88ff7a

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5d34124c78a8
https://hg.mozilla.org/mozilla-central/rev/9c5d6e88ff7a

Comment 10

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So I tried to figure out if this b2g was affected. crypto.generateCRMFRequest doesn't seem to exist in B2g 1.2. Does that mean it isn't affected?

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

As far as I understand, that is correct - b2g is built with MOZ_DISABLE_CRYPTOLEGACY=1, which disables generateCRMFRequest (among other things).