Fix CKA_NEVER_EXTRACTABLE of keys derived with CKM_CONCATENATE_BASE_AND_KEY

[juvex-fi]

Fixes CKA_NEVER_EXTRACTABLE handling for keys derived with CKM_CONCATENATE_BASE_AND_KEY, and fixes a test check that could never fail.

Changes

• Derive fix: deriveSymmetric computed the derived key's CKA_NEVER_EXTRACTABLE value, but wrote it to CKA_ALWAYS_SENSITIVE. As a result:

  • CKA_ALWAYS_SENSITIVE was overwritten, so a derived key could lose it even when both source keys were always sensitive.
  • CKA_NEVER_EXTRACTABLE was never set from the source keys.

  PKCS#11 v2.40 Current Mechanisms §2.31.3, Concatenation of a base key and another key:

  The derived key's CKA_ALWAYS_SENSITIVE attribute is set to CK_TRUE if and only if both of the original keys have their CKA_ALWAYS_SENSITIVE attributes set to CK_TRUE.
  Similarly, the derived key's CKA_NEVER_EXTRACTABLE attribute is set to CK_TRUE if and only if both of the original keys have their CKA_NEVER_EXTRACTABLE attributes set to CK_TRUE.

• Test fix: the CKA_MODIFIABLE check in aesWrapUnwrapNonModifiableGeneric read the attribute into the shared bFalse template variable and then compared bFalse with itself, so it always passed. It could also have changed bFalse for later tests. It now reads into its own variable. This was found in the review of Add support for DES3 wrap/unwrap #812.

Tests

• DeriveTests::testMiscDerivations now derives from generated keys, since keys from C_CreateObject always have both attributes CK_FALSE. It checks two cases:

  • both source keys are always sensitive and never extractable
  • one source key has been extractable
  • one source key has not been sensitive

  Without the fix, each case fails on its own: the first on CKA_NEVER_EXTRACTABLE, the second and third on CKA_ALWAYS_SENSITIVE.

• p11test passes 81/81 with the OpenSSL 3 backend.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Corrected the sensitivity and extractability attributes reported for keys derived by combining two keys.

• Tests

  • Added coverage for derived-key sensitivity and extractability attributes.
  • Improved validation of the non-modifiable key attribute when wrapping keys.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 05cbf530-47a6-47c0-b513-dc335c3dcea2

📥 Commits

Reviewing files that changed from the base of the PR and between e60fbfc and e597425.

📒 Files selected for processing (1)

• src/lib/test/DeriveTests.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

---

📝 Walkthrough

Walkthrough

The PR corrects the CKA_NEVER_EXTRACTABLE assignment in symmetric key derivation and adds checks for derived-key sensitivity and extractability. It also updates a wrapping test to verify the retrieved CKA_MODIFIABLE value and length.

Changes

Derived-key attributes

Layer / File(s)	Summary
Derived-key attribute assignment and checks src/lib/SoftHSM.cpp, src/lib/test/DeriveTests.cpp	The concatenate-base-and-key path now assigns the combined keys’ extractability state to CKA_NEVER_EXTRACTABLE. Tests check CKA_ALWAYS_SENSITIVE and CKA_NEVER_EXTRACTABLE for derived keys.

Modifiable attribute test

Layer / File(s)	Summary
Modifiable attribute retrieval check src/lib/test/SymmetricAlgorithmTests.cpp	The test retrieves CKA_MODIFIABLE into a separate value and checks that the returned length matches the boolean size and the value is CK_FALSE.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: bukka

Merge Risk: ⚪ Minimal · up to e5974

Derived keys now retain the separate sensitivity and extractability history attributes, with tests covering the relevant input combinations. The supplied change context indicates no actionable merge risk.

Security Architecture Review

Security architecture risk: 🔵 Low · up to e5974

The change corrects key-attribute reporting without widening access to key material. Existing key-access and failure-cleanup behavior merits separate verification, but this PR does not appear to make it riskier.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The behavior change is limited to keys derived through the existing CKM_CONCATENATE_BASE_AND_KEY mechanism. It changes reported key-history attributes, not the separate current-extractability rule or API reachability.

Trust Boundaries and Controls

• inferred — A caller able to supply a valid second-key handle reaches the pre-existing second-key lookup path. Whether a handle from another session is practically usable is unverified; this PR neither adds that path nor changes its authorization checks.

Resilience and Maintainability Implications

• observed — Object creation can return on template-save failure after allocating an object without visibly destroying it. The corrected attribute write occurs later and does not introduce this failure path; exception and recovery guarantees were not established.

Hardening Proposals

• proposed — Separately verify session ownership and read authorization for the second input handle, and make object-creation failure cleanup explicit. These are hardening questions about unchanged paths, not findings attributed to this PR.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: correcting CKA_NEVER_EXTRACTABLE for keys derived with CKM_CONCATENATE_BASE_AND_KEY.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}