Fix issues found in static code analysis - #212
Open
LinuxJedi wants to merge 14 commits into
Open
Conversation
Apply version and flag validation even when C_GetInterface receives a NULL interface name. Add static and dynamic API regression coverage for rejected filters.
Remove CKM_SSL3_MASTER_KEY_DERIVE from the advertised mechanism tables because no handler implements it. Add coverage confirming applications can no longer select the unavailable mechanism.
Require a normal user login for private object discovery, handle lookup, and creation, preventing an SO session from using private objects. Preserve NSS public-session behavior and add authorization regression coverage.
Keep AES key-wrap-pad decryption state intact when the output buffer is too small so the caller can retry. Extend the regression test to exercise the size query and retry paths.
Cache final CBC-PAD plaintext until the caller supplies a sufficient output buffer instead of advancing or corrupting state on CKR_BUFFER_TOO_SMALL. Add coverage for a successful retry.
Honor ulCounterBits when incrementing AES-CTR counters, including non-byte-aligned counters, and reject input that would exhaust the counter space. Add single-part and multipart boundary tests.
Replace the fixed WP11_FIND_MAX result storage with a dynamically growing array and propagate allocation failures. Add coverage for enumerating more objects than the former limit.
Add CKK_HKDF to the symmetric-key encode, decode, load, and store paths so HKDF token keys survive library reinitialization. Add persistence regression coverage.
Generate a fresh AES-GCM nonce immediately before each private or secret object encoding so persisted ciphertext never reuses an all-zero or prior IV. Compare persisted IVs and ciphertext for equal keys in the regression test.
Restore the token object list, count, and next identifier when persistence fails after insertion so the failed object cannot remain dangling. Add a read-only-store regression test covering failure and recovery.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple findings from static code analysis across wolfPKCS11’s storage/persistence layer, object enumeration, login/private-object access rules, PKCS#11 v3 interface filtering, and AES operation boundary handling, and adds/updates regression tests to lock in the fixes.
Changes:
- Fix persistence and token-state correctness (rollback on store failure, fresh IV generation timing, HKDF token-key persistence coverage).
- Improve crypto operation correctness and retry semantics (AES-CTR counter boundary enforcement, CBC-PAD final retry, AES key-wrap-pad decrypt retry).
- Tighten API/visibility behavior (private objects restricted to user sessions, stop advertising unimplemented SSL3 derive, validate default-interface version/flags).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfpkcs11/internal.h | Updates internal constants/comments and exposes new helper prototypes and error code. |
| src/internal.c | Core implementation changes: dynamic find handle list, persistence IV generation timing, HKDF persistence path, login-state private access rules, CBC-PAD retry buffer, AES-CTR overflow enforcement, key-wrap-pad retry semantics, store rollback fix. |
| src/crypto.c | Plumbs new find/CTR behaviors into PKCS#11 entry points and maps CTR overflow to CKR_DATA_LEN_RANGE. |
| src/slot.c | Removes advertising and mechanism-info support for unimplemented SSL3 master derive in NSS builds. |
| src/wolfpkcs11.c | Tightens C_GetInterface selection rules for default interface + version/flags filtering. |
| tests/pkcs11v3test.c | Adds negative tests for default interface retrieval with unsupported version/flags. |
| tests/pkcs11test.c | Adds SSL3 mechanism visibility checks, “find many objects” regression, and expands private-object access tests (SO restrictions). |
| tests/aes_ctr_counterbits_test.c | Adds AES-CTR counter overflow regression test coverage. |
| tests/decrypt_final_bufsize_test.c | Adds CBC-PAD final retry regression asserting plaintext is retained. |
| tests/aes_keywrap_pad_test.c | Adds key-wrap-pad decrypt retry regression without reinit after BUFFER_TOO_SMALL. |
| tests/copyobject_token_test.c | Adds regression test for rollback on token persistence failure during copy. |
| tests/include.am | Adds new HKDF persistence test binary to the test build. |
| tests/hkdf_persistence_test.c | New regression test validating HKDF token key persistence and fresh IV usage for persisted secrets. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Limit normal-user-only enforcement to standard PKCS#11 builds. Restore the NSS internal-module lookup and SO creation behavior required by TLS ticket handling, with regression coverage for both modes.
Build persisted-store test paths with PATH_SEP so the HKDF persistence and nonce regression coverage works on Windows as well as POSIX platforms.
Include io.h for the Windows _chmod declaration used by the token-store failure regression test.
Expose CKM_SSL3_MASTER_KEY_DERIVE in NSS builds as a target-key marker without CKF_DERIVE, preserving ticket-secret unwrap and session resumption without claiming C_DeriveKey support. Update mechanism discovery coverage for both NSS and standard builds. Validated with make check in both configurations and the Firefox CI container.
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.
Contains the following: