334183 - Double free on error because CERT_FindCertIssuer unexpectedly calls CERT_DestroyCertificate

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

this is the chain to the first death:
CERT_DestroyCertificate
CERT_FindCertIssuer
seckey_UpdateCertPQGChain
SECKEY_UpdateCertPQG
CERT_ExtractPublicKey
CRMF_CreateEncryptedKeyWithEncryptedValue
nsSetEscrowAuthority

This is the second free:
CERT_DestroyCertificate
nsSetEscrowAuthority

The return on not found could be reordered, but we'll need to verify that there aren't any other possible codepaths.

Comment 1

[timeless]

it also happens in nsNSSCertificate::GetChain

Comment 2

[timeless]

see bug 334238, same problem, but i actually spent a bit of time thinking about what's going on.

at this point i'm fairly certain that CERT_FindCertIssuer doesn't own the cert and has no right to delete it.

Comment 3

[timeless]

more instances:

first free:
CERT_DestroyCertificate
CERT_FindCertIssuer
CERT_FilterCertListByCANames

second free:
CERT_DestroyCertificate
CERT_FilterCertListByCANames

Comment 4

[timeless]

CERT_DestroyCertificate
CERT_FindCertIssuer
seckey_UpdateCertPQGChain
SECKEY_UpdateCertPQG
CERT_ExtractPublicKey
sec_pkcs7_encoder_start_encrypt

CERT_DestroyCertificate
sec_pkcs7_encoder_start_encrypt

Comment 5

[timeless]

*** Bug 334238 has been marked as a duplicate of this bug. ***

Comment 6

[timeless]

first stack:
CERT_DestroyCertificate
CERT_FindCertIssuer
seckey_UpdateCertPQGChain
SECKEY_UpdateCertPQG
CERT_ExtractPublicKey
SECKEY_ConvertToPublicKey

second stack:
CERT_DestroyCertificate
SECKEY_ConvertToPublicKey

---

CERT_DestroyCertificate
CERT_FindCertIssuer
cert_VerifyCertChain

CERT_DestroyCertificate
cert_VerifyCertChain

Comment 7

[timeless]

CERT_DestroyCertificate
CERT_FindCertIssuer
cert_VerifyCertChain

CERT_DestroyCertificate
cert_VerifyCertChain

Comment 8

[timeless]

CERT_DestroyCertificate
CERT_FindCertIssuer
seckey_UpdateCertPQGChain
SECKEY_UpdateCertPQG
CERT_ExtractPublicKey
NSS_CMSUtil_EncryptSymKey_MISSI

CERT_DestroyCertificate
NSS_CMSUtil_EncryptSymKey_MISSI

Comment 9

[timeless]

Attachment: this would make us stop destroying it

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218645 [details] [diff] [review]
this would make us stop destroying it

I don't believe this patch is correct.  
The call to CERT_DestroyCertificate is attempting to free the 
newly created reference to that certificate, which new refrerence
is now held in chain[0].  (I'm sure that is VERY inobvious).
Failing to destroy the newly created reference in chain[0] will 
leak that reference.  See bug 225525 for some other possible 
explanations for the issue you're trying to resolve.

> /*
>  * Find the issuer of a cert.  Use the authorityKeyID if it exists.
>  */
> CERTCertificate *
> CERT_FindCertIssuer(CERTCertificate *cert, int64 validTime, SECCertUsage usage)
> {

>     NSSCertificate *me;
>     NSSTime *nssTime;
>     NSSTrustDomain *td;
>     NSSCryptoContext *cc;
>     NSSCertificate *chain[3];
>     NSSUsage nssUsage;
>     PRStatus status;
> 
>     me = STAN_GetNSSCertificate(cert);
>     if (!me) {
>         PORT_SetError(SEC_ERROR_NO_MEMORY);
> 	return NULL;
>     }
>     nssTime = NSSTime_SetPRTime(NULL, validTime);
>     nssUsage.anyUsage = PR_FALSE;
>     nssUsage.nss3usage = usage;
>     nssUsage.nss3lookingForCA = PR_TRUE;
>     memset(chain, 0, 3*sizeof(NSSCertificate *));
>     td   = STAN_GetDefaultTrustDomain();
>     cc = STAN_GetDefaultCryptoContext();
>     (void)NSSCertificate_BuildChain(me, nssTime, &nssUsage, NULL, 
>                                     chain, 2, NULL, &status, td, cc);
>     nss_ZFreeIf(nssTime);
>     if (status == PR_SUCCESS) {
> 	/* if it's a root, the chain will only have one cert */
> 	if (!chain[1]) {
> 	    /* already has a reference from the call to BuildChain */
> 	    return cert;
>-	} else {
>-	    CERT_DestroyCertificate(cert); /* the first cert in the chain */

Although it may be very inobvious, I believe the above call to 
CERT_DestroyCertificate is equivalent to this line of code:
            NSSCertificate_Destroy(chain[0]);
You could add a run time test to ensure that they are equivalent, e.g. 
            PORT_Assert(me == chain[0]);
In the event that you ever found that me != chain[0], then the 
NSSCertificate_Destroy(chain[0]) would be correct and the call to
CERT_DestroyCertificate would not.  So, I'd argue that changing the
code to call NSSCertificate_Destroy(chain[0]) is more correct and
more obviously correct.  But simply leaking the new reference in 
chain[0] seems like it should not be an option.

>-	    return STAN_GetCERTCertificate(chain[1]); /* return the 2nd */
> 	}
>+	return STAN_GetCERTCertificate(chain[1]); /* return the 2nd */
>     } else {
> 	if (chain[0]) {
> 	    CERT_DestroyCertificate(cert);

Note that the very same issue applies to the line just above, also.
Its destruction of cert is no more or less valid than the one a few 
lines earlier.  The reason for doing it is the same, and the solution 
to the inobviousness is the same.    

> 	}
> 	PORT_SetError (SEC_ERROR_UNKNOWN_ISSUER);
>     }
>     return NULL;

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Julien, this may be related to the work you're doing on bug 225525.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Of all the coverity bugs filed against NSS recently, this one worries me most.  

When me == chain[0], I think this code is correct as is.
And me should always match chain[0].  
But I wouldn't be shocked to learn that sometimes me != chain[0]
especially in light of bug 225525.  

So, I'd suggest making some changes to 
a) help us detect this case, and 
b) reduce the damage that occurs when it happens.
patch forthcoming.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: detect me != chain[0] and free chain[0]

This is not intended to be a fix as much as problem 
detection and damage control.

Comment 14

[Alexei Volkov]

tip:
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.47; previous revision: 1.46

3.11 branch:
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.44.10.3; previous revision: 1.44.10.2

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218745 [details] [diff] [review]
detect me != chain[0] and free chain[0]

removing old review request, now that bug is resolved/fixed.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

CID 672

This was a false positive.  The "fix" we put in was to make the false
positive more obvious to coverity.