Fix two PKCS#7/CMS Go-interop gaps: SignedData DigestInfo verify + EnvelopedData definite [0] decrypt#10928
Open
Frauschi wants to merge 4 commits into
Open
Fix two PKCS#7/CMS Go-interop gaps: SignedData DigestInfo verify + EnvelopedData definite [0] decrypt#10928Frauschi wants to merge 4 commits into
Frauschi wants to merge 4 commits into
Conversation
|
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10928
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
…rams wc_PKCS7_VerifySignedData() returned SIG_VERIFY_E for a cryptographically valid RSA SignedData whose SignerInfo digestAlgorithm parameter encoding (NULL present vs absent, a CMS field per RFC 5652/5754) differs from the parameter encoding of the AlgorithmIdentifier inside the PKCS#1 v1.5 DigestInfo that the signature actually covers (RFC 8017). The two encodings are independent, but the RSA verify rebuilt its comparison DigestInfo mirroring only the SignerInfo digestAlgorithm, so it never byte-matched the recovered DigestInfo. Go's crypto/rsa (micromdm/scep and other Go CMS/SCEP stacks) omits the SignerInfo NULL while signing a NULL-present DigestInfo, which triggers this. wc_PKCS7_BuildSignedDataDigest() now takes an explicit hashParamsAbsent argument, and the RSA verify path tries both DigestInfo encodings before failing: as-parsed, plain digest, then the flipped parameter encoding. This keeps wolfSSL's encode-and-compare approach (no parsing of attacker-controlled recovered bytes) and handles both mismatch directions. Add test_wc_PKCS7_VerifySignedData_NoDigestParams, which builds the mismatch entirely from wolfSSL's own signer (sign the same content twice, NULL-absent and NULL-present, then splice one signature onto the other message) and verifies all four direction/attribute combinations plus a tampered-signature negative control.
Frauschi
commented
Jul 16, 2026
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10928
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
wc_PKCS7_DecodeEnvelopedData() failed to decrypt a valid CMS/SCEP EnvelopedData whose encryptedContent is a definite-length constructed context [0] wrapping a single definite-length OCTET STRING (A0 82 xx xx 04 82 yy yy <ciphertext>). This is the encoding emitted by Go's crypto/pkcs7 stacks (e.g. micromdm/scep), so SCEP enrollment against those servers failed at the CertRep decrypt step. The decoder recognized the constructed [0] tag but its handler assumed the BER indefinite-length fragmented form: a run of OCTET STRINGs terminated by an EOC (00 00). The definite-length single-OCTET-STRING form has no EOC, so after decrypting the one OCTET STRING the loop kept scanning for a terminator that never comes. Streaming builds returned WC_PKCS7_WANT_READ_E; NO_PKCS7_STREAM builds spun. The primitive (80 ...) and constructed-indefinite (A0 80 ... 00 00) encodings were already handled. Right after reading the [0] length, detect the case where the [0] definite length is filled exactly by a single inner OCTET STRING, skip that inner header, and fall into the existing primitive single-shot decrypt path. The check runs before the NO_PKCS7_STREAM bounds check so both streaming and non-streaming builds are corrected, and the inner length is read with NO_USER_CHECK because the ciphertext may not be fully buffered yet in streaming mode; the size-equality test validates the structure instead. That equality is computed in word32 to avoid signed-overflow undefined behavior on a crafted oversized inner length. The unwrap only fires for the constructed-definite single-OCTET-STRING shape; primitive and indefinite encodings are untouched. Add test_wc_PKCS7_DecodeEnvelopedData_constructedDefiniteOctet (tests/api/test_pkcs7.c). wolfSSL's encoder never emits this form, so the test transcodes a normal encode into it. It covers the positive case plus three negatives that pin the unwrap guard: two OCTET STRINGs, a [0] longer than the inner OCTET STRING, and a non-OCTET-STRING inner.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10928
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
wc_PKCS7_Free() released cachedEncryptedContent but never released pkcs7->decryptKey, the content-cipher context (Aes/Des/Des3) allocated by wc_PKCS7_DecryptContentInit() on the streaming EnvelopedData decode path. On WC_PKCS7_WANT_READ_E the decode deliberately keeps that context to resume on the next input chunk, so a decode abandoned mid-message (e.g. a truncated or malformed streaming message the caller gives up on) and then freed leaked the cipher context. wc_PKCS7_DecodeEnvelopedData()'s own error cleanup only runs for hard errors (ret < 0 && ret != WC_PKCS7_WANT_READ_E), so it does not cover the abandoned-WANT_READ case; the release has to happen in wc_PKCS7_Free(). Before tearing down the stream, and only when a decrypt context is still pending (decryptKey non-NULL), retrieve the content cipher OID from the stream's saved var and call wc_PKCS7_DecryptContentFree(). decryptKey is only ever left allocated by this flow, where that saved var holds the content cipher OID; gating on it leaves completed decodes and other streaming flows (whose saved var is unrelated to any cipher) untouched, and avoids a spurious "Unsupported content cipher type" debug message on their teardown. Exposed by the new constructed-definite [0] negative tests, which decode a malformed message (returning WC_PKCS7_WANT_READ_E) and then free the wc_PKCS7 -- flagged by LeakSanitizer in CI.
The fragmented-OCTET-STRING loop in wc_PKCS7_DecodeEnvelopedData() only breaks on error under !NO_PKCS7_STREAM; the non-streaming build has no error exit and relies solely on finding an indefinite EOC (00 00). A constructed definite-length [0] whose inner OCTET STRING(s) are consumed without an EOC terminator (e.g. a definite [0] wrapping multiple OCTET STRINGs, or a single OCTET STRING followed by trailing bytes) leaves the scan running past the end of the buffer: GetASNTag keeps failing, ret is set but the loop never breaks, idx stops advancing, and it spins forever -- a hang / denial of service on a NO_PKCS7_STREAM decoder fed a malformed message. Add the equivalent non-streaming error exit: after the EOC check (so a valid indefinite message still terminates on its EOC first) break when ret is non-zero. Streaming behavior is unchanged -- the break is guarded by #ifdef NO_PKCS7_STREAM. Exposed by test_wc_PKCS7_DecodeEnvelopedData_constructedDefiniteOctet: its negative cases decode exactly these malformed shapes and hung the NO_PKCS7_STREAM CI configs (make check timed out and was cancelled) until this fix. The single definite-length OCTET STRING form that the decoder now unwraps to the primitive path was already handled and is unaffected.
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
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.
This PR contains four related PKCS#7/CMS fixes. The first two are Go-interop gaps uncovered by the same SCEP enrollment flow against Go-based servers (e.g.
micromdm/scep); the second was exposed only once the first let enrollment advance past the signature check to the enveloped-content decrypt step. The last two are pre-existing robustness bugs in theEnvelopedDatadecode path that the second fix's new negative tests surfaced: a memory leak on abandoned streaming teardown, and aNO_PKCS7_STREAMinfinite loop on malformed constructed-definite content. All four touch independent code paths and can be reviewed separately.SignedDatawith mismatched DigestInfo params (commita09c0db6f)EnvelopedDatadecrypt of a definite-length constructed[0](commit855f70d10)EnvelopedDatadecode (commit2c3c5608a)NO_PKCS7_STREAMinfinite loop on a malformed constructed-definite[0](commit26c90d4a6)Fix 1 — RSA verify of SignedData with mismatched DigestInfo params
Summary
wc_PKCS7_VerifySignedData()returnedSIG_VERIFY_E (-229)for a cryptographically valid RSASignedDatawhen the SignerInfodigestAlgorithmparameter encoding (NULL present vs. absent) differs from the parameter encoding of the AlgorithmIdentifier inside the PKCS#1 v1.5 DigestInfo that the RSA signature actually covers.These two encodings are independent:
digestAlgorithmis a CMS field (RFC 5652 / RFC 5754).AlgorithmIdentifierlives inside the signed value (RFC 8017).wolfSSL rebuilt its comparison DigestInfo mirroring only the SignerInfo
digestAlgorithm(pkcs7->hashParamsAbsent), so it never byte-matched the DigestInfo recovered from the signature, and every candidate signer was rejected.Go's
crypto/rsa.SignPKCS1v15always signs a NULL-present DigestInfo while Go CMS/SCEP stacks (e.g.micromdm/scep) omit the NULL in the SignerInfodigestAlgorithm. That combination is legal DER and OpenSSL verifies it, but wolfSSL rejected it. It is a generic PKCS#7/CMS interop gap, not SCEP-specific.Root cause
The RSA verify path compares the encoded signed value byte-for-byte. The comparison DigestInfo is built by
wc_PKCS7_BuildSignedDataDigest(), which sets the AlgorithmIdentifier parameters frompkcs7->hashParamsAbsent— a value captured from the SignerInfodigestAlgorithmduring parsing. Because that is independent of the DigestInfo encoding covered by the signature, the sizes differ (e.g. 33 vs. 35 bytes for SHA-1),XMEMCMPnever runs, and no signer matches. The plain-digest fallback compares a bare hash against a full DigestInfo, so it does not rescue the case either.The fix
wc_PKCS7_BuildSignedDataDigest()now takes an explicithashParamsAbsentargument instead of always readingpkcs7->hashParamsAbsent.as-parsed→plain digest→flipped parameter encoding. The flipped rebuild runs last because it overwrites the digest buffer, keeping the plain-digest attempt valid.This keeps wolfSSL's existing encode-and-compare approach (the same posture as the certificate verify path in
asn.c) — it never parses the attacker-controlled recovered bytes, so it does not reintroduce the DigestInfo-parsing signature-forgery surface that a lenient decode would. NULL-present and NULL-absent are the only two legal encodings, so trying both fully covers the space, in both mismatch directions.Signers that omit the NULL in the DigestInfo itself (RFC 5754-strict) continue to verify — the change only adds an attempt, it does not relax any comparison.
Tests
Adds
test_wc_PKCS7_VerifySignedData_NoDigestParams(tests/api/test_pkcs7.c). The mismatch is produced entirely from wolfSSL's own signer — no externally captured message is embedded. The same content is signed twice (once NULL-absent, once NULL-present); because RSA signatures are fixed length, one signature is spliced over the other message to create the parameter mismatch without any ASN.1 re-encoding.The test exercises the full 2×2 matrix plus a negative control:
digestAlgorithm!= 0)Each mismatch case was confirmed to return
SIG_VERIFY_E (-229)before the fix and0after, so no case is degenerate. The signed-attribute cases use deterministic attributes (contentType + messageDigest, nosigningTime) so the two encodes stay byte-identical and the splice remains valid.Testing done
Built with
./configure --enable-all --enable-debug(warnings-as-errors):./wolfcrypt/test/testwolfcrypt— pass (exit 0), all PKCS7 tests pass./tests/unit.test— 0 failed, 1670 passed-WerrorbuildCompatibility / risk
statichelperwc_PKCS7_BuildSignedDataDigest()gained an argument; its single caller is updated.PKCS7_SIGNEEDS_CHECK, no embedded signer cert) is unchanged and still hands both digest forms to the caller; extending dual-encoding there is out of scope for this fix.References
Fix 2 — EnvelopedData decrypt of a definite-length constructed [0]
Summary
wc_PKCS7_DecodeEnvelopedData()failed to decrypt a valid CMS/SCEPEnvelopedDatawhoseencryptedContentis a definite-length constructed context[0]wrapping a single definite-lengthOCTET STRING:This is the encoding emitted by Go's
crypto/pkcs7stacks (e.g.micromdm/scep). Streaming builds returnedWC_PKCS7_WANT_READ_E;NO_PKCS7_STREAMbuilds spun. SCEP enrollment against those servers failed at the CertRep decrypt step.The decoder already handled the other three
encryptedContentencodings; only the constructed-definite form was unhandled:encryptedContentencoding80 …(definite)A0 80 … 00 00A0 82 … 04 82 …Root cause
The handler for the constructed
[0]tag (explicitOctet == 1) assumed the BER indefinite-length fragmented form — a run ofOCTET STRINGs terminated by an EOC (00 00). The definite-length single-OCTET-STRING form has no EOC, so after decrypting the oneOCTET STRINGthe loop kept scanning for a terminator that never comes, ran past the buffer, andAddDataToStreamasked for more input (WANT_READ). WithNO_PKCS7_STREAMthe same mismatch spun.The fix
Right after reading the
[0]length, detect the case where the[0]definite length is filled exactly by a single innerOCTET STRING, skip that inner header, and fall into the existing primitive single-shot decrypt path (explicitOctet = 0).NO_PKCS7_STREAMbounds check, so it corrects the sizes for both streaming and non-streaming builds (fixes the spin too).NO_USER_CHECKbecause the full ciphertext may not be buffered yet in streaming mode; the size-equality test (inner header + inner length == [0] length) validates the structure instead.word32to avoid signed-overflow undefined behavior when a crafted message declares an oversized inner length. It fails safe either way (the value can never match), but the UB itself would trip UBSan /-ftrapv/ fuzz builds — and this PKCS7 path is fuzz-hardened.explicitOctetalready 0) and indefinite ([0]length 0) encodings are untouched → no behavior change for existing cases.Tests
Adds
test_wc_PKCS7_DecodeEnvelopedData_constructedDefiniteOctet(tests/api/test_pkcs7.c). wolfSSL's own encoder never emits this form (SetImplicit(ASN_OCTET_STRING, 0, …)produces the primitive or the indefinite form), so no encode→decode round-trip can reach the fixed branch. The test therefore transcodes a normal wolfSSL encode into the constructed-definite form, then decodes it.encryptedContent [0]shapeOCTET STRING(the Go form)OCTET STRINGs (fragmented)[0]longer than the innerOCTET STRINGOCTET STRINGinnerThe three negatives pin the unwrap guard's decline conditions. Verified by mutation:
WC_PKCS7_WANT_READ_E (-270)(the reported symptom);innerTag == OCTET STRINGcheck → the non-OS case wrongly decrypts;Testing done
./wolfcrypt/test/testwolfcrypt— PKCS7 signed/enveloped/encrypted/authenveloped all pass./tests/unit.testpkcs7,pkcs7_sd,pkcs7_ed,pkcs7_sed,pkcs7_cdgroups — 0 failed-WerrorbuildCompatibility / risk
wc_PKCS7_DecodeEnvelopedData().Fix 3 — decrypt-context leak on an abandoned streaming EnvelopedData decode
Summary
wc_PKCS7_Free()releasedcachedEncryptedContentbut never releasedpkcs7->decryptKey— the content-cipher context (Aes/Des/Des3) allocated bywc_PKCS7_DecryptContentInit()on the streamingEnvelopedDatadecode path. OnWC_PKCS7_WANT_READ_Ethe decode deliberately keeps that context to resume on the next input chunk, so a decode abandoned mid-message (a truncated or malformed streaming message the caller gives up on) and then freed leaked the cipher context.This is a pre-existing leak, not introduced by Fix 2 — but Fix 2's new negative tests are the first to
Freeawc_PKCS7that is mid-WANT_READ, so LeakSanitizer in CI surfaced it.Root cause
wc_PKCS7_DecodeEnvelopedData()'s own error cleanup only runs for hard errors (ret < 0 && ret != WC_PKCS7_WANT_READ_E), so it deliberately does not free the decrypt context onWANT_READ(it expects to resume). Nothing then frees it if the object is destroyed instead of resumed, becausewc_PKCS7_Free()did not releasedecryptKey.The fix
In
wc_PKCS7_Free(), before tearing down the stream, retrieve the content cipher OID from the stream's saved var and call the existingwc_PKCS7_DecryptContentFree():decryptKeyis only ever left allocated by the streamingEnvelopedDataflow, where that saved var holds the content cipher OID.decryptKeyis zeroed atwc_PKCS7_New()and NULLed byDecryptContentFree()after a completed decode, so there is no double-free).Tests
No new test — the leak is exercised by Fix 2's
test_wc_PKCS7_DecodeEnvelopedData_constructedDefiniteOctetnegative cases (which decode a malformed message that returnsWC_PKCS7_WANT_READ_E, then free the object). Those cases fail under LeakSanitizer without this fix and pass with it.Compatibility / risk
wc_PKCS7_Free()and runs only in streaming builds (!NO_PKCS7_STREAM).Fix 4 — NO_PKCS7_STREAM infinite loop on a malformed constructed-definite [0]
Summary
In a
NO_PKCS7_STREAMbuild,wc_PKCS7_DecodeEnvelopedData()could spin forever on a malformedEnvelopedDatawhoseencryptedContentis a definite-length constructed[0]that the single-OCTET-STRING unwrap (Fix 2) declines — e.g. a definite[0]wrapping two OCTET STRINGs, or one OCTET STRING followed by trailing bytes. This is a denial-of-service on the non-streaming decoder for attacker-supplied input.This is a pre-existing bug, not introduced by Fix 2 — but Fix 2's new negative tests are the first to feed exactly these shapes, so it hung the
NO_PKCS7_STREAMCI configs (themake checkshard timed out and was cancelled).Root cause
The fragmented-OCTET-STRING loop's error exit —
if (ret != 0) break;— is compiled only under#ifndef NO_PKCS7_STREAM. The non-streaming build has no error exit; it relies solely on finding an indefiniteEOC(00 00). A definite-length[0]has no EOC, so once its OCTET STRING(s) are consumed the scan runs off the end of the buffer:GetASNTagkeeps failing,retis set but the loop never breaks,idxstops advancing, and it loops indefinitely (observed: ~16 million iterations before the CI kill).The fix
Add the equivalent non-streaming error exit — after the
EOCcheck (so a valid indefinite message still terminates on its EOC first),breakwhenretis non-zero. The break is guarded by#ifdef NO_PKCS7_STREAM, so streaming behavior is unchanged.Tests
No new test — Fix 2's
test_wc_PKCS7_DecodeEnvelopedData_constructedDefiniteOctetnegative cases (two OCTET STRINGs, trailing bytes, non-OCTET-STRING inner) hang aNO_PKCS7_STREAMbuild without this fix and return a clean error (< 0) with it. Verified locally:NO_PKCS7_STREAMbuild goes from a 16 M-iteration hang to all PKCS7 groups passing;--enable-allstreaming build unaffected.Compatibility / risk
wc_PKCS7_DecodeEnvelopedData().