Closed Bug 849553 Opened 9 years ago Closed 8 years ago

crypto.generateCRMFRequest does not validate its parameters, leading to a hang and a segmentation fault

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox-esr17 --- wontfix
firefox-esr24 - wontfix
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- fixed

People

(Reporter: ainsaar, Assigned: keeler)

References

Details

(Keywords: crash, csectype-dos, testcase, Whiteboard: [adv-main26-])

Attachments

(4 files, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130222194407

Steps to reproduce:

I tried giving bogus key lengths to crypto.generateCRMFRequest. The test scripts I used are attached. 


Actual results:

Trying to generate an "rsa-ex" key with length 10240 spins the CPU at 100% and shows the private key generation dialog indefinitely. Trying to close the dialog hangs the whole browser.

Trying to generate an "rsa-ex" key again, but with length 65536, crashes with a segmentation fault. I will attach a gdb output shortly (I can try making a debug build for more comprehensive output, if you cannot reproduce the problem).
This is the backtrace of the segmentation fault with key length 65536. If you insist, I can try making a debug build where gdb would show more.
By the way, as https://bugzilla.mozilla.org/show_bug.cgi?id=430328#c7 hints, the key length is not the only thing needing better validation in the workings of crypto.generateCRMFRequest. It looks like if the argument of nsKeyGenType() consists only of spaces or is empty, then the "end" pointer will run to the left of the string and isspace is called on something outside, possibly also segfaulting! I have not got it to crash in that way though. Perhaps I have overlooked some validation elsewhere.
Sorry, in the last comment I meant the argument of cryptojs_interpret_key_gen_type(), not nsKeyGenType.
Component: Untriaged → Security
Product: Firefox → Core
Brian: is this a potential security problem or a safish out-of-resources crash?
Component: Security → Security: PSM
Flags: needinfo?(bsmith)
Keywords: crash, testcase
I can at least confirm the hang/cpu use. A 10K key is well beyond anything  useful in practice, can we just cap the keylength there or at 8K until machines catch up with those lengths? If we end up needing keys that long for safety we'll probably have switched to a different algorithm instead by that time.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Dan Veditz says DOS.
Group: core-security
Keywords: csec-dos
mhamrick, can you take a look at this? If not, please unassign it.
Assignee: nobody → mhamrick
Flags: needinfo?(bsmith)
so here's some code that checks the keyLength before trying to generate a key. per dveditz's request, the JS_CRYPTO_MAX_KEYLENGTH is set to 8192.

during testing, however, i didn't experience a hang or crash when trying to generate a 10240 bit key. it did take FOREVER to complete though. i'm going to keep this bug open for a bit while i spend a little bit of time looking at what happens on 32-bit systems (only tested it on 64 bit MacOS X and 64 bit Leenucks)

also want to generate some automated tests for this bug.

also also want to see if there's more input requiring scrubbing.

and i'm willing to entertain the notion that we check if we're asking for an ECC key and then lower the max keylength 571 (which is the old large curve from NIST 800-56A and draft-ietf-pkix-ecc-nist-recommended-curves.) but could be convinced to use a different max if someone else has a better reference.
Duplicate of this bug: 891145
As I found out in bug 891145, a key size that is too small is also a problem.
Attachment #723110 - Attachment is obsolete: true
Attachment #723113 - Attachment is obsolete: true
Attachment #723115 - Attachment is obsolete: true
Attachment #761135 - Attachment is obsolete: true
Attachment #783323 - Flags: review?(brian)
nsConvertToActualKeyGenParams already seems to sanity check the parameters:

    if (keySize > 0) {
      rsaParams->keySizeInBits = keySize;
    } else {
      rsaParams->keySizeInBits = 1024;
    }

It is concerning that there is a segmentation fault. But, I think that, looking at the gdb output, the problem causing the segmentation fault is elsewhere. In particular, it looks like the crash is occurring in PK11_FreeSlot. That means we're probably trying to free a slot that we don't have; i.e. I think the real issue is probably that the code in NSS is correctly preventing badness, but our error handling code (either in NSS or in PSM) causes us to attempt to free an object that never was created (due to bad parameters).

Anyway, any parameter range validation should generally be happening in NSS and not PSM. NSS has a 16K max key size for RSA (#define RSA_MAX_MODULUS_BITS 16384 in blapit.h). That will take a long time, especially on slow machines. But, that's what the user asked for, if the user wanted any certificate generation to happen at all. A more fundamental problem is that the website can trigger this slowness without user input. I think the solution for this "make the computer compute a lot and block the user from doing things" aspect of the bug is likely to remove the API (which we're making progress on) and/or making the action cancelable (like <keygen> is, IIRC).

Anyway, I think this bug should focus on the segfault in attachment 723113 [details] and attachment 723115 [details]. Let's file a separate bug for the "make the computer compute too much" aspect.
Group: crypto-core-security
Comment on attachment 783323 [details] [diff] [review]
added check for key being less than 1

See previous comment.
Attachment #783323 - Flags: review?(brian)
Attachment #723115 - Attachment is obsolete: false
Attachment #723113 - Attachment is obsolete: false
Attachment #723110 - Attachment is obsolete: false
Assignee: mhamrick → nobody
Attached patch patch (obsolete) — Splinter Review
I did some other work on this file, so I think it makes sense if I pick this up.
This patch guards against this null dereference. It does no other parameter validation, as suggested by Brian (i.e., NSS should be doing the validation, and this code should do sane things when NSS errors out).
Assignee: nobody → dkeeler
Attachment #783323 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #785223 - Flags: review?(brian)
Comment on attachment 785223 [details] [diff] [review]
patch

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

::: security/manager/ssl/src/nsCrypto.cpp
@@ +820,5 @@
>              mustMoveKey = true;
>            }
>          
> +          if (used_slot) {
> +            PK11_FreeSlot(used_slot);

Look at CERT_DestroyCertificate, SECKEY_DestroyPublicKey, SECKEY_DestroyPublicKey, etc. each of those functions is a no-op if it is passed null. Perhaps it would be better to change Pk11_FreeSlot to do the same?

AFAICT, it may make sense to change ConsumeResult so that you call it like this:

rv = KeyGenRunnable->ConsumeResult(&mustMoveKey, &keyPairInfo->privateKey, &keyPairInfo->pubKey);

Then you avoid all the PK11_ReferenceSlot and PK11_FreeSlot stuff completely.

::: security/manager/ssl/tests/mochitest/bugs/test_generateCRMFRequest.html
@@ +24,5 @@
>      }
>  
> +    // bug 849553
> +    try {
> +      var crmfObject = crypto.generateCRMFRequest("CN=undefined", "regToken",

Add a comment explaining why you expect the call to fail.
Attachment #785223 - Flags: review?(brian) → review+
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) from comment #16)
> Look at CERT_DestroyCertificate, SECKEY_DestroyPublicKey,
> SECKEY_DestroyPublicKey, etc. each of those functions is a no-op if it is
> passed null. Perhaps it would be better to change Pk11_FreeSlot to do the
> same?
> 
> AFAICT, it may make sense to change ConsumeResult so that you call it like
> this:
> 
> rv = KeyGenRunnable->ConsumeResult(&mustMoveKey, &keyPairInfo->privateKey,
> &keyPairInfo->pubKey);
> 
> Then you avoid all the PK11_ReferenceSlot and PK11_FreeSl

Those sound like good candidates for follow-up bugs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/31f0115e4f41
Carrying over r+.
Attachment #785223 - Attachment is obsolete: true
Attachment #796347 - Flags: review+
Group: crypto-core-security → core-security
https://hg.mozilla.org/mozilla-central/rev/31f0115e4f41
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Version: 19 Branch → Trunk
QA Contact: mwobensmith
Confirmed hang (attachment 1 [details] [diff] [review]) and crash (attachment 2 [details] [diff] [review]) on m-c 2013-08-26.
Verified fixed on m-c 2013-08-29.
Status: RESOLVED → VERIFIED
Whiteboard: [adv-main26-]
Are you sure we don't want to fix this for ESR 24? The fix is very simple and it prevents a content-induced crash in a function that is very rarely ever used for any legit purpose. Even if the crash isn't exploitable, it seems like a good cost/benefit.
Pretty sure release drivers don't want the work/distraction unless it's causing real problems for people. There are dozens of crashes (more?) like this fixed every release and we can't backport them all. But nominating it is good.
(In reply to Daniel Veditz [:dveditz] from comment #22)
> Pretty sure release drivers don't want the work/distraction unless it's
> causing real problems for people. There are dozens of crashes (more?) like
> this fixed every release and we can't backport them all. But nominating it
> is good.

dveditz's intuition is correct - we're going to pass on this. Low security/stability reward means no code change needed on the ESR branch.
Is this wontfix for b2g18* as well then?
Group: core-security
You need to log in before you can comment on or make changes to this bug.