Skip to content

Add bounds checks for MP integer size in SizeASN_Items #10051

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

Open
anhu wants to merge 3 commits into
base: master
Choose a base branch
from
+126 −1
Open

Add bounds checks for MP integer size in SizeASN_Items #10051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
599d02e
Add bounds checks for MP integer size in SizeASN_Items
anhu Mar 23, 2026
573ff92
test improvement
anhu Apr 9, 2026
6377ed0
better gating.
anhu Apr 10, 2026
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
113 changes: 113 additions & 0 deletions tests/api/test_rsa.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1152,3 +1152,116 @@ int test_wc_RsaDecrypt_BoundsCheck(void)
return EXPECT_RESULT();
} /* END test_wc_RsaDecryptBoundsCheck */

/*
* Test wc_RsaKeyToDer with an mp_int large enough to wrap size calculations.
*/
int test_wc_RsaKeyToDer_SizeOverflow(void)
{
EXPECT_DECLS;
#if !defined(NO_RSA) && defined(USE_INTEGER_HEAP_MATH) && \
Copy link
Copy Markdown
Contributor

@dgarske dgarske Apr 9, 2026

Choose a reason for hiding this comment

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

🟡 [Medium] Test only covers USE_INTEGER_HEAP_MATH; tfm.c has same overflow pattern
💡 SUGGEST test

The test is gated on USE_INTEGER_HEAP_MATH because the pointer-biasing trick relies on heap-allocated dp. However, fp_count_bits in tfm.c (line 4116) has the same (a->used - 1) * DIGIT_BIT overflow pattern as mp_count_bits in integer.c (line 262). With TFM (fastmath), dp is a fixed-size stack array, so the pointer-biasing trick won't work, but the underlying SizeASN_Items fix is still relevant for TFM builds. Consider adding a separate test path for TFM (e.g., using mp_set_int or another approach) or at minimum adding a comment explaining why TFM is excluded.

Recommendation: Consider adding a comment or separate test variant for TFM builds. Even if the pointer trick doesn't work with stack-allocated digits, verifying that SizeASN_Items rejects a negative mp_unsigned_bin_size result is valuable across all math backends.

Copy link
Copy Markdown
Member Author

@anhu anhu Apr 10, 2026

Choose a reason for hiding this comment

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

Will not leave revealing comment.

defined(WOLFSSL_ASN_TEMPLATE) && defined(WOLFSSL_PUBLIC_MP) && \
(defined(WOLFSSL_KEY_GEN) || defined(WOLFSSL_KEY_TO_DER))
RsaKey key;
int i;
int derRet;
int crafted_used;
int top_bits;
mp_digit top_digit;
mp_digit storage = 0; /* the only digit mp_count_bits ever reads */
mp_digit* fake_dp = NULL;

int orig_used = 0;
int orig_alloc = 0;
int orig_sign = 0;
mp_digit* orig_dp = NULL;

mp_int* fields[8];

XMEMSET(&key, 0, sizeof(key));

/* Skip on 32-bit: biasing dp by ~half the address space is unsafe. */
if (sizeof(void*) < 8) {
return TEST_SKIPPED;
}

/* Find 'used' count that makes (used-1)*DIGIT_BIT + top_bits = -48
* as signed int, causing mp_unsigned_bin_size to return -6. */
{
unsigned int target = 0xFFFFFFD0u; /* -48 as unsigned 32-bit */
int found = 0;

crafted_used = 0;
top_bits = 0;
top_digit = 0;

for (top_bits = 1; top_bits < DIGIT_BIT; top_bits++) {
unsigned int base = target - (unsigned int)top_bits;
if (base % (unsigned int)DIGIT_BIT == 0) {
crafted_used = (int)(base / (unsigned int)DIGIT_BIT) + 1;
top_digit = (mp_digit)1 << (top_bits - 1);
found = 1;
break;
}
}
if (!found) {
return TEST_SKIPPED;
}
}

ExpectIntEQ(wc_InitRsaKey(&key, HEAP_HINT), 0);

/* Set up dummy RSA private key fields. */
key.type = RSA_PRIVATE;
fields[0] = &key.n;
fields[1] = &key.e;
fields[2] = &key.d;
fields[3] = &key.p;
fields[4] = &key.q;
fields[5] = &key.dP;
fields[6] = &key.dQ;
fields[7] = &key.u;

for (i = 0; i < 8; i++) {
if (EXPECT_SUCCESS()) {
ExpectIntEQ(mp_init(fields[i]), 0);
mp_set(fields[i], 0x42);
}
}

if (EXPECT_SUCCESS()) {
orig_used = key.p.used;
orig_alloc = key.p.alloc;
orig_sign = key.p.sign;
orig_dp = key.p.dp;
}

/* The vulnerable path (mp_unsigned_bin_size -> mp_count_bits, and
* mp_leading_bit) only reads dp[used-1]. Bias dp so that index
* (used-1) lands on our single real digit -- no giant allocation
* (and no mmap/VirtualAlloc) needed. */
if (EXPECT_SUCCESS()) {
storage = top_digit;
fake_dp = (mp_digit*)((wc_ptr_t)&storage
- (wc_ptr_t)(crafted_used - 1) * sizeof(mp_digit));

key.p.dp = fake_dp;
key.p.used = crafted_used;
key.p.alloc = crafted_used;
key.p.sign = 0; /* MP_ZPOS */
}

/* Should return an error, not a bogus small size. */
derRet = wc_RsaKeyToDer(&key, NULL, 0);
Copy link
Copy Markdown
Contributor

@dgarske dgarske Apr 9, 2026

Choose a reason for hiding this comment

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

🔵 [Low] Unused variable derRet could use (void) cast or direct use in Expect
🔧 NIT style

The variable derRet is assigned from wc_RsaKeyToDer and immediately passed to ExpectIntLT. Some compilers may warn about the extra variable being unnecessary. The pattern could be simplified to avoid the intermediate variable, or the derRet value could be used in the ExpectIntLT call directly (which it already is). This is a very minor style point—the existing code is clear and readable.

Recommendation: No action required. The code is clear as-is. If the project convention is to avoid intermediate variables for single-use returns, consider ExpectIntLT(wc_RsaKeyToDer(&key, NULL, 0), 0) instead.

Copy link
Copy Markdown
Member Author

@anhu anhu Apr 10, 2026

Choose a reason for hiding this comment

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

Not going to bother with this.

ExpectIntLT(derRet, 0);

/* Restore key.p before cleanup. */
key.p.dp = orig_dp;
key.p.used = orig_used;
key.p.alloc = orig_alloc;
key.p.sign = orig_sign;

DoExpectIntEQ(wc_FreeRsaKey(&key), 0);
#endif
return EXPECT_RESULT();
} /* END test_wc_RsaKeyToDer_SizeOverflow */

