Skip to content

Commit 0fc9484

Browse files
embhornembhorn
committed
Fix pkcs12 parse issue
1 parent 1d363f3 commit 0fc9484

3 files changed

Lines changed: 98 additions & 15 deletions

File tree

‎tests/api/test_pkcs12.c‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,78 @@ int test_wc_d2i_PKCS12_oid_underflow(void)
272272
return EXPECT_RESULT();
273273
}
274274

275+
/* Test that validates the fix for heap OOB read vulnerability where
276+
* ASN.1 parsing after DecryptContent() would use stale ContentInfo bounds.
277+
* This is a basic test that verifies the fix compiles and basic PKCS#12
278+
* functionality still works after adding contentSz bounds checking. */
279+
int test_wc_PKCS12_encrypted_content_bounds(void)
280+
{
281+
EXPECT_DECLS;
282+
#if !defined(NO_ASN) && !defined(NO_PWDBASED) && defined(HAVE_PKCS12) && \
283+
!defined(NO_RSA) && !defined(NO_AES) && !defined(NO_SHA) && \
284+
!defined(NO_SHA256) && defined(USE_CERT_BUFFERS_2048)
285+
286+
/* This test validates that the fix for heap OOB read is in place.
287+
* The fix ensures ASN.1 parsing uses contentSz (actual decrypted size)
288+
* instead of ci->dataSz (original ContentInfo size) as bounds.
289+
*
290+
* We test this by exercising the PKCS#12 parsing path with encrypted
291+
* content to ensure the fix doesn't break normal operation. */
292+
293+
byte* inKey = (byte*) server_key_der_2048;
294+
const word32 inKeySz = sizeof_server_key_der_2048;
295+
byte* inCert = (byte*) server_cert_der_2048;
296+
const word32 inCertSz = sizeof_server_cert_der_2048;
297+
WC_DerCertList inCa = {
298+
(byte*)ca_cert_der_2048, sizeof_ca_cert_der_2048, NULL
299+
};
300+
char pkcs12Passwd[] = "test_bounds_fix";
301+
302+
WC_PKCS12* pkcs12Export = NULL;
303+
WC_PKCS12* pkcs12Import = NULL;
304+
byte* pkcs12Der = NULL;
305+
byte* outKey = NULL;
306+
byte* outCert = NULL;
307+
WC_DerCertList* outCaList = NULL;
308+
word32 pkcs12DerSz = 0;
309+
word32 outKeySz = 0;
310+
word32 outCertSz = 0;
311+
312+
/* Create a PKCS#12 with encrypted content */
313+
ExpectNotNull(pkcs12Export = wc_PKCS12_create(pkcs12Passwd,
314+
sizeof(pkcs12Passwd) - 1, NULL, inKey, inKeySz, inCert, inCertSz,
315+
&inCa, -1, -1, 2048, 2048, 0, NULL));
316+
317+
/* Serialize to DER */
318+
ExpectIntGE((pkcs12DerSz = wc_i2d_PKCS12(pkcs12Export, &pkcs12Der, NULL)), 0);
319+
320+
/* Parse it back - this exercises the fixed bounds checking code path */
321+
ExpectNotNull(pkcs12Import = wc_PKCS12_new_ex(NULL));
322+
ExpectIntGE(wc_d2i_PKCS12(pkcs12Der, pkcs12DerSz, pkcs12Import), 0);
323+
324+
/* This parse operation now uses contentSz instead of ci->dataSz for bounds,
325+
* preventing the heap OOB read that existed before the fix */
326+
ExpectIntEQ(wc_PKCS12_parse(pkcs12Import, pkcs12Passwd, &outKey, &outKeySz,
327+
&outCert, &outCertSz, &outCaList), 0);
328+
329+
/* Verify the parsing worked correctly */
330+
ExpectIntEQ(outKeySz, inKeySz);
331+
ExpectIntEQ(outCertSz, inCertSz);
332+
ExpectNotNull(outCaList);
333+
ExpectIntEQ(outCaList->bufferSz, inCa.bufferSz);
334+
335+
/* Clean up */
336+
XFREE(outKey, NULL, DYNAMIC_TYPE_PUBLIC_KEY);
337+
XFREE(outCert, NULL, DYNAMIC_TYPE_PKCS);
338+
wc_FreeCertList(outCaList, NULL);
339+
wc_PKCS12_free(pkcs12Import);
340+
XFREE(pkcs12Der, NULL, DYNAMIC_TYPE_PKCS);
341+
wc_PKCS12_free(pkcs12Export);
342+
343+
#endif
344+
return EXPECT_RESULT();
345+
}
346+
275347
int test_wc_PKCS12_PBKDF(void)
276348
{
277349
EXPECT_DECLS;

‎tests/api/test_pkcs12.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ int test_wc_i2d_PKCS12(void);
2828
int test_wc_PKCS12_create(void);
2929
int test_wc_d2i_PKCS12_bad_mac_salt(void);
3030
int test_wc_d2i_PKCS12_oid_underflow(void);
31+
int test_wc_PKCS12_encrypted_content_bounds(void);
3132
int test_wc_PKCS12_PBKDF(void);
3233
int test_wc_PKCS12_PBKDF_ex(void);
3334
int test_wc_PKCS12_PBKDF_ex_sha1(void);
@@ -42,6 +43,7 @@ int test_wc_PKCS12_PBKDF_ex_sha512_256(void);
4243
TEST_DECL_GROUP("pkcs12", test_wc_PKCS12_create), \
4344
TEST_DECL_GROUP("pkcs12", test_wc_d2i_PKCS12_bad_mac_salt), \
4445
TEST_DECL_GROUP("pkcs12", test_wc_d2i_PKCS12_oid_underflow), \
46+
TEST_DECL_GROUP("pkcs12", test_wc_PKCS12_encrypted_content_bounds), \
4547
TEST_DECL_GROUP("pkcs12", test_wc_PKCS12_PBKDF), \
4648
TEST_DECL_GROUP("pkcs12", test_wc_PKCS12_PBKDF_ex), \
4749
TEST_DECL_GROUP("pkcs12", test_wc_PKCS12_PBKDF_ex_sha1), \

‎wolfcrypt/src/pkcs12.c‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1335,6 +1335,7 @@ int wc_PKCS12_parse_ex(WC_PKCS12* pkcs12, const char* psw,
13351335
byte* buf = NULL;
13361336
word32 i, oid;
13371337
word32 algId;
1338+
word32 contentSz;
13381339
int ret, pswSz;
13391340
#ifdef ASN_BER_TO_DER
13401341
int curIdx;
@@ -1450,6 +1451,11 @@ int wc_PKCS12_parse_ex(WC_PKCS12* pkcs12, const char* psw,
14501451
goto exit_pk12par;
14511452
}
14521453

1454+
/* DecryptContent strips the PBE ASN.1 wrapper and returns the
1455+
* actual decrypted payload size, which is smaller than the
1456+
* allocated buf. Track the real bounds so subsequent ASN.1
1457+
* parsing does not read past the decrypted content. */
1458+
contentSz = (word32)ret;
14531459
data = buf;
14541460
idx = 0;
14551461

@@ -1486,36 +1492,39 @@ int wc_PKCS12_parse_ex(WC_PKCS12* pkcs12, const char* psw,
14861492
goto exit_pk12par;
14871493
}
14881494

1495+
/* DATA branch: data still points into ci->data, so the
1496+
* ContentInfo size is the correct parsing bound. */
1497+
contentSz = ci->dataSz;
14891498
}
14901499

14911500
/* parse through bags in ContentInfo */
1492-
if ((ret = GetSequence(data, &idx, &totalSz, ci->dataSz)) < 0) {
1501+
if ((ret = GetSequence(data, &idx, &totalSz, contentSz)) < 0) {
14931502
goto exit_pk12par;
14941503
}
14951504
totalSz += (int)idx;
14961505

14971506
while ((int)idx < totalSz) {
14981507
int bagSz;
1499-
if ((ret = GetSequence(data, &idx, &bagSz, ci->dataSz)) < 0) {
1508+
if ((ret = GetSequence(data, &idx, &bagSz, contentSz)) < 0) {
15001509
goto exit_pk12par;
15011510
}
15021511
bagSz += (int)idx;
15031512

15041513
if ((ret = GetObjectId(data, &idx, &oid, oidIgnoreType,
1505-
ci->dataSz)) < 0) {
1514+
contentSz)) < 0) {
15061515
goto exit_pk12par;
15071516
}
15081517

15091518
switch (oid) {
15101519
case WC_PKCS12_KeyBag: /* 667 */
15111520
WOLFSSL_MSG("PKCS12 Key Bag found");
1512-
if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) {
1521+
if (GetASNTag(data, &idx, &tag, contentSz) < 0) {
15131522
ERROR_OUT(ASN_PARSE_E, exit_pk12par);
15141523
}
15151524
if (tag != (ASN_CONSTRUCTED | ASN_CONTEXT_SPECIFIC)) {
15161525
ERROR_OUT(ASN_PARSE_E, exit_pk12par);
15171526
}
1518-
if ((ret = GetLength(data, &idx, &size, ci->dataSz)) <= 0) {
1527+
if ((ret = GetLength(data, &idx, &size, contentSz)) <= 0) {
15191528
if (ret == 0)
15201529
ret = ASN_PARSE_E;
15211530
goto exit_pk12par;
@@ -1553,14 +1562,14 @@ int wc_PKCS12_parse_ex(WC_PKCS12* pkcs12, const char* psw,
15531562
byte* k;
15541563

15551564
WOLFSSL_MSG("PKCS12 Shrouded Key Bag found");
1556-
if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) {
1565+
if (GetASNTag(data, &idx, &tag, contentSz) < 0) {
15571566
ERROR_OUT(ASN_PARSE_E, exit_pk12par);
15581567
}
15591568
if (tag != (ASN_CONSTRUCTED | ASN_CONTEXT_SPECIFIC)) {
15601569
ERROR_OUT(ASN_PARSE_E, exit_pk12par);
15611570
}
15621571
if ((ret = GetLength(data, &idx, &size,
1563-
ci->dataSz)) < 0) {
1572+
contentSz)) < 0) {
15641573
goto exit_pk12par;
15651574
}
15661575

@@ -1626,51 +1635,51 @@ int wc_PKCS12_parse_ex(WC_PKCS12* pkcs12, const char* psw,
16261635
{
16271636
WC_DerCertList* node;
16281637
WOLFSSL_MSG("PKCS12 Cert Bag found");
1629-
if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) {
1638+
if (GetASNTag(data, &idx, &tag, contentSz) < 0) {
16301639
ERROR_OUT(ASN_PARSE_E, exit_pk12par);
16311640
}
16321641
if (tag != (ASN_CONSTRUCTED | ASN_CONTEXT_SPECIFIC)) {
16331642
ERROR_OUT(ASN_PARSE_E, exit_pk12par);
16341643
}
1635-
if ((ret = GetLength(data, &idx, &size, ci->dataSz)) < 0) {
1644+
if ((ret = GetLength(data, &idx, &size, contentSz)) < 0) {
16361645
goto exit_pk12par;
16371646
}
16381647

16391648
/* get cert bag type */
1640-
if ((ret = GetSequence(data, &idx, &size, ci->dataSz)) <0) {
1649+
if ((ret = GetSequence(data, &idx, &size, contentSz)) <0) {
16411650
goto exit_pk12par;
16421651
}
16431652

16441653
if ((ret = GetObjectId(data, &idx, &oid, oidIgnoreType,
1645-
ci->dataSz)) < 0) {
1654+
contentSz)) < 0) {
16461655
goto exit_pk12par;
16471656
}
16481657

16491658
switch (oid) {
16501659
case WC_PKCS12_CertBag_Type1: /* 675 */
16511660
/* type 1 */
16521661
WOLFSSL_MSG("PKCS12 cert bag type 1");
1653-
if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) {
1662+
if (GetASNTag(data, &idx, &tag, contentSz) < 0) {
16541663
ERROR_OUT(ASN_PARSE_E, exit_pk12par);
16551664
}
16561665
if (tag != (ASN_CONSTRUCTED |
16571666
ASN_CONTEXT_SPECIFIC)) {
16581667
ERROR_OUT(ASN_PARSE_E, exit_pk12par);
16591668
}
1660-
if ((ret = GetLength(data, &idx, &size, ci->dataSz))
1669+
if ((ret = GetLength(data, &idx, &size, contentSz))
16611670
<= 0) {
16621671
if (ret == 0)
16631672
ret = ASN_PARSE_E;
16641673
goto exit_pk12par;
16651674
}
1666-
if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) {
1675+
if (GetASNTag(data, &idx, &tag, contentSz) < 0) {
16671676
ERROR_OUT(ASN_PARSE_E, exit_pk12par);
16681677
}
16691678
if (tag != ASN_OCTET_STRING) {
16701679
ERROR_OUT(ASN_PARSE_E, exit_pk12par);
16711680

16721681
}
1673-
if ((ret = GetLength(data, &idx, &size, ci->dataSz))
1682+
if ((ret = GetLength(data, &idx, &size, contentSz))
16741683
< 0) {
16751684
goto exit_pk12par;
16761685
}

0 commit comments

Comments
 (0)