Closed Bug 935618 (CVE-2014-1498) Opened 6 years ago Closed 6 years ago

nsConvertToActualKeyGenParams uses the union in a SECKEYPublicKey without checking its type

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox25 --- wontfix
firefox26 --- affected
firefox27 --- affected
firefox28 --- fixed
firefox-esr24 --- wontfix
b2g18 --- ?
b2g-v1.1hd --- ?
b2g-v1.2 --- ?

People

(Reporter: keeler, Assigned: keeler)

Details

(Keywords: csectype-dos, sec-low, Whiteboard: [adv-main28+])

Attachments

(2 files, 1 obsolete file)

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.
Attached patch fixSplinter Review
This checks that we actually have an ec certificate before treating it like one.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #828346 - Flags: review?(cviecco)
Attached patch test (obsolete) — Splinter Review
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.
Attachment #828347 - Flags: review?(cviecco)
Attachment #828346 - Flags: review?(cviecco) → review+
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.
Attachment #828347 - Flags: review?(cviecco) → review+
Attached patch test v2Splinter Review
Thanks, Camilo. Carrying over r+.
Attachment #828347 - Attachment is obsolete: true
Attachment #828888 - Flags: review+
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.
Attachment #828346 - Flags: sec-approval?
This needs a security rating before approval. Can you suggest one?

I assume this affects ESR24, 25, and more current.
(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
Attachment #828346 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/5d34124c78a8
https://hg.mozilla.org/mozilla-central/rev/9c5d6e88ff7a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite?
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?
Flags: needinfo?(dkeeler)
As far as I understand, that is correct - b2g is built with MOZ_DISABLE_CRYPTOLEGACY=1, which disables generateCRMFRequest (among other things).
Flags: needinfo?(dkeeler)
Whiteboard: [adv-main28+]
Alias: CVE-2014-1498
Group: core-security
You need to log in before you can comment on or make changes to this bug.