Closed Bug 984608 Opened 7 years ago Closed 7 years ago

SECKEY_EncodeDERSubjectPublicKeyInfo and PK11_DEREncodePublicKey take non-const SECKEYPublicKey*

Categories

(NSS :: Libraries, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
3.16.1

People

(Reporter: briansmith, Assigned: mz_mhs-ctb)

References

Details

Attachments

(1 file, 3 obsolete files)

The input parameter to these functions should be |const SECKEYPublicKey*|.
Assignee: nobody → mz_mhs-ctb
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Attachment #8392630 - Flags: review?(brian)
Comment on attachment 8392630 [details] [diff] [review]
Patch

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

In order to fix this bug properly, we need to remove the need for calling the prepare_* functions. One way of doing that would be to create a copy of the input key using SECKEY_CopyPublicKey. Then, call the prepare_* functions on the copy. Then destroy the copy. The performance impact of the additional work should not matter because these are not performance-critical functions.

::: security/nss/lib/cryptohi/seckey.c
@@ +1219,5 @@
>      CERTSubjectPublicKeyInfo *spki;
>      PLArenaPool *arena;
>      SECItem params = { siBuffer, NULL, 0 };
>  
> +    if (!pubk/*cpubk*/) {

Please undo this change.

@@ +1234,5 @@
>      spki = (CERTSubjectPublicKeyInfo *) PORT_ArenaZAlloc(arena, sizeof (*spki));
>      if (spki != NULL) {
>  	SECStatus rv;
>  	SECItem *rv_item;
> +	//SECKEYPublicKey pubk = (SECKEYPublicKey *)cpubk;

Please undo this change.

@@ +1245,5 @@
>  	    if (rv == SECSuccess) {
>  		/*
>  		 * DER encode the public key into the subjectPublicKeyInfo.
>  		 */
> +		prepare_rsa_pub_key_for_asn1((SECKEYPublicKey *)pubk);

If we're going to fix this bug, then we need to fix prepare_rsa_pub_key_for_asn1 or remove the need for it. We can't have these functions take a const pointer and then cast away the constness to modify the pointed-to-value.

In particular, callers would likely assume that a function that takes a "const SECKEYPublicKey *" is fully thread-safe, but these functions are not thread-safe.

::: security/nss/lib/pk11wrap/pk11akey.c
@@ +1896,2 @@
>  {
> +    return SECKEY_EncodeDERSubjectPublicKeyInfo((SECKEYPublicKey*)pubk);

The cast is not necessary, given your other change.
Attachment #8392630 - Flags: review?(brian) → review-
Also, thanks for working on this.

Like I mentioned, the way you copy a key is through the SECKEY_CopyPublicKey function. In order to free the memory allocated for that copy when you are done with it, you need to call SECKEY_DestroyPublicKey on the copy.
Attached patch const-sec-functions.patch (obsolete) — Splinter Review
Review comments addressed.
Attachment #8392630 - Attachment is obsolete: true
Attachment #8398332 - Flags: review?(brian)
Comment on attachment 8398332 [details] [diff] [review]
const-sec-functions.patch

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

Thanks. There are some bugs (memory leaks) and style issues in this patch. See below.

::: security/nss/lib/cryptohi/seckey.c
@@ +1218,5 @@
>  {
>      CERTSubjectPublicKeyInfo *spki;
>      PLArenaPool *arena;
>      SECItem params = { siBuffer, NULL, 0 };
> +	

Please remove this blank line (which has trailing whitespace, BTW).

@@ +1219,5 @@
>      CERTSubjectPublicKeyInfo *spki;
>      PLArenaPool *arena;
>      SECItem params = { siBuffer, NULL, 0 };
> +	
> +    SECKEYPublicKey *tempKey = SECKEY_CopyPublicKey(pubk);

Add a blank line here, so that all the variable declarations are grouped together. (NSS is C89, which requires all variables to be declared at the start of the block, and this is the convention we follow in NSS.)

Don't remove the "if (!pubk)" check. SECKEY_CopyPublicKey doesn't do that check and so if pubk is null, then we'll segfault.

So, the checks should look like something like this:

if (!pubk) {
    PORT_SetError(SEC_ERROR_INVALID_ARGS);
    return NULL;
}

tempKey = SECKEY_CopyPublicKey(pubk);
if (!tempKey) {
    return NULL; /* SECKEY_CopyPublicKey set error code */
}

@@ +1228,5 @@
>  
>      arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
>      if (arena == NULL) {
>  	PORT_SetError(SEC_ERROR_NO_MEMORY);
>  	return NULL;

There are a lot of places, like here, where we don't call SECKEY_DestroyPublicKey, and so we leak tempKey.

I suggest refactoring the function by splitting everything from "arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);" into its own function:

static CERTSubjectPublicKeyInfo *
seckey_CreateSubjectPublicKeyInfo_helper(SECKEYPublicKey *pubk);

    ...

    spki = seckey_CreateSubjectPublicKeyInfo_helper(tempKey);
    SECKEY_DestroyPublicKey(tempKey);
    return spki;
}

static CERTSubjectPublicKeyInfo *
seckey_CreateSubjectPublicKeyInfo_helper(SECKEYPublicKey *pubk) {
    arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
    ...

Alternatively, you could switch this function to the "goto done" pattern that is used elsewhere in NSS. I'd prefer to see the non-goto version though.
Attachment #8398332 - Flags: review?(brian) → review-
Attached patch Review comments addressed (obsolete) — Splinter Review
Attachment #8398332 - Attachment is obsolete: true
Attachment #8398851 - Flags: review?(brian)
Comment on attachment 8398851 [details] [diff] [review]
Review comments addressed

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

The NSS indention style is to have tabs set at 8 spaces, not 4 spaces. IIRC, it is OK to always use spaces in new patches.

Please make the following changes and attach the updated patch.

Thanks for fixing this!

::: security/nss/lib/cryptohi/seckey.c
@@ +1332,5 @@
> +{
> +    CERTSubjectPublicKeyInfo *spki;
> +    SECKEYPublicKey *tempKey;
> +
> +	if (!pubk) {

Nit: Use four spaces for indention here, instead of a tab.

@@ +1337,5 @@
> +        PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +        return NULL;
> +    }
> +
> +	tempKey = SECKEY_CopyPublicKey(pubk);

Nit: Use four spaces for indention here, instead of a tab.

@@ +1339,5 @@
> +    }
> +
> +	tempKey = SECKEY_CopyPublicKey(pubk);
> +    if (!tempKey) {
> +        PORT_SetError(SEC_ERROR_INVALID_ARGS);

There's no need to call PORT_SetError here because SECKEY_CopyPublicKey is responsible for setting the error code when it returns NULL. Also, SEC_ERROR_INVALID_ARGS would be the wrong error code anyway.

@@ +1343,5 @@
> +        PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +        return NULL;
> +    }
> +    spki = seckey_CreateSubjectPublicKeyInfo_helper(tempKey);
> +	SECKEY_DestroyPublicKey(tempKey);

Nit: Use four spaces for indention here instead of a tab.
Attachment #8398851 - Flags: review?(brian) → review+
Attachment #8398851 - Attachment is obsolete: true
Attachment #8398962 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8e30768d372c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Michael: I checked in your patch to the NSS trunk. I forgot to use the -u argument to mark you as the patch author when I converted the patch from a mozilla-central patch to an NSS patch. Sorry about that.

Wes, Carsten: Patches for the "NSS" product need to be checked into the NSS repo (hg.mozilla.org/projects/nss), not mozilla-central, before being resolved as fixed. Otherwise, the next time we import NSS into mozilla-central, the patch will be silently overwritten. Also, the target milestone needs to be set.

http://hg.mozilla.org/projects/nss/rev/e955c6438204
Target Milestone: --- → 3.16.1
You need to log in before you can comment on or make changes to this bug.