Skip to content

Fix 32-bit size_t overflow in AES needed_size checks and two SHE size guards - #487

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/aesSizeHandling
Open

Fix 32-bit size_t overflow in AES needed_size checks and two SHE size guards#487
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/aesSizeHandling

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Problem

Every AES request handler validates its packet with a needed_size sum:

uint64_t needed_size = sizeof(whMessageCrypto_AesEcbRequest) + len + key_len;
if (needed_size != inSize) { return WH_ERROR_BADARGS; }

sizeof is size_t and len/key_len are uint32_t, so on a 32-bit target
every operand is 32-bit and the sum wraps before the widening assignment
— the
uint64_t declaration buys nothing. inSize is uint16_t, so a client picks
len such that the wrapped sum equals a small legitimate packet size, the guard
passes, and len reaches wc_AesEcbEncrypt(..., (word32)len) unmodified: a
~4 GB read/write off a small buffer. Reachable pre-auth
(wh_Server_HandleRequestMessage), and every supported port is 32-bit — the
64-bit POSIX build where this cannot trigger is only CI. Pre-auth memory
corruption on every real target.

Closes findings F-4325 / F-4327 / F-4328 / F-4331.

Fix (src/wh_server_crypto.c, src/wh_server_she.c)

Cast every operand to uint64_t so the sum is 64-bit at any word size, at all
8
needed_size sites — the 4 reported handlers plus _HandleAesGcm (the worst
site, 5 attacker-controlled addends, unreported) and the 4 DMA variants. Also
guards the 2 unreported SHE sites (_GenerateMac, _SecureBootUpdate in
src/wh_server_she.c) with the same > WOLFHSM_CFG_COMM_DATA_LEN overflow check
their 5 neighbours already carry. No behavior change on 64-bit; added lines only.
This PR is now source-only.

Tests

The tests added by the earlier revision have been removed. They were
test-refactor/server/wh_test_crypto_reqsize.c (280 lines,
whTest_CryptoReqSize) and two sub-tests appended to
whTest_SheReqSizeChecking, all of which hand-built wrap-crafted request packets
and fed them to server handlers directly. Per review, a test that hand-builds a
packet and calls a server handler is testing the harness, not the fix, so both
are dropped along with the wh_test_list.c entry. This also removes the add/add
conflict with #466, which created the same file.

No replacement test is added: reaching the wrap requires a len value no
wh_Client_* call will send, so the overflow is not expressible through the
normal client API. The 32-bit CI job in #491 covers this bug class at the
compiler/config level instead, which is the durable check.

Verification

  • test-refactor: default 43 passed / 26 skipped / 0 failed of 69 · DMA=1 47/22/0 · SHE=1 49/20/0
  • Legacy test/ suite clean.
  • Clean under -std=c90 -Werror -Wall -Wextra.
  • 32-bit (i386/QEMU) negative control, run while the test existed: with the
    fix whTest_CryptoReqSize passed; reverting only the two source files made the
    same binary SIGSEGV on the ~4 GB read — the actual exploit path.

Note: the wh_test_list.c revert also restored the 9-line brace-style
reformat of whTestsServer[] that this PR had carried incidentally.

Not in this PR: a permanent 32-bit CI job — that is #491.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jul 24, 2026
Copilot AI review requested due to automatic review settings July 24, 2026 06:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens request-size validation on 32-bit targets to prevent size_t wraparound from bypassing AES and SHE packet length checks, avoiding pre-auth memory corruption paths in server request handlers.

Changes:

  • Fixes 32-bit overflow in AES needed_size computations by promoting all operands to uint64_t across CTR/ECB/CBC/GCM and DMA variants.
  • Adds explicit magnitude guards in two SHE handlers to prevent sizeof(req) + req.sz from overflowing on 32-bit.
  • Introduces/refactors tests to exercise wrap-crafted request vectors for the affected handlers and registers the new test in the refactor test runner.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test-refactor/wh_test_list.c Registers the new whTest_CryptoReqSize in the server test set.
test-refactor/server/wh_test_she_server.c Adds overflow-focused SHE req_size tests and mirrors the UPDATE state value for setup.
test-refactor/server/wh_test_crypto_reqsize.c New refactor test covering AES handler req_size validation against 32-bit wrap-crafted vectors (incl. DMA where enabled).
src/wh_server_she.c Adds pre-checks to prevent 32-bit size_t overflow in two variable-length SHE request size validations.
src/wh_server_crypto.c Fixes AES handler needed_size calculations to be 64-bit regardless of target word size.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #487

Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src

No new issues found in the changed files. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants