Skip to content

Fix OOB possibility in PKCS7_VerifySignedData #10441

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.

Sign up for GitHub

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

Draft
padelsbach wants to merge 1 commit into
base: master
Choose a base branch
from
+133 −1
Draft

Fix OOB possibility in PKCS7_VerifySignedData #10441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 108 additions & 0 deletions tests/api/test_pkcs7.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5320,3 +5320,111 @@ int test_wc_PKCS7_VerifySignedData_IndefLenOOB(void)
#endif /* HAVE_PKCS7 && !NO_PKCS7_STREAM */
return EXPECT_RESULT();
}

/*
* SignedData bundle truncated at the eContent [0] EXPLICIT tag in
* encapContentInfo. Verifies that the parser rejects the malformed
* input rather than dereferencing past the end of the buffer.
*/
int test_wc_PKCS7_VerifySignedData_TruncEContentTag(void)
{
EXPECT_DECLS;
#if defined(HAVE_PKCS7)
PKCS7* pkcs7 = NULL;

WOLFSSL_SMALL_STACK_STATIC byte der[] = {
/* outer ContentInfo SEQUENCE (75 bytes content) */
0x30, 0x4B,
/* contentType OID 1.2.840.113549.1.7.2 (signedData) */
0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x07, 0x02,
/* [0] EXPLICIT (62 bytes content) */
0xA0, 0x3E,
/* SignedData SEQUENCE (60 bytes content) */
0x30, 0x3C,
/* version INTEGER 1 */
0x02, 0x01, 0x01,
/* digestAlgorithms SET (empty - degenerate) */
0x31, 0x00,
/* encapContentInfo SEQUENCE (53 bytes content) */
0x30, 0x35,
/* eContentType OID with 50 bytes of arbitrary payload */
0x06, 0x32,
0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x07, 0x01, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
/* eContent [0] EXPLICIT - buffer ends here, no length, no content */
0xA0
};
word32 derSz = (word32)sizeof(der);

ExpectNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, testDevId));
ExpectIntEQ(wc_PKCS7_Init(pkcs7, HEAP_HINT, INVALID_DEVID), 0);
ExpectIntEQ(wc_PKCS7_InitWithCert(pkcs7, NULL, 0), 0);
ExpectIntNE(wc_PKCS7_VerifySignedData(pkcs7, der, derSz), 0);
wc_PKCS7_Free(pkcs7);

#endif /* HAVE_PKCS7 */
return EXPECT_RESULT();
}

/*
* SignedData bundle truncated at the certificates [0] IMPLICIT tag.
* Verifies that the parser rejects the malformed input rather than
* dereferencing past the end of the buffer.
*
* TODO: limited to NO_PKCS7_STREAM because the streaming parser's stage 3
* early-exit check (pkcs7.c near line 6594) accepts any bundle
* whose remaining footer is < 6 bytes as a successful degenerate end,
* so the bounds check at line 6765 is unreachable in streaming mode.
* Drop the NO_PKCS7_STREAM gate if/when the early-exit check becomes
* more accurate.
*/
int test_wc_PKCS7_VerifySignedData_TruncCertSetTag(void)
{
EXPECT_DECLS;
#if defined(HAVE_PKCS7) && defined(NO_PKCS7_STREAM)
PKCS7* pkcs7 = NULL;

WOLFSSL_SMALL_STACK_STATIC byte der[] = {
/* outer ContentInfo SEQUENCE (78 bytes content) */
0x30, 0x4E,
/* contentType OID signedData */
0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x07, 0x02,
/* [0] EXPLICIT (65 bytes content) */
0xA0, 0x41,
/* SignedData SEQUENCE (63 bytes content) */
0x30, 0x3F,
/* version INTEGER 1 */
0x02, 0x01, 0x01,
/* digestAlgorithms SET (empty) */
0x31, 0x00,
/* encapContentInfo SEQUENCE (55 bytes content) */
0x30, 0x37,
/* eContentType OID 1.2.840.113549.1.7.1 (data) */
0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x07, 0x01,
/* eContent [0] EXPLICIT (42 bytes content) */
0xA0, 0x2A,
/* OCTET STRING (40 bytes content) */
0x04, 0x28,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
/* certificates [0] IMPLICIT - buffer ends here, no length */
0xA0
};
word32 derSz = (word32)sizeof(der);

ExpectNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, testDevId));
ExpectIntEQ(wc_PKCS7_Init(pkcs7, HEAP_HINT, INVALID_DEVID), 0);
ExpectIntEQ(wc_PKCS7_InitWithCert(pkcs7, NULL, 0), 0);
ExpectIntNE(wc_PKCS7_VerifySignedData(pkcs7, der, derSz), 0);
wc_PKCS7_Free(pkcs7);

#endif /* HAVE_PKCS7 && NO_PKCS7_STREAM */
return EXPECT_RESULT();
}

6 changes: 5 additions & 1 deletion tests/api/test_pkcs7.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ int test_wc_PKCS7_DecodeEnvelopedData_multiple_recipients(void);
int test_wc_PKCS7_DecodeEnvelopedData_forgedRecipientSetLen(void);
int test_wc_PKCS7_VerifySignedData_PKCS7ContentSeq(void);
int test_wc_PKCS7_VerifySignedData_IndefLenOOB(void);
int test_wc_PKCS7_VerifySignedData_TruncEContentTag(void);
int test_wc_PKCS7_VerifySignedData_TruncCertSetTag(void);


#define TEST_PKCS7_DECLS \
Expand Down Expand Up @@ -115,7 +117,9 @@ int test_wc_PKCS7_VerifySignedData_IndefLenOOB(void);
TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_BER), \
TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_NoDefaultSignedAttribs), \
TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_PKCS7ContentSeq), \
TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_IndefLenOOB)
TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_IndefLenOOB), \
TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_TruncEContentTag), \
TEST_DECL_GROUP("pkcs7_sd", test_wc_PKCS7_VerifySignedData_TruncCertSetTag)

#define TEST_PKCS7_ENCRYPTED_DATA_DECLS \
TEST_DECL_GROUP("pkcs7_ed", test_wc_PKCS7_DecodeEnvelopedData_stream), \
Expand Down
20 changes: 20 additions & 0 deletions wolfcrypt/src/pkcs7.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6333,6 +6333,14 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf,
* OCTET_STRING will be next. If so, we use the length retrieved
* there. PKCS#7 spec defines ANY as eContent type. In this case
* we fall back and save this content length for use later */
if (ret == 0 && localIdx >= pkiMsgSz) {
/* Truncated input: don't dereference past the buffer.
* Break out of the switch directly so the degenerate-
* recovery path below cannot mask this error. */
ret = BUFFER_E;
break;
}

if (ret == 0 && pkiMsg[localIdx] != ASN_INDEF_LENGTH) {
if (GetLength_ex(pkiMsg, &localIdx, &length, pkiMsgSz,
NO_USER_CHECK) <= 0) {
Expand Down Expand Up @@ -6590,6 +6598,14 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf,

/* 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.
*/
if (ret == 0 && pkcs7->stream->maxLen > 0 &&
(pkcs7->stream->maxLen - pkcs7->stream->totalRd)
Expand Down Expand Up @@ -6759,6 +6775,10 @@ static int PKCS7_VerifySignedData(wc_PKCS7* pkcs7, const byte* hashBuf,
&& tag == (ASN_CONSTRUCTED | ASN_CONTEXT_SPECIFIC | 0)) {
idx++;

if (localIdx >= pkiMsg2Sz) {
ret = BUFFER_E;
}

/* if certificates set has indefinite length, try to get
* the first certificate length of the set.
*/
Expand Down
Loading