384459 - Certification path validation fails when "Authority Key Identifier" extension contains key identifier, serial and issuer name

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Jeandré]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20061201 Firefox/2.0.0.4 (Ubuntu-feisty)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20061201 Firefox/2.0.0.4 (Ubuntu-feisty)

Certification path validation fails when "Authority Key Identifier" extension contains key identifier, serial and issuer name

When the AuthorityKeyIdentifier extension is available containing not just the keyIdentifier, but also the authorityCertIssuer and authorityCertSerialNumber, the certification path validation seems not to agree with the expected behaviour in RCF3280. 

 id-ce-authorityKeyIdentifier OBJECT IDENTIFIER ::=  { id-ce 35 }

   AuthorityKeyIdentifier ::= SEQUENCE {
      keyIdentifier             [0] KeyIdentifier           OPTIONAL,
      authorityCertIssuer       [1] GeneralNames            OPTIONAL,
      authorityCertSerialNumber [2] CertificateSerialNumber OPTIONAL  }

   KeyIdentifier ::= OCTET STRING

According to the RFC3280,
  "The value of the subject key identifier
   MUST be the value placed in the key identifier field of the Authority
   Key Identifier extension (section 4.2.1.1) of certificates issued by
   the subject of this certificate."
but the other fields is supposed to be additional, just to help in the certification path construction.

In Firefox, if I have a chain of trusted authorities and the root auhtority certificate is, for an example, updated, those certificates that have the basic AuthorityKeyIdentifier (containing just the keyIdentifier) will recognize the new certificate as the trusted point, while certificates containing a complete AuthorityKeyIdentifier won't, because the authorityCertSerialNumber points to another serial number and maybe a different issuer.

The white paper "Understanding Certification Path Construction" (http://www.oasis-pki.org/pdfs/Understanding_Path_construction-DS2.pdf) better explains this situation.

The correct behaviour, according to the above sources, would be a valid path even when the authorityCertIssuer and authorityCertSerialNumber point to values that doesn't match the issuer's certificate ones. The only restriction would be the keyIdentifier match.

The tests was made also in Internet Explorer and worked as expected, so that's why I'm reporting te issue.

Reproducible: Always

Steps to Reproduce:
1. Issue an Root CA Certificate (self-signed).
2. Issue Certificates for intermediate CAs signed by the above CA with the AuthorityKeyIdentifier with all fields filled.
3. Import the certificates to firefox.
4. Test the certification paths of the intermediate CAs. They would have the Root CA like trust anchor.
5. Renew Root CA certificate (new certificate using the same key pair).
6. Replace the old root ca certificate with the new one in firefox. 
7. Verify those paths again and now they are no more valid.
Actual Results:  
The end entities an intermediates CAs below the CA whose the certificate was updated doesn't recognize the new certificate.

Expected Results:  
The Certificates should recognize the new certificate as trust anchor, once it has  the same keyIdentifier information that the old certificate.

Comment 1

[Jeandré]

Sorry about the poor english :)

Comment 2

[Daniel Veditz [:dveditz]]

You have no reason to be defensive about your English!

If this bug only causes perfectly good certs to appear invalid then I don't think this is an exploitable security problem that needs to be protected with the confidential flag. (Accepting bad certs as valid would be another thing entirely, of course.)

I suspect this is NSS code, but I'll route it through PSM in case it's a security/manager issue. Kai will know.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

This is NSS code.  It is intentional.  I think it is a duplicate bug. (This is
not the first complaint about this.)

The idea that an AKID has to have an issuer and serial number to be "complete"
appears to be the result of a lot of "how to" documents on using OpenSSL to 
create certificates.  AKIDs in certificates created by OpenSSL always have 
both the issuer's subject key ID *AND* the (issuer+serial number).
But RFC 3280 doesn't even HINT that that is expected or desired.  Nowhere 
does RFC 3280 say "you can ignore the other optional parts of the AKID".

The idea that the AKID's (issuer+serial number) is somehow useful or helpful 
if we must ignore it is ... not very logical.  The issuer and serial number 
exists to select ONE issuer cert from MANY CA certs with the same subject name.
It is there to say "only pick the CA cert whose issuer and serial number are
these values".  And that's what NSS does with it.  This is intentional.  

