From e0a19a798ee015ecec5209839cf7c0e283ceef3d Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Thu, 19 Mar 2026 15:09:02 -0500 Subject: [PATCH 1/2] Fix GetSafeContent to check length --- tests/api/test_pkcs12.c | 34 ++++++++++++++++++++++++++++++++++ tests/api/test_pkcs12.h | 6 ++++-- wolfcrypt/src/pkcs12.c | 6 ++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/tests/api/test_pkcs12.c b/tests/api/test_pkcs12.c index a2cc3d69b4c..f1e3cc72e6b 100644 --- a/tests/api/test_pkcs12.c +++ b/tests/api/test_pkcs12.c @@ -235,3 +235,37 @@ int test_wc_d2i_PKCS12_bad_mac_salt(void) return EXPECT_RESULT(); } +/* Test that a crafted PKCS12 with a ContentInfo SEQUENCE length smaller than + * the contained OID is rejected, rather than causing an integer underflow + * in ci->dataSz calculation. */ +int test_wc_d2i_PKCS12_oid_underflow(void) +{ + EXPECT_DECLS; +#if !defined(NO_ASN) && !defined(NO_PWDBASED) && defined(HAVE_PKCS12) + WC_PKCS12* pkcs12 = NULL; + + /* Crafted PKCS12 DER: the inner ContentInfo SEQUENCE declares length 5, + * but contains a valid 11-byte OID (1.2.840.113549.1.7.1). Without the + * bounds check, (word32)curSz - (localIdx - curIdx) = 5 - 11 underflows + * to ~4GB. */ + static const byte crafted[] = { + 0x30, 0x23, /* outer SEQ */ + 0x02, 0x01, 0x03, /* version 3 */ + 0x30, 0x1E, /* AuthSafe wrapper SEQ */ + 0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, + 0x01, 0x07, 0x01, /* OID pkcs7-data */ + 0xA0, 0x11, /* [0] CONSTRUCTED ctx */ + 0x04, 0x0F, /* OCTET STRING */ + 0x30, 0x0D, /* SEQ of ContentInfo arr */ + 0x30, 0x05, /* ContentInfo SEQ, length=5 LIE */ + 0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, + 0x01, 0x07, 0x01 /* OID: 11 bytes actual */ + }; + + ExpectNotNull(pkcs12 = wc_PKCS12_new()); + ExpectIntLT(wc_d2i_PKCS12(crafted, (word32)sizeof(crafted), pkcs12), 0); + wc_PKCS12_free(pkcs12); +#endif + return EXPECT_RESULT(); +} + diff --git a/tests/api/test_pkcs12.h b/tests/api/test_pkcs12.h index 3a7e18c3795..bc4789a82ff 100644 --- a/tests/api/test_pkcs12.h +++ b/tests/api/test_pkcs12.h @@ -27,10 +27,12 @@ int test_wc_i2d_PKCS12(void); int test_wc_PKCS12_create(void); int test_wc_d2i_PKCS12_bad_mac_salt(void); +int test_wc_d2i_PKCS12_oid_underflow(void); #define TEST_PKCS12_DECLS \ TEST_DECL_GROUP("pkcs12", test_wc_i2d_PKCS12), \ - TEST_DECL_GROUP("pkcs12", test_wc_PKCS12_create), \ - TEST_DECL_GROUP("pkcs12", test_wc_d2i_PKCS12_bad_mac_salt) + TEST_DECL_GROUP("pkcs12", test_wc_PKCS12_create), \ + TEST_DECL_GROUP("pkcs12", test_wc_d2i_PKCS12_bad_mac_salt), \ + TEST_DECL_GROUP("pkcs12", test_wc_d2i_PKCS12_oid_underflow) #endif /* WOLFCRYPT_TEST_PKCS12_H */ diff --git a/wolfcrypt/src/pkcs12.c b/wolfcrypt/src/pkcs12.c index 3edfd36830a..62b9b85f8dc 100644 --- a/wolfcrypt/src/pkcs12.c +++ b/wolfcrypt/src/pkcs12.c @@ -334,6 +334,12 @@ static int GetSafeContent(WC_PKCS12* pkcs12, const byte* input, return ret; } + /* Check that OID did not consume more than the sequence length */ + if ((localIdx - curIdx) > (word32)curSz) { + freeSafe(safe, pkcs12->heap); + return ASN_PARSE_E; + } + /* create new content info struct ... possible OID sanity check? */ ci = (ContentInfo*)XMALLOC(sizeof(ContentInfo), pkcs12->heap, DYNAMIC_TYPE_PKCS); From b4d2cd6d9cefd358deaa1cf37ad07597c55a8225 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Thu, 19 Mar 2026 15:22:39 -0500 Subject: [PATCH 2/2] Fix feedback from review --- tests/api/test_pkcs12.c | 8 +++++--- wolfcrypt/src/pkcs12.c | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/api/test_pkcs12.c b/tests/api/test_pkcs12.c index f1e3cc72e6b..877f538449d 100644 --- a/tests/api/test_pkcs12.c +++ b/tests/api/test_pkcs12.c @@ -245,8 +245,9 @@ int test_wc_d2i_PKCS12_oid_underflow(void) WC_PKCS12* pkcs12 = NULL; /* Crafted PKCS12 DER: the inner ContentInfo SEQUENCE declares length 5, - * but contains a valid 11-byte OID (1.2.840.113549.1.7.1). Without the - * bounds check, (word32)curSz - (localIdx - curIdx) = 5 - 11 underflows + * but contains a valid OID (1.2.840.113549.1.7.1) that is 11 bytes + * on the wire (tag 06 + length 09 + 9 value bytes). Without the bounds + * check, (word32)curSz - (localIdx - curIdx) = 5 - 11 underflows * to ~4GB. */ static const byte crafted[] = { 0x30, 0x23, /* outer SEQ */ @@ -263,7 +264,8 @@ int test_wc_d2i_PKCS12_oid_underflow(void) }; ExpectNotNull(pkcs12 = wc_PKCS12_new()); - ExpectIntLT(wc_d2i_PKCS12(crafted, (word32)sizeof(crafted), pkcs12), 0); + ExpectIntEQ(wc_d2i_PKCS12(crafted, (word32)sizeof(crafted), pkcs12), + ASN_PARSE_E); wc_PKCS12_free(pkcs12); #endif return EXPECT_RESULT(); diff --git a/wolfcrypt/src/pkcs12.c b/wolfcrypt/src/pkcs12.c index 62b9b85f8dc..5f00282f387 100644 --- a/wolfcrypt/src/pkcs12.c +++ b/wolfcrypt/src/pkcs12.c @@ -335,7 +335,7 @@ static int GetSafeContent(WC_PKCS12* pkcs12, const byte* input, } /* Check that OID did not consume more than the sequence length */ - if ((localIdx - curIdx) > (word32)curSz) { + if (localIdx > curIdx + (word32)curSz) { freeSafe(safe, pkcs12->heap); return ASN_PARSE_E; }