524013 - CERT_PKIXVerifyCert reports SEC_ERROR_BAD_SIGNATURE for https://ecommerce.amga.org/iMISPublic/

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

This bug is originally reported in http://crbug.com/25059.

The server at https://ecommerce.amga.org/iMISPublic/ sends only the
leaf certificate, without the intermediate CA certificate.

CERT_VerifyCert reports the expected SEC_ERROR_UNKNOWN_ISSUER error.

CERT_PKIXVerifyCert reports SEC_ERROR_BAD_SIGNATURE.

Steps to reproduce:

1. Use 'vfyserv' to get the server certificate.

$ vfyserv -c ecommerce.amga.org  
Connecting to host ecommerce.amga.org (addr 63.119.96.6) on port 443
Cert file cert.000 was created.
PROBLEM WITH THE CERT CHAIN:
CERT 0. CN=ecommerce.amga.org,OU=Secure Link SSL,OU=AMGA,O=American Medical Group Association,OID.2.5.4.9=1422 Duke St,L=Alexandria,ST=VA,postalCode=22314-3403,C=US :
  ERROR -8179: Peer's Certificate issuer is not recognized.
    CN=Network Solutions Certificate Authority,O=Network Solutions L.L.C.,C=US
Error in function PR_Write: -8179
 - Peer's Certificate issuer is not recognized.

2. Use 'vfychain', with and without the -p option, to verify the
certificate.  Observe that the error codes are different.

$ vfychain cert.000 
Chain is bad, -8179 = Peer's Certificate issuer is not recognized.
PROBLEM WITH THE CERT CHAIN:
CERT 0. CN=ecommerce.amga.org,OU=Secure Link SSL,OU=AMGA,O=American Medical Group Association,OID.2.5.4.9=1422 Duke St,L=Alexandria,ST=VA,postalCode=22314-3403,C=US :
  ERROR -8179: Peer's Certificate issuer is not recognized.
    CN=Network Solutions Certificate Authority,O=Network Solutions L.L.C.,C=US
$ vfychain -p cert.000 
Chain is bad, -8182 = Peer's certificate has an invalid signature.
PROBLEM WITH THE CERT CHAIN:
CERT 1. Builtin Object Token:Network Solutions Certificate Authority [Certificate Authority]:
  ERROR -8182: Peer's certificate has an invalid signature.
$ vfychain -p -p cert.000 
Chain is bad, -8182 = Peer's certificate has an invalid signature.
PROBLEM WITH THE CERT CHAIN:
CERT 1. Builtin Object Token:Network Solutions Certificate Authority [Certificate Authority]:
  ERROR -8182: Peer's certificate has an invalid signature.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I'm not sure this is a valid bug.
CERT_PKIXVerifyCert is a new function, and I don't believe its definition
says that it must always give the same error code as the old library in 
all cases.  

However, as you know, the old library API can now be made to do its job by 
calling libPKIX, and it IS a goal of that API to retain the same error codes
as much as possible.  So, if this bug were to be changed to report that the 
vfychain command produces different error codes when used with the -p option 
than when used without the -p option, then it would definitely be valid, IMO.

Comment 2

[Wan-Teh Chang]

I edited the summary of the bug and the "regression" keyword
to avoid the comparison with CERT_VerifyCert.  The bug is that
CERT_PKIXVerifyCert reports SEC_ERROR_BAD_SIGNATURE on a
certificate chain of only one non-self-signed certificate.
Without the public key of the issuer, how can CERT_PKIXVerifyCert
verify the signature on the certificate?  Clearly
CERT_PKIXVerifyCert does something wrong.  Another evidence is
that Windows CryptoAPI can verify that certificate (presumably
it downloads the intermediate CA certificate using the URL in
the certificate's AIA extension) and reports that the certificate
is good.

Comment 3

[Wan-Teh Chang]

I've confirmed that this is a libpkix bug, caused by
a trusted root CA certificate with the same Subject
field as the Subject field of the missing intermediate
CA certificate.

Using IE or Google Chrome on Windows, we get the
complete certificate chain:

  USERTrust
  |
  +-- Network Solutions Certificate Authority
      |
      +-- ecommerce.amga.org

The Subject field of the intermediate CA certificate
is:

  CN = Network Solutions Certificate Authority
  O = Network Solutions L.L.C.
  C = US

Our libnssckbi.so has a root CA certificate with the
same Subject field.  If I remove that root CA certificate
from certdata.txt and rebuild libnssckbi.so, then
CERT_PKIXVerifyCert reports the expected
SEC_ERROR_UNKNOWN_ISSUER.

I guess libpkix must have used the root CA certificate's
public key to verify the signature on the server
certificate, which is why it gets SEC_ERROR_BAD_SIGNATURE.
libpkix should check both the Subject field and Subject
Key Identifier extension of a CA certificate before using
its public key to verify a signature.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

This is another consequence of libPKIX ignoring the pairing of Authority 
Key ID (AKID) in the issued cert with the Subject Key ID (SKID) in the 
issuer cert when building cert chains.  This has been previously reported 
in bug 489714 comment 2 where it was seen to cause loops.

Alexei, I agree that this is a serious bug.  Since Sun is not using libPKIX, 
I suppose Sun would consider this work to be lower priority than the server 
side SNI work you're doing, but products that use libPKIX would certainly 
consider this bug higher priority than any RFE, IMO.

Comment 5

[Wan-Teh Chang]

I'm going to work around this bug by passing cert_pi_useAIACertFetch
to CERT_PKIXVerifyCert.

Firefox still isn't fetching certs using the AIA extension.  Is
that feature not available in CERT_VerifyCert?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, 
For reasons of backwards binary compatibility, CERT_VerifyCert does not 
fetch certs using the AIA extension's URIs by default.  
In theory, it can be made to do so in two steps, by 
1) calling the function that switches CERT_VerifyCert, so that CERT_VerifyCert will call libPKIX to do the chain building and verification, and 
2) calling the function that sets the default behavior for libPKIX, so that
fetching certs from AIA URIs is enabled by default in libPKIX.  

However, I'm not certain that the latter of those two functions has actually 
been implemented.

Comment 7

[Wan-Teh Chang]

I ran into this bug again with another cert issued by Network Solutions
Certificate Authority.

This bug is more serious in NSS 3.12.4+ if you enable CRL DP because
libpkix incorrectly uses the root CA cert to verify the signature
of the CL, and with Julien's strict policy of considering invalid
CRls as revoking all certs, libpkix reports the cert is revoked:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/crl.c&rev=1.69&mark=1827-1833#1816

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/crl.c&rev=1.69&mark=2706#2697

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/crl.c&rev=1.69&mark=2796,2799#2796

We should consider changing the dpcacheInvalidCacheError case in
cert_CheckCertRevocationStatus to set 'status' to
certRevocationStatusUnknown, allowing the caller to decide based on
its policy, but the root cause is this bug, because the CL is actually
valid.

Comment 8

[Wan-Teh Chang]

Attachment: Proposed patch

Match not only the issuer name but also auth key id.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

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

Wan-Teh, 
This is a patch review comment.  
I think the solution you seek involves a LOT more work than this patch.

As you may recall, in the non-PKIX cert lib, we have a rather complicated
algorithm that gathers a set of potential matches (based on match by subject name), and searches them all for a match on subject Key ID, but if NONE 
matches, and some of them have no subject key ID extension, then it will 
find a match from one of those.  So, it won't fail simply because NO issuer
has the expected explicit subject KEY ID.  I believe lots of certs in the 
Internet depend on that logic.  

So, while your patch will have the effect of ensuring that only issuers with
matching SKIDs are chosen, I think that's not what you really want in all 
cases.

Looking at the libPKIX logic, in function pkix_CertSelector_Match_SubjKeyId
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/certsel/pkix_certselector.c&rev=1.11&mark=848,872-875#848
and its caller, it appears that if the potential issuer has no SKID, it is
simply not a match, and I see no logic that takes a second pass ignoring 
the AKID/SKID match.

In short, I think the AKID/AKID matching logic in libPKIX is too simplistic 
at present.  Sadly, AKID/SKID matching has been defined in a way that's absurdly complicated, and that just isn't reflected in libPKIX yet.

Comment 10

[Wan-Teh Chang]

Attachment: Proposed patch v2 (checked in)

Do not report a SKID mismatch if the potential issuer has no SKID.

Nelson, thank you very much for your comment.  This patch addresses the
issue you pointed out.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 440936 [details] [diff] [review]
Proposed patch v2 (checked in)

