-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix truncation and noDegenerate bypasses in wc_PKCS7_VerifySignedData #11017
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
fcb13ed
72c4aa5
a600695
8777c2b
59c4c70
472ec62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6391,7 +6391,7 @@ static int wc_PKCS7_ParseSignerInfo(wc_PKCS7* pkcs7, byte* in, word32 inSz, | |
|
|
||
| WOLFSSL_ENTER("wc_PKCS7_ParseSignerInfo"); | ||
| /* require a signer if degenerate case not allowed */ | ||
| if (inSz == 0 && pkcs7->noDegenerate == 1) { | ||
| if (pkcs7->noDegenerate == 1 && (inSz == 0 || degenerate == 1)) { | ||
| WOLFSSL_MSG("Set to not allow degenerate cases"); | ||
| return PKCS7_NO_SIGNER_E; | ||
| } | ||
|
|
@@ -7506,26 +7506,33 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf, | |
| pkcs7->content = pkcs7->contentDynamic; | ||
| } | ||
|
|
||
| /* check if bundle has more elements or footer, if not, set content | ||
| * to pkcs7->content and hash to pkcs7->hash. | ||
| * | ||
| * NOTE: this check returns success whenever fewer than 6 bytes | ||
| * follow the content within the outer ContentInfo, which also | ||
| * accepts truncated bundles whose footer was cut short (e.g. a | ||
| * lone certificates [0] tag with no length). Distinguishing a | ||
| * legitimate degenerate end (such as an empty signerInfos SET | ||
| * "31 00") from truncated junk would require peeking at the | ||
| * remaining bytes or making stage 4's `expected` window smaller. | ||
| /* expect data length to be enough to check set and seq of certs, | ||
| * but never request more than the bundle has left. The old code | ||
| * unconditionally requested this much and silently treated a | ||
| * short read here as a successful degenerate end, which also | ||
| * accepted a truncated bundle. Capping the request lets stage | ||
| * 4/5/6's existing bounds checks and noDegenerate enforcement | ||
| * run instead: a genuine short degenerate end (empty signerInfos | ||
| * SET) still parses and succeeds, while anything truncated or | ||
| * malformed fails there with a real parse error. | ||
| */ | ||
| if (ret == 0 && pkcs7->stream->maxLen > 0 && | ||
| (pkcs7->stream->maxLen - pkcs7->stream->totalRd) | ||
| < ASN_TAG_SZ + MAX_LENGTH_SZ) { | ||
|
|
||
| ret = 0; | ||
| break; | ||
| } | ||
| /* expect data length to be enough to check set and seq of certs */ | ||
| pkcs7->stream->expected = (ASN_TAG_SZ + MAX_LENGTH_SZ) * 2; | ||
| if (!pkcs7->stream->indefLen && | ||
| pkcs7->stream->totalRd >= pkcs7->stream->maxLen) { | ||
| /* definite-length bundle with no bytes left per the outer | ||
| * length: force stage 4's bounds check to fail now instead | ||
| * of requesting bytes that will never arrive. For | ||
| * indefinite-length (BER) bundles maxLen is only a running | ||
| * estimate, so totalRd catching up to it does not mean the | ||
| * bundle is actually exhausted. */ | ||
| pkcs7->stream->expected = 0; | ||
| } | ||
| else if (pkcs7->stream->totalRd < pkcs7->stream->maxLen && | ||
| pkcs7->stream->expected > | ||
| pkcs7->stream->maxLen - pkcs7->stream->totalRd) { | ||
| pkcs7->stream->expected = | ||
| pkcs7->stream->maxLen - pkcs7->stream->totalRd; | ||
| } | ||
|
Comment on lines
7519
to
+7535
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed. Reproduced with Fixed by capping Also strengthened Fixed in 8777c2b. |
||
|
|
||
| #else | ||
| /* Break out before content because it can be optional in degenerate | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 [Medium] TruncSignerInfosTag test does not catch WC_PKCS7_WANT_READ_E stall · Weak or missing assertions
The
31 01signerInfos header (declared length 1, 0 bytes available) makes the default streaming build's single-shot call returnWC_PKCS7_WANT_READ_E(a stall, not a rejection), yetExpectIntNE(ret, 0)passes since -270 != 0.Fix: Add
ExpectIntNE(ret, WC_NO_ERR_TRACE(WC_PKCS7_WANT_READ_E))as done in the siblingNoSignerInfosTagtest.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed — good catch.
ExpectIntNE(ret, 0)was passing trivially since single-shot on this truncated buffer returnsWC_PKCS7_WANT_READ_E(-270), not a real rejection. Added the missingExpectIntNE(ret, WC_NO_ERR_TRACE(WC_PKCS7_WANT_READ_E)), matching NoSignerInfosTag.That assertion then exposed an actual bug: the STAGE6→STAGE7 transition in
PKCS7_VerifySignedData(pkcs7.c) requested the signerInfos content length without capping it against the bundle's remaining declared length, so a truncated SET header stalled forever instead of failing. Fixed by adding the same totalRd >= maxLen zero-cap guard STAGE3 already uses. Verified with fulltests/unit.test(0 failures) and testwolfcrypt to rule out regressions in the encode/decode streaming paths.