Repository navigation
Save digest operation state as plain hash fields - #300
Merged
Merged
Conversation
C_GetOperationState saved a digest by calling wc_*Copy() straight into the caller's buffer, after the mechanism type, and C_SetOperationState copied that buffer back into the session the same way. The saved state was therefore the raw wolfCrypt hash object, which is not plain data: - wolfSSL #12035 made wc_Sha256Copy() and wc_Sha224Copy() return BAD_ALIGN_E for a destination below the type's alignment on x86_64. The return value was ignored, so the saved state was garbage and restoring it failed or produced the wrong digest. - Depending on the build the object holds pointers to heap memory the copy allocated (the W cache, or with WOLFSSL_HASH_KEEP the whole message), device and async handles, a heap hint and flags. Every save leaked those allocations and handed their addresses to the application. - Copying into the caller's buffer first freed whatever pointers the buffer happened to contain, since newer wc_*Copy() versions free the destination before overwriting it. - On restore, wolfCrypt dereferenced pointers taken from the application's bytes: a forged state could make the copy read from an arbitrary address, pick a heap hint or select a crypto callback device. - The whole hash buffer was saved, including the bytes after buffLen. The digest shares the session's params union with the other operations, wc_HashInit() does not clear the buffer, and single-part AES operations end without zeroing params. Against wolfSSL v5.6.6, whose wc_AesFree() keeps the key schedule, C_Encrypt with a sensitive key followed by C_DigestInit and C_GetOperationState handed parts of the AES key schedule to the application. The resumable state of MD5, SHA-1 and SHA-2 is small and fully defined: the chaining value, the input not yet processed and the total length. The saved state is now just that, after the mechanism type: loLen, hiLen, buffLen, the digest words and the first buffLen bytes of the buffer, zero padded, followed by the existing optional updated byte. No pointer, handle or flag is saved. C_DigestInit and C_SetOperationState also zero the hash before wc_HashInit(), so a new digest never starts with data of an earlier operation. The layout uses host byte order and the size of CK_MECHANISM_TYPE, so a state still only restores in a build with the same ABI. C_GetOperationState reads the fields from the session's hash, loads them into a freshly initialized hash and compares wc_*GetHash() of both. If they differ, the state is not held in those fields and it returns CKR_STATE_UNSAVEABLE instead of a state that would restore wrongly. The check cannot see how a fresh hash continues an update, so builds whose hash state is not all in those fields are excluded at compile time instead: wolfpkcs11/internal.h defines WOLFPKCS11_NO_DIGEST_STATE for WOLFSSL_NO_HASH_RAW, which wolfSSL sets for ports without raw hash access, for WOLFSSL_HASH_KEEP, whose backends may read the kept message, and for the ports wolfSSL itself excludes from its SHA-256 hash block code because their state is outside sha256->digest, such as the ESP32 engine that restarts from the IV. With the define, C_GetOperationState always returns CKR_STATE_UNSAVEABLE, which PKCS#11 allows, and C_SetOperationState returns CKR_SAVED_STATE_INVALID. A port missing from the list fails to build the digest state code and can define it itself; the README documents it. C_SetOperationState requires the exact length, with or without the updated byte, a supported mechanism, a buffLen smaller than the block size, lengths that fit the hash's length fields and an updated byte of 0 or 1 that is set when any data has been hashed. A state with hashed data but no updated flag would let a following C_Digest start over and silently drop the restored data. loLen is not checked against buffLen: the wolfSSL v5.6.6 ARMv8 SHA-256 code counts only whole blocks in loLen until the final block. The checks run before the current operation is touched, so an invalid state leaves it intact. The digest is then started through WP11_Digest_Init(), as C_DigestInit does, and the fields are written into the fresh hash, so a forged state can only choose a hash state. A failed hash initialization now returns CKR_HOST_MEMORY or CKR_FUNCTION_FAILED instead of the wolfCrypt error code; the previous operation has already ended at that point. C_GetOperationState maps hash failures in its check the same way. States saved by earlier versions are rejected with CKR_SAVED_STATE_INVALID; they were only usable by the same build. The test saves and restores every enabled MD5, SHA-1 and SHA-2 mechanism through buffers at odd offsets, after more than one block of input so the chaining value is exercised. It checks that a trailing byte, a truncated state, a full buffer, a SHA-256 length beyond 32 bits and a SHA-3 or non-digest mechanism, an updated byte of 2 and a cleared updated byte after hashed data are rejected, and that the running digest still completes correctly after those rejections. A state saved before any update restores and still allows C_Digest. It checks that a state saved right after a single-part AES-CBC encrypt holds no buffer bytes; against wolfSSL v5.6.6 that fails only when both the buffLen-only save and the zeroing in WP11_Digest_Init are removed. In a WOLFPKCS11_NO_DIGEST_STATE build it checks that a state save is refused, a state is rejected and the digest still completes. The save and restore tests, here and in the existing tests, are compiled out under WOLFPKCS11_NO_DIGEST_STATE, so make check passes in those builds while a normal build that cannot save a state fails them. A new CI job builds with WOLFPKCS11_NO_DIGEST_STATE.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive serialization relies on version- and hardware-specific wolfCrypt internals, so missing dependency sources and runtime validation leave compatibility and hardware-port exclusions requiring human verification.
No open issues 🎉
What changed in this PR
This PR replaces raw wolfCrypt digest snapshots with plain hash fields, avoiding alignment failures and preventing saved operation states from exposing internal pointers or stale operation data.
Changes:
- Serialize resumable digest fields, verify exports, validate restores, and clear hash storage before initialization.
- Add and document
WOLFPKCS11_NO_DIGEST_STATE, with automatic detection for unsupported hash configurations. - Expand state regression tests and add CI coverage for disabled digest-state support.
| File | Description |
|---|---|
wolfpkcs11/internal.h |
Detects configurations that disable digest-state support. |
tests/single_part_op_state_test.c |
Tests misaligned buffers, forged states, stale data, and disabled support. |
tests/session_op_lifecycle_test.c |
Gates save/restore lifecycle tests on digest-state support. |
tests/pkcs11test.c |
Gates successful operation-state tests on feature availability. |
tests/digest_length_test.c |
Skips successful digest-state saves when support is disabled. |
src/internal.c |
Implements field-based serialization, validation, and hash-storage clearing. |
README.md |
Documents the digest-state disable macro and automatic detection. |
.github/workflows/unit-test.yml |
Adds a CI job with digest-state support disabled. |
Review effort: Balanced
Member
Author
|
@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 #300
Scan targets checked: wolfpkcs11-src, wolfpkcs11-bugs
Not reviewed (no scan target covers them): .github/workflows/unit-test.yml, README.md
Coverage: 6 of 6 in-scope changed file(s) opened by the reviewer
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: 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 rewrites how C_GetOperationState and C_SetOperationState serialize and validate digest state from untrusted application bytes, so a maintainer familiar with wolfCrypt hash internals should also take a look.
What was reviewed:
- The save, check and load helpers in src/internal.c, including the compile-time size assert, zero-padding of the buffer past buffLen, and the 32-bit length check for MD5, SHA-1, SHA-224 and SHA-256.
- Export verification: the saved fields are loaded into a fresh wc_HashAlg, both GetHash outputs are compared, and the temporaries are wiped with ForceZero.
- The restore path runs every validation before it aborts the running operation. Also covered: WOLFPKCS11_NO_DIGEST_STATE gating in internal.h and the tests, and the new CI job.
Extended reasoning...
The change replaces raw wc_*Copy() serialization of wolfCrypt hash objects with a pointer-free WP11_DigestState struct in src/internal.c, roughly 430 changed lines. It also adds an auto-defined opt-out macro in wolfpkcs11/internal.h, plus tests and a CI job. The security-sensitive surface is crypto state handling: application-supplied bytes are parsed and loaded into live hash contexts, and saved state could expose leftover params-union data. The bug hunt ran until it found nothing more (dry_streak) and reported no findings. I deferred because this is a sizable crypto and PKCS#11 API change with compatibility effects: states saved by earlier versions are now rejected, and hardware ports are opted out.
LinuxJedi
approved these changes
Nov 13, 2026
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.
C_GetOperationState saved a digest by calling wc_*Copy() into the caller's buffer at a misaligned offset, so wolfSSL master's new alignment check (#12035) broke it and CI went red. The saved blob was also the raw wolfCrypt object, leaking heap pointers, stale AES key schedule bytes on v5.6.6, and letting a forged state steer pointers on restore.
Changes
Testing