I'm going to give this r+ because it's more correct than the present day behavior of libPKIX, and maybe that's good enough for now.  But IMO, it's still not completely right.  With this patch, I think it would still be possible to have a situation in which you had two certs, one with no SKID and one with a matching SKID, and end up picking the one with no SKID rather than the one with the matching SKID.  Our old non-libPKIX code would always pick the one with the matching SKID in that case.  I'll leave it up to you, Wan-Teh, if you want to spend more time to achieve greater perfection.

Comment 12

[Wan-Teh Chang]

Comment on attachment 440936 [details] [diff] [review]
Proposed patch v2 (checked in)

I checked in the patch on the NSS trunk (NSS 3.12.7).

Checking in lib/libpkix/include/pkix_errorstrings.h;
/cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h,v  <--  pkix_errorstrings.h
new revision: 1.34; previous revision: 1.33
done
Checking in lib/libpkix/pkix/certsel/pkix_certselector.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/certsel/pkix_certselector.c,v  <--  pkix_certselector.c
new revision: 1.12; previous revision: 1.11
done
Checking in lib/libpkix/pkix/top/pkix_build.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/top/pkix_build.c,v  <--  pkix_build.c
new revision: 1.59; previous revision: 1.58
done

Comment 13

[Wan-Teh Chang]

I looked into what what Nelson suggested in comment 11, but didn't
do it for two reasons.

1. I don't have a test case with that property (two candidate
certificates, one with no SKID and one with a matching SKID) to
test with.

2. The change is not simple.  The following are my notes for those
who want to tackle this.

I found at least one code path that needs changing:

#0  pkix_CertSelector_Match_SubjKeyId (params=0x7fffeba60568, 
    cert=0x7fffed2eba28, pResult=0x45911414, plContext=0x7fffeba24c30)
    at pkix_certselector.c:869
#1  0x00007ffff7ebb24c in pkix_CertSelector_DefaultMatch (
    selector=0x7fffed0c8628, cert=0x7fffed2eba28, plContext=0x7fffeba24c30)
    at pkix_certselector.c:1345
#2  0x00007ffff7f5ddf8 in pkix_pl_Pk11CertStore_GetCert (
    store=0x7fffeba1ed48, selector=0x7fffed0c8628, 
    parentVerifyNode=0x7fffed101c08, pNBIOContext=0x459115a8, 
    pCertList=0x459115b8, plContext=0x7fffeba24c30)
    at pkix_pl_pk11certstore.c:657

We need to change all three functions as follows.

1. pkix_CertSelector_Match_SubjKeyId should return an error if
'params' has an expected SKID but 'cert' has no SKID.  This error
should have a new error code, say PKIX_CERTSELECTORMATCHSUBJKEYIDUNKNOWN,
that is different from the error code for an SKID mismatch
(PKIX_CERTSELECTORMATCHSUBJKEYIDFAILED).

2. pkix_CertSelector_DefaultMatch should not return an error
immediately if the pkix_CertSelector_Match_SubjKeyId call returns
PKIX_CERTSELECTORMATCHSUBJKEYIDUNKNOWN.  Instead, it should check
the rest of the criteria, and return PKIX_CERTSELECTORMATCHSUBJKEYIDUNKNOWN
if everything else matches.  It can do this easily by calling
pkix_CertSelector_Match_SubjKeyId last, right before the comment
"if we reach here, the cert has successfully matched criteria":

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/certsel/pkix_certselector.c&rev=1.11&mark=1345,1361#1345

3. Finally, pkix_pl_Pk11CertStore_GetCert should remember any 'candidate'
for which selectorCallback returns PKIX_CERTSELECTORMATCHSUBJKEYIDUNKNOWN,
and add this 'candidate' to the 'filtered' list only if we finish the for
loop without any real match (a null selectorError):

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_pk11certstore.c&rev=1.18&mark=657-659,670,672#645

Comment 14

[Wan-Teh Chang]

Attachment: Incomplete patch to illustrate what I outlined in comment 13

This patch is for future reference only.

This patch makes concrete the first two of the three changes I
described at the end of the previous comment.

Comment 15

[Robert Relyea]

Comment on attachment 440936 [details] [diff] [review]
Proposed patch v2 (checked in)

This looks fine in my cursory review, and already has an R+ from nelson and is checked in, so I'm cancelling the review.

Note: this issue is an error code issue, if there were a valid path, it would have taken that path in any case. The larger question is which error code is the right one to return when you have multiple paths, each with different error codes.

bob