Add 'unit-run' target, coverage via gcov. Fixed a bunch of fenrir reports. - #29
Open
danielinux wants to merge 21 commits into
Open
danielinux wants to merge 21 commits into
danielinux wants to merge 21 commits into
Conversation
The POSIX store mapped every failed read-open to WOLFPSA_STORE_NOT_AVAILABLE, so an existing record that could not be opened (permissions, I/O fault) surfaced as PSA_ERROR_INVALID_HANDLE instead of PSA_ERROR_STORAGE_FAILURE. Only ENOENT means the record is absent; anything else is a storage fault.
The store accepted any pre-existing directory as its location. A directory owned by another user, or writable by group or other, lets a local peer rename record files out from under the store and replace a stored key. Require the directory to be owned by the effective user with no group or other write bits, and fail closed with a storage error otherwise.
Close was void, so a failed atomic commit (rename of the temp record onto the final name) or a failed file close was silently discarded: a persistent key write could report success while the record was never stored. Close now returns WOLFPSA_STORE_IO_ERROR after aborting the temp record; the posix write path in psa_key_storage propagates it. New psa_store_commit_test drives the backend directly (a commit failure is not reachable deterministically through the public API) and links the static library, since the internal store API is not exported from the shared one.
The store and KDF regression tests added this month left their build artifacts untracked; add them to .gitignore alongside the others.
The HKDF-Expand step validator rejected SALT but let every other unrecognized step fall through to success, so a CONTEXT input was accepted and stored while the expand backend consumes only SECRET and INFO: the derivation silently produced the same output as one without it. The validator is now a positive whitelist - SECRET (exactly one hash length, first) and INFO (after SECRET) are accepted, everything else is rejected with PSA_ERROR_INVALID_ARGUMENT. New psa_kdf_expand_context_test checks the rejection, the INFO-before-SECRET ordering, and the RFC 5869 TC1 OKM. The vector test stays within one HKDF block: the second block is broken in the wolfSSL wc_HKDF_Expand_ex backend (a separate upstream bug).
psa_key_derivation_input_bytes appends each value to the step buffer, but only the SP800-108 branch rejected a step already recorded in steps_set, so a repeated SALT, INFO, LABEL, SEED, PASSWORD, or PBKDF2 input was silently concatenated instead of returning PSA_ERROR_BAD_STATE. The duplicate check now runs once for every KDF, before the per-algorithm dispatch, preserving the ordering checks; the redundant SP800-108 check is dropped. New psa_kdf_repeat_step_test covers every branch: for each KDF it sets the legal prefix steps and expects the repeated step to be rejected.
PSA_TLS12_PSK_TO_MS_PSK_MAX_SIZE (128) was advertised but never enforced: a 129-byte PSK was accepted, and a premaster component over 65535 bytes serialized with a wrapped 16-bit length. Reject an oversized SECRET and OTHER_SECRET in the input validator, and independently reject either premaster component that would wrap before it is serialized.
The direct-output path streams completed KDF blocks straight into the caller buffer, so a mid-stream failure left partial derived output behind. Zeroize the output when the backend compute fails. Regression test drives the failure through a crypto callback (first HMAC block succeeds, second fails) and asserts the buffer is zeroized.
Zero-length blobs with a supplied bit count slipped past the import for RSA/ECC/DH (no per-type length check; bits inference is skipped when bits are set) and were stored, leaving an unusable key that occupied its id. Reject data_length == 0 up front with INVALID_ARGUMENT.
psa_copy_key rejected copies whose destination lifetime differed from the source in both source paths, but the PSA API allows a different destination lifetime and local volatile/persistent copies share no security boundary. Drop the two lifetime-equality checks (psa_import_key still validates the destination location); update the psa_api_test case and add psa_copy_key_cross_lifetime_test for both directions.
Add a unit-run target that builds and runs all 50 unit tests in CI order, and a cov target that instruments the library and tests with gcov, runs the suite, and emits an HTML report of src/*.c line/function/branch coverage via gcovr (wolfIP/wolfBoot pattern). Ignore gcov artifacts. Verification: make unit-run -> 50/50 pass; make cov -> build/coverage/index.html (74.8% line / 87.3% function / 57.2% branch of src/*.c).
The software AES fallback uses secret-indexed T-table loads (a cache- timing channel). Enable WC_AES_BITSLICED (portable, constant-time by construction) and add a build-time guard in psa_aead.c that fails the build if neither WC_AES_BITSLICED nor WOLFSSL_AESNI is selected.
A second-look review of this branch found five real defects in the fixes, plus two store regression tests that were built but never run. Each fix has a regression test that fails without it. - psa_import_key: propagate the store Close() status on success (a successful write returns the byte count, not zero), so a failed commit no longer reports PSA_SUCCESS for an unstored key. - POSIX store: a failed fclose now marks the write failed, so the commit is aborted instead of renaming a truncated record over a good one. - KDF: exempt the PBKDF2 multi-part salt from the single-use rule (the PSA API allows it) and reject a repeated COST (its path bypasses the step validator). - Store: validate the directory on the read path too (F-10457). - Tests: wire the two store tests into unit-run/CI; add a close-commit-failure integration test; correct the PBKDF2 repeat-step test; make the cov target clean up its --coverage objects so the next plain build is not broken. make unit-run: green (52 tests).
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the AES build-matrix failure, HKDF INFO validation regression, and incomplete coverage cleanup.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR hardens PSA storage and KDF handling while adding constant-time AES enforcement, regression tests, and coverage tooling.
Changes:
- Improves storage validation and error propagation.
- Tightens KDF and key-import validation.
- Adds unit, coverage, regression-test, and CI support.
File summaries
| File | Summary |
|---|---|
wolfpsa/psa_store.h |
Updates store-close status API. |
user_settings.h |
Enables bitsliced AES. |
test/psa_server/psa_store_read_open_test.c |
Tests storage read-open errors. |
test/psa_server/psa_store_dir_validation_test.c |
Tests directory validation. |
test/psa_server/psa_store_commit_test.c |
Tests store commit behavior. |
test/psa_server/psa_kdf_zeroize_output_test.c |
Tests KDF output zeroization. |
test/psa_server/psa_kdf_repeat_step_test.c |
Tests repeated KDF-step rejection. |
test/psa_server/psa_kdf_psk_to_ms_size_test.c |
Tests PSK-to-MS size limits. |
test/psa_server/psa_kdf_expand_context_test.c |
Tests HKDF-Expand validation. |
test/psa_server/psa_import_zero_length_test.c |
Tests empty key-import rejection. |
test/psa_server/psa_import_key_probe_test.c |
Tests commit failure propagation. |
test/psa_server/psa_copy_key_cross_lifetime_test.c |
Tests cross-lifetime key copies. |
test/psa_server/psa_api_test.c |
Updates lifetime-copy expectations. |
test/Makefile |
Builds regression tests and coverage instrumentation. |
src/psa_store_zephyr.c |
Updates Zephyr store-close handling. |
src/psa_store_posix.c |
Validates directories and reports storage failures. |
src/psa_key_storage.c |
Handles storage errors and key validation. |
src/psa_key_derivation.c |
Adds KDF validation, bounds checks, and zeroization. |
src/psa_aead.c |
Requires a constant-time AES backend. |
Makefile |
Adds unit-run and coverage targets. |
.gitignore |
Ignores coverage and test artifacts. |
.github/workflows/test-psa-api.yml |
Runs regression tests in CI. |
Review details
Suppressed comments (2)
Makefile:249
make covcleandoes not remove the gcov files generated by the two custom sub-builds.psa_kdf_zeroize_output_testbuilds the instrumented library undertest/build-cryptocb*/obj, and the probe build is similarly undertest/build-probe*/; those.gcda/.gcnofiles survive this target and can be picked up by later reports. Remove those generated sub-build directories (or clean them recursively) here as well.
rm -f test/psa_server/*.gcda test/psa_server/*.gcno
src/psa_key_derivation.c:307
- This global duplicate-step check also rejects a second
PSA_KEY_DERIVATION_INPUT_INFOcall for HKDF. HKDF permits INFO to be supplied in multiple calls, with the chunks concatenated; the existing HKDF validator below already accepted INFO without a single-use check. As written, split HKDF info inputs now fail withPSA_ERROR_BAD_STATE, and the new repeat-step regression test will lock in non-compliant behavior. Include HKDF INFO in the multipart exception (while still rejecting it for HKDF-Extract in the algorithm-specific branch).
multipart_step = (PSA_ALG_IS_PBKDF2(ctx->alg) &&
step == PSA_KEY_DERIVATION_INPUT_SALT);
if (!multipart_step &&
(ctx->steps_set & wolfpsa_kdf_step_mask(step)) != 0) {
return PSA_ERROR_BAD_STATE;
- Files reviewed: 21/22 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The matrix uses build-test/user_settings.h (invariants) plus -D defines from build-variant.sh. F-13878 requires a constant-time AES backend, but WC_AES_BITSLICED was not defined in the matrix invariants, so the #error guard in psa_aead.c fired on every lane. Add it to the invariants (always required for consttime AES). The aes-ecb lane disabled HAVE_AES_ECB, which WC_AES_BITSLICED requires, so it is no longer a valid configuration. Drop it.
PSA treats the psa_sign_hash/psa_verify_hash hash as opaque bytes, so an all-zero digest is legal input (the PSA API suite signs one for SECP384R1/SHA-384); wolfCrypt rejects it by default, surfacing as PSA_ERROR_INVALID_ARGUMENT. Define WC_ALLOW_ECC_ZERO_HASH in the wolfCrypt config and fail the build in psa_ecc.c if a downstream config drops it.
Restore the lifetime-equality checks in the volatile and persistent source paths. The psa-arch-tests (the standard certification) expect a destination lifetime that differs from the source to fail (c044 'invalid lifetime'), so the rejection is correct and F-13859's premise (PSA supports a cross-lifetime copy) is contradicted by the certification suite. Update the cross-lifetime regression test to assert rejection for both cross directions and keep the same-lifetime cases.
Reject a key whose declared bit count does not match the curve implied by the key data length. Montgomery/Twisted-Edwards public keys are raw points (no 0x04 prefix), so use the full data length as the coordinate size for those families; this keeps X25519/X448 import working.
The default software AES uses secret-indexed T-table loads (a cache-timing channel). WC_AES_BITSLICED is the portable consttime core; psa_aead.c fails the build if neither it nor WOLFSSL_AESNI is selected (F-13878).
danielinux
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
September 17, 2026 20:51
wolfSSL-Fenrir-bot
previously requested changes
Sep 17, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #29
Scan targets checked: wolfpsa-src, wolfpsa-bugs
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
- Require the exact 1 + 2*coord length and 0x04 prefix for imported Weierstrass public keys. The old code floored the length and never checked the prefix, so a 66-byte or wrong-prefix P-256 key was stored. - Scope the ECC zero-hash and AES backend #error guards to builds where wolfPSA actually controls the wolfCrypt config (WOLFSSL_SETTINGS_FILE / WOLFSSL_PSA_ENGINE + !NO_AES), so the module-default config and inactive sources do not fail the build. - build-test: define WC_ALLOW_ECC_ZERO_HASH so matrix lanes accept an all-zero ECDSA digest.
wolfSSL-Fenrir-bot
previously requested changes
Sep 17, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #29
Scan targets checked: wolfpsa-src, wolfpsa-bugs
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
psa_tls_coexist used the module-default Kconfig config, which selects neither WC_AES_BITSLICED nor WOLFSSL_AESNI, failing the psa_aead.c consttime-AES guard (F-13878) on Zephyr v4.3.0 and v4.4.0. Add a coexistence settings file (the example feature set minus WOLFCRYPT_ONLY, so the TLS layer stays in the build) that selects WC_AES_BITSLICED, and point CONFIG_WOLFSSL_SETTINGS_FILE at it.
psa_aead.c: require WC_AES_BITSLICED (not 'or WOLFSSL_AESNI') - the AES-NI runtime fallback (AesSetKey_C) is the same non-constant-time T-table path, so AES-NI alone leaves a timing channel when the instruction set is unavailable. psa_ecc.c: scope the WC_ALLOW_ECC_ZERO_HASH guard to every PSA ECC build (WOLFSSL_PSA_ENGINE && HAVE_ECC), not just WOLFSSL_SETTINGS_FILE builds. psa_key_storage.c: family-specific 448-bit lengths - X448 is 56 bytes (Montgomery, x-only), Ed448 is 57 bytes (Twisted-Edwards, full point); a 57-byte X448 or 56-byte Ed448 key no longer maps to 448 bits and gets stored.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #29
Scan targets checked: wolfpsa-src, wolfpsa-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
wolfSSL-Fenrir-bot
dismissed stale reviews from themself
September 18, 2026 00:38
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add 'unit-run' target, coverage via gcov. Fixed a bunch of fenrir reports.
6553cdf F-13878: require a constant-time AES backend
1e717da test: add make unit-run and gcov coverage (make cov)
54a4249 F-13859: permit cross-lifetime psa_copy_key112b95d F-13856: reject zero-length key import
e34d843 F-10453: zeroize KDF output on mid-stream error
42cd2d8 F-11573: enforce PSK-TO-MS PSK size limit and 16-bit premaster bound
94b14b0 F-11572: reject repeated single-use steps in all KDF validators
b6d7d31 F-11571: reject unconsumed steps in HKDF-Expand validation
3eaf694 test: ignore new regression test binaries
551bcea F-8726: return commit/close status from wolfPSA_Store_Close
f999b6c F-10457: reject store directories that are not private
a27998a F-10431: map non-ENOENT store read-open failures to I/O error