Conversation
F/2052
F/2418
There was a problem hiding this comment.
Pull request overview
This PR batches a set of wolfPSA correctness/security hardening changes plus expanded regression test coverage across key storage, KDF, cipher/AEAD, MAC, hash, and init flows.
Changes:
- Add extensive PSA API regression tests (export/copy key, policy mismatch, error-state abort behavior, KDF policy/sequence, RSA OAEP, etc.) and a new
psa_crypto_inittest binary. - Tighten key/policy validation (reject
PSA_ALG_NONEkey operations, reject secondaryalg2, validate stored key lengths, enforce ChaCha20 key sizing, fix ECC public-bit inference for raw curves). - Improve scrubbing/cleanup on failures (cipher/MAC/hash/AEAD contexts) and require
psa_crypto_init()for hash/random entry points.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
wolfpsa/psa_key_storage.h |
Adds new exported test-hook declarations for key-id sequencing. |
wolfpsa.map |
Exports the new test-hook symbols from the shared library ABI. |
test/psa_server/psa_ecc_bit_inference_test.c |
Refactors and expands ECC bit-inference test coverage (including raw public keys). |
test/psa_server/psa_crypto_init_test.c |
New test validating init preconditions and mapping of wolfCrypt_Init() failures. |
test/psa_server/psa_api_test.c |
Large expansion of PSA API regression coverage (policies, error states, KDF behavior, etc.). |
test/Makefile |
Builds the new crypto init test and adds a new link rule with wrapping. |
src/psa_random.c |
Rejects RNG usage before psa_crypto_init(). |
src/psa_mac.c |
Ensures error paths abort/zeroize operations; tightens MAC policy checks. |
src/psa_key_storage.c |
Validates stored key sizes, rejects alg2, validates ChaCha20 sizing, key-id wrap guard, ECC bit inference fix. |
src/psa_key_derivation.c |
Adds sequential-output tracking, zero-input handling, policy tightening, HKDF extract prefix support, PBKDF2 guards. |
src/psa_hash_engine.c |
Requires init for hash entry points and aborts/zeroizes operations on error. |
src/psa_engine.c |
Enforces ChaCha20 key size validity (256-bit only). |
src/psa_crypto.c |
Implements psa_crypto_init() via wolfCrypt_Init() and tracks init state. |
src/psa_cipher.c |
Aborts/zeroizes multipart cipher ops on error; fixes CCM* no-tag multipart update behavior; tighter key policy enforcement. |
src/psa_asymmetric_api.c |
Enforces PSA_ALG_NONE rejection and algorithm mismatch policy for asymmetric keys. |
src/psa_aead.c |
Normalizes empty buffers, improves scrubbing, and fixes ChaCha20-Poly1305 finish buffer handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 11 total — 8 posted, 3 skipped
Posted findings
- [Medium] KDF output cache size calculation may overflow —
src/psa_key_derivation.c:1185 - [Medium] ChaCha20 AEAD decrypt path does not zero tmp buffer before free —
src/psa_aead.c:704 - [Medium] ChaCha20 AEAD decrypt path missing overflow check on ciphertext_len —
src/psa_aead.c:691 - [Medium] Test-only functions exported in production shared library symbol map —
wolfpsa.map:92-93 - [Medium] psa_crypto_init is not thread-safe —
src/psa_crypto.c:42-58 - [Low] psa_cipher_finish does not call abort on success paths —
src/psa_cipher.c:1211-1334 - [Low] Static local in wolfpsa_aead_nonnull_data may not be thread-safe conceptually —
src/psa_aead.c:84 - [Medium] Missing capacity decrement tracking for raw KDF sequential output —
src/psa_key_derivation.c:1119-1124
Skipped findings
- [High] wolfpsa_kdf_compute_output raw_kdf path reads from wrong offset
- [High] Volatile key ID wraparound check is incomplete
- [Medium] wolfpsa_aead_nonnull_data does not guard against non-NULL data with zero length
Review generated by Skoll via openclaw
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/psa_hash_engine.c:641
psa_hash_finish()frees the heap-allocated operation context directly afterpsa_hash_cleanup_ctx(ctx)without scrubbing thepsa_hash_operation_ctx_tstruct itself. Sincepsa_hash_abort()now force-zeros the context before free, it would be more consistent (and safer) towc_ForceZero(ctx, sizeof(*ctx))here as well beforeXFREE().
*hash_length = expected_hash_size;
ctx->finalized = 1;
psa_hash_cleanup_ctx(ctx);
XFREE(ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER);
operation->opaque = (uintptr_t)NULL;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
F/2404 - Add
psa_export_keyusage test (712dff3)F/2405 - Add
psa_copy_keyAPI coverage (872ab2e)F/2421 - Fix sequential KDF output slices (
bd0368b)F/2396 - Reject
PSA_ALG_NONEkey operations (f76ded2)F/2406 - Add cipher algorithm mismatch policy test (
dff642e)F/2408 - Add KDF input key policy coverage (
1fa2a94)F/2409 - Add key agreement KDF policy coverage (
57e1105)F/2410 - Add AEAD policy mismatch coverage (
bab54f3)F/2414 - Add MAC policy regression coverage (
8227f5c)F/2422 - Allow multipart
CCM_STAR_NO_TAGupdates (d321c58)F/2423 - Allow partial HKDF-Extract output (
e1d65f8)F/2416 - Scrub cipher context on setup failure (
1f1c240)F/2417 - Scrub MAC setup failure state (
4ef79ee)F/2051 - Fix ECC public bit inference (
ce6ceb0)F/2052 - Reject zero PBKDF2 cost (
f69741b)F/2053 - Reject invalid ChaCha20 key sizes (
e82a02a)F/2397 - Abort PSA multipart ops on error (
08c2c13)F/2411 - Add RSA OAEP asymmetric API coverage (
0eaa83c)F/2412 - Add KDF
verify_keypolicy coverage (e217aff)F/2413 - Add PSA generate key public type test (
3127df7)F/2418 - Zero AEAD stack AES state (
50f6c03)F/2419 - Zero AEAD context before free (
c0a069e)F/2054 - Fix ChaCha20-Poly1305 multipart finish buffers (
7f6b0cc)F/2055 - Guard volatile key ID wraparound (
ce2e7cf)F/2398 - Reject unsupported secondary key algorithms (
d2e4a1d)F/2399 - Initialize wolfCrypt from
psa_crypto_init(8c50962)F/2049 - Force zero key buffer on read failure (
bfcd563)F/2050 - Validate stored key lengths before reads (
ec67dc4)F/2420 - Zero hash context before abort free (
e2e910b)F/2400 - Require PSA init for hash entry points (
24750b8)F/2403 - Guard
psa_generate_randombefore init (fd989c2)F/2407 - Add asymmetric algorithm mismatch regression (
dfe9540)F/2425 - Normalize empty AEAD buffers (
71ad729)F/2426 - Guard zero-length PBKDF2 inputs (
65d0673)