Repository navigation
Conversation
A custom store (WOLFPKCS11_CUSTOM_STORE) closes a written record with wolfPKCS11_Store_Close(), which returns void. A store that commits on close, such as wolfBoot's flash keyvault, cannot report a failed commit, so storeDirty stays clear and the next store does not write the records again. Declare wolfPKCS11_Store_CloseCommit() in store.h. The file and TPM stores already implement it. A custom store opts in by defining WOLFPKCS11_CUSTOM_STORE_CLOSE_COMMIT and implementing it; written records are then closed through it, and a non-zero return fails the operation and marks the token for a full store. Without the define, custom stores build and behave as before. wolfPKCS11_Store_Close() still closes records that were read. A custom store's failure code is now mapped to BUFFER_E, on close and on write. Passed through, wolfBoot's -1 from a failed write read as PIN_INVALID_E, so C_CreateObject returned CKR_USER_NOT_LOGGED_IN, and a -4 on close would read as storage being turned off. tests/custom_store_test.c builds the library sources with a store kept in memory, once with the define and once without. It checks that a failed commit or write fails the create, that a failed write closes the record with commit set to 0, and that the next create writes every object again, then that the objects load back after C_Finalize. It is not built for NSS, which needs the file store, or with WOLFPKCS11_TPM_STORE, which cannot be built with a custom store.
|
@claude review |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #265
Scan targets checked: wolfpkcs11-src, wolfpkcs11-bugs
Not reviewed (no scan target covers them): README.md, configure.ac, tests/README.md, tests/include.am
Coverage: 1 of 3 in-scope changed file(s) opened by the reviewer; not opened: tests/custom_store_test.c, wolfpkcs11/store.h
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
There was a problem hiding this comment.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/internal.c— pre-existing: Custom stores whose open or read fails with their own code (e.g. wolfBoot's -1) still have it read as PIN_INVALID_E, so callers get CKR_USER_NOT_LOGGED_IN. This change maps store codes to BUFFER_E only on write and close. wp11_storage_open passes the store's code through unchanged, and so does wp11_storage_read'selse if (ret == 0)branch. Fix: under WOLFPKCS11_CUSTOM_STORE, turn every non-zero open result except NOT_AVAILABLE_E into BUFFER_E, and every short read into BUFFER_E, which covers the 2 sites listed. Same pattern at 2 sites (src/internal.c:3261, src/internal.c:3358).Why this was flagged
Trigger: a custom store's wolfPKCS11_Store_Open fails a write open (keyvault full, flash error) or its wolfPKCS11_Store_Read fails, returning -1 as wolfBoot does. The test store does this too (tests/custom_store_test.c:166, :208). wp11_storage_open (src/internal.c:3261) returns that -1 straight to writers such as wp11_Object_Store_Data (src/internal.c:5566), and from there it reaches C_CreateObject. wp11_storage_read (src/internal.c:3358) only maps the result to BUFFER_E when ret == 0, so a negative store code also passes through on load. -1 equals PIN_INVALID_E, so the caller gets CKR_USER_NOT_LOGGED_IN, which is the same misreport the PR fixes for write and close (src/internal.c:3436, :3302). Open failures that are NOT_AVAILABLE_E still need to pass through unchanged, because callers treat that code as 'storage off / not found'. The base behaves the same way, so this is a partial fix rather than a regression.
Verification: Trigger: a custom store whose wolfPKCS11_Store_Open or wolfPKCS11_Store_Read fails with its own negative code, such as -1 (PIN_INVALID_E, wolfpkcs11/internal.h:386). wp11_storage_open (src/internal.c:3261) returns it unchanged, and wp11_storage_read (src/internal.c:3358) maps to BUFFER_E only
else if (ret == 0). The base code fails the same way, so this is not a regression.
|
Alpine CI fixed in #266 |
A custom store's open and read failures still passed its own code through. wolfBoot's -1 from a failed open for writing reached C_CreateObject as PIN_INVALID_E, so it returned CKR_USER_NOT_LOGGED_IN. Map them to BUFFER_E as for write and close, keeping NOT_AVAILABLE_E from an open, which means the data is not stored. Read failures of the TPM store keep their codes. custom_store_test now fails a record's open for writing in both builds and expects CKR_FUNCTION_FAILED. It also skips when NO_AES is defined: the library then turns storage off, which the test cannot see without including internal.h, so it failed instead of skipping.
|
Re the open/read finding in the review above: fixed in 9c01d2f. For a custom store, a failed open now maps to |
wp11_Object_Load_Data() and wp11_Object_Store_Data() closed the record even when opening it had failed, so a custom store's close got NULL. wolfBoot's store writes through the handle, so creating a data object whose record cannot be opened, or loading one whose record is missing, would fault. Records of the other types are closed only once they have opened. custom_store_test now fails the open of a data record on create, removes one before loading the token, and checks that the store is never closed without an open.
With WOLFPKCS11_CUSTOM_STORE_CLOSE_COMMIT, custom_store_test now fails the first commit of a C_DestroyObject. It expects CKR_FUNCTION_FAILED, the object gone, and the token written in full by C_Finalize, so the object does not load back. Master already maps the failure. wolfPKCS11 2.1.0 returned the internal error code, as a board run of a backport of this change showed.
#257 writes only the token objects that changed, and relies on
storeDirty: a store pass that fails or is skipped makes the next pass write every object again. A custom store (WOLFPKCS11_CUSTOM_STORE) cannot set it when a record fails to commit, becausewolfPKCS11_Store_Close()returnsvoid. wolfBoot's TrustZone keyvault commits flash on close, so a failed flash commit leaves the record missing or stale while wolfPKCS11 carries on as if it were stored.This declares
wolfPKCS11_Store_CloseCommit(void* store, int commit)instore.h. The file and TPM stores already implement it (it was a static function). A custom store opts in by definingWOLFPKCS11_CUSTOM_STORE_CLOSE_COMMITand implementing it. Written records are then closed through it, and a non-zero return fails the operation and marks the token for a full store.commitis 0 when writing the record failed, so a store that can discard the record may do so. Without the define, custom stores build and behave exactly as before, andwolfPKCS11_Store_Close()still closes records that were read.Why a define: changing
wolfPKCS11_Store_Close()to returnintwould break every existing custom store at compile time, and weak symbols are not portable to every compiler a custom store is built with. A store can define the macro and the function unconditionally: older wolfPKCS11 versions ignore the define and keep callingwolfPKCS11_Store_Close(). wolfSSL/wolfBoot#957 does this for the keyvault.A custom store's failure code is now mapped to
BUFFER_Eon open, read, write and close (an open still passesNOT_AVAILABLE_Ethrough, since it means the data is not stored). Passed through, wolfBoot's-1from a failed open or write reads asPIN_INVALID_E, soC_CreateObjectreturnedCKR_USER_NOT_LOGGED_IN, and a-4on close would read as storage being turned off. Read failures of the TPM store keep their codes.wp11_Object_Load_Data()andwp11_Object_Store_Data()now close the data record only when it opened. They passedNULLto the store's close when the open failed, and wolfBoot's store wrote through it, so creating a data object whose record could not be opened, or loading one whose record was missing, faulted. wolfSSL/wolfBoot#957 also makes wolfBoot's close ignoreNULL.New test
tests/custom_store_test.c. It builds the library sources with a store kept in memory, twice: with the define (custom_store_test) and without it (custom_store_legacy_test, which checks that a store with onlywolfPKCS11_Store_Close()still builds and keeps objects). Both check that a record failing to open for writing fails the create withCKR_FUNCTION_FAILED, and that the store is never closed without an open: they fail the open of a data record on create and remove one before loading the token. With the define, it also checks that a failed commit and a failed write both fail the create withCKR_FUNCTION_FAILED, that the failed write closes the record withcommitset to 0, that a failed commit failsC_DestroyObjectwithCKR_FUNCTION_FAILED, that the next create writes every object again and the one after only its own, and that the objects load back afterC_Finalize. It fails on master at the failed open check. The tests skip without AES, where the library turns storage off, and are not built with--enable-nss, which needs the file store, or with-DWOLFPKCS11_TPM_STORE, which cannot be built with a custom store (BUILD_CUSTOM_STORE_TESTinconfigure.ac). They are also the first CI coverage of a custom store build.Testing:
make checkpasses with no failures in the default,--enable-debug,--enable-nss,--enable-static --disable-shared, C++ (CC=clang++) and C++ debug configs. The pre-commit hookmake testpasses.stm32h5-tzPKCS#11 test app passes in m33mu (as in the wolfBoot integration job) and wolfPKCS11 closes written records withwolfPKCS11_Store_CloseCommit(). Thestm32u5-wolfcrypt-tzconfig builds. wolfBoot master with this change also passes, using the old close.stm32u5-wolfcrypt-tz.config) with Report failed flash commits from the PKCS#11 keyvault, check STM32U5 flash errors wolfBoot#957. Creating 8 token objects, reading them back and destroying them across reboots reported no failures. With the keyvault pages write-protected (WRP), eachC_DestroyObjectfailed withCKR_FUNCTION_FAILEDinstead of returningCKR_OK, and once the protection was removed the objects were intact. Master itself was not run on hardware; itsC_DestroyObjectalready maps the failure (2.1.0 needed that too).ZD 21908