Skip to content

Bound SHE LoadKey auth key read to WH_SHE_KEY_SZ - #495

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

Bound SHE LoadKey auth key read to WH_SHE_KEY_SZ#495
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_7155

Conversation

@yosuke-wolfssl

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

Copy link
Copy Markdown
Contributor

Problem

_LoadKey read the SHE authentication key with the read capacity set to the whole KDF input buffer (keySz = sizeof(kdfInput), 32 bytes) instead of the size of a SHE key. wh_Server_KeystoreReadKey returns the actual stored object length in *outSz, so keySz could come back as anything up to 32. The handler then appends the 16-byte key-update constant at kdfInput + keySz and runs the Miyaguchi-Preneel KDF over keySz + 16 bytes, writing and reading up to 16 bytes past the end of a 32-byte stack buffer.

SHE keys and NVM objects share the 16-bit id space, and wh_Nvm_CheckPolicy grants by default on an unused id, so an authenticated client can plant an oversized object at WH_MAKE_KEYID(WH_KEYTYPE_SHE, client_id, authId) and reach it via WH_SHE_LOAD_KEY. The overflowing bytes are a fixed constant, but the overflow length is attacker-selected. _LoadKey was the only SHE handler not pinning the capacity; the other eleven sites already used WH_SHE_KEY_SZ. Requires WOLFHSM_CFG_SHE_EXTENSION (non-default).
Addressed by f_7155.

Fix (src/wh_server_she.c)

  • keySz = WH_SHE_KEY_SZ before the auth-key read, matching every other SHE handler.
  • WH_SHE_ERC_KEY_INVALID for both rejection paths — WH_ERROR_NOSPACE from the keystore, and a short read. _TranslateSheReturnCode has no case for NOSPACE, so it would otherwise degrade to GENERAL_ERROR; _SecureBootInit already returns KEY_INVALID for the identical condition.
  • Second read pinned too, rather than inheriting keySz from the first.
  • Label parsing moved under the size check, so the write-protect and counter decisions only run on a slot that is correctly sized or absent.
Auth-key slot Before After
32 bytes 16-byte stack overflow WH_SHE_ERC_KEY_INVALID
8 bytes KDF over a short key WH_SHE_ERC_KEY_INVALID
16 bytes works unchanged
absent GENERAL_ERROR unchanged

Test harness

New test-refactor/server/wh_test_she_loadkey.c: one registered parent, three static sub-tests planting 32/8/16-byte objects at a SHE AuthID slot via wh_Server_KeystoreCacheKey (cache-only, no NVM artifact) and driving the handler directly. Registered in wh_test_list.c.

Verification

  • Refactored suite: 50 passed, 20 skipped, 0 failed of 70. Legacy suite (SHE=1) passes, exit 0.
  • Pre-fix the new test aborts on the stack canary; under ASan it reports stack-buffer-overflow, WRITE of size 16 at offset 96 of kdfInput [64, 96) in _LoadKey. Post-fix ASan is clean.
  • Both harnesses build clean under -std=c90 -Werror -Wall -Wextra.

Not in this PR

The NVM message API is the only client-facing path accepting a raw keyId, and its policy layer is per-object flags rather than a namespace boundary. Restricting it is a design change — test-refactor/client-server/wh_test_keywrap.c asserts flag-stripping is the intended defense — so it is written up separately.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jul 27, 2026
Copilot AI review requested due to automatic review settings July 27, 2026 04:39

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 the server-side SHE LOAD_KEY handler against oversized/undersized auth-key objects by bounding keystore reads to WH_SHE_KEY_SZ, ensuring consistent error mapping, and adding targeted regression tests in the refactored test suite.

Changes:

  • Fix _LoadKey to read the auth key with capacity WH_SHE_KEY_SZ and reject short reads / WH_ERROR_NOSPACE as WH_SHE_ERC_KEY_INVALID.
  • Pin the target-slot keystore read size to WH_SHE_KEY_SZ (instead of inheriting keySz from the auth-key read).
  • Add a new refactored server test that plants 32/8/16-byte auth-key objects and validates the handler’s return codes.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/wh_server_she.c Bounds auth-key read size, normalizes error returns for invalid auth-key storage sizes, and pins the subsequent target-slot read size.
test-refactor/server/wh_test_she_loadkey.c Adds a regression test for LOAD_KEY auth-key length handling (oversized/undersized/valid).
test-refactor/wh_test_list.c Registers the new server test in the refactored test suite.

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

Comment thread src/wh_server_she.c

@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 #495

Scan targets checked: wolfhsm-core-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.

4 participants