422859 - libPKIX builds & validates chain to root not in the caller-provided anchor list

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

This bug was originally reported by Kai Engert in bug 406755 comment 37.

He reports that he called CERT_PKIXVerifyCert passing it a set of one or
more certificates to be used as explicit trust anchors, and a policy OID,
and a cert from a particular server.

He expected the function call to either 
a) succeed and return a chain that was anchored by one of the roots he 
supplied or 
b) fail
Instead, he reports that it succeeded, but the root CA cert that it returned
as being the matching trust anchor was NOT one of the certs he had passed 
in to the call, but rather was one of NSS's built-in trusted root certs.

So, it appears that NSS did not use the caller-supplied list of anchors 
to the exclusion of all the other usual trust anchors, but merely in 
addition to the usual trust anchors.  

This is bad news for EV.  

I think we should try to reproduce this with vfychain or vfyserv.  
I will attempt to attach to this bug a zip file containing the certs 
sent by the server in the TLS handshake.  

I don't know exactly how many root CA certs Kai passed to CERT_PKIXVerifyCert,
but I believe that one of them was the cert found at 
http://crt.comodoca.com/COMODOCertificationAuthority.crt
and the EV policy OID used with it was 1.3.6.1.4.1.6449.1.2.1.5.1

Kai, can you tell us if there were any other certs passed as trust anchors?

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: zip file of the 5 certs sent by secure.comodo.com:443

In the attached zip file, the 5 certs sent by the server are in files
named cert.001 ... cert.005.  The server cert, in cert.001, bears the 
policy OID 1.3.6.1.4.1.6449.1.2.1.5.1 
The file cert.005 appears to bear the (er, a) root CA cert.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The cert in file cert.005 is the root CA cert that (I believe) Kai 
supplied to CERT_PKIXVerifyCert in his test.

I was able to reproduce the bug with this test command:

vfychain -ppvv -o OID.1.3.6.1.4.1.6449.1.2.1.5.1 -u 1 \
 cert.001 cert.002 cert.003 cert.004 cert.005 -t cert.005 

Just as Kai reported in bug 406755 comment 37, the root cert returned
by the call to CERT_PKIXVerifyCert was a cert from nssckbi, namely:

Version: 3 (0x2)
Serial Number:
    44:be:0c:8b:50:00:21:b4:11:d3:2a:68:06:a9:ad:69
Signature Algorithm: PKCS #1 SHA-1 With RSA Encryption
Issuer: "CN=UTN - DATACorp SGC,OU=http://www.usertrust.com,O=The USER
    TRUST Network,L=Salt Lake City,ST=UT,C=US"
Validity:
    Not Before: Thu Jun 24 18:57:21 1999
    Not After : Mon Jun 24 19:06:30 2019
Subject: "CN=UTN - DATACorp SGC,OU=http://www.usertrust.com,O=The USE
    RTRUST Network,L=Salt Lake City,ST=UT,C=US"

Comment 3

[Alexei Volkov]

Attachment: Patch v1

Trust only certs from user defined trust anchor list if it was specified.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 311894 [details] [diff] [review]
Patch v1

This patch appears to make 3 unrelated changes (plus adding 
and removing a bunch of blank lines).  

One of the changes is the subject of this bug.   r+
One fixes a double-free of an arena.             r+
One changes the behavior of the output parameter cert_po_errorLog.  r?

I have a question about that errorLog change.  
Q: Is it true that, in case the cert chain is valid and no error
occurs, this "errorlog" parameter outputs the validated chain?

If not, is there any means defined whereby this function outputs 
the entire validated cert chain (or chains)?  If so, is that 
implemented?

Comment 5

[Rob Stradling]

