Fix PKCS#7 verification of signature created with OpenSSL#10099
Open
haxtibal wants to merge 3 commits intowolfSSL:masterfrom
Open
Fix PKCS#7 verification of signature created with OpenSSL#10099haxtibal wants to merge 3 commits intowolfSSL:masterfrom
haxtibal wants to merge 3 commits intowolfSSL:masterfrom
Conversation
|
Can one of the admins verify this patch? |
haxtibal
force-pushed
the
tdmg/fix_digestinfo_rfc8017
branch
from
517efc3 to
55b85a6
Compare
Contributor
|
@haxtibal please resolve merge conflicts. For external contributions we need a signed contributor agreement. You can email support at wolfssl dot com to get that process started. Please reference the PR and tell us more about your project. Thanks, David Garske, wolfSSL |
haxtibal
force-pushed
the
tdmg/fix_digestinfo_rfc8017
branch
3 times, most recently
from
d8709e9 to
245d260
Compare
Author
@dgarske Thanks, done. Let me know if something's missing. |
Member
|
@haxtibal has been accepted as a contributor |
haxtibal
force-pushed
the
tdmg/fix_digestinfo_rfc8017
branch
from
245d260 to
7bae308
Compare
dgarske
reviewed
Apr 9, 2026
Contributor
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 3 total — 3 posted, 0 skipped
Posted findings
- [Low] Missing null-termination guarantee after XSTRNCPY in interop test —
tests/api/test_pkcs7.c:5057-5058 - [Medium] Signing-path DigestInfo still uses hashParamsAbsent — potential interop asymmetry —
wolfcrypt/src/pkcs7.c:2505-2507 - [Info] Test helper comment says 'WolfSSL' instead of 'wolfSSL' —
tests/api/test_pkcs7.c:5044
Review generated by Skoll via openclaw
Intention is to verify PKCS#7 / CMS signatures produced by third party tools. The optional test is enabled by providing a comma separated list of external DER files that have previously been signed with a third party tool to configure --with-pkcs7-test-signed-data. Usage: openssl cms -sign -in <(echo -n "openssl cms interop test message") \ -signer certs/client-cert.pem -inkey certs/client-key.pem \ -md sha256 -nodetach -outform DER -out /tmp/cms.der openssl smime -sign -in <(echo -n "openssl smime interop test message") \ -signer certs/client-cert.pem -inkey certs/client-key.pem \ -md sha256 -binary -nodetach -outform DER -out /tmp/smime.der ./configure --enable-pkcs7 --enable-certext --enable-examples \ --with-pkcs7-test-signed-data=/tmp/cms.der,/tmp/smime.der make -j$(nproc) tests/unit.test -test_wc_PKCS7_VerifySignedData_interop
…rameters = NULL RFC 8017 hardcodes DER serialization samples of DigestInfo, where the parameter part is always NULL (05 00) for known hash algorithm [1]. This value does thus *not* depend on SignerInfo.digestAlgorithm.parameters. Starting with 75c3030 ("Add option for absent hash params in PKCS7"), wolfSSL wrongly assumed and implemented such a dependency. This non-conformance caused an interoperability bug with OpenSSL: A signature created with openssl cms could not be verified in wolfSSL. OpenSSL correctly leaves SignerInfo.digestAlgorithm.parameters absent and adds explicit NULL to DigestInfo. wolfSSL saw the absence and wrongly inferred DigestInfo would also have no explicit NULL - but it has - leading to size mismatch. Fix it by constructing the expected DigestInfo always with NULL (05 00). 4f21117 ("tests: Add PKCS#7 verification interoperability test") and 8d8170e (".github: Test PKCS7 interoperability for OpenSSL and GnuTLS") can be used to reproduce the bug and to demonstrate this commit fixes it. [1] https://www.rfc-editor.org/rfc/rfc8017#section-9.2
haxtibal
force-pushed
the
tdmg/fix_digestinfo_rfc8017
branch
from
7bae308 to
4ec4c5e
Compare
Author
|
Regarding recently failed Test interop with mbedtls and Test interop with nss: I don't know what the message |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
RFC 8017 hardcodes DER serialization samples of DigestInfo, where the parameter part is always NULL (05 00) for known hash algorithm. This value does thus not depend on SignerInfo.digestAlgorithm.parameters. Starting with 75c3030, wolfSSL wrongly assumed and implemented such a dependency.
This non-conformance caused an interoperability bug with OpenSSL: A signature created with
openssl cmscould not be verified in WolfSSL. OpenSSL correctly leaves SignerInfo.digestAlgorithm.parameters absent and adds explicit NULL to DigestInfo. WolfSSL saw the absence and wrongly inferred DigestInfo would also have no explicit NULL - but it has - leading to size mismatch.Fix it by constructing the expected DigestInfo always with NULL (05 00).
The PR further adds an optional unit test to verify externally provided DER signature samples. A new CI job uses this to verify signatures created with OpenSSL and GnuTLS.
Testing
This was originally found, discussed and tested at Debian via a swugenerator autopkgtest. Pipelines in my wolfSSL fork run the newly added tests:
Checklist