[security-dev 00967]: Re: code review request 6852744: PIT b61: PKI test suite fails because self signed certificates are being rejected

[Xuelei Fan]

new webrev: http://cr.openjdk.java.net/~xuelei/6852744/webrev.01/

Sean Mullan wrote:
> Hi Andrew,
>
> Here are some comments -
>
> ForwardBuilder:
>
> line 864:
>
> typo: s/abchor/anchor
>
yes, a typo.
> In this block of code:
>
>  858                 if (principal != null && publicKey != null &&
>  859                         
> principal.equals(cert.getSubjectX500Principal())) {
>  860                     if (publicKey.equals(cert.getPublicKey())) {
>  861                         this.trustAnchor = anchor;
>  862                         return true;
>  863                     }
>  864                     // else, it is a self-issued certificate of 
> the abchor
>  865                 }
>
> you never check if the trust anchor name is equal to the issuer of the 
> cert before returning true. That seems to violate RFC 5280.
>
At line 859, when the cert's "subject" equals to the trust anchor 
principal, and the public key equals  at the same time(line 860), I 
think the cert is the trust anchor itself.

Add a comment line in the update:
-----------
   858                  if (principal != null && publicKey != null &&
   859                          
principal.equals(cert.getSubjectX500Principal())) {
   860                      if (publicKey.equals(cert.getPublicKey())) {
   861                          // the cert itself is a trust anchor
   862                          this.trustAnchor = anchor;
   863                          return true;
   864                      }
   865                      // else, it is a self-issued certificate of 
the anchor
   866                  }
------------
> lines 977-995
>
> RFC 5280 requires (MUST) that the KeyIdentifier be used in the 
> AuthorityKeyIdentifier extension of the CRL, so I don't think we need 
> to check the serial number/general names as those would be non-compliant.
In a informational RFC, RFC4158, it says(section 3.5.12):
--------------
If the current certificate expresses the issuer name and serial number 
in the AKID, certificates that match both these identifiers have highest 
priority.  Certificates that match only the issuer name in the AKID have 
medium priority.
--------------

A weak implement would only compare KeyIdentifier only, and ignore 
serial number/general names if KeyIdentifier presents. I would prefer a 
strict one in case of a AKID indicates a serial number, but we ignore it 
when building a path. I do believe it is possible that two certificate 
would have the same subject KeyIdentifier, and would have to be 
identified by serial number/general names. For example, for CA key 
update, it is possible that a pari of certificates, OldWithNew and 
OldWithOld, NewWithOld and NewWithNew, have the same subject 
KeyIdentifier, because their public keys are identical.

> I would remove this block. In any case I think the intention is that 
> there only be one form of identifier (key id OR issuer/serial) per 
> AKID/SKID extension, so you should only have to check one of them.
>
Yes, I think most of the practical certificate only has one form. But as 
a identification process, I think it would be better to be strict and 
check both if present, otherwise, it will be easy to get criticized that 
we don't check the other one.

Thanks,
Andrew
> I'm still looking over the tests but these are some comments so far.
>
> --Sean
>
>
> Xuelei Fan wrote:
>> Hi,
>>
>> bug description: 
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6852744
>> webrev: http://cr.openjdk.java.net/~xuelei/6852744/webrev/
>>
>> Evaluation of the bug:
>> 1. There is a loop of forward builder for self-issused intermediate 
>> certificates.
>>   The ForwardBuilder looks for the next certificate based on 
>> IssuerDN/SubjectDN. However, a self-issued certificate has the same 
>> IssuerDN and SubjectDN, the looking will loop on the self-issued 
>> certificate untill the loop detected.
>>
>> 2. Circular dependences
>>   In the PIT tests,  the valid of the intermediate CA certificate 
>> (oldCA) depends on the CRL; the valid of CRL depends on its issuer, 
>> the self-issued intermediate CA certificate (newWithOldCA); the valid 
>> of newWithOldCA depends on its issuer, the oldCA, here comes a dead 
>> loop.
>>
>> Thanks,
>> Xuelei
>