Defensive code additions for sanity checks on input arguments with Base64, PEM write, mp_read_unsigned_bin#10721
Defensive code additions for sanity checks on input arguments with Base64, PEM write, mp_read_unsigned_bin#10721JacobBarthelmeh wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Hardens several low-level routines against integer wrap/overflow by tightening bounds checks when parsing lengths, encoding Base64/PEM, and reading big integers.
Changes:
- Added size/argument validation to prevent overflow in Base64 encoding and PEM size calculations.
- Strengthened ASN.1 buffer bounds checks to avoid
word32wrap. - Added defensive checks in
mp_read_unsigned_binto reject problematic input sizes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| wolfcrypt/src/integer.c | Adds input length validation intended to prevent overflow in bit/digit calculations. |
| wolfcrypt/src/coding.c | Adds a guard intended to prevent Base64 encoded-size calculation from wrapping. |
| wolfcrypt/src/asn.c | Reworks index/length comparisons to avoid word32 addition overflow. |
| src/pk.c | Adds a guard against overflow in PEM buffer size calculations and rejects negative lengths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* reject sizes where the bit count would overflow an int */ | ||
| if (c > (WOLFSSL_MAX_32BIT - (DIGIT_BIT - 1)) / CHAR_BIT) { | ||
| return MP_VAL; | ||
| } | ||
|
|
||
| digits_needed = ((c * CHAR_BIT) + DIGIT_BIT - 1) / DIGIT_BIT; |
| /* Reject lengths that would wrap the encoded-size calculation below. */ | ||
| if (inLen >= (WOLFSSL_MAX_32BIT / 4)) | ||
| return BAD_FUNC_ARG; | ||
|
|
||
| outSz = (inLen + 3 - 1) / 3 * 4; | ||
| addSz = (outSz + BASE64_LINE_SZ - 1) / BASE64_LINE_SZ; /* new lines */ |
| /* Reject lengths that would wrap the PEM size calculation below. */ | ||
| if ((len < 0) || ((word32)len >= (WOLFSSL_MAX_32BIT / 4))) { | ||
| return BAD_FUNC_ARG; | ||
| } | ||
| derLen = (word32)len; |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10721
Scan targets checked: wolfcrypt-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| } | ||
|
|
||
| /* reject sizes where the bit count would overflow an int */ | ||
| if (c > (WOLFSSL_MAX_32BIT - (DIGIT_BIT - 1)) / CHAR_BIT) { |
There was a problem hiding this comment.
🟠 [Medium] mp_read_unsigned_bin overflow guard uses unsigned max for signed int math · Incorrect sizeof/type usage
The guard bounds c by WOLFSSL_MAX_32BIT (0xffffffff), but c, CHAR_BIT and digits_needed are signed int. Values of c in (INT_MAX/8, UINT_MAX/8] pass the check yet still overflow c * CHAR_BIT in digits_needed, the exact signed-int overflow the check claims to prevent.
Fix: Bound c against INT_MAX instead of WOLFSSL_MAX_32BIT: c > (INT_MAX - (DIGIT_BIT - 1)) / CHAR_BIT.
JacobBarthelmeh
changed the title
|
ZD21992