Translate the key wrap metadata trailer#472
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes cross-endian interoperability for the key-wrap protocol by translating the whNvmMetadata “raw struct trailer” that follows the fixed key-wrap request/response headers. It introduces a dedicated metadata translation helper and applies it in the server handlers so policy checks and storage always operate on native-order metadata, while the wire-facing trailer is translated appropriately.
Changes:
- Add
wh_MessageNvm_TranslateMetadata()to translatewhNvmMetadatatrailers (including in-place translation support). - Update key wrap / unwrap-and-export server handlers to translate the metadata trailer on ingress/egress based on the request
magic. - Add a dedicated endianness regression test for key-wrap metadata trailers and register it in the server test list.
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 |
|---|---|
| wolfhsm/wh_message_nvm.h | Declares the new wh_MessageNvm_TranslateMetadata() helper for raw-struct metadata trailers. |
| src/wh_message_nvm.c | Implements wh_MessageNvm_TranslateMetadata() using existing translation macros and label passthrough. |
| src/wh_server_keystore.c | Translates key-wrap metadata trailer on request ingest and on unwrap-and-export response egress using magic. |
| test-refactor/server/wh_test_keywrap_endian.c | Adds endianness coverage: helper property checks plus wrap→unwrap/export round-trips under native/swapped magic. |
| test-refactor/wh_test_list.c | Registers the new whTest_KeyWrapEndian server test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #472
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
|
|
||
| /* The policy checks above run on native metadata, so put the trailer into | ||
| * the client's byte order only once it is on its way out */ | ||
| (void)wh_MessageNvm_TranslateMetadata(magic, metadata, metadata); |
There was a problem hiding this comment.
Please check the error return from this call.
Don't we need to do the translation prior to the policy checks (right after _AesGcmKeyUnwrap)? Else the metadata coming in would still be in the native client endianess
There was a problem hiding this comment.
Error return is now checked, using the fallthrough guard so it can't mask a
prior failure:
if (ret == WH_ERROR_OK) {
ret = wh_MessageNvm_TranslateMetadata(magic, metadata, metadata);
}On the placement — I kept it on egress, because the metadata here doesn't come
off the wire. It comes out of the decrypted blob, and the blob always holds
metadata in server order:
_AesGcmKeyWrapWithKek()memcpys thewhNvmMetadatastruct into the
plaintext blob verbatim, and_HandleKeyWrapRequesttranslates
client -> native before wrapping._HandleKeyWrapExportRequestwraps
metadata read straight from the keystore, which is native by definition._HandleKeyUnwrapAndCacheRequestdoes no translation at all and stores blob
metadata directly into the keystore. That is only correct if blob metadata is
native, which is the same invariant.
So translating right after _AesGcmKeyUnwrap would byte-swap the values that
WH_KEYID_IS_UNASSIGNED(metadata->id), the WH_KEYTYPE_WRAPPED check,
metadata->flags & WH_NVM_FLAGS_NONEXPORTABLE, and the USER ownership check
all read — a swapped-endian client would then bypass or trip those checks
incorrectly. Egress-last keeps every policy check on native values.
I've expanded the comment at the call site to say this explicitly.
There was a problem hiding this comment.
Checked now, and it mattered more than expected — the check made a
previously-dead branch live:
if (ret == WH_ERROR_OK) {
ret = wh_MessageNvm_TranslateMetadata(magic, metadata, metadata);
}
if (ret != WH_ERROR_OK && respDataSz >= sizeof(*metadata)) {
resp->keySz = 0;
wh_Utils_ForceZero(metadata, scrubSz);
}I had these as if / else if first, which fails open: a translation failure
would skip the scrub and leave resp->keySz set, shipping the decrypted key
alongside an error rc. Two ifs keyed off the final ret close it.
The scrub came out of the same audit — *out_resp_size here unconditionally
counts a trailer, so a failed unwrap shipped up to 32 bytes of stale buffer.
Placement answer unchanged: egress-last, since the blob holds metadata in server
order and every policy check must run on native values.
There was a problem hiding this comment.
I would like to see an end-to-end test of this with a client and server if possible
There was a problem hiding this comment.
Added: test-refactor/client-server/wh_test_keywrap_endian_e2e.c
(whTest_KeyWrapEndianE2E, client group). Wrap -> unwrap-and-export against the
live server over the real transport, once as a same-endian peer and once as a
swapped one.
There was a problem hiding this comment.
Expanded since my last reply. whTest_KeyWrapEndianE2E now also covers a denied
export (NONEXPORTABLE) and asserts the response trailer comes back zeroed.
That case needs the real harness: the server test uses separate request and
response buffers, but wh_Server_HandleRequestMessage passes one pointer as
both. For KEYUNWRAPEXPORT both fixed structs are 8 bytes, so the blob and the
response trailer share an offset and _AesGcmKeyUnwrap writes the decrypted
trailer there before the scrub.
Byte swaps in both files are hand-rolled rather than reusing the Translate*
helpers, so a bug under test cannot cancel itself out — a flags bug injected
into wh_MessageNvm_TranslateMetadata is now caught by the round trip and the
NONEXPORTABLE negative, where it previously passed undetected.
d8f30fb to
9be189c
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #472
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
|
Hello @AlexLanzano , |
Problem
The key wrap protocol carries a
whNvmMetadataas a raw struct trailerafter the fixed request/response headers, but the server translated only the
headers.
whNvmMetadatais fouruint16_tfields (id,access,flags,len), so every one is byte-swapped for a cross-endian client/server pair:WH_KEY_KEYWRAPrequestwhNvmMetadata+ keyid0x4025(TYPE=WRAPPED) read as0x2540(TYPE=SHE) → valid wrap rejectedWH_ERROR_BADARGSWH_KEY_KEYUNWRAPEXPORTresponsewhNvmMetadata+ keyid,access,flags,lenreturned byte-swappedEvery other NVM message flattens the struct into message fields and translates
each one; key wrap is the only place the raw struct crosses the boundary.
Addressed by f_5469.
Fix (
src/wh_server_keystore.c)Server-side only: clients always send native byte order, and the server
translates both directions from the comm-header
magic.wh_MessageNvm_TranslateMetadata()— new helper inwh_message_nvm.[ch],in-place safe, label copied verbatim.
_HandleKeyWrapRequesttranslates on ingress before any field isinspected, establishing the invariant that blobs always store metadata in
server-native order.
_HandleKeyUnwrapAndExportRequesttranslates on egress last, after everypolicy check has run on native values.
Both handlers are
staticwith one call site each, so they gain amagicparameter. No client, wire-format, or struct change.
Also fixed, found while auditing the failure paths:
*out_resp_sizeforKEYUNWRAPEXPORTunconditionally counts awhNvmMetadatatrailer, so a failedunwrap shipped up to 32 bytes of stale comm-buffer content to the client. The
handler now zeroes the trailer and any decrypted key with
wh_Utils_ForceZero()on every failure exit, and the dispatcher scrubs as a backstop. In-switch
returns becameret = X; break;so one epilogue covers them all._HandleKeyWrapRequestalso setsresp->cipherTypebefore its first failureexit — the client checks
cipherTypeahead ofrc, so an early returnpreviously surfaced as
WH_ERROR_ABORTEDinstead of the real error.Tests
test-refactor/server/wh_test_keywrap_endian.c— helper property test,wrap → unwrap-and-export round trip under both magics, keyId and
NONEXPORTABLEpolicy negatives, malformed-request residue check, and across-magic case (wrap as one peer's byte order, consume as the other's).
test-refactor/client-server/wh_test_keywrap_endian_e2e.c— raw-wireforeign-endian client against a live server. Packets are hand-built and pushed
at the transport, since
wh_CommClientalways sends native magic and rejectsa non-native reply. Reaches what the server test cannot: the comm layer
accepting a foreign magic, and
wh_Server_HandleRequestMessagepassing onebuffer as both
req_packetandresp_packet, making the egress translationan in-place swap inside the wrapped blob.
Byte swaps in both tests are hand-rolled rather than reusing the library
helpers, so a bug in the code under test cannot cancel itself out.
Verification
wrap step. With a
flagsbug injected into the new helper, the round trip andthe
NONEXPORTABLEnegative both fail. Without the scrub, the e2e denied-exportcase fails on residue.
DMA=143/0 ·SHE=145/0 ·AUTH=147/0 ·NOCRYPTO=111/0(both new tests SKIPPED) ·
THREADSAFE=140/0 across 10 runs.