Closed
Bug 985070
Opened 10 years ago
Closed 10 years ago
SECKEY_DecodeDERPublicKey double-frees the arena when decoding fails
Categories
(NSS :: Libraries, defect, P2)
NSS
Libraries
Tracking
(firefox28 wontfix, firefox29 wontfix, firefox30 wontfix, firefox31 fixed, firefox32 fixed, firefox-esr24- fixed, b2g-v1.2 wontfix, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)
People
(Reporter: keeler, Assigned: wtc)
Details
(Keywords: sec-low, Whiteboard: [qa-][adv-main31+][adv-esr24.7+])
Attachments
(3 files)
652 bytes,
patch
|
keeler
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
3.03 KB,
patch
|
keeler
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
1.18 KB,
patch
|
keeler
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
SECKEYPublicKey * SECKEY_DecodeDERPublicKey(const SECItem *pubkder) { PLArenaPool *arena; SECKEYPublicKey *pubk; SECStatus rv; SECItem newPubkder; arena = PORT_NewArena (DER_DEFAULT_CHUNKSIZE); if (arena == NULL) { PORT_SetError (SEC_ERROR_NO_MEMORY); return NULL; } pubk = (SECKEYPublicKey *) PORT_ArenaZAlloc (arena, sizeof (SECKEYPublicKey)); if (pubk != NULL) { pubk->arena = arena; pubk->pkcs11Slot = NULL; pubk->pkcs11ID = 0; prepare_rsa_pub_key_for_asn1(pubk); /* copy the DER into the arena, since Quick DER returns data that points into the DER input, which may get freed by the caller */ rv = SECITEM_CopyItem(arena, &newPubkder, pubkder); if ( rv == SECSuccess ) { rv = SEC_QuickDERDecodeItem(arena, pubk, SECKEY_RSAPublicKeyTemplate, &newPubkder); } if (rv == SECSuccess) return pubk; SECKEY_DestroyPublicKey (pubk); } else { PORT_SetError (SEC_ERROR_NO_MEMORY); } PORT_FreeArena (arena, PR_FALSE); return NULL; } If SEC_QuickDERDecodeItem fails, SECKEY_DestroyPublicKey will be called, which calls PORT_FreeArena on pubk->arena. pubk->arena is an alias for arena. PORT_FreeArena then gets called on arena for the second time.
Comment 1•10 years ago
|
||
Keeler, can you give us an idea of how easily this is triggered externally?
Keywords: sec-high
Reporter | ||
Comment 2•10 years ago
|
||
Actually, it doesn't look like we use this function at all, ever. :rbarnes was looking into using it for webcrypto. But, if we did use it, it would be very easy to trigger - just pass in a bad DER encoding of a public key.
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
SECKEY_DecodeDERPublicKey is not yet exported by NSS, and is nor used by NSS internally. So this bug doesn't need to be a security bug. Richard, have you found the right function to use? You can search for "PublicKey" in http://mxr.mozilla.org/nss/source/lib/nss/nss.def I guess SECKEY_ImportDERPublicKey should work for you: http://mxr.mozilla.org/nss/ident?i=SECKEY_ImportDERPublicKey Perhaps we should just delete the SECKEY_DecodeDERPublicKey function.
Comment 5•10 years ago
|
||
(In reply to Wan-Teh Chang from comment #4) > SECKEY_DecodeDERPublicKey is not yet exported by NSS, > and is nor used by NSS internally. So this bug doesn't > need to be a security bug. Unfortunately, as I discovered, the Firefox build process on MacOS will happily link to it. If it's really supposed to be non-public, then it probably shouldn't be in keyhi.h > Richard, have you found the right function to use? > You can search for "PublicKey" in > http://mxr.mozilla.org/nss/source/lib/nss/nss.def > > I guess SECKEY_ImportDERPublicKey should work for you: > http://mxr.mozilla.org/nss/ident?i=SECKEY_ImportDERPublicKey Right now, my code follows what the Chromium team did: SECKEY_DecodeDERSubjectPublicKeyInfo followed by SECKEY_ExtractPublicKey (see Bug 865789). That seems like a better approach anyway, because it can handle all the different types of public key (EC, DH, etc.). I'll take a look at SECKEY_ImportPublicKey to see if it has a similar level of functionality.
Assignee | ||
Comment 6•10 years ago
|
||
If the input is a DER-encoded SubjectPublicKeyInfo, then SECKEY_DecodeDERSubjectPublicKeyInfo followed by SECKEY_ExtractPublicKey is the right solution.
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8394553 [details] [diff] [review] Fix a double free on error path in SECKEY_DecodeDERPublicKey Review of attachment 8394553 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. However, since nothing seems to be using it, I think it would be best to remove this function entirely.
Attachment #8394553 -
Flags: review?(dkeeler) → review+
Comment 8•10 years ago
|
||
(In reply to Wan-Teh Chang from comment #4) > SECKEY_DecodeDERPublicKey is not yet exported by NSS, > and is nor used by NSS internally. It seems that function SECKEY_ConvertAndDecodePublicKey does call it! However, I can only see one place that calls SECKEY_ConvertAndDecodePublicKey, it's the "certcgi" test code.
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8394553 [details] [diff] [review] Fix a double free on error path in SECKEY_DecodeDERPublicKey I checked in this patch so that the code will be in its most correct state when it's deleted: https://hg.mozilla.org/projects/nss/rev/96512d249e7d
Attachment #8394553 -
Flags: checked-in+
Assignee | ||
Comment 10•10 years ago
|
||
Kai, the function used by certcgi is SECKEY_ConvertAndDecodePublicKeyAndChallenge, not SECKEY_ConvertAndDecodePublicKey. (I was also confused when I searched for "SECKEY_ConvertAndDecodePublicKey" as a string in MXR.)
Attachment #8394909 -
Flags: review?(dkeeler)
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8394909 [details] [diff] [review] Delete unused functions SECKEY_DecodeDERPublicKey and SECKEY_ConvertAndDecodePublicKey Review of attachment 8394909 [details] [diff] [review]: ----------------------------------------------------------------- Awesome.
Attachment #8394909 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Before I deleted SECKEY_DecodeDERPublicKey, I compared its code with SECKEY_ImportDERPublicKey to see if there is anything good worth salvaging. I noticed these two minor issues. When NSS frees memory, it often zeros the memory first if the memory contains sensitive info such as passwords or private keys. In SECKEY_ImportDERPublicKey the memory contains public keys, so we don't need to zero the memory. See SECKEY_DestroyPublicKey: http://mxr.mozilla.org/nss/ident?i=SECKEY_DestroyPublicKey
Attachment #8394917 -
Flags: review?(dkeeler)
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8394909 [details] [diff] [review] Delete unused functions SECKEY_DecodeDERPublicKey and SECKEY_ConvertAndDecodePublicKey Patch checked in: https://hg.mozilla.org/projects/nss/rev/d75c6f53426b
Attachment #8394909 -
Flags: checked-in+
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8394917 [details] [diff] [review] SECKEY_ImportDERPublicKey cleanup: Update comment on key type support. Don't zero memory before it's freed on error Review of attachment 8394917 [details] [diff] [review]: ----------------------------------------------------------------- Ok - that seems reasonable.
Attachment #8394917 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8394917 [details] [diff] [review] SECKEY_ImportDERPublicKey cleanup: Update comment on key type support. Don't zero memory before it's freed on error Patch checked in: https://hg.mozilla.org/projects/nss/rev/eb035f52f2ec
Attachment #8394917 -
Flags: checked-in+
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 16•10 years ago
|
||
What version of NSS is this going out in?
Comment 17•10 years ago
|
||
(In reply to Al Billings [:abillings] from comment #16) > What version of NSS is this going out in? see tharget milestone, currently set to 3.16.1 m-c already uses 3.16.1 beta and should have this bug fixed
Assignee | ||
Comment 18•10 years ago
|
||
Al: I guess you asked that question because this bug is marked security-sensitive. After evaluation, I think this bug is not a security bug. I already lowered the rating to sec-low (there is no "sec-none").
Comment 19•10 years ago
|
||
Wan-Teh, thanks. Kai, I missed the target milestone, great. I have no concerns here then.
status-firefox28:
--- → wontfix
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox-esr24:
--- → affected
Updated•10 years ago
|
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
status-firefox31:
--- → fixed
status-firefox32:
--- → fixed
Updated•10 years ago
|
Updated•10 years ago
|
Comment 22•10 years ago
|
||
NSS was updated to version 3.16.2 across all supported branches in bug 1020695.
Updated•10 years ago
|
Whiteboard: [qa-] → [qa-][adv-main31+][adv-esr24.7+]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•