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

nsConvertToActualKeyGenParams uses the union in a SECKEYPublicKey without checking its type


(Core :: Security: PSM, defect)

Not set



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


(Reporter: keeler, Assigned: keeler)


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


(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);
567             if (keyPairInfo->ecPopCert)
568             {
569               keyPairInfo->ecPopPubKey = CERT_ExtractPublicKey(keyPairInfo->ecPopCert);
570             }
571           }
572         }
573       }
574     }
576     // first try to use the params of the provided CA cert
577     if (keyPairInfo->ecPopPubKey)
578     {
579       returnParams = SECITEM_DupItem(&keyPairInfo->ecPopPubKey->;
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
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]

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]

[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:
Attachment #828346 - Flags: sec-approval? → sec-approval+
Closed: 10 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.