The default bug view has changed. See this FAQ.

Double free on error because CERT_FindCertIssuer unexpectedly calls CERT_DestroyCertificate

RESOLVED FIXED in 3.11.1

Status

NSS
Libraries
P2
critical
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: timeless, Assigned: Alexei Volkov)

Tracking

({coverity, crash})

3.11
3.11.1
coverity, crash

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse] [CID 672 668 665 664 416 412 -- false positive], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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.

Updated

11 years ago
Group: security
Whiteboard: [sg:critical?]
(Reporter)

Comment 1

11 years ago
it also happens in nsNSSCertificate::GetChain
Summary: Double free on error under nsSetEscrowAuthority → Double free on error under nsSetEscrowAuthority also nsNSSCertificate::GetChain
(Reporter)

Comment 2

11 years ago
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.
(Reporter)

Comment 3

11 years ago
more instances:

first free:
CERT_DestroyCertificate
CERT_FindCertIssuer
CERT_FilterCertListByCANames

second free:
CERT_DestroyCertificate
CERT_FilterCertListByCANames
Assignee: kengert → nobody
Component: Security: UI → Libraries
Product: Core → NSS
QA Contact: libraries
Version: Trunk → 4.0
(Reporter)

Comment 4

11 years ago
CERT_DestroyCertificate
CERT_FindCertIssuer
seckey_UpdateCertPQGChain
SECKEY_UpdateCertPQG
CERT_ExtractPublicKey
sec_pkcs7_encoder_start_encrypt

CERT_DestroyCertificate
sec_pkcs7_encoder_start_encrypt
Summary: Double free on error under nsSetEscrowAuthority also nsNSSCertificate::GetChain → Double free on error because CERT_FindCertIssuer unexpectedly calls CERT_DestroyCertificate
(Reporter)

Comment 5

11 years ago
*** Bug 334238 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 6

11 years ago
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
(Reporter)

Comment 7

11 years ago
CERT_DestroyCertificate
CERT_FindCertIssuer
cert_VerifyCertChain

CERT_DestroyCertificate
cert_VerifyCertChain
(Reporter)

Comment 8

11 years ago
CERT_DestroyCertificate
CERT_FindCertIssuer
seckey_UpdateCertPQGChain
SECKEY_UpdateCertPQG
CERT_ExtractPublicKey
NSS_CMSUtil_EncryptSymKey_MISSI

CERT_DestroyCertificate
NSS_CMSUtil_EncryptSymKey_MISSI
(Reporter)

Comment 9

11 years ago
Created attachment 218645 [details] [diff] [review]
this would make us stop destroying it
Attachment #218645 - Flags: review?(nelson)
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;
Attachment #218645 - Flags: review?(nelson) → review-
Julien, this may be related to the work you're doing on bug 225525.
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.
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → 3.1.1
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.
Attachment #218645 - Attachment is obsolete: true
Attachment #218745 - Flags: review?(julien.pierre.bugs)
Target Milestone: 3.1.1 → 3.11.1
Version: 4.0 → 3.11
Attachment #218745 - Flags: review?(alexei.volkov.bugs)
(Assignee)

Updated

11 years ago
Attachment #218745 - Flags: review?(alexei.volkov.bugs) → review+
(Assignee)

Comment 14

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Assignee: nobody → alexei.volkov.bugs
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.
Attachment #218745 - Flags: review?(julien.pierre.bugs)
CID 672

This was a false positive.  The "fix" we put in was to make the false
positive more obvious to coverity.
Group: security
Whiteboard: [sg:critical?] → [sg:critical?] CID 672
(Assignee)

Updated

11 years ago
Whiteboard: [sg:critical?] CID 672 → [sg:critical?] [CID 672 668]
(Assignee)

Updated

11 years ago
Whiteboard: [sg:critical?] [CID 672 668] → [sg:critical?] [CID 672 668 665]
(Assignee)

Updated

11 years ago
Whiteboard: [sg:critical?] [CID 672 668 665] → [sg:critical?] [CID 672 668 665 664]
(Assignee)

Updated

11 years ago
Whiteboard: [sg:critical?] [CID 672 668 665 664] → [sg:critical?] [CID 672 668 665 664416 ]
(Assignee)

Updated

11 years ago
Whiteboard: [sg:critical?] [CID 672 668 665 664416 ] → [sg:critical?] [CID 672 668 665 664 416]
(Assignee)

Updated

11 years ago
Whiteboard: [sg:critical?] [CID 672 668 665 664 416] → [sg:critical?] [CID 672 668 665 664 416 412]
Whiteboard: [sg:critical?] [CID 672 668 665 664 416 412] → [sg:nse] [CID 672 668 665 664 416 412 -- false positive]
You need to log in before you can comment on or make changes to this bug.