232380 - A cert may be destroyed twice in CERT_FindExpiredIssuer and cert_VerifyCertChain

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

I found these problems during code inspection.

In lib/certhigh/certfvy.c, search for the string 
"subjectCert = issuerCert", you will find two for 
loops. 

The first for loop (in CERT_FindExpiredIssuer) 
has the following problem: 

When the for loop exits because
count >= CERT_MAX_CERT_CHAIN, subjectCert equals
issuerCert and both are non-NULL. So we will
destroy that cert twice before we return from
the function.

Note: Nelson pointed out that test for a root CA
(self signed) cert is incorrect.  However, he found
that this function is dead code, so the solution
for this function is to delete it.

The second for loop (in cert_VerifyCertChain) has
the following problem: 

The "subjectCert = NULL;" assignment statement after 
the for loop defends against the problem above. 
However, if we take the "goto loser;" jump inside 
the if (subjectCertIsSelfIssued == PR_FALSE) {...} 
block at the beginning of the for loop, we also end 
up with the same condition: subjectCert equals 
issuerCert and both are non-NULL, and we will destroy 
that cert twice before we return from the function.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

1. Removed dead function CERT_FindExpiredIssuer.
Please make sure that we do not plan to export
this function.

2. In cert_VerifyCertChain, fix the double-destroy
problem by never having subjectCert and issuerCert
point at the same cert at any time.

Comment 2

[Julien Pierre]

Comment on attachment 140038 [details] [diff] [review]
Proposed patch

I agree with this fix.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 140038 [details] [diff] [review]
Proposed patch

Good catch!

Comment 4

[Wan-Teh Chang]

Patch checked in.

Checking in certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.44; previous revision: 1.43
done
Checking in certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.40; previous revision: 1.39
done

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Setting priorities on unprioritized bugs resolved fixed for NSS 3.10.
Marking P1 since this fixes a double-free.