Fix FIDO-related issues. #125
Conversation
WalkthroughThe changes update various firmware modules. In Bluetooth handling ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CTAP
participant Display
participant Button
User->>CTAP: Initiate credential/authentication request
CTAP->>CTAP: Convert sign count to big-endian
CTAP->>CTAP: Check account name length
alt Name exceeds display limit
CTAP->>Display: Show scroll indicators
User->>Button: Press scroll button
Button->>CTAP: Signal scroll event
CTAP->>Display: Update account name display
end
CTAP->>User: Return result with updated UI feedback
sequenceDiagram
participant Main
participant BLE_Module
participant Comparator
Main->>BLE_Module: Call ble_get_version()
BLE_Module-->>Main: Return ble_ver
Main->>Comparator: compare_str_version(ble_ver, "1.5.3")
alt ble_ver >= "1.5.3"
Main->>BLE_Module: Call ble_get_hw_version()
BLE_Module-->>Main: Return hardware version status
else
Main->>Main: Skip hardware version query
end
Main->>Main: Proceed with firmware initialization
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
legacy/firmware/fido2/ctap.c (2)
1018-1071: 🧹 Nitpick (assertive)Avoid repetitive layout updates with goto.
The new scrolling logic is neat, but it uses
gotoand repeatedlayoutDialogAdapterEx. Consider refactoring this into a reusable function withoutgoto. It will make the code simpler.
1854-1920: 🧹 Nitpick (assertive)Reduce duplication in scrolling logic.
You have nearly the same scrolling code as in the other function. Consider extracting a helper method. This will keep your code DRY and more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
legacy/ble.c(3 hunks)legacy/firmware/fido2/ctap.c(6 hunks)legacy/firmware/fido2/ctap_trans.c(3 hunks)legacy/firmware/layout2.c(2 hunks)legacy/firmware/layout2.h(1 hunks)legacy/firmware/menu_core.c(1 hunks)legacy/firmware/menu_list.c(4 hunks)legacy/firmware/protect.c(2 hunks)legacy/firmware/trezor.c(2 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
legacy/firmware/layout2.c
[style] 196-196: The function 'get_truncate_position' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Style check
- GitHub Check: Gen check
- GitHub Check: Defs check
🔇 Additional comments (17)
legacy/firmware/menu_core.c (1)
17-17: Proper menu initialization ensures immediate refresh.The added line guarantees that the menu display updates right after initialization.
legacy/firmware/layout2.h (1)
41-41: New string truncation function supports FIDO UI improvements.This function helps handle long account names in the FIDO interface by determining where to truncate text based on display constraints.
legacy/firmware/protect.c (2)
54-54: New flag for FIDO abort events added.This flag allows FIDO operations to signal when they need to abort wait processes.
815-818: Added FIDO abort handler to key wait function.This new condition allows the device to exit wait states when FIDO operations need attention, improving responsiveness when using FIDO/U2F features.
legacy/firmware/fido2/ctap_trans.c (3)
103-103: External reference to FIDO abort flag.This connects the FIDO transport module with the protection module for coordinated abort handling.
292-292: Set FIDO abort flag when USB command processing starts.Triggers interrupt of key wait process when USB FIDO/U2F operations begin.
1713-1713: Set FIDO abort flag when BLE command processing starts.Triggers interrupt of key wait process when Bluetooth FIDO/U2F operations begin.
legacy/ble.c (2)
22-22: Buffer size increased for BLE version string.Expanded array size from 6 to 16 bytes allows longer version strings.
345-348: Improved hardware version validation.Now only sets
get_ble_hw_verto true when hardware version matches specific valid values, instead of accepting any 2-byte command length. This adds proper validation rather than just size checking.legacy/firmware/menu_list.c (3)
22-22: New flag controls credential list refresh behavior.Added static flag that optimizes when the resident credential list gets refreshed.
350-350: Forward declaration added for FIDO2 credential function.Function declaration allows the function to be called before its definition.
413-417: Improved handling when last FIDO credential is removed.Previous approach used
go_prev = trueto exit menu, now properly resets count and rebuilds menu structure. Commented code shows the old approach for reference.Cleaner implementation that:
- Resets credential count to zero
- Disables immediate refresh
- Reinitializes the menu system
legacy/firmware/layout2.c (2)
5674-5675: Good improvement to buffer sizes.Increasing buffer sizes from 32 to 64 bytes prevents potential buffer overflows when handling longer application and user names in FIDO2 credentials.
196-209:✅ Verification successful
This function is declared but never used.
The
get_truncate_positionfunction calculates where to truncate strings to fit the screen width, but static analysis shows it's never called anywhere in the codebase.
🏁 Script executed:
#!/bin/bash # Look for calls to get_truncate_position in the codebase rg "get_truncate_position\(" --type cLength of output: 412
Usage Confirmed: get_truncate_position Is Actively Utilized
The function now clearly appears in
legacy/firmware/fido2/ctap.cwith calls such as:
get_truncate_position(account_name + current_position, &is_end);Its logic to compute the truncation point is sound and correctly applied, and the increased buffer sizes further enhance security and functionality.
🧰 Tools
🪛 Cppcheck (2.10-2)
[style] 196-196: The function 'get_truncate_position' is never used.
(unusedFunction)
legacy/firmware/trezor.c (1)
199-199: Use a safe version check.This switch to
compare_str_versionis good. Ensure the function handles unexpected inputs (like a shorter or missing version string) gracefully.legacy/firmware/fido2/ctap.c (2)
679-681: Verify endianness correctness.This logic for storing counter in big-endian form looks right. Confirm it aligns with the FIDO spec for signCount encoding, and consider using a helper function to keep the code concise.
1786-1789: Double-check sign count.This code again sets
signCountto big-endian. Keep it consistent with the earlier usage. Confirm no further offset or alignment is required for the final buffer.
Summary by CodeRabbit