Closed Bug 934716 Opened 8 years ago Closed 8 years ago

nsConvertToActualKeyGenParams can leak if generateCRMFRequest is given multiple curve key parameters

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: keeler, Assigned: keeler)

Details

(Keywords: csectype-dos, memory-leak, Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

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.
Attached patch fix (obsolete) — Splinter Review
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #827728 - Flags: review?(cviecco)
Attached patch test (obsolete) — Splinter Review
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+
Attached patch test v2 (obsolete) — Splinter Review
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+
Attached patch patchSplinter Review
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+
This isn't really security-sensitive.
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/fe7fe682aa17
Status: ASSIGNED → RESOLVED
Closed: 8 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.