Fenrir fixes 2026 08 05 - #22
Merged
Merged
Conversation
test_cipher_cbc_pkcs7_multipart_decrypt and friends only exercised valid-padding roundtrips, so no test ever expected PSA_ERROR_INVALID_PADDING from psa_cipher_finish. Add test_cipher_cbc_pkcs7_reject_invalid_padding: encrypt a one-block plaintext, flip one bit in the ciphertext block preceding the padding block (which, via CBC chaining P_last = Dec(C_last) XOR C_prev, deterministically corrupts exactly one byte of the recovered padding while pad_len itself stays valid), and assert psa_cipher_finish rejects it. Verified this fails if the padding-byte comparison loop is deleted.
test_missing_usage only covered psa_encapsulate lacking ENCRYPT and decapsulating with a public key (wrong type). No test exercised the key_usage & PSA_KEY_USAGE_DECRYPT check in psa_decapsulate itself, so removing or inverting that check would go undetected. Add test_decapsulate_missing_usage: generate an ML-KEM key pair with ENCRYPT but not DECRYPT, encapsulate successfully, then assert psa_decapsulate returns PSA_ERROR_NOT_PERMITTED.
…unwrap psa_wrap_key/psa_unwrap_key reject a KEK whose permitted algorithm is not PSA_ALG_KW (or is PSA_ALG_NONE), even if it carries WRAP/UNWRAP usage. No existing test exercised this branch since every KEK in the suite was imported with its permitted algorithm equal to the alg passed in. Add a test importing KEKs with PSA_ALG_CTR and PSA_ALG_NONE to pin the PSA_ERROR_NOT_PERMITTED behavior on both entry points.
psa_hash_verify was a live public API with no test in the repo, leaving its length check and ConstantCompare deletable by mutation without any test failure. Add setup/update/verify coverage for the correct-digest, one-byte-flipped, and wrong-length cases, mirroring the existing one-shot psa_hash_compare test.
test_kdf_verify_key_policy only exercised the matching-value path of psa_key_derivation_verify_bytes/verify_key, so a mutation that made the ConstantCompare mismatch branch unreachable (always PSA_SUCCESS) would go undetected. Add HKDF cases that flip one byte of the expected value and assert PSA_ERROR_INVALID_SIGNATURE for both APIs.
…verage psa_asymmetric_verify_rsa() compared the padding-stripped output of wc_RsaSSL_Verify_ex2() directly against the caller's raw hash for PSA_ALG_RSA_PKCS1V15_SIGN(hash). Since wc_RsaUnPad_ex() only removes the PKCS#1 v1.5 padding and does not strip the DER DigestInfo wrapper that wc_EncodeSignature() added on the sign side, the recovered value is the full DigestInfo, never equal in length to the raw hash: verification of a validly-signed value always failed. Fix it by re-encoding the expected DigestInfo (same as the sign path) and comparing against that, matching PSA_ALG_RSA_PKCS1V15_SIGN_RAW and RSA-PSS which already round-trip correctly. Add negative test coverage (signature-substitution across RAW, hashed PKCS#1v1.5 and PSS) that exercises the hash-binding checks this finding flagged as untested.
There was a problem hiding this comment.
Pull request overview
This PR strengthens PSA Crypto correctness and negative-path handling by fixing RSA PKCS#1 v1.5 hash-binding verification and expanding test coverage for multiple policy/validation failure modes.
Changes:
- Fix RSA PKCS#1 v1.5 hashed-verify to compare against DER-encoded DigestInfo (not raw hash bytes).
- Add new negative/edge-case tests for: ML-KEM decapsulate usage-policy, KW/unwrap KEK algorithm-policy, HKDF verify mismatch, multipart psa_hash_verify, and CBC-PKCS7 invalid padding detection.
- Extend test runners to execute the newly added cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/psa_server/psa_mlkem_test.c | Adds negative test ensuring decapsulate rejects keypairs missing DECRYPT usage. |
| test/psa_server/psa_key_wrap_test.c | Adds KEK algorithm-policy negative coverage for wrap/unwrap. |
| test/psa_server/psa_api_test.c | Adds new test cases for hash verify, CBC-PKCS7 invalid padding, RSA substituted-hash rejection, and HKDF mismatch verification. |
| src/psa_rsa.c | Fixes PKCS#1 v1.5 hashed verify to validate the recovered DigestInfo against an encoded DigestInfo for the provided hash. |
Suppressed comments (2)
test/psa_server/psa_api_test.c:350
- Same as above: if setup/update fails, abort the hash operation before returning to avoid leaking operation state/resources into later tests.
st = psa_hash_setup(&op, PSA_ALG_SHA_256);
if (check_status(st, "psa_hash_setup(verify mismatch)") != TEST_OK) return TEST_FAIL;
st = psa_hash_update(&op, msg, sizeof(msg) - 1);
if (check_status(st, "psa_hash_update(verify mismatch)") != TEST_OK) return TEST_FAIL;
test/psa_server/psa_api_test.c:361
- Same as above: ensure psa_hash_abort(&op) is called before returning on setup/update failure so operation resources are released deterministically.
st = psa_hash_setup(&op, PSA_ALG_SHA_256);
if (check_status(st, "psa_hash_setup(verify wrong length)") != TEST_OK) return TEST_FAIL;
st = psa_hash_update(&op, msg, sizeof(msg) - 1);
if (check_status(st, "psa_hash_update(verify wrong length)") != TEST_OK) return TEST_FAIL;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot flagged the encoded[RSA_MAX_SIZE/8] buffer added in 7067e3e as a stack-usage concern. It is worse than that: wc_EncodeSignature() writes a DER header plus hash_length bytes with no bound of its own, and hash_length is caller-supplied all the way down. wolfpsa_verify_hash_worker() only null-checks it and psa_asymmetric_verify_rsa() only range-checks it against word32, so the buffer size was the overflow bound rather than a headroom margin. The verify path is the regression: the DigestInfo encode happens before wc_RsaSSL_Verify_ex2(), so psa_verify_hash() with a public key and an over-long hash_length overruns the 384-byte stack buffer without needing a valid signature. ASan, on this branch before the fix: ERROR: AddressSanitizer: stack-buffer-overflow WRITE of size 4096 wc_EncodeSignature asn.c:16193 psa_asymmetric_verify_rsa src/psa_rsa.c:303 psa_verify_hash src/psa_asymmetric_api.c:713 [496, 880) 'encoded' (line 300) Size the buffer as Copilot suggested (PSA_HASH_MAX_SIZE + 32, matching the sign path) and add the range check that makes that sizing safe. psa_asymmetric_sign_rsa() has the same unbounded encode into a 96-byte heap buffer at psa_rsa.c:185. That one predates this branch and reproduces on master at 62f2931; it needs SIGN_HASH usage, so it is lower severity, but it is the same defect and is guarded here too. Add test_rsa_pkcs1v15_rejects_oversized_hash_length covering both entry points. The verify case signs a correctly sized hash first so the signature carries valid PKCS#1 padding, otherwise the call fails padding validation before reaching the guard and would pass for the wrong reason. Confirmed the test trips ASan when the guards are reverted. Also abort the hash operation on the update-failure paths of test_hash_verify_multipart, per Copilot's other comment, matching the existing convention.
Frauschi
reviewed
Aug 6, 2026
Frauschi
left a comment
Frauschi
left a comment
Contributor
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 20 total — 9 posted, 11 skipped
Posted findings
- [Medium] Hashed PKCS#1 v1.5 verify folds unsupported-hash, oversized-hash and allocation failures into PSA_ERROR_INVALID_SIGNATURE; split them into explicit early-return guards mirroring the sign path —
src/psa_rsa.c:308-332 - [Medium] hash_length is bounded by PSA_HASH_MAX_SIZE rather than what wolfCrypt can actually produce, so a trimmed PSA_WANT_ config rejects valid digests* —
src/psa_rsa.c:177-184,308-317 - [Medium] test_rsa_verify_rejects_bad_signatures swallows TEST_SKIPPED and reports a pass when no sub-case actually ran —
test/psa_server/psa_api_test.c:5824-5851 - [Medium] PSS sub-case hard-fails instead of skipping on a build without WC_RSA_PSS —
test/psa_server/psa_api_test.c:5684-5695,5842-5846 - [Medium] Substituted-hash rejection assertion is
st != PSA_SUCCESS, too weak to catch a regression in psa_verify_hash —test/psa_server/psa_api_test.c:5706-5716 - [Low] Helper hardcodes sig[128] and 1024-bit keys while taking key size as a parameter —
test/psa_server/psa_api_test.c:5671,5825,5834,5843 - [Low] Test name and CLI selector do not match what the test does —
test/psa_server/psa_api_test.c:5800,9020-9025 - [Low] Fenrir finding ID F-6970 embedded in a source comment —
test/psa_server/psa_api_test.c:2246 - [Low] KDF test reuses attrs without psa_reset_key_attributes, unlike every other reuse in the function —
test/psa_server/psa_api_test.c:7286-7288
Skipped findings
- [Low] DigestInfo re-encode is only exercised with SHA-256; the +32 header headroom is untested
- [Low] Bare return bypasses the cleanup label in the new PKCS7 padding test
- [Low] test_hash_verify does not cover the NULL-argument branch of psa_hash_verify
- [Low] psa_hash_operation_t declared without an initializer
- [Low] psa_destroy_key return value discarded without a (void) cast
- [Low] PSA_ALG_NONE half of the new key-wrap policy test does not cover a distinct branch
- [Low] New negative tests do not assert that rejected calls left their output parameters untouched
- [Info] Mutable static fixture where the file uses static const or plain locals
- [Info] Helper continuation parameters misaligned by one column
- [Info] New main() entry in psa_mlkem_test.c is misaligned by one column
- [Info] Encapsulate step in test_decapsulate_missing_usage is not load-bearing for the branch under test
Review generated by Skoll via Claude/Codex
Status codes (src/psa_rsa.c): the hashed PKCS#1 v1.5 verify branch folded an unsupported hash OID, an unencodable hash_length and an encode failure into encoded_len == 0, all reported as PSA_ERROR_INVALID_SIGNATURE. The sign path returns NOT_SUPPORTED and INVALID_ARGUMENT for the same two conditions, so the entry points disagreed on identical bad input, and a build problem was indistinguishable from an attack. Split them into explicit early-return guards mirroring the sign path; an encode failure now reports INSUFFICIENT_MEMORY, since wc_EncodeSignature() returning 0 means its internal allocation failed. This also lifts the buffer-filling call out of the ternary initializer. Digest bound: both guards used PSA_HASH_MAX_SIZE, derived from the PSA_WANT_ALG_SHA* set, while wc_GetCTC_HashOID() reflects the wolfCrypt build. A Zephyr config selecting only CONFIG_PSA_WANT_ALG_SHA_256 gets PSA_HASH_MAX_SIZE 32 while wolfCrypt still encodes SHA-512, so a valid 64-byte digest would be rejected. Bound both guards and both DigestInfo buffers by WC_MAX_DIGEST_SIZE via one shared WOLFPSA_RSA_DIGESTINFO_MAX. test_rsa_pkcs1v15_rejects_oversized_hash_length: the verify half asserted INVALID_SIGNATURE, which the vulnerable code also returns once the overrun DigestInfo fails the following length comparison, so without a sanitizer it passed against the bug it was written to pin. It now asserts INVALID_ARGUMENT. Verified: with the guard removed but the buffer enlarged so no overflow occurs, the assertion fails on its own. test_hash_verify_rejects_substituted_hash: propagate skips instead of returning TEST_OK when every sub-case skipped, which reported a pass having executed zero assertions; treat NOT_SUPPORTED from psa_sign_hash as a skip so a build without WC_RSA_PSS skips rather than fails; assert the exact expected status per algorithm instead of any non-success, and print it on mismatch; size sig from PSA_SIGNATURE_MAX_SIZE and move to 2048-bit keys for consistency with the sibling test. Renamed the caller and its selector to rsa_verify_substituted_hash, since it substitutes hashes rather than corrupting signatures. Also drop an internal finding ID from a test comment and add the missing psa_reset_key_attributes() before the HKDF mismatch-key import.
Four review findings, all in the RSA sign/verify pair. hash_length was bounded only by WC_MAX_DIGEST_SIZE, which is the buffer precondition but not the semantic one. PSA requires the digest to be exactly PSA_HASH_LENGTH(PSA_ALG_SIGN_GET_HASH(alg)). Without that check wc_EncodeSignature() wraps a 20-byte digest in a DigestInfo carrying the SHA-256 OID, and because the verify path re-encodes the same way, the malformed structure round-tripped: sign(20-byte hash under SHA-256) -> 0 verify(same 20-byte hash) -> 0 which violates RFC 8017 A.2.4. Previously only the sign side was lax and hashed verify always failed, so this became observable only once verify started working. Both guards now check the exact length and keep the WC_MAX_DIGEST_SIZE term as the buffer backstop, since the two constants can diverge. Confirmed the same input now returns INVALID_ARGUMENT on both entry points while the correct 32-byte length still round-trips. RSA-PSS reported a hash mismatch as PSA_ERROR_INVALID_PADDING, which psa_verify_hash does not define; the spec requires INVALID_SIGNATURE for any signature that does not verify. The previous commit's test pinned the non-conformant status as expected behaviour, which would have left the psa-arch-tests job as the only thing able to catch it. Normalize the padding-check failure to SIG_VERIFY_E and assert INVALID_SIGNATURE for all three sub-cases. WOLFPSA_RSA_DIGESTINFO_MAX used a hand-picked +32 header margin, below wolfCrypt's own declared worst case. It is safe for every OID currently encodable (real overhead is 19-21 bytes) but rests on inspection of the OID table rather than on a published bound - the same class of accidental margin the guard was added to remove. Use MAX_DER_DIGEST_ASN_SZ, taking the buffer from 96 to 100 bytes. The sign path still assigned wc_EncodeSignature()'s return straight into sig_input_len while verify checked it, so an allocation failure surfaced as INVALID_ARGUMENT via wc_RsaSSL_Sign(sig_input, 0, ...) on one side and INSUFFICIENT_MEMORY on the other. Mirror the verify-side check. Extend test_rsa_pkcs1v15_rejects_oversized_hash_length with the under-sized case on both entry points.
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.
7067e3e F-6071: fix RSA PKCS#1v1.5 hashed-verify hash binding and add test coverage
26e2d1d F-6072: add mismatch-value coverage for KDF verify_bytes/verify_key
dc1a7f5 F-6073: add multipart test coverage for psa_hash_verify
898d164 F-6252: add negative test for KEK algorithm-policy check in key wrap/unwrap
934c0d6 F-6253: add negative test for psa_decapsulate DECRYPT usage policy
7da3d84 F-6970: add negative test for CBC-PKCS7 padding validation