Skip to content

PKCS7 Fixes#10128

Open
kareem-wolfssl wants to merge 5 commits intowolfSSL:masterfrom
kareem-wolfssl:zd21526_21530
Open

PKCS7 Fixes#10128
kareem-wolfssl wants to merge 5 commits intowolfSSL:masterfrom
kareem-wolfssl:zd21526_21530

Conversation

@kareem-wolfssl
Copy link
Copy Markdown
Contributor

Description

Fixes zd#21526,zd#21530

Testing

Built in tests, provided reproducers.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@kareem-wolfssl kareem-wolfssl self-assigned this Apr 3, 2026
Copilot AI review requested due to automatic review settings April 3, 2026 23:10
@kareem-wolfssl kareem-wolfssl added the For This Release Release version 5.9.1 label Apr 3, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes PKCS7 decoding/signing edge cases tied to attribute counting and enveloped-data bounds checking (zd#21526, zd#21530).

Changes:

  • Adjust signed attributes count to reflect the number of attributes actually added.
  • Add a bounds check before caching encrypted content during EnvelopedData decode.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

wolfcrypt/src/pkcs7.c:13237

  • pkcs7->cachedEncryptedContent is allocated with XMALLOC but the return value isn’t checked before being passed to wc_PKCS7_DecryptContent(). On allocation failure this can dereference NULL and crash (remote DoS). Add a NULL check and return MEMORY_E (and ensure any partially-set cached size/state is consistent).
                pkcs7->cachedEncryptedContentSz =
                    (word32)encryptedContentTotalSz;
                pkcs7->totalEncryptedContentSz =
                    (word32)encryptedContentTotalSz;
                pkcs7->cachedEncryptedContent = (byte*)XMALLOC(
                        pkcs7->cachedEncryptedContentSz, pkcs7->heap,
                    DYNAMIC_TYPE_PKCS7);

                /* decrypt encryptedContent */
                ret = wc_PKCS7_DecryptContent(pkcs7, encOID, decryptedKey,
                        (word32)blockKeySz, tmpIv, expBlockSz, NULL, 0, NULL, 0,
                        &pkiMsg[idx], encryptedContentTotalSz,
                        pkcs7->cachedEncryptedContent,
                        pkcs7->devId, pkcs7->heap);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +13217 to +13222
word32 tmpSum;
if (!WC_SAFE_SUM_WORD32(idx, (word32)encryptedContentTotalSz, tmpSum) ||
tmpSum > pkiMsgSz) {
ret = BUFFER_E;
break;
}
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new bounds/overflow check for idx + encryptedContentTotalSz is a security-relevant behavior change, but there’s no regression test covering a malformed/truncated EnvelopedData where the encryptedContent length exceeds the remaining input. Add a decode-negative test vector (or construct one in the test) that previously would read past the input and now reliably returns BUFFER_E.

Copilot uses AI. Check for mistakes.
…e are skipped by using the current idx rather than the total array size.

Thanks to Zou Dikai for the report.
… not exceed the total message size before using it in the non-streaming case.

Thanks to Zou Dikai for the report.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

For This Release Release version 5.9.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants