fix: opt change pin logic and update i18n #167
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
WalkthroughAdds check-type PIN handling to avoid state resets on verification failures, updates PIN prompt logic with expect_standard_prompt, and adjusts hidden-session behavior during PIN changes. Removes a Security Keys menu item. Updates i18n: bumps item count, renumbers keys, deletes one PIN string, and adds four new safety/transaction strings across locales. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI
participant CFG as config_verifyPin
participant SES as Session State
Note over CFG: Classify PIN type as check vs non-check
UI->>CFG: Verify PIN (type, pin)
CFG-->>UI: result = OK/FAIL
alt result == FAIL and type is non-check
CFG->>SES: se_clearPinStateCache()
CFG->>SES: Disable passphrase-pin, clear cached seeds
else result == FAIL and type is check
Note over SES: No side effects
end
sequenceDiagram
autonumber
participant UI as UI
participant PRO as protectPinOnDevice
participant SES as Session
PRO->>SES: Read is_passphrase_pin_enabled, session_isUnlocked
PRO->>PRO: Compute expect_standard_prompt
UI-->>PRO: PIN entry (STANDARD or ENTER_PIN prompt)
PRO-->>UI: Verify result
alt Passphrase-PIN fails
PRO->>PRO: Fallback to standard prompt (set expect_standard_prompt)
end
sequenceDiagram
autonumber
participant UI as UI
participant PRO as protectChangePinOnDevice
participant SES as Session
PRO->>PRO: started_in_hidden_env = session hidden?
UI-->>PRO: Enter old/new PINs
PRO-->>UI: Confirm new PIN
alt Overwrite passphrase PIN in hidden env
PRO->>SES: Reset/lock per hidden-session policy
else Otherwise
PRO->>SES: Keep unlocked, update state
end
Note over PRO: Aggressive memzero on cancel/mismatch/errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
legacy/firmware/menu_list.c (1)
984-999: Don’t keep PINs in static buffers (leak risk).main_pin_copy/pin1_copy/pin2_copy are static and never zeroed. They persist in memory after return.
Min‑change fix: make them non‑static to avoid persistence.
- static char main_pin_copy[MAX_PIN_LEN + 1] = ""; - static char pin1_copy[MAX_PIN_LEN + 1] = ""; - static char pin2_copy[MAX_PIN_LEN + 1] = ""; + char main_pin_copy[MAX_PIN_LEN + 1] = ""; + char pin1_copy[MAX_PIN_LEN + 1] = ""; + char pin2_copy[MAX_PIN_LEN + 1] = "";Further hardening (recommended): memset these buffers to 0 before each early return, or refactor to reuse the existing g_temp_* buffers only, avoiding extra copies.
Also applies to: 1015-1024, 1069-1072
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
legacy/firmware/config.c(1 hunks)legacy/firmware/i18n/i18n.h(1 hunks)legacy/firmware/i18n/keys.h(1 hunks)legacy/firmware/i18n/locales/de.inc(1 hunks)legacy/firmware/i18n/locales/en.inc(1 hunks)legacy/firmware/i18n/locales/es.inc(1 hunks)legacy/firmware/i18n/locales/ja.inc(1 hunks)legacy/firmware/i18n/locales/ko_kr.inc(1 hunks)legacy/firmware/i18n/locales/pt_br.inc(1 hunks)legacy/firmware/i18n/locales/zh_cn.inc(2 hunks)legacy/firmware/i18n/locales/zh_tw.inc(2 hunks)legacy/firmware/menu_list.c(8 hunks)legacy/firmware/protect.c(13 hunks)
🔇 Additional comments (15)
legacy/firmware/i18n/locales/ja.inc (1)
428-431: LGTM: i18n strings added consistently.Four new Japanese locale strings added. Structure matches other locale files in this PR. Translation accuracy requires native speaker review.
legacy/firmware/i18n/i18n.h (1)
8-8: LGTM: Item count updated correctly.The count increases from 427 to 430 (net +3), matching the locale changes: 1 removed + 4 added = +3.
legacy/firmware/i18n/locales/zh_cn.inc (2)
395-395: LGTM: Text simplified.The Chinese string explaining PIN/Passphrase linking was made more concise. Translation accuracy requires native speaker verification.
428-431: LGTM: i18n strings added consistently.Four new Chinese locale strings added, matching the pattern in other locale files.
legacy/firmware/i18n/locales/ko_kr.inc (1)
428-431: LGTM: i18n strings added consistently.Four new Korean locale strings added. Translation accuracy requires native speaker review.
legacy/firmware/i18n/locales/en.inc (1)
428-431: LGTM: English i18n strings added.Four new strings added for Safe transactions and PIN change flow. Text is clear and grammatically correct.
legacy/firmware/i18n/locales/de.inc (1)
428-431: LGTM: i18n strings added consistently.Four new German locale strings added. Translation accuracy requires native speaker review.
legacy/firmware/i18n/locales/es.inc (1)
428-431: LGTM: i18n strings added consistently.Four new Spanish locale strings added. Translation accuracy requires native speaker review.
legacy/firmware/config.c (1)
500-509: Skipping cache clear on check‐type PIN failures is intentional. Check‐type PIN calls only probe PIN presence and must not reset the SE unlock cache or session seeds on failure.legacy/firmware/i18n/locales/zh_tw.inc (1)
428-431: New strings look good.Translations read well and match new keys.
legacy/firmware/i18n/locales/pt_br.inc (1)
428-431: LGTM on new i18n entries.Strings map cleanly to new keys.
legacy/firmware/menu_list.c (2)
96-104: Use of new change‑PIN prompt key is correct.Nice UX tweak.
1296-1331: State handling around standard vs hidden PIN is solid.You preserve/restore is_passphrase_pin_enabled and drive titles via expect_standard_prompt. Good.
legacy/firmware/protect.c (1)
1176-1184: Consistent “Standard PIN” prompt logic is a win.Title selection + retry behavior are clear and predictable.
Also applies to: 1199-1202
legacy/firmware/i18n/keys.h (1)
894-901: Locale counts verified All languages_*.inc files contain 430 entries matching keys.h.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
legacy/firmware/config.c(1 hunks)legacy/firmware/i18n/i18n.h(1 hunks)legacy/firmware/i18n/keys.h(1 hunks)legacy/firmware/i18n/locales/de.inc(1 hunks)legacy/firmware/i18n/locales/en.inc(1 hunks)legacy/firmware/i18n/locales/es.inc(1 hunks)legacy/firmware/i18n/locales/ja.inc(1 hunks)legacy/firmware/i18n/locales/ko_kr.inc(1 hunks)legacy/firmware/i18n/locales/pt_br.inc(1 hunks)legacy/firmware/i18n/locales/zh_cn.inc(2 hunks)legacy/firmware/i18n/locales/zh_tw.inc(2 hunks)legacy/firmware/menu_list.c(8 hunks)legacy/firmware/protect.c(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Gen check
- GitHub Check: Defs check
- GitHub Check: Style check
🔇 Additional comments (13)
legacy/firmware/i18n/locales/pt_br.inc (1)
428-431: LGTM!The four new locale strings are properly formatted and align with the broader i18n expansion across all language files in this PR.
legacy/firmware/i18n/locales/ja.inc (1)
428-431: LGTM!The Japanese translations mirror the structure and content of other locale updates in this PR.
legacy/firmware/i18n/i18n.h (1)
8-8: LGTM!The item count increase from 427 to 430 correctly reflects the net addition of three i18n entries (4 added, 1 removed) across all locale files.
legacy/firmware/i18n/locales/ko_kr.inc (1)
428-431: LGTM!The Korean locale additions are consistent with the parallel updates in other language files.
legacy/firmware/i18n/locales/en.inc (1)
428-431: LGTM!The English source strings are clear and provide good context for Safe transactions and PIN change flows.
legacy/firmware/i18n/locales/zh_tw.inc (2)
395-395: LGTM!The refined wording makes the Passphrase PIN explanation more concise while preserving the meaning.
428-431: LGTM!The Traditional Chinese translations align with the i18n additions in other locale files.
legacy/firmware/i18n/locales/de.inc (1)
428-431: LGTM!The German translations are consistent with the i18n expansion across all locale files.
legacy/firmware/config.c (1)
500-509: se_verifyPin delegates to the SE which enforces hardware retry limits.se_verifyPin always sends a PIN‐verify APDU (including the pin_type byte), so the secure element itself tracks and limits retries for all PIN types. se_clearPinStateCache only clears firmware‐side caches and does not affect SE counters. This change does not weaken security.
legacy/firmware/protect.c (1)
1176-1187: Prompt switching logic looks goodUsing expect_standard_prompt to show STANDARD PIN vs ENTER PIN matches the hidden-session flow.
Please confirm there’s no path where config_hasPin() is false and config_unlock("", PIN_TYPE_USER_AND_PASSPHRASE_PIN) can fail repeatedly, which would loop back to input without a way to enter a PIN.
Also applies to: 1199-1202
legacy/firmware/menu_list.c (2)
96-101: Header copy improvement“Next enter the PIN you want to change” aligns with the new flow. Looks good.
987-1009: Consistent prompt behavior across flows
- expect_standard_prompt + restoration of is_passphrase_pin_enabled keep prompts consistent.
- require_standard_pin falls back to STANDARD PIN after failures. Good.
Verify the UI sequence matches protectPinOnDevice in all branches.
Also applies to: 1017-1024, 1295-1330
legacy/firmware/i18n/keys.h (1)
894-901: i18n indices and counts are correct
I18N_ITEMS_COUNT matches max key ID + 1 and all locale arrays have 430 entries.
4 participants
Summary by CodeRabbit