Repository navigation
Refuse protected derived keys shorter than 16 bytes - #292
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A critical flaw allows protected P-256 ECDH derivation to bypass the length guard and recover the secret byte by byte through varying CKA_VALUE sizes.
1 open issue
What changed in this PR
This PR adds a default minimum length of 16 bytes for keys derived from protected bases, aiming to prevent byte-at-a-time secret recovery in wolfPKCS11. A build-time opt-out restores PKCS#11 specification behavior.
Changes:
- Add derived-key length checks and make concatenation cut protection configurable.
- Expose the opt-out through Autotools and CMake, with documentation of behavior and limitations.
- Add regression tests and a guard-disabled CI configuration.
| File | Description |
|---|---|
tests/pkcs11test.c |
Adjust concatenation expectations when the guard is disabled. |
tests/include.am |
Register and link the new regression test. |
tests/derive_length_guard_test.c |
Test derivation lengths across supported mechanisms. |
src/crypto.c |
Enforce minimum lengths and conditionally enable concatenation protection. |
README.md |
Document configuration, compatibility, and security limitations. |
configure.ac |
Add the Autotools guard option and configuration summary. |
CMakeLists.txt |
Add the CMake guard option. |
cmake/options.h.in |
Export the guard opt-out define. |
.github/workflows/unit-test.yml |
Add guard-disabled CI coverage. |
Review effort: Balanced
cef9166 to
2db6b0e
Compare
|
@claude review |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #292
Scan targets checked: wolfpkcs11-src, wolfpkcs11-bugs
Not reviewed (no scan target covers them): .github/workflows/unit-test.yml, CMakeLists.txt, README.md, cmake/options.h.in, configure.ac, tests/include.am
Coverage: 2 of 3 in-scope changed file(s) opened by the reviewer; not opened: tests/derive_length_guard_test.c
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Max
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It changes C_DeriveKey's key-protection behavior in src/crypto.c, so a maintainer should still look at it before it merges.
What was reviewed:
- The new DerivedKeyTooShort check in the generic derive path and in SetKeyExtract (TLS12 key-and-MAC). The secret object is freed before the early return. An empty MAC secret (length 0) still passes because the check requires len > 0.
- The concat flag, which keeps the new 16-byte check off CKM_CONCATENATE_*. Those mechanisms are still covered by ConcatCutsProtectedKey while the guard is on.
- Two choices that need a maintainer's call: with --disable-derive-length-guard, the existing concatenate cut check is also turned off, and the new CKA_VALUE-in-template rejection applies whether or not the guard is enabled.
Extended reasoning...
The change adds a derive-length guard, on by default, to C_DeriveKey in src/crypto.c, plus a build option, docs, CI and an 813-line test. It is crypto and key-protection code, where a mistake could leak protected key material. It also changes PKCS#11 return codes for existing callers. The opt-out switch also turns off the earlier concatenate protection, which is a design decision for maintainers. The one inline review comment, from Copilot, was resolved by the PR author, and that alone doesn't show it was addressed.
2db6b0e to
36228ad
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #292
Scan targets checked: wolfpkcs11-src, wolfpkcs11-bugs
Not reviewed (no scan target covers them): .github/workflows/unit-test.yml, CMakeLists.txt, README.md, cmake/options.h.in, configure.ac, tests/include.am
Coverage: 1 of 2 in-scope changed file(s) opened by the reviewer; not opened: tests/derive_length_guard_test.c
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
C_DeriveKey keeps the last CKA_VALUE_LEN bytes of the derived output, and the output is the same for the same inputs (ECDH or DH with a fixed peer value, the TLS master secret, AES-CBC-ENCRYPT-DATA, HKDF). Keys of 1, 2, 3, ... bytes are therefore nested suffixes of one secret. A caller allowed to derive from a sensitive key could read CKA_CHECK_VALUE of each, or HMAC a known message with it, and find one more byte of the secret per step with 256 offline trials. HKDF expand gives nested prefixes the same way through its caller-chosen length, and so do the key and MAC sizes of CKM_TLS12_KEY_AND_MAC_DERIVE. Refuse a key derived from a sensitive or non-extractable base key when it is shorter than 16 bytes, with CKR_ATTRIBUTE_VALUE_INVALID and no key. Such a key inherits the base key's protection. A TLS 1.2 key or MAC size that short returns CKR_MECHANISM_PARAM_INVALID. The 16 bytes leave at least 128 unknown bits at every step. A key derived from a readable base, including a data object, is not limited, as the secret can be computed from it, and empty TLS MAC secrets are still made. NSS and WOLFPKCS11_LEGACY_DERIVE_NO_INHERIT builds do not limit these mechanisms, as derived keys there can be made readable. The CKM_CONCATENATE_* mechanisms keep their check against a CKA_VALUE_LEN that ends inside a sensitive or non-extractable key. They still allow a caller to append known data to a protected key, so with them enabled the bytes of a protected derived secret beyond 16 can be found by comparing a longer derived key with a shorter one plus a guessed byte, through HMAC or CKA_CHECK_VALUE. No rule on lengths or on the protection attributes closes that, as padding both sides or a sensitive key created with a known value gets around it. 16 bytes stay unknown, and --disable-concatenate removes the comparison. C_DeriveKey also refuses a template with CKA_VALUE, with CKR_TEMPLATE_INCONSISTENT, as PKCS#11 does not allow the value of a derived key to be given. CreateObject took the key length from it and the derive kept that many trailing bytes, which got around both the length check and the concatenation check, as they read CKA_VALUE_LEN. PKCS#11 allows any length, so --disable-derive-length-guard (CMake WOLFPKCS11_DERIVE_LENGTH_GUARD=no) defines WOLFPKCS11_NO_DERIVE_LENGTH_GUARD and restores the specification's behavior. It also turns off the concatenation check above. A new CI job builds with it. Fixes F-17782.
36228ad to
a6c50a1
Compare
A caller allowed to derive from a sensitive key could recover the derived secret one byte at a time: derived keys of 1, 2, 3... bytes are nested suffixes (or HKDF prefixes) of one output, and
CKA_CHECK_VALUEor an HMAC with each key lets the next byte be brute forced in 256 tries. This PR refuses such keys below 16 bytes, with an opt-out for strict PKCS#11 compliance. Fixes F-17782.Changes
CKR_ATTRIBUTE_VALUE_INVALIDand creates no key (ECDH, DH, HKDF, AES-CBC-ENCRYPT-DATA, TLS master secret)CKM_TLS12_KEY_AND_MAC_DERIVEkey or MAC sizes that short returnCKR_MECHANISM_PARAM_INVALID; empty MAC secrets still work--disable-derive-length-guard/ CMakeWOLFPKCS11_DERIVE_LENGTH_GUARD=nodefinesWOLFPKCS11_NO_DERIVE_LENGTH_GUARD, restoring spec behavior; it also turns off the existingCKM_CONCATENATE_*cut checkTesting
tests/derive_length_guard_test.c: ECDH, DH, HKDF, AES-CBC-ENCRYPT-DATA, TLS and concatenation lengths, with the guard on and offpkcs11testconcatenation cut checks expect success with the guard offno_derive_length_guardCI jobLimitations
CKM_CONCATENATE_*enabled, the bytes of a protected derived secret beyond 16 can still be found by comparing a longer derived key with a shorter one plus a guessed byte;--disable-concatenatecloses thisRelated