934716 - nsConvertToActualKeyGenParams can leak if generateCRMFRequest is given multiple curve key parameters

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

When something like this is called:

window.crypto.generateCRMFRequest("CN=subject", "auth", "auth", null,
                                  "", 1024, "curve=secp256r1;curve=secp256k1",
                                  "ec-dual-use");

This code eventually runs:

477 nsConvertToActualKeyGenParams(uint32_t keyGenMech, char *params,
...
541     char *curve = nullptr;
...
552       while (getNextNameValueFromECKeygenParamString(
553               next_input, name, name_len, value, value_len,
554               next_input))
555       {
556         if (PL_strncmp(name, "curve", std::min(name_len, 5)) == 0)
557         {
558           curve = PL_strndup(value, value_len);
559         }

Since there are two "curve=..." values in the parameter string, curve gets reassigned to new memory without checking for or freeing the old memory.

Comment 1

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

Attachment: fix

Comment 2

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

Attachment: test

Comment 3

[Camilo Viecco (:cviecco)]

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

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

No test for dup popcert? (yes there seem no tests for popcert so r+ for this issue)

Also, since we are here we up the length constraints for keys here to at last 1024 bits (maybe new bug?)

Comment 4

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

Attachment: test v2

Thanks for the quick reviews. I added a test for popcert (and good idea, too - see bug 935618). I'm not sure what you mean about key sizes, but if it's an issue with pre-existing tests, I think we can do that in another bug.

Comment 5

[Camilo Viecco (:cviecco)]

Comment on attachment 828296 [details] [diff] [review]
test v2

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

Looks good to me.

Comment 6

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

Attachment: patch

Thanks for the quick reviews. I added a comment in the test to be clear about what's going on. Carrying over r+ and folding together test and fix.

Comment 7

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

This isn't really security-sensitive.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe7fe682aa17

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fe7fe682aa17

Comment 10

[Ioana (away)]

https://hg.mozilla.org/mozilla-central/diff/fe7fe682aa17/security/manager/ssl/tests/mochitest/bugs/test_generateCRMFRequest.html