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

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Siim Ainsaar]

Attachment: test-hang.html, key length 10240, hangs

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).

Comment 1

[Siim Ainsaar]

Attachment: test-segfault.html, key length 65536, segmentation fault

Comment 2

[Siim Ainsaar]

Attachment: gdb output from the official Firefox-19.0.2 64-bit Linux build

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.

Comment 3

[Siim Ainsaar]

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.

Comment 4

[Siim Ainsaar]

Sorry, in the last comment I meant the argument of cryptojs_interpret_key_gen_type(), not nsKeyGenType.

Comment 5

[Daniel Veditz [:dveditz]]

Brian: is this a potential security problem or a safish out-of-resources crash?

Comment 6

[Daniel Veditz [:dveditz]]

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.

Comment 7

[Matt Wobensmith [:mwobensmith][:matt:]]

Dan Veditz says DOS.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

mhamrick, can you take a look at this? If not, please unassign it.

Comment 9

[Meadhbh Hamrick :mhamrick]

Attachment: add key length checking for crypto.generateCRMFRequest()

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.

Comment 11

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

As I found out in bug 891145, a key size that is too small is also a problem.

Comment 12

[Meadhbh Hamrick :mhamrick]

Attachment: added check for key being less than 1

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 783323 [details] [diff] [review]
added check for key being less than 1

See previous comment.

Comment 15

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

Attachment: patch

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).

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 17

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

(In reply to Brian Smith (:briansmith, was :[e-mail]) 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

Comment 18

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

Attachment: patch with updated comment

Carrying over r+.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/31f0115e4f41

Comment 20

[Matt Wobensmith [:mwobensmith][:matt:]]

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.

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 22

[Daniel Veditz [:dveditz]]

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.

Comment 23

[Alex Keybl [:akeybl]]

(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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Is this wontfix for b2g18* as well then?