Fix bug in data unwrap. Remove unnecessary stack buffers in key wrap code - #510
Fix bug in data unwrap. Remove unnecessary stack buffers in key wrap code#510AlexLanzano wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes silent corruption in AES-GCM data unwrap/wrap paths caused by in-place buffer overlap, removes several large stack buffers in key handling code, and strengthens tests to catch regressions around AES block boundaries.
Changes:
- Stage AES-GCM wrap/unwrap input/output to prevent plaintext/ciphertext aliasing and add output-size bounds checking.
- Refactor key wrap/unwrap handlers to use request/response buffers directly (removing multi-KB stack arrays) and correct scrubbing sizes.
- Expand/augment tests with larger payloads and a round-trip size sweep around AES block boundaries.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/wh_test_keywrap.c | Increases legacy data-wrap test payload size to span multiple AES blocks (better overlap detection). |
| test-refactor/client-server/wh_test_keywrap.c | Adds data wrap/unwrap round-trip coverage for multiple sizes near AES block boundaries. |
| src/wh_server_keystore.c | Adds staging buffers for AES-GCM wrap/unwrap to avoid overlap corruption, adds size checks, removes stack key/data buffers in request handlers, and improves/limits scrubbing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
b4f9edc to
db2f442
Compare
|
CI will fail until #511 is merged |
db2f442 to
cd02ff9
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #510
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| uint8_t iv[WH_KEYWRAP_AES_GCM_IV_SIZE]; | ||
| uint8_t* encBlob; | ||
| uint16_t encBlobSz; | ||
| uint8_t plainData[WOLFHSM_CFG_KEYWRAP_MAX_DATA_SIZE]; |
There was a problem hiding this comment.
🔵 [Low] Data-unwrap path gains a full MAX_DATA_SIZE stack buffer · Resource leaks on error paths
_AesGcmDataUnwrapWithKek adds plainData[WOLFHSM_CFG_KEYWRAP_MAX_DATA_SIZE] (up to 2 KB) on the WH_KEY_DATAUNWRAP call chain, which carried no such buffer before: _HandleDataUnwrapRequest uses respData directly. Unlike the wrap path, no buffer was removed to offset it, so peak server stack for data unwrap grows by MAX_DATA_SIZE.
Fix: Bound the staging buffer to the AES block size and decrypt in chunks, or copy the ciphertext out of the overlapping region instead of staging the whole plaintext.
| encBlob = (uint8_t*)wrappedDataIn + sizeof(iv) + sizeof(authTag); | ||
| encBlobSz = wrappedDataSz - sizeof(iv) - sizeof(authTag); | ||
|
|
||
| if (encBlobSz > dataSz) { |
There was a problem hiding this comment.
⚪ [Info] New encBlobSz dataSz check is unreachable from the only caller · Dead error handling
WH_KEYWRAP_AES_GCM_HEADER_SIZE is defined as IV_SIZE + TAG_SIZE, and the sole caller _HandleDataUnwrapRequest passes dataSz = req->wrappedDataSz - WH_KEYWRAP_AES_GCM_HEADER_SIZE, which is identically encBlobSz. The guard can never return WH_ERROR_BUFFER_SIZE, so it is untestable and adds no runtime protection today.
Fix: Keep the guard but document it as an internal invariant assertion, or drop it in favour of asserting encBlobSz == dataSz.
| big[i] = (uint8_t)(i & 0xFF); | ||
| } | ||
|
|
||
| for (i = 0; i < (sizeof(sizes) / sizeof(sizes[0])); i++) { |
There was a problem hiding this comment.
🔵 [Low] New MAX_DATA_SIZE staging buffers are never exercised at their boundary · Missing edge-case coverage on a function the PR also changed
The PR adds plainData[WOLFHSM_CFG_KEYWRAP_MAX_DATA_SIZE] staging buffers to both data-wrap helpers, guarded by dataSz > WOLFHSM_CFG_KEYWRAP_MAX_DATA_SIZE. The new sweep stops at 128 bytes and _whTest_KeywrapOversizeRequest only tests MAX+1, so the exact-max case — the only input that fills the new stack buffers completely — is untested in both suites.
Fix: Add WOLFHSM_CFG_KEYWRAP_MAX_DATA_SIZE and MAX_DATA_SIZE - 1 to the round-trip sweep sizes.
Bug fix
Data wrap restructure
Stack buffers removed (~6 KB)
Tests