Bug in SigningCertificateVerifier.verify() function. Verification fails if certificate chain has more than 2 levels.

[GoogleCodeExporter]

In SigningCertificateVerifier.verify() function, the test done at the end :

        if (nMatchedRefs < certRefs.size())
            throw new SigningCertificateCertsNotInCertPathException();

seems wrong. It seems that 'nMatchedRefs' should be compared to 
'certChainData.getCertificateChain().size()' as follows :

        if (nMatchedRefs < certChainData.getCertificateChain().size())
            throw new SigningCertificateCertsNotInCertPathException();

The consequence of the bug is that the verification fails if we verify 
signatures with a trustore containing a lot of certificates.
Indeed, in this case, certRefs.size() is always bigger than nMatchedRefs.

This bug is in version 1.2.0, but also in the latest version 1.3.0.

Original issue reported on code.google.com by fchris...@gmail.com on 3 Dec 2012 at 2:50

[GoogleCodeExporter]

By 2 levels you mean a chain with leaf-interm-root or more than that? Because 
there's a test with a certificate chain with 3 certificates.

I'm going to review this anyway. Thanks.

Original comment by luis.fgoncalv on 4 Dec 2012 at 9:49

[GoogleCodeExporter]

The condition in the lib is correct. With the condition you suggested, if some 
of the certificates on the chain weren't reference in the SigningCertificate 
property, verification would fail. However, on section G.2.2.5, the XAdES spec 
clearly states:

"Should one or more certificates in the certification path not be referenced by 
this property, the verifier should assume that the verification is successful 
(...)"

The same section clearly states:

"Should this property contain one or more references to certificates other than 
those present in the certification path, the verifier should assume that a 
failure has occurred during the verification"

The test as is ensures that all the refs are matched. Maybe the test could be 
more assertive using "!=". Also, the reference checks could be done by 
iterating the references instead of the certificates on the chain...but either 
way the result is the same.

The code includes comments with this quotes from the spec on the corresponding 
lines. I'm marking this issue as invalid. If something is missing it can be 
reopened.

Original comment by luis.fgoncalv on 4 Dec 2012 at 10:51

• Changed state: Invalid

[GoogleCodeExporter]

Hi luis,

I'm sorry but I still think that the bug is valid.

In the 'verify' function you manage 2 lists : 
- certRefs : this list contains the reference certificates (the ones we trust). 
I assume that this list can contain a big number of certificates, but it must 
at least contain the certificates of the CA (certificate authorities) that 
generated the certificate used to sign the XML.
- certChainData : this list contains the certificates to check. This list 
contains the certificate used to sign the XML and its parent CA.

The objective of the 'verify' function is to check that the issuer of every 
certificate of 'certChainData' list can be found in the 'certRefs' list. In the 
function, you update a 'nMatchedRefs' counter that contains the number of 
certificates that matched this condition.

So, to validate the signature, we must check that all certificates of 
'certChainData' are valid, so we must check that 
'nMatchedRefs'='certChainData.size()'.

If you compare 'nMatchedRefs' to 'certRefs' size, signature will always fail if 
'certRefs' size is big (this is my case because my trustore contains 10 
certificates).

Original comment by fchris...@gmail.com on 5 Dec 2012 at 4:02

[GoogleCodeExporter]

My mistake,

I did not understand the code, my logic was wrong.
The bug is somewhere else ... in my own code!

Thanks for your quick answser Luis.

Original comment by fchris...@gmail.com on 5 Dec 2012 at 6:47

[GoogleCodeExporter]

No problem. The code is a bit tricky since it iterates the certificates instead 
of the references.

Original comment by luis.fgoncalv on 5 Dec 2012 at 11:06