985070 - SECKEY_DecodeDERPublicKey double-frees the arena when decoding fails

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

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

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

[Matt Wobensmith [:mwobensmith][:matt:]]

Keeler, can you give us an idea of how easily this is triggered externally?

Comment 2

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

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.

Comment 3

[Wan-Teh Chang]

Attachment: Fix a double free on error path in SECKEY_DecodeDERPublicKey

Comment 4

[Wan-Teh Chang]

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

[Richard Barnes [:rbarnes]]

(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.

Comment 6

[Wan-Teh Chang]

If the input is a DER-encoded SubjectPublicKeyInfo, then
SECKEY_DecodeDERSubjectPublicKeyInfo followed by SECKEY_ExtractPublicKey
is the right solution.

Comment 7

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

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.

Comment 8

[Kai Engert [:KaiE:]]

(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 9

[Wan-Teh Chang]

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

Comment 10

[Wan-Teh Chang]

Attachment: Delete unused functions SECKEY_DecodeDERPublicKey and SECKEY_ConvertAndDecodePublicKey

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.)

Comment 11

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

Comment on attachment 8394909 [details] [diff] [review]
Delete unused functions SECKEY_DecodeDERPublicKey and SECKEY_ConvertAndDecodePublicKey

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

Awesome.

Comment 12

[Wan-Teh Chang]

Attachment: SECKEY_ImportDERPublicKey cleanup: Update comment on key type support. Don't zero memory before it's freed on error

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

Comment 13

[Wan-Teh Chang]

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

Comment 14

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

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.

Comment 15

[Wan-Teh Chang]

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

Comment 16

[Al Billings [:abillings - ex-MoCo]]

What version of NSS is this going out in?

Comment 17

[Kai Engert [:KaiE:]]

(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

Comment 18

[Wan-Teh Chang]

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

[Al Billings [:abillings - ex-MoCo]]

Wan-Teh, thanks. Kai, I missed the target milestone, great. I have no concerns here then.

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Marking as esr24- based on comment 18.

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] as per comment 2.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

NSS was updated to version 3.16.2 across all supported branches in bug 1020695.