Add bounds checks for MP integer size in SizeASN_Items by anhu · Pull Request #10051 · wolfSSL/wolfssl

anhu · 2026-03-23T20:18:45Z

Fixes ZD 21401

dgarske · 2026-03-24T15:50:12Z

Jenkins retest this please -history lost

anhu · 2026-03-25T19:26:31Z

Jenkins retest this please.

Unable to get pull request trigger.

anhu · 2026-03-25T23:59:29Z

Jenkins retest this please

Build was aborted

anhu · 2026-03-26T00:33:50Z

jenkins retest this please

remote hung up.

anhu · 2026-03-26T12:12:21Z

jenkins retest this please

ABORTED

anhu · 2026-03-27T05:28:50Z

jenkins retest this please.

anhu · 2026-03-27T22:26:27Z

Jenkins retest this please.

anhu · 2026-03-30T18:31:19Z

Jenkins retest this please.

anhu · 2026-04-01T18:29:03Z

jenkins retest this please

Not found.

anhu · 2026-04-01T21:08:02Z

Jenkins retest this please.

anhu · 2026-04-08T21:51:53Z

jenkins retest this please.

anhu · 2026-04-08T23:51:58Z

jenkins retest this please.

SparkiDev · 2026-04-09T08:32:06Z

tests/api/test_rsa.c

+
+    /* Sparse mmap — only the page at dp[used-1] is faulted in. */
+    if (EXPECT_SUCCESS()) {
+        crafted_dp = (mp_digit*)mmap(NULL, dp_bytes,


This seems excessive.
Can't you test it another way that works on platforms not Windows?

dgarske

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 3 total — 3 posted, 0 skipped

Posted findings

[Medium] SetASN_Items MP path lacks matching bounds checks — wolfcrypt/src/asn.c:1097-1099
[Medium] Test only covers USE_INTEGER_HEAP_MATH; tfm.c has same overflow pattern — tests/api/test_rsa.c:1161-1163
[Low] Unused variable derRet could use (void) cast or direct use in Expect — tests/api/test_rsa.c:1254-1255

Review generated by Skoll via openclaw

dgarske · 2026-04-09T22:27:38Z

wolfcrypt/src/asn.c

+                }
                break;

            case ASN_DATA_TYPE_REPLACE_BUFFER:


🟡 [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);

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

dgarske · 2026-04-09T22:27:38Z

tests/api/test_rsa.c

+int test_wc_RsaKeyToDer_SizeOverflow(void)
+{
+    EXPECT_DECLS;
+#if !defined(NO_RSA) && defined(USE_INTEGER_HEAP_MATH) && \


🟡 [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.

Will not leave revealing comment.

dgarske · 2026-04-09T22:27:38Z

tests/api/test_rsa.c

+    }
+
+    /* Should return an error, not a bogus small size. */
+    derRet = wc_RsaKeyToDer(&key, NULL, 0);


🔵 [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.

Not going to bother with this.

anhu · 2026-04-10T17:19:11Z

jenkins retest this please

anhu requested a review from wolfSSL-Bot March 23, 2026 20:18

anhu self-assigned this Mar 23, 2026

anhu added the Not For This Release Not for release 5.9.1 label Apr 1, 2026

anhu removed the Not For This Release Not for release 5.9.1 label Apr 8, 2026

SparkiDev reviewed Apr 9, 2026

View reviewed changes

anhu force-pushed the mp_int_bounds branch from 5bbce8a to 1358fa4 Compare April 9, 2026 18:12

dgarske reviewed Apr 9, 2026

View reviewed changes

anhu added 2 commits April 10, 2026 11:17

Add bounds checks for MP integer size in SizeASN_Items

599d02e

test improvement

573ff92

anhu force-pushed the mp_int_bounds branch from 1358fa4 to 573ff92 Compare April 10, 2026 15:23

better gating.

6377ed0

-            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);

Conversation

anhu commented Mar 23, 2026

Uh oh!

dgarske commented Mar 24, 2026

Uh oh!

anhu commented Mar 25, 2026

Uh oh!

anhu commented Mar 25, 2026

Uh oh!

anhu commented Mar 26, 2026

Uh oh!

anhu commented Mar 26, 2026

Uh oh!

anhu commented Mar 27, 2026

Uh oh!

anhu commented Mar 27, 2026

Uh oh!

anhu commented Mar 30, 2026

Uh oh!

anhu commented Apr 1, 2026

Uh oh!

anhu commented Apr 1, 2026

Uh oh!

anhu commented Apr 8, 2026

Uh oh!

anhu commented Apr 8, 2026

Uh oh!

SparkiDev Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

🐺 Skoll Code Review

Posted findings

Uh oh!

dgarske Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

anhu Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

dgarske Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

anhu Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

dgarske Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

anhu Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

anhu commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants