Enforce NONMODIFIABLE and TRUSTED NVM policy on keystore key commit

[yosuke-wolfssl]

Problem

wh_Server_KeystoreCommitKey writes the cached key to NVM through the
unchecked wh_Nvm_AddObjectWithReclaim, so nothing consulted the stored
object's flags. A client could overwrite an existing NONMODIFIABLE or
TRUSTED NVM object — including a trusted KEK — by caching a key under its id
and committing. Closes f-4225.

Fix (src/wh_server_keystore.c)

Added a WH_KS_OP_COMMIT branch to _KeystoreCheckPolicy that reads the
stored object's metadata and denies the overwrite:

• Stored flags, not cached flags — an unchecked cache path
  (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 as wh_Nvm_AddObjectChecked,
  the correct model for an add-shaped operation. NONDESTROYABLE gates
  destroy, not overwrite.
• Independent of cache residency — the verdict comes from nvmMeta whether
  or not the cache slot survived, so a denied commit always reports
  WH_ERROR_ACCESS instead of the WH_ERROR_NOTFOUND raised by a missing slot.
• Unreadable flags fail closed — a backend that cannot report metadata
  cannot be policed, so the commit is denied rather than blind.

Client-visible behavior change: re-committing a cached key whose stored
object is NONMODIFIABLE now returns WH_ERROR_ACCESS where a byte-identical
rewrite previously returned WH_ERROR_OK. This makes commit consistent with
every other immutable-write path — _NvmCheckPolicy (WH_NVM_OP_ADD) already
refuses 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 must
treat WH_ERROR_ACCESS as "already committed". Revoke is unaffected; it keeps
its own "already revoked and committed" short-circuit.

Tests

Added to test-refactor/client-server/wh_test_crypto_keystore.c, driven only
by wh_Client_*:

• _whTest_NonModifiableCommit — first commit succeeds, repeat commit returns
  WH_ERROR_ACCESS, the denial still holds once the slot is evicted, and the
  stored bytes and label survive the denial. Gated behind
  WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS, since a committed
  NONMODIFIABLE object cannot be erased.
• _whTest_ModifiableRecommit — ungated; a key without the flag still commits
  repeatedly.

Making that gate runnable. test-refactor could not build with the macro
defined at all: wh_test_crypto_keypolicy.c calls WH_CLIENT_DEVID(client)
inside a helper that takes no client context (pre-existing on main, so both
gated suites were dead code). Threading the context into
whTest_RevocationTryAESEncrypt fixes it, and a new trailing step in
.github/workflows/build-and-test-refactor.yml builds and runs with the macro
defined. That revives this PR's deny-path test and the keypolicy AES-CBC
revocation test, dormant since 606866e.

Verification

Config	Gate off	Gate on
default	45 passed, 26 skipped, 0 failed of 71	45 / 26 / 0
DMA=1 ASAN=1	49 passed, 22 skipped, 0 failed of 71	49 / 22 / 0

• Identical counts either way; clean under -std=c90 -Werror -Wall -Wextra,
  ASan clean. Legacy test/ suite exits 0.
• Negative control: with only the WH_KS_OP_COMMIT branch reverted, the gated
  run fails — Non-modifiable key was re-committed unexpectedly: 0.

[Copilot]

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_COMMIT policy branch that consults the stored NVM object’s flags and denies overwrite when NONMODIFIABLE or TRUSTED.
• 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]

Fenrir Automated Review — PR #492

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

No new issues found in the changed files. ✅

[Frauschi]

🐺 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

[yosuke-wolfssl]

Hello @Frauschi ,
I fixed this based on your feedbacks.
Could you review it again ?

[Frauschi]

🐺 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

[Frauschi]

🐺 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

[Frauschi]

🟡 [Medium] Commit is no longer idempotent for NONMODIFIABLE keys, and the contract is undocumented
💡 SUGGEST api

The new WH_KS_OP_COMMIT policy branch makes a byte-identical re-commit of a NONMODIFIABLE key return WH_ERROR_ACCESS where it previously returned WH_ERROR_OK. This is a deliberate, defensible change — docs/src/5-Features.md:211 already documents NONMODIFIABLE as "the object cannot be overwritten or destroyed through the policy-checked APIs", and _NvmCheckPolicy/WH_NVM_OP_ADD already refuses a no-op rewrite — so the fix aligns code with documented intent.

What is missing is the contract update. The doxygen for wh_Server_KeystoreCommitKeyChecked in wolfhsm/wh_server_keystore.h:163-167 still says only "Runs keystore policy checks before committing", with no @return block and no mention of WH_ERROR_ACCESS. The practical consequence the PR body itself identifies — a client retrying a commit after a dropped response now sees a hard failure on a commit that actually succeeded — is a client-visible behavior change that integrators need to be told about.

Suggestion:

Suggested change

	case WH_KS_OP_COMMIT:
	/**
	* @brief Commit a cached key to NVM with policy enforcement
	*
	* Runs keystore policy checks before committing. The verdict is taken from
	* the flags of the *stored* NVM object, not the cache slot, so an unchecked
	* cache path cannot launder them.
	*
	* @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. Note
	* this makes commit non-idempotent for such keys: a repeated commit
	* of unchanged bytes is refused, so a client retrying after a lost
	* response must treat WH_ERROR_ACCESS as "already committed".
	* @return WH_ERROR_NOTFOUND if the key is in neither cache nor NVM
	*/
	int wh_Server_KeystoreCommitKeyChecked(whServerContext* server, whNvmId keyId);

[yosuke-wolfssl]

Applied as suggested. wh_Server_KeystoreCommitKeyChecked now documents the
full contract:

• @param[in] server / @param[in] keyId
• WH_ERROR_OK on success
• WH_ERROR_ACCESS when an NVM object already exists under keyId carrying
  WH_NVM_FLAGS_NONMODIFIABLE or WH_NVM_FLAGS_TRUSTED, with the explicit
  note that commit is therefore not idempotent for such keys and a client
  retrying after a lost response must treat WH_ERROR_ACCESS as "already
  committed"
• WH_ERROR_NOTFOUND when the key is in neither cache nor NVM

It also states that the verdict comes from the stored object's flags rather
than the cache slot, which is the property the fix depends on.

[Frauschi]

🟡 [Medium] New NONMODIFIABLE commit regression test never compiles or runs in any buildable configuration
💡 SUGGEST

The only test that exercises the behavior this PR adds (_whTest_NonModifiableCommit) is compiled out in every configuration that currently builds, so the security fix ships with no automated coverage of its deny path. Verified empirically in this worktree:

1. Default build (make check WOLFSSL_DIR=...): 45 passed / 26 skipped / 0 failed, and test-refactor/posix/test-suite.log contains only MODIFIABLE COMMIT TEST SUCCESS — the Testing non-modifiable commit enforcement... banner never appears.
2. Forcing the gate on (CFLAGS_EXTRA=... -DWOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS, after make clean) fails the build at test-refactor/client-server/wh_test_crypto_keypolicy.c:527: error: use of undeclared identifier 'client' (pre-existing on main, and correctly disclosed in the PR description). Because the macro is a whole-build -D, not a per-file define, there is no way to enable the new test without hitting that error.

The consequence is not merely "the test is skipped": nothing in CI compiles this function, so it can silently rot (unused statics, renamed APIs, changed error codes) and no regression of _KeystoreCheckPolicy's WH_KS_OP_COMMIT branch will be caught. The other new test, _whTest_ModifiableRecommit, is ungated but only exercises the allow path — deleting the entire WH_KS_OP_COMMIT block from src/wh_server_keystore.c still leaves it passing. So the net test delta that actually executes cannot detect a regression of the fix.

I confirmed the test itself is sound: after locally repairing the keypolicy.c:527 blocker, the gated build runs both new tests and passes (NON-MODIFIABLE COMMIT TEST SUCCESS), and a negative control that reverts only the WH_KS_OP_COMMIT branch produces whTest_Crypto_Keystore test FAILED (rc=-1). The test works — it just never runs.

Recommendation: Either (a) fix the one-line blocker in the same PR so the gate becomes usable — whTest_RevocationTryAESEncrypt needs the client context threaded in:

static int whTest_RevocationTryAESEncrypt(whClientContext* client,
whKeyId keyId, WC_RNG* rng,
int* encryptRes)
...
ret = whTest_RevocationTryAESEncrypt(client, keyId, rng, &encryptRes);

(verified: this makes the gated build compile and both new tests pass); or (b) restructure _whTest_NonModifiableCommit so it does not need the gate. The stored-flags denial can be exercised without stranding an undeletable NVM object by making the cached slot NONMODIFIABLE only, e.g. commit a plain key, then re-cache it and verify the denial comes from NVM metadata — no permanent artifact, so the test runs in the default config. If neither is done, at minimum add an ungated deny-path assertion so the fix has some executing coverage.

[Frauschi]

As already mentioned above, I'd be fine if this is handled in a follow-up PR.

[yosuke-wolfssl]

Fixed in this PR rather than deferred, and the root cause turned out to be
broader than the one blocker.

The macro is defined in test/config/wolfhsm_cfg.h:65. The legacy harness
can afford it because whTest_ClientServerSequential
(test/wh_test_clientserver.c:839) declares uint8_t memory[FLASH_RAM_SIZE]
on its own stack and builds the ramsim NVM from it per scenario, so an
undeletable object dies when that function returns. test-refactor shares one
static _flashMem and one whTestPosix_Server_Init
(test-refactor/posix/wh_test_posix_server.c:67) across all 71 tests — which
is what "Not implemented yet" in the config was recording. I replaced that
comment with the actual reason.

Two changes:

• whTest_RevocationTryAESEncrypt now takes whClientContext* client
  (your option (a)), 5 call sites updated — the gate could not compile at all
  before this.
• A trailing step in build-and-test-refactor.yml builds with
  CFLAGS_EXTRA=-DWOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS ASAN=1.
  Placed last by convention; cross-step contamination is not possible either
  way, since each step is its own make clean && make && make run and
  whFlashRamsim_Init memsets the whole array to erasedByte at init.

Results:

Config	Gate off	Gate on
default	45 passed, 26 skipped, 0 failed / 71	45 passed, 26 skipped, 0 failed / 71
DMA=1 ASAN=1	49 passed, 22 skipped, 0 failed / 71	49 passed, 22 skipped, 0 failed / 71

Identical counts — the accumulated artifacts break nothing, ASan clean. Two
previously-dead tests now execute: NON-MODIFIABLE COMMIT TEST SUCCESS (this
PR's deny path) and AES-CBC revocation enforcement: PASS, which had been
dormant since 606866e. Legacy test/ suite still exits 0.

Negative control: with only the WH_KS_OP_COMMIT branch reverted, the gated
run fails with Non-modifiable key was re-committed unexpectedly: 0.