Enforce NONMODIFIABLE and TRUSTED NVM policy on keystore key commit - #492
Enforce NONMODIFIABLE and TRUSTED NVM policy on keystore key commit#492yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request closes a defense-in-depth gap in the server keystore “gate” by ensuring WH_KEY_COMMIT cannot overwrite existing NVM objects that are marked NONMODIFIABLE or TRUSTED. The policy enforcement is implemented centrally in the keystore policy checker (rather than in the lower-level commit function), and is validated via a new server-side test suite.
Changes:
- Add a
WH_KS_OP_COMMITpolicy branch that consults the stored NVM object’s flags and denies overwrite whenNONMODIFIABLEorTRUSTED. - Register and add a new server test (
whTest_KeystoreCommitPolicy) covering overwrite-denial, first-commit allowance, normal commit round-trip, and the “uncached commit returns NOTFOUND” guard. - Extend the server test registry to include the new test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/wh_server_keystore.c |
Enforces NONMODIFIABLE/TRUSTED overwrite denial for commit by checking stored NVM metadata when a cache slot is being committed. |
test-refactor/server/wh_test_keystore_policy.c |
Adds targeted server-side tests validating commit policy behavior and ensuring stored bytes/flags remain unchanged on denied overwrite. |
test-refactor/wh_test_list.c |
Registers the new keystore commit policy test in the server test group. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #492
Scan targets checked: wolfhsm-core-bugs, wolfhsm-src
No new issues found in the changed files. ✅
6041613 to
0a5c6c1
Compare
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: CONDITIONAL
Findings: 6 total — 3 posted, 3 skipped
Posted findings
- [Low] Repeat/idempotent commit of a NONMODIFIABLE key now fails with WH_ERROR_ACCESS (undeclared client-visible behavior change) —
src/wh_server_keystore.c:247-263 - [Info] The TRUSTED half of the new check is only reachable via unchecked cache paths; comment does not say so —
src/wh_server_keystore.c:248-257 - [Info] Commit-overwrite does not consider NONDESTROYABLE, unlike the evict gate —
src/wh_server_keystore.c:247-263
Skipped findings
- [Low] New access-control branch has no test coverage at any level, and the stated rationale for omitting tests does not hold
- [Info] Re-committing an already-committed NONMODIFIABLE key now returns WH_ERROR_ACCESS, breaking idempotent commit retries
- [Info] Commit path now hard-depends on the optional NVM GetMetadata callback
Review generated by Skoll via Claude/Codex
0a5c6c1 to
69ca15e
Compare
|
Hello @Frauschi , |
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: CONDITIONAL
Findings: 6 total — 2 posted, 4 skipped
Posted findings
- [Low] New test leaves an undeletable NONMODIFIABLE NVM object, bypassing the repo's WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS convention —
test-refactor/client-server/wh_test_crypto_keystore.c:909-911 - [Low] Commit denial returns WH_ERROR_NOTFOUND instead of WH_ERROR_ACCESS when the cache slot is gone —
src/wh_server_keystore.c:247-263
Skipped findings
- [Medium] New keystore test permanently leaks an unerasable NVM object into the shared test fixture
- [Low] wh_Client_KeyCommit is no longer idempotent for NONMODIFIABLE keys, making commit retries a hard failure
- [Info] Fail-closed unreadable-metadata branch in the new commit policy has no test coverage and returns a non-ACCESS error code
- [Info] Revoke path still writes cached bytes to NVM unchecked, contradicting the new comment on the shared switch case
Review generated by Skoll via Claude/Codex
69ca15e to
69effbc
Compare
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 10 total — 3 posted, 7 skipped
Posted findings
- [Medium] Fix guards only one of the two client-reachable commit paths; LMS/XMSS DMA keygen still commits unchecked —
src/wh_server_crypto.c:7816 - [Medium] Commit is no longer idempotent for NONMODIFIABLE keys, and the contract is undocumented —
src/wh_server_keystore.c:247 - [Medium] New NONMODIFIABLE commit regression test never compiles or runs in any buildable configuration —
test-refactor/client-server/wh_test_crypto_keystore.c:842
Skipped findings
- [Medium] New commit-denial test is compiled out in every supported configuration
- [Low] Discarded evict return lets the uncached-denial test pass for the wrong reason
- [Low] Fail-closed comment does not cover the server->nvm == NULL path
- [Low] COMMIT branch duplicates the NVM metadata lookup already performed above the switch
- [Low] Discarded evict return leaves Test 4's uncached-commit premise unverified
- [Info] Identical 32-byte key literal duplicated across the two new tests
- [Info] _KeystoreCheckPolicy now carries two divergent notions of "the key's flags"
Review generated by Skoll via Claude/Codex
69effbc to
f4dc122
Compare
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 10 total — 5 posted, 5 skipped
Posted findings
- [Medium] New CI step's CFLAGS_EXTRA override silently disables -Werror -Wall -Wextra for the only job that compiles the gated tests —
.github/workflows/build-and-test-refactor.yml:133-136 - [Medium] No test covers the TRUSTED half of the new commit mask - the actual KEK-overwrite scenario —
src/wh_server_keystore.c:261-265 - [Medium] Deny-path test could live in test-refactor/server/ and avoid the persistence gate and the new CI job entirely —
test-refactor/client-server/wh_test_crypto_keystore.c:842-931 - [Medium] Client-facing header not updated with the new non-idempotent commit contract —
wolfhsm/wh_server_keystore.h:162-179 - [Low] New WH_ERROR_NOTFOUND doc on wh_Server_KeystoreCommitKeyChecked is inaccurate, and the retry contract it documents is incomplete —
wolfhsm/wh_server_keystore.h:171-178
Skipped findings
- [Low] Gated CI job permanently consumes NVM object slots out of a budget of 30
- [Low] WH_KS_OP_REVOKE keeps the unconditional allow the COMMIT branch was added to close, under a new comment asserting it is safe
- [Info] Comment says "deny" but the code propagates the backend error code
- [Info] Header doc overstates that the verdict never comes from cache flags
- [Info] New gated test permanently consumes an NVM slot mid-run, and the TRUSTED half of the new check is untested
Review generated by Skoll via Claude/Codex
f4dc122 to
f1ed323
Compare
|
Hello @Frauschi , |
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 5 total — 3 posted, 2 skipped
Posted findings
- [Medium] Revoke still launders cached flags into NVM, contradicting the new "Revocation only tightens policy" comment —
src/wh_server_keystore.c:269-271 - [Low] Suite header comment and README not updated for the two new tests —
test-refactor/client-server/wh_test_crypto_keystore.c:19-30 - [Info] New CI step covers only the ASAN configuration —
.github/workflows/build-and-test-refactor.yml:159-162
Skipped findings
- [Medium] NONMODIFIABLE deny path only runs in the new opt-in CI job
- [Low] Commit of a cached key now requires a working GetMetadata callback
Review generated by Skoll via Claude/Codex
f1ed323 to
9738bdf
Compare
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 6 total — 3 posted, 3 skipped
Posted findings
- [High] Revoke still overwrites a stored TRUSTED/NONMODIFIABLE object, contradicting the new comment and leaving f-4225 half-open —
src/wh_server_keystore.c:269-273 (comment), 1435-1440 (write) - [Medium] Uncached-commit assertion can silently degrade into a duplicate of the cached case —
test-refactor/client-server/wh_test_crypto_keystore.c:924-932 - [Medium] Fail-closed branch on unreadable NVM metadata is untested —
src/wh_server_keystore.c:252-261
Skipped findings
- [Low] README says gated suites "report SKIPPED", but the new sub-tests vanish silently
- [Low] New header contract ("caller must hold the NVM lock") is not followed by the new test added in the same PR
- [Low] 32-byte key literal and label setup duplicated verbatim between the two new client tests
Review generated by Skoll via Claude/Codex
9738bdf to
d111d86
Compare
| * Runs keystore policy checks before committing. The verdict comes from the | ||
| * stored NVM object's flags, not the cache slot, so an unchecked cache path | ||
| * cannot launder them. | ||
| * | ||
| * Caller must hold the NVM lock (the WH_KEY_COMMIT handler does): the verdict | ||
| * reads flags that the commit then overwrites. The lock is not recursive. | ||
| * | ||
| * @param[in] server Server context | ||
| * @param[in] keyId Key ID to commit | ||
| * @return WH_ERROR_OK on success | ||
| * @return WH_ERROR_ACCESS if an NVM object already exists under keyId and | ||
| * carries WH_NVM_FLAGS_NONMODIFIABLE or WH_NVM_FLAGS_TRUSTED, so | ||
| * commit is not idempotent for such keys | ||
| * @return WH_ERROR_NOTFOUND if the key is not resident in the cache, including | ||
| * after an already-committed slot was reclaimed | ||
| * @return the NVM layer's error, typically WH_ERROR_ABORTED, when the stored | ||
| * flags cannot be read: an unenforceable policy fails closed | ||
| * | ||
| * A retry after a lost response can therefore see either code on a commit that | ||
| * already succeeded; confirm with an export or metadata read. |
There was a problem hiding this comment.
This has too much development context LLM slop. Please remove and keep as tight usage documentation.
| * Runs keystore policy checks before committing. The verdict comes from the | |
| * stored NVM object's flags, not the cache slot, so an unchecked cache path | |
| * cannot launder them. | |
| * | |
| * Caller must hold the NVM lock (the WH_KEY_COMMIT handler does): the verdict | |
| * reads flags that the commit then overwrites. The lock is not recursive. | |
| * | |
| * @param[in] server Server context | |
| * @param[in] keyId Key ID to commit | |
| * @return WH_ERROR_OK on success | |
| * @return WH_ERROR_ACCESS if an NVM object already exists under keyId and | |
| * carries WH_NVM_FLAGS_NONMODIFIABLE or WH_NVM_FLAGS_TRUSTED, so | |
| * commit is not idempotent for such keys | |
| * @return WH_ERROR_NOTFOUND if the key is not resident in the cache, including | |
| * after an already-committed slot was reclaimed | |
| * @return the NVM layer's error, typically WH_ERROR_ABORTED, when the stored | |
| * flags cannot be read: an unenforceable policy fails closed | |
| * | |
| * A retry after a lost response can therefore see either code on a commit that | |
| * already succeeded; confirm with an export or metadata read. | |
| * Runs keystore policy checks before committing. Caller must hold the NVM lock. | |
| * | |
| * @param[in] server Server context | |
| * @param[in] keyId Key ID to commit | |
| * @return WH_ERROR_OK on success | |
| * @return WH_ERROR_ACCESS if an NVM object already exists under keyId and | |
| * carries WH_NVM_FLAGS_NONMODIFIABLE or WH_NVM_FLAGS_TRUSTED | |
| * @return WH_ERROR_NOTFOUND if the key is not resident in the cache | |
| * @return the NVM layer's error, typically WH_ERROR_ABORTED, if the stored | |
| * flags cannot be read | |
| @@ -229,6 +248,20 @@ int wh_Server_KeystoreEraseKeyChecked(whServerContext* server, whNvmId keyId); | |||
| * @brief Revoke a key (clears usage and marks non-modifiable) | |||
| * | |||
| * Placeholder implementation for key revocation. | |||
There was a problem hiding this comment.
Remove this line, as it is no longer a placeholder
| * Revoking rewrites the stored object from the cache slot, so the stored | ||
| * flags gate it whatever the cached copy says. WH_NVM_FLAGS_NONMODIFIABLE | ||
| * still permits it, so an already-revoked key can be revoked again. | ||
| * | ||
| * Caller must hold the NVM lock (the WH_KEY_REVOKE handler does): the verdict | ||
| * reads flags that the revoke then overwrites. The lock is not recursive. |
There was a problem hiding this comment.
| * Revoking rewrites the stored object from the cache slot, so the stored | |
| * flags gate it whatever the cached copy says. WH_NVM_FLAGS_NONMODIFIABLE | |
| * still permits it, so an already-revoked key can be revoked again. | |
| * | |
| * Caller must hold the NVM lock (the WH_KEY_REVOKE handler does): the verdict | |
| * reads flags that the revoke then overwrites. The lock is not recursive. | |
| * | |
| * Caller must hold the NVM lock |
| * @return int 0 on success, else a negative error code. WH_ERROR_ACCESS if a | ||
| * non-modifiable or server-trusted object already exists under keyId, so commit | ||
| * is not idempotent for those; WH_ERROR_NOTFOUND if the key left the cache; | ||
| * WH_ERROR_ABORTED if the stored flags are unreadable. A retry after a lost | ||
| * response can see either of the first two on a commit that succeeded. |
There was a problem hiding this comment.
| * @return int 0 on success, else a negative error code. WH_ERROR_ACCESS if a | |
| * non-modifiable or server-trusted object already exists under keyId, so commit | |
| * is not idempotent for those; WH_ERROR_NOTFOUND if the key left the cache; | |
| * WH_ERROR_ABORTED if the stored flags are unreadable. A retry after a lost | |
| * response can see either of the first two on a commit that succeeded. | |
| * @return int 0 on success, else a negative error code. WH_ERROR_ACCESS if a | |
| * non-modifiable or server-trusted object already exists under keyId. | |
| * WH_ERROR_ABORTED if the stored flags are unreadable. |
| * available, or a negative error code on failure. The server-side error codes | ||
| * are the ones documented on wh_Client_KeyCommit(). |
There was a problem hiding this comment.
| * available, or a negative error code on failure. The server-side error codes | |
| * are the ones documented on wh_Client_KeyCommit(). | |
| * available, or a negative error code on failure. |
|
|
||
| Results are printed via `WOLFHSM_CFG_PRINTF` from the wolfHSM build. `test-suite.log` contains the detailed output. | ||
|
|
||
| Some sub-tests are opt-in because they leave NVM objects that cannot be erased, so they occupy slots for the rest of the run. Build with `make PERSISTENT_NVM_ARTIFACTS=1` to define `WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS` and include them. Without it they are compiled out of their parent suite, which still runs and passes; unlike a whole entry point that is compiled out, no SKIPPED line is printed for them. |
There was a problem hiding this comment.
| Some sub-tests are opt-in because they leave NVM objects that cannot be erased, so they occupy slots for the rest of the run. Build with `make PERSISTENT_NVM_ARTIFACTS=1` to define `WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS` and include them. Without it they are compiled out of their parent suite, which still runs and passes; unlike a whole entry point that is compiled out, no SKIPPED line is printed for them. | |
| Some sub-tests are opt-in because they leave NVM objects that cannot be erased, so they occupy slots for the rest of the run. Build with `make PERSISTENT_NVM_ARTIFACTS=1` to define `WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS` and include them. |
Problem
wh_Server_KeystoreCommitKeywrites the cached key to NVM through theunchecked
wh_Nvm_AddObjectWithReclaim, so nothing consulted the storedobject's flags. A client could overwrite an existing
NONMODIFIABLEorTRUSTEDNVM object — including a trusted KEK — by caching a key under its idand committing. Closes f-4225.
Fix (
src/wh_server_keystore.c)Added a
WH_KS_OP_COMMITbranch to_KeystoreCheckPolicythat reads thestored object's metadata and denies the overwrite:
(unwrap-and-cache, SHE) cannot launder the policy by populating a slot whose
flags do not reflect the stored object.
NONMODIFIABLE | TRUSTED— the same pair aswh_Nvm_AddObjectChecked,the correct model for an add-shaped operation.
NONDESTROYABLEgatesdestroy, not overwrite.
nvmMetawhetheror not the cache slot survived, so a denied commit always reports
WH_ERROR_ACCESSinstead of theWH_ERROR_NOTFOUNDraised by a missing slot.cannot be policed, so the commit is denied rather than blind.
Client-visible behavior change: re-committing a cached key whose stored
object is
NONMODIFIABLEnow returnsWH_ERROR_ACCESSwhere a byte-identicalrewrite previously returned
WH_ERROR_OK. This makes commit consistent withevery other immutable-write path —
_NvmCheckPolicy(WH_NVM_OP_ADD) alreadyrefuses a no-op rewrite, and commit was the only exception. The contract is now
documented on
wh_Server_KeystoreCommitKeyChecked(
wolfhsm/wh_server_keystore.h): a client retrying after a lost response musttreat
WH_ERROR_ACCESSas "already committed". Revoke is unaffected; it keepsits own "already revoked and committed" short-circuit.
Tests
Added to
test-refactor/client-server/wh_test_crypto_keystore.c, driven onlyby
wh_Client_*:_whTest_NonModifiableCommit— first commit succeeds, repeat commit returnsWH_ERROR_ACCESS, the denial still holds once the slot is evicted, and thestored bytes and label survive the denial. Gated behind
WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS, since a committedNONMODIFIABLEobject cannot be erased._whTest_ModifiableRecommit— ungated; a key without the flag still commitsrepeatedly.
Making that gate runnable.
test-refactorcould not build with the macrodefined at all:
wh_test_crypto_keypolicy.ccallsWH_CLIENT_DEVID(client)inside a helper that takes no client context (pre-existing on
main, so bothgated suites were dead code). Threading the context into
whTest_RevocationTryAESEncryptfixes it, and a new trailing step in.github/workflows/build-and-test-refactor.ymlbuilds and runs with the macrodefined. That revives this PR's deny-path test and the keypolicy AES-CBC
revocation test, dormant since
606866e.Verification
DMA=1 ASAN=1-std=c90 -Werror -Wall -Wextra,ASan clean. Legacy
test/suite exits 0.WH_KS_OP_COMMITbranch reverted, the gatedrun fails —
Non-modifiable key was re-committed unexpectedly: 0.