XML-Security / signxml

Python XML Signature and XAdES library
https://xml-security.github.io/signxml/
Apache License 2.0
137 stars 107 forks source link

XAdESSigner creates certificate digests that fail to verify #246

Open msetina opened 6 months ago

msetina commented 6 months ago

Adding 3 certificates to XAdESSigner cert parameter creates 3 X509Certificate nodes in X509Data and 3 elements. This is not the problem. If this goes to XAdESVerifier if fails with: signxml.exceptions.InvalidDigest: Digest mismatch for certificate digest

Looking at the code, I think it does not match a certificate with its digest. _verify_cert_digest goes over a list and compares to one given in call parameter. In the given case this will always raise an exception of not matching.

The cert parameter to XAdESSigner delineates that it can accept multiple certs. The one that represents the signature and its CA chain. If I read the spec properly (https://www.w3.org/TR/xmldsig-core/#sec-X509Data). The certificate representing the signature should be in separate X509Data with detailed info from certificate, and CA chain certificates should be in separate X509Data with X509Certificate elementc. This would also eliminate the problem as there would be only one xades:Cert, the one for signing certificate, that would be checked against the data in separate X509Data element for signing certificate. The rest of CA chain would be ignored for comparing to CertDigest.

msetina commented 6 months ago

Looking at XADES spec I see there can also be digests for CA chain so writing part is ok. The verify should probably go by list index? _verify_cert_digests should expect multiple not just one as in self._find(x509_data, "X509Certificate").text

kislyuk commented 6 months ago

Hi, thanks for pointing this out. Your analysis sounds notionally correct, although I'll need to take a closer look at the spec to confirm. PRs are welcome, or I might be able to carve out some time to fix this on a weekend in the next few weeks.

msetina commented 6 months ago

I can fix verification so it works by list index for start. Is that OK?

msetina commented 6 months ago

247 fixes the erroneus signxml.exceptions.InvalidDigest: Digest mismatch for certificate digest by matching X509Certificate to xades:Cert by index.

Other things are more complex...

msetina commented 6 months ago

Test suite has some problems linked to problematic code. I added:

if b64decode(digest_value.text) != self._get_digest(
            der_encoded_cert, algorithm=digest_alg
        ):
            raise InvalidDigest("Digest mismatch for certificate digest")
        else:
            print("Digest {0} OK".format(idx))

and confirmed: test_xades_interop_examples (main.TestXAdES) ... Verifying /home/miha/signxml/signxml/./test/xades/nonconformant-X_BE_CONN_10.xml Digest 1 OK Digest 2 OK FAIL Previous code would raise InvalidDigest

msetina commented 6 months ago

By continuing test, I found test cases with 2 X509Certificate and only one xades:Cert.

and they were not marked as raising error.

Can we ignore the missing CertDigest?

msetina commented 6 months ago

Ignoring missing xades:Cert nodes and removing erroneus error_condition, alows test suite to finish OK.