4 changes: 3 additions & 1 deletion tests/api/test_rsa.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ int test_wc_RsaEncryptSize(void);
int test_wc_RsaSSL_SignVerify(void);
int test_wc_RsaFlattenPublicKey(void);
int test_wc_RsaDecrypt_BoundsCheck(void);
int test_wc_RsaKeyToDer_SizeOverflow(void);

#define TEST_RSA_DECLS \
TEST_DECL_GROUP("rsa", test_wc_InitRsaKey), \
Expand All @@ -61,6 +62,7 @@ int test_wc_RsaDecrypt_BoundsCheck(void);
TEST_DECL_GROUP("rsa", test_wc_RsaEncryptSize), \
TEST_DECL_GROUP("rsa", test_wc_RsaSSL_SignVerify), \
TEST_DECL_GROUP("rsa", test_wc_RsaFlattenPublicKey), \
TEST_DECL_GROUP("rsa", test_wc_RsaDecrypt_BoundsCheck)
TEST_DECL_GROUP("rsa", test_wc_RsaDecrypt_BoundsCheck), \
TEST_DECL_GROUP("rsa", test_wc_RsaKeyToDer_SizeOverflow)

#endif /* WOLFCRYPT_TEST_RSA_H */
10 changes: 10 additions & 0 deletions wolfcrypt/src/asn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,8 +864,18 @@ int SizeASN_Items(const ASNItem* asn, ASNSetData *data, int count,
case ASN_DATA_TYPE_MP:
/* Calculate the size of the MP integer data. */
length = mp_unsigned_bin_size(data[i].data.mp);
if (length < 0) {
return ASN_PARSE_E;
}
length += mp_leading_bit(data[i].data.mp) ? 1 : 0;
if (length < 0) {
return ASN_PARSE_E;
}
len = (word32)SizeASNHeader((word32)length) + (word32)length;
/* Check for overflow: header + length must not wrap word32. */
if (len < (word32)length) {
return ASN_PARSE_E;
}
break;

case ASN_DATA_TYPE_REPLACE_BUFFER:
Copy link
Copy Markdown
Contributor

@dgarske dgarske Apr 9, 2026

Choose a reason for hiding this comment

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

🟡 [Medium] SetASN_Items MP path lacks matching bounds checks
💡 SUGGEST bug

The ASN_DATA_TYPE_MP case in SetASN_Items (the write path) calls mp_unsigned_bin_size() and mp_leading_bit() on lines 1097-1099 with the same pattern that was just hardened in SizeASN_Items, but without the new bounds checks. In normal usage SizeASN_Items is called first and would reject a malformed mp_int before SetASN_Items runs, so this is not directly exploitable. However, as defense-in-depth—and to prevent future callers from hitting the same issue—the same length < 0 checks should be added here.

Suggestion:

Suggested change
case ASN_DATA_TYPE_REPLACE_BUFFER:
case ASN_DATA_TYPE_MP:
/* Get length in bytes. */
length = mp_unsigned_bin_size(data[i].data.mp);
if (length < 0) {
return ASN_PARSE_E;
}
/* Add one for leading zero to make encoding a positive num. */
length += mp_leading_bit(data[i].data.mp) ? 1 : 0;
if (length < 0) {
return ASN_PARSE_E;
}
/* Write out length. */
idx += SetASNLength((word32)length, out + idx);

Copy link
Copy Markdown
Member Author

@anhu anhu Apr 10, 2026

Choose a reason for hiding this comment

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

The suggestion removes the handling of ASN_DATA_TYPE_REPLACE_BUFFER . This feels wrong. Not aking the suggestion.

Expand Down
Loading