AES reject-no-key guard and RSA-PSS hash-binding test (F-6151, F-6157)#10762
Draft
julek-wolfssl wants to merge 4 commits into
Draft
AES reject-no-key guard and RSA-PSS hash-binding test (F-6151, F-6157)#10762julek-wolfssl wants to merge 4 commits into
julek-wolfssl wants to merge 4 commits into
Conversation
wc_AesInit zeroes the Aes struct, leaving keylen and rounds at 0. The CTR/CBC/GCM/ECB/CFB/OFB entry points never verified that a key had been installed with wc_AesSetKey, so calling them right after wc_AesInit ran the software cipher against an all-zero key schedule and returned success instead of an error. For CTR this exposes a reconstructable keystream; for GCM the hash subkey H is also zero, making the tag forgeable. Add the keylen == 0 guard already used by wc_AesXtsEncrypt to the software mode paths. The check is placed after the crypto callback fall-through so device-managed keys are unaffected. Hardware paths that call wc_AesGetKeySize already reject this case via rounds == 0. Add aes_no_key_set_test(), run from aes_test(), which inits an Aes context and confirms every mode rejects use before a key is set.
wc_RsaPSS_CheckPadding_ex2 binds a PSS signature to the message with a single comparison of the recomputed hash against the encoded hash H. Every positive PSS test passes the correct digest and the negative tests use a wrong salt length that fails before that comparison is reached, so deleting the comparison left all default tests passing even though any well-formed PSS structure would then verify against any message. After each successful sign/verify round-trip in rsa_pss_test, flip one bit of the message digest, keep the correct salt length, and require BAD_PADDING_E. This makes the hash comparison the deciding check and catches its removal.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces two security hardening changes in wolfCrypt: (1) prevent AES mode operations from running after wc_AesInit() but before a key is set, and (2) strengthen RSA-PSS regression coverage to ensure signatures are bound to the message digest.
Changes:
- Add
aes->keylen == 0rejection guards to multiple AES mode implementations (CBC/CTR/GCM/ECB/CFB/OFB) inwolfcrypt/src/aes.c. - Add a new
aes_no_key_set_test()and run it fromaes_test()to ensure AES mode APIs reject use beforewc_AesSetKey(). - Extend
rsa_pss_test()to tamper the digest post sign/verify and requireBAD_PADDING_E, making the hash-compare step decisive.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| wolfcrypt/src/aes.c | Adds “key not set” guards to prevent AES modes from operating with an uninitialized (zero) key schedule in the software paths. |
| wolfcrypt/test/test.c | Adds AES regression test for “no key set” behavior and adds RSA-PSS digest-tampering negative verification to ensure hash binding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace the keylen == 0 guards with a new Aes.keySet bitfield, set by the key-installation paths and checked in the mode APIs. keylen alone is not a reliable "key installed" signal across backends: some ports map a zero key length to a default (e.g. PSOC6 maps keylen 0 to AES-128), so a keylen-based check placed only in the software paths left those builds running with the all-zero key schedule and would fail aes_no_key_set_test. keySet is set in wc_AesSetKeyLocal (the funnel for the software SetKey/SetKeyDirect/GcmSetKey paths) and in the PSOC6 wc_AesSetKey wrapper, and checked in the public CBC/CTR/GCM/ECB/CFB/OFB entry points, including the PSOC6 port variants.
Address review of the keySet bitfield: - Rename keySet to keyInstalled. struct Aes already has a Cavium-only keySet member (HAVE_CAVIUM_OCTEON_SYNC), so an unconditional keySet bitfield was a duplicate that would not compile on that build. - Set keyInstalled on every key install, not just the software funnel. The ARM, PPC64 and other hardware wc_AesSetKey/SetKeyDirect variants populate keylen/rounds directly; without also setting the flag they would have made the shared mode APIs reject a validly-keyed context. The flag is now set at every keylen-assignment point in aes.c and in the PSOC6 SetKey wrapper.
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.
Two independent hardening fixes with regression tests.
AES: reject mode operations when no key has been set (F-6151)
wc_AesInitzeroes theAesstruct, leavingkeylen/roundsat 0. The CTR/CBC/GCM/ECB/CFB/OFB entry points never verified a key had been installed viawc_AesSetKey, so calling them right afterwc_AesInitran the software cipher against an all-zero key schedule and returned success. For CTR this exposes a reconstructable keystream; for GCM the hash subkey H is also zero, making the tag forgeable.Adds the
keylen == 0guard (already used bywc_AesXtsEncrypt) to the software mode paths, placed after the crypto-callback fall-through so device-managed keys are unaffected. Newaes_no_key_set_test()(run fromaes_test()) confirms every mode rejects use before a key is set.RSA-PSS: test signature-to-message hash binding (F-6157)
wc_RsaPSS_CheckPadding_ex2binds a PSS signature to the message with a single comparison of the recomputed hash against the encoded hash H. Every positive PSS test passed the correct digest and the negative tests failed earlier on a wrong salt length, so removing the comparison left all default tests passing even though any well-formed PSS structure would verify against any message.After each successful sign/verify round-trip in
rsa_pss_test, flips one bit of the message digest, keeps the correct salt length, and requiresBAD_PADDING_E— making the hash comparison the deciding check.