The entire purpose of having the issuer name and serial number is to limit
selection of issuer certs.  If you don't want cert path construction to 
limit the path to only use the cert with the specified issuer name and serial number, then what reason have you to specify those values in an AKID?

If it's "to make the AKID complete" then understand that filling in all the
optional parts of an AKID, unnecessarily, is neither required or desirable.

Comment 4

[Marcelo Carlomagno Carlos]

Nelson,

The paper referenced in this ticket was written by Steve Lloyd (author of the book Understang PKI) and have Russ Housley (who writes the RFC 3280) as contributor. At page 5, we can see the following: 

"The keyIdentifier form can be used to select CA certificates during path contruction. The AuthorityCertIssuer, authorityCertSerialNumber pair can only be used to provide preference to one certificate over other during path construction".
                                                                                   So, we don't expect to ignore the serial number and the issuer (actually, it's not the issuer, but the issuer of the issuer), but even if this 2 values doesn't match, it doesn't mean there is no valid paths to contruct. The only value that is mandatory to match is the keyIdentifier. The serial and the issuer exist only to provide preference, but not to deny the validation.

I also suggest to you make this test using Internet Explorer that in this case works perfectly.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

That white paper is not authoritative. 
IE is HARDLY a definition of correct behavior!
The NSS team has been working with X.509 and the relevant RFCs for over 13
years now.  We're hardly newcomers to this area.  Please don't address us 
as though we are newcomers.

The present code is working as designed and intended.  
However, it is all being rewritten for NSS 3.12 to support many newer features, 
such as policy extensions and building of cert paths by fetching missing CA 
certs via LDAP or HTTP.  This behavior may change when that happens.  

Comment 6

[Marcelo Carlomagno Carlos]

I think we have a misunderstand here.
I don't call you or anyone from NSS as newcomers. I'm totally sure about your quality and knowledgement. And actually i'm a firefox fan. That's why i'm here discussing to help to make it better and better.
And I agree with you that IE wasn't a good paramenter, but in this case, its implementation works according to the literature (RFCs and papers)
I'm just telling you to see the example of validation that is done in IE and what exist in literature to make a comparisson.

Comment 7

[Julien Pierre]

It seems the white paper quotes X.509 about the meaning of AKID authorityCertIssuer and authorityCertSerialNumber . I don't have a copy of that spec to look at, unfortunatealy. It would be interesting to see the context and how they exactly define "preference". Somehow, I don't think the information present in the cert is supposed to be ignored. Certainly the signatures would match if the keyIdentifier alone matches, but the resulting path constructed may still not be the one expected, it seems to me.

Comment 8

[Marcelo Carlomagno Carlos]

I will post here the link to the ITU-T Recomendation:
http://www.itu.int/rec/T-REC-X.509-200508-I

I think that should be read by all that are involved with this thread, because it can solve all of the doubts about of the problem.

I still think that is a Firefox Bug. I studied it for a long time, and I found  and read many references about that. For me, and for many other researchers (with M.Sc. and PHD degree) with I discussed the problem have agreed with me and Jeandré.

So, I will ask you all to read the reference and after that, come back here and discuss the problem. Please don't take hasty conclusions.

I don't want to attack anybody here, actually, is the opposite of that. I want to help to solve the problem. In the other post that i made here, some people made a wrong interpretation of that I said. But if anyone here read the post again, will have sure that I was only trying to help.

So, lets together evaluate and solve this problem.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: absolute minimal patch

I am convinced of two things about the issuer+serial number part of the AKID 
extension:  
1. If it's not mandatory then it is utterly useless.
2. We waste FAR FAR too much time on it, for the (non)value it offers.

So, I am attaching 3 patches here, that address this in various ways.
1. An absolute minimal patch
2. A cleanup patch that has the same effect but makes the code easier 
to follow
3. A patch that gets rid of the issuer+serial check altogether.  

Bob, I'll let you pick one of these.  Vote with your review.
Wan-Teh, I'd appreciate your vote, too.
I'd ask Julien, but he's out for two more weeks.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: cleanup patch (wtc: resurrect 'return', or update the block comment before the final 'return' of the function)

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to Rid us of this nonsense (wtc: the block comment before the final 'return' of the function needs to be removed or updated)

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: remove sanity check from extension pretty printer

This patch can be evaluated separately from the preceding three.
It simply removes a sanity check that prints a warning from the 
pretty printing code for this extension.

Comment 13

[Robert Relyea]

Comment on attachment 327914 [details] [diff] [review]
cleanup patch (wtc: resurrect 'return', or update the block comment before the final 'return' of the function)

r+ relyea

(I vote for this one;).

bob

Comment 14

[Robert Relyea]

Comment on attachment 327916 [details] [diff] [review]
remove sanity check from extension pretty printer

r+ (or we should print a warning about how it's a bad idea to use issuer/serial number key authority id's period).

bob

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 327914 [details] [diff] [review]
cleanup patch (wtc: resurrect 'return', or update the block comment before the final 'return' of the function)

Thanks, Bob.
Now I'll ask Wan-Teh to SR for the 3.11 branch, as we discussed in this morning's meeting.

Comment 16

[Wan-Teh Chang]

Comment on attachment 327914 [details] [diff] [review]
cleanup patch (wtc: resurrect 'return', or update the block comment before the final 'return' of the function)

>+	} else {
>+	    /* exit immediately on failure */
>+	    return nssCertIDMatch_No;
>+	}

Nelson, are you sure you don't want the 'return' here?
Your absolute minimal patch has the 'return'.

Comment 17

[Wan-Teh Chang]

Comment on attachment 327914 [details] [diff] [review]
cleanup patch (wtc: resurrect 'return', or update the block comment before the final 'return' of the function)

Please ignore my previous comment.  Here is the comment
again with the correct code snippet.

> 	} else {
>-	    /* exit immediately on failure */
>-	    return nssCertIDMatch_No;
>+	    match = nssCertIDMatch_Unknown;
> 	}

Nelson, are you sure you don't want the 'return' here?
Your absolute minimal patch has the 'return'.

Comment 18

[Wan-Teh Chang]

Comment on attachment 327914 [details] [diff] [review]
cleanup patch (wtc: resurrect 'return', or update the block comment before the final 'return' of the function)

r=wtc.

>-	    /* exit immediately on failure */
>-	    return nssCertIDMatch_No;
>+	    match = nssCertIDMatch_Unknown;

This 'return' should be added back.  Otherwise,
the block comment after it needs to be updated,
because there is now another condition for the
state to be Unknown at that point.

Comment 19

[Wan-Teh Chang]

Comment on attachment 327915 [details] [diff] [review]
patch to Rid us of this nonsense (wtc: the block comment before the final 'return' of the function needs to be removed or updated)

r=wtc.

Same issue here: with this patch, the block comment
at the end of the function is no longer true, so it
needs to be removed or updated.

Comment 20

[Wan-Teh Chang]

Comment on attachment 327913 [details] [diff] [review]
absolute minimal patch

r=wtc.

> 	    /* exit immediately on failure */
>-	    return nssCertIDMatch_No;
>+	    return nssCertIDMatch_Unknown;

I know Unknown is better than No, but I don't know
the difference between Unknown and Yes here.

Comment 21

[Wan-Teh Chang]

Comment on attachment 327916 [details] [diff] [review]
remove sanity check from extension pretty printer

r=wtc.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Checking in cmd/lib/secutil.c;  new revision: 1.91; previous revision: 1.90
Checking in lib/pki/pki3hack.c; new revision: 1.95; previous revision: 1.94

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

In answer to Wan-Teh's question in comment 20,
The answer Yes stops the search, declaring it has found the desired cert.
The answer Unknown causes the search to continue (as does the answer No),
but it remembers the cert whose answer was Unknown.  If the search subsequently
finds a cert with a Yes answer, then the search stops there and uses that cert,
but if a Yes match is not found, then the Unknown match gets finally treated 
like a match, as if it had been yes.  

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

lib/pki/pki3hack.c; new revision: 1.86.28.6; previous revision: 1.86.28.5
cmd/lib/secutil.c;  new revision: 1.71.2.4;  previous revision: 1.71.2.3

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

This fix first appeared in these Mozilla product releases:
- FireFox     3.0.0.0
- Thunderbird 2.0.0.21
- SeaMonkey   1.1.15
- Camino      1.6.7

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 327915 [details] [diff] [review]
patch to Rid us of this nonsense (wtc: the block comment before the final 'return' of the function needs to be removed or updated)

We ultimately decided not to commit this patch.