(in reply to comment #3)
> Patch v1

Thanks Alexei and Nelson.  I've just tested this patch with Firefox 3 CVS + NSS HEAD, and it does appear to resolve the issues I was concerned about in Bug #406755 Comments #33, #34 and #35.  i.e. I now get the EV UI for https://secure.comodo.com with only the "COMODO Certification Authority" Root added locally to myTrustedEVInfos[].

(further to Bug #406755 Comment #39)
> Kai,
> I'm pretty sure that the issue with certs in the temp cert db is a real
> issue...

Nelson, Kai,

I'm following this up on this point here, since Bug #406755 is now RESOLVED FIXED.

With Alexei's patch + just the "COMODO Certification Authority" added locally to myTrustedEVInfos[], I notice that (for https://secure.comodo.com) the Firefox Certificate Viewer still shows the certificate chain up to "AddTrust External CA Root", even though PSM/NSS must be using the chain up to "COMODO Certification Authority" to do the is-valid-for-EV check.

Is this behaviour due to "the issue with certs in the temp cert db" ?
Would Kai's patch "to do the EV evaluation in the auth-cert callback" change this behaviour, so that the same certificate chain is used in both places?

This is a relatively minor issue for me, but am I interested to know what your plans are.  Thanks.

Comment 6

[Kai Engert (:KaiE:)]

In reply to Rob:

Plan:
- fix this bug
- test again

I appreciate your attempts to fix early.
I attached the patch I used to PSM in bug 406755 comment 59, in case you want to do additional early testing! Thanks!

Comment 7

[Alexei Volkov]

(In reply to comment #4)
> (From update of attachment 311894 [details] [diff] [review])
> One changes the behavior of the output parameter cert_po_errorLog.  r?
> 
> I have a question about that errorLog change.  
> Q: Is it true that, in case the cert chain is valid and no error
> occurs, this "errorlog" parameter outputs the validated chain?

This is not true. errorlog outputs a list of error that occurred during validation, the errors that describe why processed certs have not made into
the final chain.

There is no way to control what gets into errorlog except that a caller can set it to null that outputs no errorlog at all.

Comment 8

[Alexei Volkov]

(In reply to comment #7)
> (In reply to comment #4)
> > (From update of attachment 311894 [details] [diff] [review] [details])
> > One changes the behavior of the output parameter cert_po_errorLog.  r?
> > 
> > I have a question about that errorLog change.  
> > Q: Is it true that, in case the cert chain is valid and no error
> > occurs, this "errorlog" parameter outputs the validated chain?

Sorry, didn't get to the point of the question from the first time :)

No, errorLog is not used to pass valid chains. We have another entry (cert_po_certList) in output array that is used for valid chains.
Please check attachment 311690 [details] [diff] [review].

Comment 9

[Alexei Volkov]

patch is integrated.

Comment 10

[Kai Engert (:KaiE:)]

Bob, Nelson, I just reread Rob's comment (from comment 5). He says, Alexei's patch already fixed the issue!

It appears it's no longer necessary to use a PSM patch to keep around certs in the temporary DB for a longer time?


Rob, sorry I misread your comment 5 earlier today.

(In reply to comment #5)
> With Alexei's patch + just the "COMODO Certification Authority" added locally
> to myTrustedEVInfos[], I notice that (for https://secure.comodo.com) the
> Firefox Certificate Viewer still shows the certificate chain up to "AddTrust
> External CA Root", even though PSM/NSS must be using the chain up to "COMODO
> Certification Authority" to do the is-valid-for-EV check.
> 
> Is this behaviour due to "the issue with certs in the temp cert db" ?

No. It's because we still use the classic NSS functionality to find the chain that will be presented in the UI.


> Would Kai's patch "to do the EV evaluation in the auth-cert callback" change
> this behaviour, so that the same certificate chain is used in both places?

No.
We'd have to change PSM to use a new API to give us the full chain.
Unfortunately, the functionality in NSS that would give out a full chain as a result of a policy verification has not yet been implemented.

Comment 11

[Kai Engert (:KaiE:)]

Alexei, thanks a lot for your patch and checking it in.
Should this bug be marked fixed?

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

I suspect that Rob now has the intermediate CA certs in his cert DB, and 
so no longer sees the problem.  I suspect that if the experiment is tried
again, with a clean/fresh cert DB, the problem will be reproduced.

The fundamental issue hasn't changed.  Certs sent by the server, but unused
in the non-pkix cert validation, will not be around after the SSL handshake
is completed, and hence will not be available to complete a cert chain
to a different root than the root used during the handshake, UNLESS they 
are also in the cert DB.  

Comment 13

[Rob Stradling]

(in reply to common #12)
> I suspect that Rob now has the intermediate CA certs in his cert DB, and
> so no longer sees the problem.

Nelson, I don't think this is the case.  At first, the Firefox Certificate Store Viewer doesn't show me any of the relevant Intermediate CA Certificates.  Neither does "nsscertutil -L -d ~/.mozilla/firefox/5k7nlhgc.default/".
Then I browse to https://secure.comodo.com and I see the EV UI.
After that, the AddTrustExternalCARoot->COMODOEVSGCCA Intermediate CA Certificate (which is *not* part of a trusted-for-EV chain) appears in the Certificate Store Viewer and in the output from nsscertutil.

I see no sign of the COMODOCertificationAuthority->COMODOEVSGCCA Intermediate CA Certificate (which *is* part of the only relevant trusted-for-EV chain in my local build) being added to the cert db.

Am I looking in the right place?

Comment 14

[Alexei Volkov]

(In reply to comment #12)
> The fundamental issue hasn't changed.  Certs sent by the server, but unused
> in the non-pkix cert validation, will not be around after the SSL handshake
> is completed, and hence will not be available to complete a cert chain
> to a different root than the root used during the handshake, UNLESS they 
> are also in the cert DB.  
> 
If cert chain was cached, all certs are still going to be in temporary database.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

As I understand Rob Stradling's explanation in bug 406755 comment 33, 
this server is now sending out one server cert followed by 3 different 
intermediate CA certs, each of which is a "cross issued" cert for the CA
that issued the server cert.  Each of those intermediate CA certs was 
issued by a different root.  Pictorially, the 4 certs (A, B, C, D) are:

    +-->B  (--> root Rb)
   /
A-+---->C  (--> root Rc)
   \ 
    +-->D  (--> root Rd)

During the first validation (non-libPKIX) we validate one path, say, 
A -> B -> Rb  
which is NOT a path to an EV Root.  The certs in that path are held in 
the "temp cert DB" by PSM, because PSM holds references to those certs.
So, when the handshake is over, certs A and B are kept, but certs C and
D are discarded.  

Then later, in the second validation (using libPKIX) for EV, we want to 
construct and validate a path to (say) root Rc, which is an EV root, 
but since cert C has been discarded and is no longer available, the path
from A to Rc can no longer be constructed, and so the EV validation fails.

One proposed solution is to cause the EV validation to take place at 
essentially the same time as the non-EV validation, that is, before the
handshake is over, and before any of the certs sent by the server are 
discarded.  

Now, it appears that the second (EV) validation is succeeding, despite the
fact that certs C and D were previously discarded.  I think the only possible 
explanations are:

1) For some reason not yet known, the first validation did use the path 
A->C->Rc, so those certs were not discarded, and the discarded certs 
were B and D.  This raises the question: why did that change?  Is that 
server now sending out a different group of certs than it sent when I 
investigated bug 406755?

2) Certs C and D were not discarded when they should have been, that is,
were leaked, a bug.

3) libPKIX fetched the previously-discarded cert C using the AIA URL.
That would be both good news and bad news.  Good: cert fetching via AIA works.
Bad: we're fetching a cert that we've had in our posession in the last few milliseconds. :(

4) Some other web page fetch validated the chain A->C->Rc causing it to be
cached in memory by the time this test was done.  

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

I see that the certificates received from the peer in the Certificate message
are not discarded until the SSL connection is over, and the socket is closed.
They are not discarded as soon as the handshake is done.  So, if a socket & 
connection stay open (keep alive) for a long time, their certs might stay 
in the temp cert DB for a long time.  But I think our EV logic should not 
depend on the server using keep-alive for the outer-most page connection.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

I declare this bug fixed. :)

The issue that we've been discussing in comment 12 - comment 16 above 
is really the subject of bug 406755, which I will reopen and add comments.