Closed Bug 985070 Opened 6 years ago Closed 6 years ago

SECKEY_DecodeDERPublicKey double-frees the arena when decoding fails

Categories

(NSS :: Libraries, defect, P2)

defect

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)

RESOLVED FIXED
3.16.1
Tracking Status
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)

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.
Keeler, can you give us an idea of how easily this is triggered externally?
Keywords: sec-high
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: nobody → wtc
Status: NEW → ASSIGNED
Attachment #8394553 - Flags: review?(dkeeler)
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.
Keywords: sec-highsec-low
Priority: -- → P2
Target Milestone: --- → 3.16.1
(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.
If the input is a DER-encoded SubjectPublicKeyInfo, then
SECKEY_DecodeDERSubjectPublicKeyInfo followed by SECKEY_ExtractPublicKey
is the right solution.
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+
(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.
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+
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)
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+
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)
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+
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+
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+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
What version of NSS is this going out in?
(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
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").
Wan-Teh, thanks. Kai, I missed the target milestone, great. I have no concerns here then.
Marking as esr24- based on comment 18.
Marking [qa-] as per comment 2.
Whiteboard: [qa-]
NSS was updated to version 3.16.2 across all supported branches in bug 1020695.
Whiteboard: [qa-] → [qa-][adv-main31+][adv-esr24.7+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.