Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XAdESSigner creates certificate digests that fail to verify #246

Open
msetina opened this issue Mar 18, 2024 · 7 comments
Open

XAdESSigner creates certificate digests that fail to verify #246

msetina opened this issue Mar 18, 2024 · 7 comments

Comments

@msetina
Copy link
Contributor

msetina commented Mar 18, 2024

Adding 3 certificates to XAdESSigner cert parameter creates 3 X509Certificate nodes in X509Data and 3 xades:Certxades:CertDigest 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
Copy link
Contributor Author

msetina commented Mar 18, 2024

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
Copy link
Member

kislyuk commented Mar 18, 2024

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
Copy link
Contributor Author

msetina commented Mar 18, 2024

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

@msetina
Copy link
Contributor Author

msetina commented Mar 18, 2024

#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
Copy link
Contributor Author

msetina commented Mar 18, 2024

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
Copy link
Contributor Author

msetina commented Mar 18, 2024

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

  • xades-x-level-wrong-sigAndRefsTstVersion
  • xades-extended-epes
  • xades-with-multiple-signaturetimestamps

and they were not marked as raising error.

Can we ignore the missing CertDigest?

@msetina
Copy link
Contributor Author

msetina commented Mar 18, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants