Last Comment Bug 334183 - Double free on error because CERT_FindCertIssuer unexpectedly calls CERT_DestroyCertificate
: Double free on error because CERT_FindCertIssuer unexpectedly calls CERT_Dest...
Status: RESOLVED FIXED
[sg:nse] [CID 672 668 665 664 416 412...
: coverity, crash
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: All All
: P2 critical (vote)
: 3.11.1
Assigned To: Alexei Volkov
:
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
: 334238 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-15 20:18 PDT by timeless
Modified: 2006-06-14 10:25 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
this would make us stop destroying it (7.17 KB, patch)
2006-04-16 23:08 PDT, timeless
nelson: review-
Details | Diff | Splinter Review
detect me != chain[0] and free chain[0] (1.41 KB, patch)
2006-04-17 17:30 PDT, Nelson Bolyard (seldom reads bugmail)
alvolkov.bgs: review+
Details | Diff | Splinter Review

Description timeless 2006-04-15 20:18:21 PDT
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 2006-04-15 21:46:08 PDT
it also happens in nsNSSCertificate::GetChain
Comment 2 timeless 2006-04-16 08:18:05 PDT
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 2006-04-16 08:25:34 PDT
more instances:

first free:
CERT_DestroyCertificate
CERT_FindCertIssuer
CERT_FilterCertListByCANames

second free:
CERT_DestroyCertificate
CERT_FilterCertListByCANames
Comment 4 timeless 2006-04-16 08:35:26 PDT
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 2006-04-16 08:36:09 PDT
*** Bug 334238 has been marked as a duplicate of this bug. ***
Comment 6 timeless 2006-04-16 08:43:41 PDT
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 2006-04-16 15:48:24 PDT
CERT_DestroyCertificate
CERT_FindCertIssuer
cert_VerifyCertChain

CERT_DestroyCertificate
cert_VerifyCertChain
Comment 8 timeless 2006-04-16 16:02:42 PDT
CERT_DestroyCertificate
CERT_FindCertIssuer
seckey_UpdateCertPQGChain
SECKEY_UpdateCertPQG
CERT_ExtractPublicKey
NSS_CMSUtil_EncryptSymKey_MISSI

CERT_DestroyCertificate
NSS_CMSUtil_EncryptSymKey_MISSI
Comment 9 timeless 2006-04-16 23:08:53 PDT
Created attachment 218645 [details] [diff] [review]
this would make us stop destroying it
Comment 10 Nelson Bolyard (seldom reads bugmail) 2006-04-17 01:59:26 PDT
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) 2006-04-17 02:00:44 PDT
Julien, this may be related to the work you're doing on bug 225525.
Comment 12 Nelson Bolyard (seldom reads bugmail) 2006-04-17 17:26:42 PDT
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) 2006-04-17 17:30:18 PDT
Created attachment 218745 [details] [diff] [review]
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 2006-04-21 19:10:39 PDT
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) 2006-05-15 23:07:44 PDT
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) 2006-06-10 17:46:28 PDT
CID 672

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

Note You need to log in before you can comment on or make changes to this bug.