fix for load token with empty pin#158
fix for load token with empty pin#158JacobBarthelmeh wants to merge 3 commits intowolfSSL:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds coverage and a code-path fix for loading/storing encrypted token objects when the user PIN is empty (i.e., token usable without calling C_Login).
Changes:
- Add a new automated test that initializes a token with an empty user PIN, stores an encrypted token object, finalizes, reloads, and verifies the object can be decoded without login.
- Update token load logic to derive the token key from the empty PIN + seed before decoding stored objects.
- Register the new test binary in the Automake test build.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
tests/include.am |
Adds tests/empty_pin_store_test to the test programs and links it appropriately. |
tests/empty_pin_store_test.c |
New test covering empty user PIN token storage/load/decode behavior. |
src/internal.c |
Attempts to derive token->key during token load for empty-PIN scenarios prior to object decode. |
Comments suppressed due to low confidence (1)
src/internal.c:5306
- wp11_Token_Load is already inside
#ifndef WOLFPKCS11_NO_STORE, so this extra nested#ifndef WOLFPKCS11_NO_STOREblock is redundant and makes the control flow harder to read. Consider removing the inner guard.
#ifndef WOLFPKCS11_NO_STORE
/* Derive token->key from empty PIN + seed before decoding */
ret = HashPIN((char*)"", 0, token->seed, sizeof(token->seed),
token->key, sizeof(token->key));
object = token->object;
while (ret == 0 && object != NULL) {
ret = wp11_Object_Decode(object);
object = object->next;
}
#endif
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3a014e0 to
2a6c80e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/internal.c:5306
wp11_Token_Load()is already inside#ifndef WOLFPKCS11_NO_STORE(starting near line 5148), so the inner#ifndef WOLFPKCS11_NO_STORE/#endifaround the empty-PIN decode block is redundant. Removing the inner guard will simplify control flow and reduce the risk of mismatched preprocessor blocks in future edits.
#ifndef WOLFPKCS11_NO_STORE
/* Derive token->key from empty PIN + seed before decoding */
ret = HashPIN((char*)"", 0, token->seed, sizeof(token->seed),
token->key, sizeof(token->key), slot);
object = token->object;
while (ret == 0 && object != NULL) {
ret = wp11_Object_Decode(object);
object = object->next;
}
#endif
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2a6c80e to
3be9048
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* If there is no pin, there is no login, so decode now */ | ||
| if (WP11_Slot_Has_Empty_Pin(slot) && (ret == 0)) { | ||
| #ifndef WOLFPKCS11_NO_STORE | ||
| /* Derive token->key from empty PIN + seed before decoding */ |
There was a problem hiding this comment.
Could this break anything we backwards compatibility?
There was a problem hiding this comment.
This would break the use case of store token (old code base) -> load token (old code base) -> encrypt objects -> store token (old code base) -> loaded token (new code base). I think it is okay to, and should break that compatibility, because before this change it would fail to decrypt objects when reloading a stored token if they were encrypted before the first store of the token.
There was a problem hiding this comment.
Jacob is right. The empty pin workflow in this case was really only tested for the initial setup. We should relnote this though.
| /* If there is no pin, there is no login, so decode now */ | ||
| if (WP11_Slot_Has_Empty_Pin(slot) && (ret == 0)) { | ||
| #ifndef WOLFPKCS11_NO_STORE | ||
| /* Derive token->key from empty PIN + seed before decoding */ |
There was a problem hiding this comment.
Jacob is right. The empty pin workflow in this case was really only tested for the initial setup. We should relnote this though.
ZD21110