nsConvertToActualKeyGenParams can leak if generateCRMFRequest is given multiple curve key parameters

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

({csectype-dos, memory-leak})

unspecified
mozilla28
csectype-dos, memory-leak
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 827728 [details] [diff] [review]
fix
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #827728 - Flags: review?(cviecco)
(Assignee)

Comment 2

5 years ago
Created attachment 827729 [details] [diff] [review]
test
Attachment #827729 - Flags: review?(cviecco)
Attachment #827728 - Flags: review?(cviecco) → review+
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?)
Attachment #827729 - Flags: review?(cviecco) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 828296 [details] [diff] [review]
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.
Attachment #827729 - Attachment is obsolete: true
Attachment #828296 - Flags: review?(cviecco)
Comment on attachment 828296 [details] [diff] [review]
test v2

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

Looks good to me.
Attachment #828296 - Flags: review?(cviecco) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 828789 [details] [diff] [review]
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.
Attachment #827728 - Attachment is obsolete: true
Attachment #828296 - Attachment is obsolete: true
Attachment #828789 - Flags: review+
(Assignee)

Comment 7

5 years ago
This isn't really security-sensitive.
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/fe7fe682aa17
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Keywords: mlk
You need to log in before you can comment on or make changes to this bug.