support fido2 #121
support fido2 #121
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 buf (1.47.2)common/protob/messages-webauthn.protoConfig file YAML parsing error: yaml: unmarshal errors: legacy/firmware/protob/messages-webauthn.protoConfig file YAML parsing error: yaml: unmarshal errors: WalkthroughThis pull request adds extensive support for FIDO2 (WebAuthn) functionality in the firmware. Key changes include the addition of the TinyCBOR library, implementation of resident credential management, and enhancements to USB and I2C communication. The updates also improve internationalization support in multiple languages. These modifications significantly expand the device's authentication capabilities, introducing new protocols for credential storage, retrieval, and management. Changes
Sequence DiagramsequenceDiagram
participant User
participant Device
participant WebAuthn Server
User->>Device: Initiate WebAuthn Registration
Device->>WebAuthn Server: Create Credential Request
WebAuthn Server-->>Device: Credential Challenge
Device->>User: Request Confirmation
User->>Device: Confirm Registration
Device->>WebAuthn Server: Send Credential
WebAuthn Server-->>Device: Store Credential
Device->>User: Registration Complete
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: 48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
legacy/firmware/fido2/trezordevkey.pemis excluded by!**/*.pem
📒 Files selected for processing (60)
.gitmodules(1 hunks)common/protob/messages-webauthn.proto(2 hunks)core/src/trezor/messages.py(2 hunks)legacy/Makefile.include(1 hunks)legacy/firmware/Makefile(4 hunks)legacy/firmware/chinese.c(1 hunks)legacy/firmware/chinese.h(1 hunks)legacy/firmware/fido2/cose_key.h(1 hunks)legacy/firmware/fido2/ctap.c(1 hunks)legacy/firmware/fido2/ctap.h(1 hunks)legacy/firmware/fido2/ctap_errors.h(1 hunks)legacy/firmware/fido2/ctap_parse.c(1 hunks)legacy/firmware/fido2/ctap_parse.h(1 hunks)legacy/firmware/fido2/ctap_trans.c(27 hunks)legacy/firmware/fido2/ctap_trans.h(3 hunks)legacy/firmware/fido2/resident_credential.c(1 hunks)legacy/firmware/fido2/resident_credential.h(1 hunks)legacy/firmware/fido2/u2f_hid.h(2 hunks)legacy/firmware/fido2/u2f_knownapps.h.mako(1 hunks)legacy/firmware/filecoin/cbor.h(0 hunks)legacy/firmware/filecoin/cborinternal_p.h(0 hunks)legacy/firmware/filecoin/cborparser.c(0 hunks)legacy/firmware/filecoin/cborvalidation.c(0 hunks)legacy/firmware/filecoin/compilersupport_p.h(0 hunks)legacy/firmware/filecoin/tinycbor-version.h(0 hunks)legacy/firmware/filecoin/utf8_p.h(0 hunks)legacy/firmware/fsm.c(1 hunks)legacy/firmware/fsm.h(2 hunks)legacy/firmware/fsm_msg_webauthn.h(1 hunks)legacy/firmware/i18n/keys.h(2 hunks)legacy/firmware/i18n/locales/en.inc(2 hunks)legacy/firmware/i18n/locales/es.inc(1 hunks)legacy/firmware/i18n/locales/ja.inc(1 hunks)legacy/firmware/i18n/locales/pt_br.inc(1 hunks)legacy/firmware/i18n/locales/zh_cn.inc(1 hunks)legacy/firmware/i18n/locales/zh_tw.inc(1 hunks)legacy/firmware/layout2.c(23 hunks)legacy/firmware/layout2.h(1 hunks)legacy/firmware/menu_core.c(5 hunks)legacy/firmware/menu_core.h(1 hunks)legacy/firmware/menu_list.c(19 hunks)legacy/firmware/protect.c(4 hunks)legacy/firmware/protob/Makefile(2 hunks)legacy/firmware/protob/messages-webauthn.options(1 hunks)legacy/firmware/protob/messages-webauthn.proto(1 hunks)legacy/firmware/se_chip.c(3 hunks)legacy/firmware/se_chip.h(2 hunks)legacy/firmware/usb.c(4 hunks)legacy/firmware/usb.h(1 hunks)legacy/firmware/version.h(1 hunks)legacy/script/cibuild(1 hunks)legacy/si2c.c(2 hunks)legacy/si2c.h(1 hunks)legacy/timer.c(2 hunks)legacy/timer.h(1 hunks)legacy/util.c(1 hunks)legacy/util.h(1 hunks)legacy/vendor/tiny_cbor(1 hunks)python/src/trezorlib/messages.py(2 hunks)vendor/tiny_cbor(1 hunks)
💤 Files with no reviewable changes (7)
- legacy/firmware/filecoin/cborinternal_p.h
- legacy/firmware/filecoin/cborvalidation.c
- legacy/firmware/filecoin/cbor.h
- legacy/firmware/filecoin/compilersupport_p.h
- legacy/firmware/filecoin/cborparser.c
- legacy/firmware/filecoin/tinycbor-version.h
- legacy/firmware/filecoin/utf8_p.h
👮 Files not reviewed due to content moderation or server errors (4)
- legacy/firmware/fsm.c
- legacy/firmware/i18n/keys.h
- legacy/firmware/protect.c
- legacy/firmware/layout2.c
🧰 Additional context used
🪛 cppcheck (2.10-2)
legacy/firmware/chinese.c
[style] 121-121: The function 'oledCharWidthEx' is never used.
(unusedFunction)
legacy/util.c
[style] 148-148: The function 'compare_str_version' is never used.
(unusedFunction)
legacy/si2c.c
[style] 301-301: The function 'i2c_slave_send_fido' is never used.
(unusedFunction)
legacy/firmware/menu_core.c
[style] 12-12: The function 'menu_refresh' is never used.
(unusedFunction)
legacy/firmware/usb.c
[style] 567-567: The function 'usb_u2f_data_send' is never used.
(unusedFunction)
legacy/timer.c
[style] 154-154: The function 'register_loop_callback' is never used.
(unusedFunction)
[style] 162-162: The function 'unregister_loop_callback' is never used.
(unusedFunction)
[style] 168-168: The function 'loop_callback_handler' is never used.
(unusedFunction)
legacy/firmware/fido2/resident_credential.c
[style] 8-8: The function 'resident_credential_get_count' is never used.
(unusedFunction)
[style] 10-10: The function 'resident_credential_find_by_rp_id_hash' is never used.
(unusedFunction)
[style] 45-45: The function 'resident_credential_store' is never used.
(unusedFunction)
[style] 104-104: The function 'resident_credential_info' is never used.
(unusedFunction)
[style] 127-127: The function 'resident_credential_get_desc' is never used.
(unusedFunction)
[style] 149-149: The function 'resident_credential_delete' is never used.
(unusedFunction)
legacy/firmware/layout2.c
[style] 5777-5777: The function 'layout_fido2_resident_credential' is never used.
(unusedFunction)
legacy/firmware/se_chip.c
[style] 1658-1658: The function 'se_slip21_fido_node' is never used.
(unusedFunction)
[style] 1667-1667: The function 'se_derive_fido_keys' is never used.
(unusedFunction)
[style] 1696-1696: The function 'se_fido_hdnode_sign_digest' is never used.
(unusedFunction)
[style] 1710-1710: The function 'se_fido_att_sign_digest' is never used.
(unusedFunction)
[style] 1777-1777: The function 'se_get_fido2_resident_credentials' is never used.
(unusedFunction)
[style] 1801-1801: The function 'se_set_fido2_resident_credentials' is never used.
(unusedFunction)
[style] 1813-1813: The function 'se_delete_fido2_resident_credentials' is never used.
(unusedFunction)
[style] 1819-1819: The function 'se_delete_all_fido2_credentials' is never used.
(unusedFunction)
[style] 1826-1826: The function 'se_check_fido2_resident_credential_simple' is never used.
(unusedFunction)
legacy/firmware/fido2/ctap_parse.c
[style] 28-28: The function 'cbor_value_get_type_string' is never used.
(unusedFunction)
[style] 624-624: The function 'ctap_parse_make_credential' is never used.
(unusedFunction)
[style] 947-947: The function 'ctap_parse_cred_mgmt' is never used.
(unusedFunction)
[style] 1025-1025: The function 'ctap_parse_get_assertion' is never used.
(unusedFunction)
[style] 1238-1238: The function 'ctap_parse_client_pin' is never used.
(unusedFunction)
[style] 1368-1368: The function 'ctap_parse_credential_id' is never used.
(unusedFunction)
legacy/firmware/fido2/ctap_trans.c
[style] 171-171: The function 'dialog_get_timer_start' is never used.
(unusedFunction)
[style] 180-180: The function 'u2fhid_read' is never used.
(unusedFunction)
[style] 1301-1301: The function 'set_ble_fido_data' is never used.
(unusedFunction)
[style] 1306-1306: The function 'set_ble_fido_data_len' is never used.
(unusedFunction)
[style] 1308-1308: The function 'get_ble_fido_data_ptr' is never used.
(unusedFunction)
[style] 1310-1310: The function 'check_ble_timeout' is never used.
(unusedFunction)
[style] 1633-1633: The function 'ctap_ble_u2f_error' is never used.
(unusedFunction)
[style] 1690-1690: The function 'ctap_ble_cmd' is never used.
(unusedFunction)
legacy/firmware/fido2/ctap.c
[style] 124-124: The function 'ctap_get_info' is never used.
(unusedFunction)
[style] 959-959: The function 'ctap_make_credential' is never used.
(unusedFunction)
[style] 1752-1752: The function 'ctap_get_assertion' is never used.
(unusedFunction)
[style] 1996-1996: The function 'ctap_client_pin' is never used.
(unusedFunction)
🪛 Ruff (0.8.2)
python/src/trezorlib/messages.py
10662-10665: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
10718-10718: Remove quotes from type annotation
Remove quotes
(UP037)
10719-10719: Remove quotes from type annotation
Remove quotes
(UP037)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Gen check
- GitHub Check: Defs check
- GitHub Check: Style check
🔇 Additional comments (62)
legacy/firmware/fsm_msg_webauthn.h (1)
88-93: Validate credential data correctlyThe check
ctap_authenticate_credential_data(cred_id_storage.rp_id_hash, &desc) == 0treats a return value of zero as failure. Confirm that this function returns zero on success and adjust the condition accordingly to prevent logic errors.legacy/firmware/fido2/ctap.h (1)
494-495: Validate function return typesThe function
ctap_authenticate_credential_datareturns anint. Ensure that the return values are consistent with the expected success or error codes and handle them appropriately throughout the code.legacy/firmware/usb.c (2)
34-34: Include the correct header fileThe inclusion of
"fido2/ctap_trans.h"replaces the previous U2F header. Confirm that all necessary functions and definitions are migrated correctly to support FIDO2.
567-579: Confirm necessity of the unused functionStatic analysis indicates that
usb_u2f_data_sendis unused. If it's intended for future use, add a comment explaining its purpose. Otherwise, remove it to prevent confusion.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 567-567: The function 'usb_u2f_data_send' is never used.
(unusedFunction)
legacy/firmware/fido2/ctap_parse.c (2)
137-138: HandleCborErrorOutOfMemorycorrectlyIgnoring
CborErrorOutOfMemorymight lead to incomplete data parsing. Ensure that the error is handled appropriately.Please verify if it's acceptable to ignore
CborErrorOutOfMemoryin this context. If not, consider handling the error to ensure data integrity.
371-378: 🧹 Nitpick (assertive)Ensure
keybuffer is null-terminatedAfter copying the key, confirm it's properly null-terminated to prevent buffer overflows or unexpected behavior.
Add null termination to the
keybuffer:key[sizeof(key) - 1] = 0; +key[sz < sizeof(key) ? sz : sizeof(key) - 1] = '\0';Likely invalid or redundant comment.
legacy/firmware/fido2/ctap_trans.c (1)
1500-1506:⚠️ Potential issueCheck for
NULLinvalidateKeyHandleEnsure that
validateKeyHandledoesn't returnNULLbefore using its result to avoid null pointer dereferences.Add a check for
NULLbefore proceeding:const HDNode *node = validateKeyHandle(req->appId, req->keyHandle); +if (!node) { + send_u2f_error(U2F_SW_WRONG_DATA); + return; +}Likely invalid or redundant comment.
legacy/firmware/se_chip.c (9)
1658-1665: Confirm the necessity ofse_slip21_fido_nodeThe function
se_slip21_fido_nodeis defined but not used in the provided codebase. Please verify if this function is required. If not, consider removing it to keep the code clean.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1658-1658: The function 'se_slip21_fido_node' is never used.
(unusedFunction)
1667-1694: Confirm the necessity ofse_derive_fido_keysThe function
se_derive_fido_keysis defined but not used in the provided codebase. Please verify if this function is required. If not, consider removing it to keep the code clean.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1667-1667: The function 'se_derive_fido_keys' is never used.
(unusedFunction)
1696-1708: Confirm the necessity ofse_fido_hdnode_sign_digestThe function
se_fido_hdnode_sign_digestis defined but not used in the provided codebase. Please verify if this function is required. If not, consider removing it to keep the code clean.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1696-1696: The function 'se_fido_hdnode_sign_digest' is never used.
(unusedFunction)
1710-1722: Confirm the necessity ofse_fido_att_sign_digestThe function
se_fido_att_sign_digestis defined but not used in the provided codebase. Please verify if this function is required. If not, consider removing it to keep the code clean.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1710-1710: The function 'se_fido_att_sign_digest' is never used.
(unusedFunction)
1777-1799: Confirm the necessity ofse_get_fido2_resident_credentialsThe function
se_get_fido2_resident_credentialsis defined but not used in the provided codebase. Please verify if this function is required. If not, consider removing it to keep the code clean.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1777-1777: The function 'se_get_fido2_resident_credentials' is never used.
(unusedFunction)
1801-1811: Confirm the necessity ofse_set_fido2_resident_credentialsThe function
se_set_fido2_resident_credentialsis defined but not used in the provided codebase. Please verify if this function is required. If not, consider removing it to keep the code clean.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1801-1801: The function 'se_set_fido2_resident_credentials' is never used.
(unusedFunction)
1813-1818: Confirm the necessity ofse_delete_fido2_resident_credentialsThe function
se_delete_fido2_resident_credentialsis defined but not used in the provided codebase. Please verify if this function is required. If not, consider removing it to keep the code clean.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1813-1813: The function 'se_delete_fido2_resident_credentials' is never used.
(unusedFunction)
1819-1824: Confirm the necessity ofse_delete_all_fido2_credentialsThe function
se_delete_all_fido2_credentialsis defined but not used in the provided codebase. Please verify if this function is required. If not, consider removing it to keep the code clean.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1819-1819: The function 'se_delete_all_fido2_credentials' is never used.
(unusedFunction)
1826-1838: Confirm the necessity ofse_check_fido2_resident_credential_simpleThe function
se_check_fido2_resident_credential_simpleis defined but not used in the provided codebase. Please verify if this function is required. If not, consider removing it to keep the code clean.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1826-1826: The function 'se_check_fido2_resident_credential_simple' is never used.
(unusedFunction)
legacy/firmware/fido2/ctap.c (2)
1277-1328: Check for integer underflow in signature encodingIn
ctap_encode_der_sig, the calculations forlead_randlead_smight result in negative values if all bytes are zero. Ensure that the variableslead_randlead_sare properly checked to prevent integer underflow.
1777-1781: Handle empty allow list correctlyWhen the allow list is empty in
ctap_get_assertion, the code setsis_resident_credentialto true. Ensure that this behavior complies with the specification and that resident credentials are handled appropriately.legacy/firmware/version.h (1)
10-14: Version update looks goodThe version numbers are correctly updated to reflect the new minor version. The hexadecimal representation matches the version string.
legacy/firmware/protob/messages-webauthn.options (2)
7-9: 🧹 Nitpick (assertive)Review credential count limitation
The
max_count:1limit on credentials seems overly restrictive. Most FIDO2 authenticators support multiple resident credentials.Consider increasing the limit based on available storage and typical usage patterns.
1-6: Verify field size limits against WebAuthn specThe size limits for credential fields need verification:
id(512 bytes): Matches common WebAuthn implementationsrp_id(256 bytes): Reasonable for domain names- Other fields (64 bytes): May be too restrictive for some use cases
✅ Verification successful
Field size limits are correctly implemented
The size limits align with FIDO2/WebAuthn specifications and are properly enforced throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for WebAuthn field size definitions in codebase rg -i "webauthn.*size|credential.*size" --type c --type hLength of output: 3479
legacy/firmware/chinese.h (1)
7-7:⚠️ Potential issueVerify UTF-8 migration impact
Changing
HZ_CODE_LENfrom 2 (GBK) to 3 (UTF-8) affects:
- Memory allocation
- String parsing
- Display rendering
Ensure all character processing code is updated for UTF-8.
legacy/si2c.h (1)
38-38:⚠️ Potential issueReview I2C buffer security
The new FIDO I2C send function needs:
- Buffer overflow protection
- Data validation
- Error handling
Ensure
data_lencannot exceedSI2C_BUF_MAX_OUT_LEN - PREFIX_SIZE.legacy/firmware/fido2/resident_credential.h (2)
9-15: 🧹 Nitpick (assertive)Document security implications of credential limits
Add comments explaining:
- Why 60 is chosen as MAX_RESIDENT_CREDENTIALS
- Security implications of these limits
- How overflow is prevented
17-25:⚠️ Potential issueReview packed structure security
The packed credential storage structure needs attention:
- Document alignment requirements
- Verify buffer sizes prevent overflow
- Explain the flash page size constraint
Consider adding runtime checks for credential_id size.
legacy/timer.h (1)
57-60: 🧹 Nitpick (assertive)Document callback safety requirements
Add documentation explaining:
- Maximum allowed intervals
- Callback execution context
- Error handling for invalid parameters
- Resource cleanup guarantees
legacy/firmware/usb.h (1)
59-60:⚠️ Potential issueDocument USB security considerations
Add documentation for
usb_u2f_data_send:
- Error handling
- Buffer overflow prevention
- USB protocol security
- Timing attack mitigations
Run this verification:
legacy/firmware/fido2/ctap_parse.h (1)
26-26: Verify buffer overflow protection in parse_fixed_byte_string.The function accepts a destination buffer and length. Ensure proper bounds checking exists in the implementation.
✅ Verification successful
Buffer overflow protection verified in parse_fixed_byte_string
The function implements proper bounds checking by verifying the input length matches the expected length and includes type validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for parse_fixed_byte_string implementation ast-grep --pattern 'parse_fixed_byte_string($dst, $len) { $$$ }'Length of output: 68
Script:
#!/bin/bash # Find the implementation file rg -l "parse_fixed_byte_string" # Then get the implementation with context rg -U "parse_fixed_byte_string.*\{" -A 30Length of output: 145
Script:
#!/bin/bash # Get the implementation cat legacy/firmware/fido2/ctap_parse.cLength of output: 42369
legacy/timer.c (1)
154-175: Address unused functions warning.Static analysis indicates these functions are unused:
- register_loop_callback
- unregister_loop_callback
- loop_callback_handler
Either implement the usage or document why they're needed for future use.
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 154-154: The function 'register_loop_callback' is never used.
(unusedFunction)
[style] 162-162: The function 'unregister_loop_callback' is never used.
(unusedFunction)
[style] 168-168: The function 'loop_callback_handler' is never used.
(unusedFunction)
legacy/firmware/menu_core.c (3)
Line range hint
37-84: Excellent refactoring of menu display logic!The changes improve maintainability by:
- Using a single 2D array instead of multiple variables
- Adding proper bounds checking
- Maintaining internationalization support
Line range hint
134-163: Good implementation of menu refresh logic!The refresh timing and state management are handled correctly.
10-12: Clean implementation of menu refresh control!The refresh mechanism is simple and effective.
Let's verify the usage of
menu_refresh:✅ Verification successful
menu_refresh()is actively used in the codebaseThe function is properly declared in the header and called from
menu_list.c.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for menu_refresh usage across the codebase rg "menu_refresh"Length of output: 199
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 12-12: The function 'menu_refresh' is never used.
(unusedFunction)
legacy/firmware/fido2/u2f_hid.h (1)
130-136: Well-structured FIDO2 protocol constants!The new constants follow the existing pattern and maintain proper command spacing.
legacy/firmware/fido2/resident_credential.c (3)
8-8: Verify if hardcoded return value is intentional.The function always returns 0, which might prevent credential management from working correctly.
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 8-8: The function 'resident_credential_get_count' is never used.
(unusedFunction)
103-125: Same UI callback issue as noted above.The UI callback needs null checking here too.
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 104-104: The function 'resident_credential_info' is never used.
(unusedFunction)
127-147: Clean implementation of credential description retrieval!The function handles errors properly and has clear success/failure paths.
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 127-127: The function 'resident_credential_get_desc' is never used.
(unusedFunction)
legacy/firmware/i18n/locales/zh_cn.inc (1)
350-368: LGTM! Translations are accurate and consistentThe Simplified Chinese translations correctly reflect the FIDO2 terminology and maintain consistent style with the rest of the UI.
legacy/firmware/i18n/locales/ja.inc (1)
350-368: LGTM! Japanese translations are well-formattedThe Japanese translations accurately convey the FIDO2 concepts while maintaining natural language flow and proper terminology.
legacy/firmware/se_chip.h (2)
22-25: Well-defined status codes.The FIDO2 slot data status codes are clear and follow a logical sequence.
161-167: Validate index parameter.The credential management functions use an index parameter. Ensure proper bounds checking in the implementation to prevent buffer overflows.
✅ Verification successful
Index validation is properly implemented.
All credential management functions check the index against FIDO2_RESIDENT_CREDENTIALS_COUNT (60). The implementation includes additional safeguards for buffer sizes and data integrity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for index validation in implementation rg -A 3 "se_.*_fido2_resident_credentials.*\(" | grep -E "if.*index"Length of output: 347
Script:
#!/bin/bash # Check constant definition and all credential function implementations echo "=== Constant definition ===" rg "FIDO2_RESIDENT_CREDENTIALS_COUNT" echo -e "\n=== Credential function implementations ===" ast-grep --pattern 'int se_get_fido2_resident_credentials($$$) { $$$ }' ast-grep --pattern 'int se_check_fido2_resident_credential_simple($$$) { $$$ }' ast-grep --pattern 'secbool se_set_fido2_resident_credentials($$$) { $$$ }' ast-grep --pattern 'secbool se_delete_fido2_resident_credentials($$$) { $$$ }'Length of output: 6319
legacy/si2c.c (1)
126-128: Correct I2C address handling.The code properly clears the address flag after detection.
legacy/firmware/chinese.c (1)
121-125: Clean and efficient implementation.The function provides a useful helper for single character width calculation.
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 121-121: The function 'oledCharWidthEx' is never used.
(unusedFunction)
legacy/firmware/i18n/locales/pt_br.inc (1)
350-368: Well-structured FIDO2 translations!The Portuguese translations are clear, consistent, and follow proper localization patterns. The messages effectively guide users through FIDO2 operations.
legacy/firmware/i18n/locales/es.inc (1)
350-368: Clear and consistent FIDO2 translations!The Spanish translations maintain proper terminology and provide clear user guidance for FIDO2 operations.
legacy/firmware/fsm.h (1)
342-349: Well-defined WebAuthn function declarations!The new functions follow consistent naming patterns and properly handle FIDO2 resident credential operations.
legacy/firmware/layout2.h (1)
321-323: Clean layout function declaration!The new FIDO2 credential layout function has clear parameters and follows the existing code style.
core/src/trezor/messages.py (2)
8644-8646: LGTM! New fields added to WebAuthnListResidentCredentialsThe new fields
indexandrequest_list_indexare properly typed and follow Python conventions.
8689-8689: LGTM! New field added to WebAuthnCredentialsThe new field
id_mapis properly typed as a list of bytes.vendor/tiny_cbor (1)
1-1: Verify TinyCBOR version security and integrationGood choice using a fixed commit. Let's verify it's secure and properly integrated.
✅ Verification successful
TinyCBOR integration looks solid
The submodule is properly integrated:
- Build system includes all required components
- Headers and source files are correctly linked
- Full CBOR functionality available (encoding, parsing, validation)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this commit is tagged in the original repo git ls-remote --tags https://github.com/intel/tinycbor.git | grep 878eb01b96c573e353b217439c0f418f69c2ca04 # Verify Makefile integration rg -l "tiny_cbor" | grep -i makefile # Look for FIDO2 CBOR usage patterns rg -l "cbor.*encode|cbor.*decode" | grep -v "vendor/tiny_cbor"Length of output: 672
Script:
#!/bin/bash # Check Makefile integration details cat legacy/Makefile.include | grep -A 5 -B 5 tiny_cbor cat legacy/firmware/Makefile | grep -A 5 -B 5 tiny_cbor # Look for FIDO2 CBOR version requirements or compatibility notes rg -i "cbor.*version|cbor.*require" legacy/firmware/fido2/Length of output: 1202
legacy/vendor/tiny_cbor (2)
1-1: Verify the symbolic link integrityThe relative path
../../vendor/tiny_cbor/might break if directory structures change. Consider using absolute paths from the project root.Let's verify the symbolic link setup:
✅ Verification successful
Symlink implementation is correct
The symbolic link setup for TinyCBOR is properly implemented and follows the project's conventions for vendor dependencies. The relative path structure is appropriate for the repository layout.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify symbolic link integrity and TinyCBOR submodule status # Check if the symbolic link is valid readlink -f legacy/vendor/tiny_cbor # Verify submodule initialization git submodule status vendor/tiny_cbor # Check for any broken symbolic links in the project find . -type l ! -exec test -e {} \; -printLength of output: 9222
1-1: Check TinyCBOR version for security updatesThe current implementation uses a fixed commit (878eb01). Let's verify it's up-to-date.
Run this check:
legacy/firmware/fido2/cose_key.h (1)
10-24: LGTM! Constants match COSE standard specifications.The constants are well-defined and follow RFC 8152 specifications for COSE.
Let's verify these constants against the COSE registry:
✅ Verification successful
Constants verified through codebase implementation
The COSE constants are correctly implemented and consistently used across the FIDO2 firmware. The code shows proper integration with CBOR encoding/decoding and follows the WebAuthn/FIDO2 specifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Fetch IANA COSE registry to verify constant values curl -s https://www.iana.org/assignments/cose/cose.txt | grep -A 20 "Registry:"Length of output: 79
Script:
#!/bin/bash # Search for COSE-related test files and RFC references echo "=== Searching for COSE tests ===" rg -l "COSE" --type=c --type=h legacy/firmware/fido2/ echo -e "\n=== Searching for RFC 8152 references ===" rg -i "rfc.?8152|cose.?standard" --type=txt --type=md echo -e "\n=== Finding other COSE constant definitions ===" rg "COSE_(KEY|ALG)" --type=c --type=h -A 1Length of output: 6163
legacy/firmware/fido2/u2f_knownapps.h.mako (1)
8-8: Verify the new include path.Check if u2f.h is accessible from the new path.
✅ Verification successful
Include path change is correct
The file
u2f.hexists in the same directory, making the new include path valid.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if u2f.h exists in the include paths fd -t f "u2f.h$" | grep -v "u2f_knownapps"Length of output: 107
common/protob/messages-webauthn.proto (1)
17-18: Good field numbering for backward compatibility.Using high field numbers (100, 101) preserves space for future additions in the 1-99 range.
legacy/firmware/protob/Makefile (1)
17-17: LGTM! Consistent Makefile updates.The changes properly integrate WebAuthn message handling into the build system.
Also applies to: 24-25
legacy/firmware/fido2/ctap_errors.h (4)
1-6: LGTM! Clear and standard license header.The dual-license approach (Apache 2.0 or MIT) is appropriate for open-source firmware.
7-15: LGTM! Well-organized CTAP1 error codes.Error codes follow the FIDO CTAP specification, covering basic protocol errors.
16-52: LGTM! Comprehensive CTAP2 error codes.Error codes cover all essential CTAP2 scenarios:
- CBOR parsing (0x10-0x14)
- Credential management (0x19-0x2E)
- PIN operations (0x31-0x38)
53-58: LGTM! Well-defined error code ranges.Clear separation between spec-defined (0x00-0xDF), extension (0xE0-0xEF), and vendor-specific (0xF0-0xFF) error codes.
legacy/firmware/Makefile (3)
88-91: LGTM! Complete FIDO2 implementation files.All essential FIDO2 components are included:
- Transport layer (ctap_trans.o)
- Core protocol (ctap.o)
- Message parsing (ctap_parse.o)
- Credential management (resident_credential.o)
226-230: LGTM! Complete TinyCBOR library integration.All required TinyCBOR components are included:
- Encoder
- Parser
- Validation
- Error strings
275-275: LGTM! WebAuthn protobuf integration.WebAuthn message support is correctly added under the non-bitcoin-only condition.
| static bool is_protect_button_pressed = false; | ||
| static uint8_t last_index = 0; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid static variables for state management
Using static variables is_protect_button_pressed and last_index can lead to issues in concurrent execution. They may retain state between function calls, causing unintended behavior. Consider using function parameters or a dedicated state structure instead.
| resp->id_map[0].bytes[count] = i; | ||
| last_index = i; | ||
| count++; | ||
| } | ||
| if (count > sizeof(resp->id_map[0].bytes)) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Prevent potential buffer overflow
The condition if (count > sizeof(resp->id_map[0].bytes)) may cause an off-by-one error. Ensure the comparison operator is correct and that count does not exceed the buffer size to prevent overflow.
| if (!is_protect_button_pressed) { | ||
| fsm_sendFailure(FailureType_Failure_ProcessError, | ||
| "Please get the index information first "); | ||
| layoutHome(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve error handling for user prompts
When is_protect_button_pressed is false, the function returns an error asking the user to get index information first. This might confuse users. Consider guiding the user through the required steps or resetting the state to allow retrying.
| memcpy(resp->credentials[0].id.bytes, cred_id_storage.credential_id, | ||
| len - RP_ID_HASH_LENGTH); | ||
| resp->credentials[0].id.size = len - RP_ID_HASH_LENGTH; |
There was a problem hiding this comment.
Ensure safe memory operations
Copying data with memcpy(resp->credentials[0].id.bytes, cred_id_storage.credential_id, len - RP_ID_HASH_LENGTH) could overflow resp->credentials[0].id.bytes if the length exceeds its size. Verify that len - RP_ID_HASH_LENGTH does not exceed the destination buffer size.
| if (!ctap_authenticate_credential_data(NULL, &desc)) { | ||
| fsm_sendFailure(FailureType_Failure_ProcessError, | ||
| "The credential you are trying to import does\nnot belong " | ||
| "to this authenticator."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Handle NULL pointers cautiously
Passing NULL for rp_id_hash in ctap_authenticate_credential_data(NULL, &desc) may lead to undefined behavior. Ensure that this function can safely handle a NULL argument or provide a valid pointer.
| FIELDS = { | ||
| 100: protobuf.Field("index", "uint32", repeated=False, required=False, default=None), | ||
| 101: protobuf.Field("request_list_index", "bool", repeated=False, required=False, default=None), | ||
| } | ||
|
|
||
| def __init__( | ||
| self, | ||
| *, | ||
| index: Optional["int"] = None, | ||
| request_list_index: Optional["bool"] = None, | ||
| ) -> None: | ||
| self.index = index | ||
| self.request_list_index = request_list_index |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider typing improvements.
The new fields look good. Consider using typing.ClassVar for mutable class attributes to improve type safety.
- FIELDS = {
+ FIELDS: typing.ClassVar[dict] = {
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
10662-10665: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
| 100: protobuf.Field("id_map", "bytes", repeated=True, required=False, default=None), | ||
| } | ||
|
|
||
| def __init__( | ||
| self, | ||
| *, | ||
| credentials: Optional[Sequence["WebAuthnCredential"]] = None, | ||
| id_map: Optional[Sequence["bytes"]] = None, | ||
| ) -> None: | ||
| self.credentials: Sequence["WebAuthnCredential"] = credentials if credentials is not None else [] | ||
| self.id_map: Sequence["bytes"] = id_map if id_map is not None else [] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Clean up type annotations.
The new id_map field looks good. Remove quotes from type annotations to follow Python typing best practices.
- self.id_map: Sequence["bytes"] = id_map if id_map is not None else []
+ self.id_map: Sequence[bytes] = id_map if id_map is not None else []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 100: protobuf.Field("id_map", "bytes", repeated=True, required=False, default=None), | |
| } | |
| def __init__( | |
| self, | |
| *, | |
| credentials: Optional[Sequence["WebAuthnCredential"]] = None, | |
| id_map: Optional[Sequence["bytes"]] = None, | |
| ) -> None: | |
| self.credentials: Sequence["WebAuthnCredential"] = credentials if credentials is not None else [] | |
| self.id_map: Sequence["bytes"] = id_map if id_map is not None else [] | |
| 100: protobuf.Field("id_map", "bytes", repeated=True, required=False, default=None), | |
| } | |
| def __init__( | |
| self, | |
| *, | |
| credentials: Optional[Sequence["WebAuthnCredential"]] = None, | |
| id_map: Optional[Sequence["bytes"]] = None, | |
| ) -> None: | |
| self.credentials: Sequence["WebAuthnCredential"] = credentials if credentials is not None else [] | |
| self.id_map: Sequence[bytes] = id_map if id_map is not None else [] |
🧰 Tools
🪛 Ruff (0.8.2)
10718-10718: Remove quotes from type annotation
Remove quotes
(UP037)
10719-10719: Remove quotes from type annotation
Remove quotes
(UP037)
| [submodule "vendor/tiny_cbor"] | ||
| path = vendor/tiny_cbor | ||
| url = https://github.com/intel/tinycbor.git |
There was a problem hiding this comment.
💡 Codebase verification
Update TinyCBOR to latest stable release v0.6.0
The current commit from April 2019 is outdated. The latest stable release v0.6.0 (October 2021) offers:
- More recent bug fixes
- Better stability
- Official release support
Update the submodule reference to tag v0.6.0 instead of commit 878eb01.
🔗 Analysis chain
Verify TinyCBOR version compatibility.
TinyCBOR is a good choice for CBOR encoding/decoding. Let's check its version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check TinyCBOR version and recent activity
if [ -d "vendor/tiny_cbor" ]; then
cd vendor/tiny_cbor
git log -1 --format="%H %s"
fi
Length of output: 51
Script:
#!/bin/bash
# Check submodule configuration and status
git config -f .gitmodules --get-regexp '^submodule\.vendor/tiny_cbor\.'
git submodule status vendor/tiny_cbor
Length of output: 290
Script:
#!/bin/bash
# Check commit details from GitHub API
gh api repos/intel/tinycbor/commits/878eb01b96c573e353b217439c0f418f69c2ca04 --jq '.commit.committer.date, .commit.message'
Length of output: 255
Script:
#!/bin/bash
# Check latest release info
gh api repos/intel/tinycbor/releases/latest --jq '.tag_name, .published_at'
Length of output: 105
| @@ -58,4 +60,5 @@ message WebAuthnCredentials { | |||
| optional sint32 algorithm = 11; | |||
| optional sint32 curve = 12; | |||
| } | |||
| repeated bytes id_map = 100; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider documenting the id_map field.
Add a comment explaining the purpose and format of the id_map field.
| @@ -290,6 +297,7 @@ CFLAGS += -I../vendor/nanopb -Iprotob -DPB_FIELD_16BIT=1 -DPB_ENCODE_ARRAYS_UNPA | |||
| CFLAGS += -DSCM_REVISION='"$(shell git rev-parse HEAD | sed 's:\(..\):\\x\1:g')"' | |||
| CFLAGS += -DBUILD_ID='"$(NAME)"' | |||
| CFLAGS += -DUSE_MONERO=0 | |||
| CFLAGS += -I./ | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding FIDO2-specific include paths.
The generic include path -I./ might be better split into specific paths for better clarity.
-CFLAGS += -I./
+CFLAGS += -I./ -I./fido2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CFLAGS += -I./ | |
| CFLAGS += -I./ -I./fido2 |
There was a problem hiding this comment.
Actionable comments posted: 31
♻️ Duplicate comments (5)
legacy/Makefile.include (1)
107-107:⚠️ Potential issueAdd TinyCBOR library linkage
The include path is added, but the build configuration needs library linkage.
Add the library linkage:
LDLIBS += -lsol +LDLIBS += -ltinycborlegacy/firmware/protob/messages-webauthn.proto (1)
1-1: 🧹 Nitpick (assertive)Add version pinning comment.
Add a comment to track which version of the proto file you're using.
+# Version: <commit-hash> from trezor-common ../../vendor/trezor-common/protob/messages-webauthn.protolegacy/script/cibuild (1)
10-12:⚠️ Potential issueAdd error handling for FIDO2 rendering
The script needs error checks for cointool.py existence and rendering completion.
if [ "$BITCOIN_ONLY" != 1 ]; then + if [ ! -f vendor/trezor-common/tools/cointool.py ]; then + echo "Error: cointool.py not found" + exit 1 + fi vendor/trezor-common/tools/cointool.py render firmware/fido2 + if [ $? -ne 0 ]; then + echo "Error: FIDO2 rendering failed" + exit 1 + fi filegacy/firmware/chinese.h (1)
10-10: 🧹 Nitpick (assertive)Document new character width function
Add documentation for
oledCharWidthExparameters and return value.+/** + * Calculate character width based on font type + * @param text Character to measure + * @param font Font identifier (0-9) + * @return Width in pixels + */ int oledCharWidthEx(const char text, uint8_t font);legacy/firmware/menu_core.h (1)
43-43: 🧹 Nitpick (assertive)Add function documentation
Add a comment block to explain the purpose and behavior of
menu_refresh().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
legacy/firmware/fido2/trezordevkey.pemis excluded by!**/*.pem
📒 Files selected for processing (60)
.gitmodules(1 hunks)common/protob/messages-webauthn.proto(2 hunks)core/src/trezor/messages.py(2 hunks)legacy/Makefile.include(1 hunks)legacy/firmware/Makefile(4 hunks)legacy/firmware/chinese.c(1 hunks)legacy/firmware/chinese.h(1 hunks)legacy/firmware/fido2/cose_key.h(1 hunks)legacy/firmware/fido2/ctap.c(1 hunks)legacy/firmware/fido2/ctap.h(1 hunks)legacy/firmware/fido2/ctap_errors.h(1 hunks)legacy/firmware/fido2/ctap_parse.c(1 hunks)legacy/firmware/fido2/ctap_parse.h(1 hunks)legacy/firmware/fido2/ctap_trans.c(27 hunks)legacy/firmware/fido2/ctap_trans.h(3 hunks)legacy/firmware/fido2/resident_credential.c(1 hunks)legacy/firmware/fido2/resident_credential.h(1 hunks)legacy/firmware/fido2/u2f_hid.h(2 hunks)legacy/firmware/fido2/u2f_knownapps.h.mako(1 hunks)legacy/firmware/filecoin/cbor.h(0 hunks)legacy/firmware/filecoin/cborinternal_p.h(0 hunks)legacy/firmware/filecoin/cborparser.c(0 hunks)legacy/firmware/filecoin/cborvalidation.c(0 hunks)legacy/firmware/filecoin/compilersupport_p.h(0 hunks)legacy/firmware/filecoin/tinycbor-version.h(0 hunks)legacy/firmware/filecoin/utf8_p.h(0 hunks)legacy/firmware/fsm.c(1 hunks)legacy/firmware/fsm.h(2 hunks)legacy/firmware/fsm_msg_webauthn.h(1 hunks)legacy/firmware/i18n/keys.h(2 hunks)legacy/firmware/i18n/locales/en.inc(2 hunks)legacy/firmware/i18n/locales/es.inc(1 hunks)legacy/firmware/i18n/locales/ja.inc(1 hunks)legacy/firmware/i18n/locales/pt_br.inc(1 hunks)legacy/firmware/i18n/locales/zh_cn.inc(1 hunks)legacy/firmware/i18n/locales/zh_tw.inc(1 hunks)legacy/firmware/layout2.c(23 hunks)legacy/firmware/layout2.h(1 hunks)legacy/firmware/menu_core.c(5 hunks)legacy/firmware/menu_core.h(1 hunks)legacy/firmware/menu_list.c(19 hunks)legacy/firmware/protect.c(4 hunks)legacy/firmware/protob/Makefile(2 hunks)legacy/firmware/protob/messages-webauthn.options(1 hunks)legacy/firmware/protob/messages-webauthn.proto(1 hunks)legacy/firmware/se_chip.c(3 hunks)legacy/firmware/se_chip.h(2 hunks)legacy/firmware/usb.c(4 hunks)legacy/firmware/usb.h(1 hunks)legacy/firmware/version.h(1 hunks)legacy/script/cibuild(1 hunks)legacy/si2c.c(2 hunks)legacy/si2c.h(1 hunks)legacy/timer.c(2 hunks)legacy/timer.h(1 hunks)legacy/util.c(1 hunks)legacy/util.h(1 hunks)legacy/vendor/tiny_cbor(1 hunks)python/src/trezorlib/messages.py(2 hunks)vendor/tiny_cbor(1 hunks)
💤 Files with no reviewable changes (7)
- legacy/firmware/filecoin/cborinternal_p.h
- legacy/firmware/filecoin/cbor.h
- legacy/firmware/filecoin/tinycbor-version.h
- legacy/firmware/filecoin/compilersupport_p.h
- legacy/firmware/filecoin/utf8_p.h
- legacy/firmware/filecoin/cborvalidation.c
- legacy/firmware/filecoin/cborparser.c
🧰 Additional context used
🪛 cppcheck (2.10-2)
legacy/firmware/chinese.c
[style] 121-121: The function 'oledCharWidthEx' is never used.
(unusedFunction)
legacy/util.c
[style] 148-148: The function 'compare_str_version' is never used.
(unusedFunction)
legacy/si2c.c
[style] 301-301: The function 'i2c_slave_send_fido' is never used.
(unusedFunction)
legacy/timer.c
[style] 154-154: The function 'register_loop_callback' is never used.
(unusedFunction)
[style] 162-162: The function 'unregister_loop_callback' is never used.
(unusedFunction)
[style] 168-168: The function 'loop_callback_handler' is never used.
(unusedFunction)
legacy/firmware/usb.c
[style] 567-567: The function 'usb_u2f_data_send' is never used.
(unusedFunction)
legacy/firmware/menu_core.c
[style] 12-12: The function 'menu_refresh' is never used.
(unusedFunction)
legacy/firmware/fido2/resident_credential.c
[style] 8-8: The function 'resident_credential_get_count' is never used.
(unusedFunction)
[style] 10-10: The function 'resident_credential_find_by_rp_id_hash' is never used.
(unusedFunction)
[style] 45-45: The function 'resident_credential_store' is never used.
(unusedFunction)
[style] 104-104: The function 'resident_credential_info' is never used.
(unusedFunction)
[style] 127-127: The function 'resident_credential_get_desc' is never used.
(unusedFunction)
[style] 149-149: The function 'resident_credential_delete' is never used.
(unusedFunction)
legacy/firmware/layout2.c
[style] 5777-5777: The function 'layout_fido2_resident_credential' is never used.
(unusedFunction)
legacy/firmware/fido2/ctap_trans.c
[style] 171-171: The function 'dialog_get_timer_start' is never used.
(unusedFunction)
[style] 180-180: The function 'u2fhid_read' is never used.
(unusedFunction)
[style] 1301-1301: The function 'set_ble_fido_data' is never used.
(unusedFunction)
[style] 1306-1306: The function 'set_ble_fido_data_len' is never used.
(unusedFunction)
[style] 1308-1308: The function 'get_ble_fido_data_ptr' is never used.
(unusedFunction)
[style] 1310-1310: The function 'check_ble_timeout' is never used.
(unusedFunction)
[style] 1633-1633: The function 'ctap_ble_u2f_error' is never used.
(unusedFunction)
[style] 1690-1690: The function 'ctap_ble_cmd' is never used.
(unusedFunction)
legacy/firmware/fido2/ctap_parse.c
[style] 28-28: The function 'cbor_value_get_type_string' is never used.
(unusedFunction)
[style] 624-624: The function 'ctap_parse_make_credential' is never used.
(unusedFunction)
[style] 947-947: The function 'ctap_parse_cred_mgmt' is never used.
(unusedFunction)
[style] 1025-1025: The function 'ctap_parse_get_assertion' is never used.
(unusedFunction)
[style] 1238-1238: The function 'ctap_parse_client_pin' is never used.
(unusedFunction)
[style] 1368-1368: The function 'ctap_parse_credential_id' is never used.
(unusedFunction)
legacy/firmware/se_chip.c
[style] 1658-1658: The function 'se_slip21_fido_node' is never used.
(unusedFunction)
[style] 1667-1667: The function 'se_derive_fido_keys' is never used.
(unusedFunction)
[style] 1696-1696: The function 'se_fido_hdnode_sign_digest' is never used.
(unusedFunction)
[style] 1710-1710: The function 'se_fido_att_sign_digest' is never used.
(unusedFunction)
[style] 1777-1777: The function 'se_get_fido2_resident_credentials' is never used.
(unusedFunction)
[style] 1801-1801: The function 'se_set_fido2_resident_credentials' is never used.
(unusedFunction)
[style] 1813-1813: The function 'se_delete_fido2_resident_credentials' is never used.
(unusedFunction)
[style] 1819-1819: The function 'se_delete_all_fido2_credentials' is never used.
(unusedFunction)
[style] 1826-1826: The function 'se_check_fido2_resident_credential_simple' is never used.
(unusedFunction)
legacy/firmware/fido2/ctap.c
[style] 124-124: The function 'ctap_get_info' is never used.
(unusedFunction)
[style] 959-959: The function 'ctap_make_credential' is never used.
(unusedFunction)
[style] 1752-1752: The function 'ctap_get_assertion' is never used.
(unusedFunction)
[style] 1996-1996: The function 'ctap_client_pin' is never used.
(unusedFunction)
🪛 Ruff (0.8.2)
python/src/trezorlib/messages.py
10662-10665: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
10718-10718: Remove quotes from type annotation
Remove quotes
(UP037)
10719-10719: Remove quotes from type annotation
Remove quotes
(UP037)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Style check
- GitHub Check: Defs check
- GitHub Check: Gen check
🔇 Additional comments (69)
legacy/si2c.c (2)
126-128: LGTM: Address flag handling is correct.The added code properly clears the I2C address flag, which is required by the I2C protocol.
301-316:⚠️ Potential issueAdd safety checks and improve error handling.
The function needs several improvements:
- Buffer overflow protection
- Null pointer check
- Configurable timeout value
- Thread safety for shared buffer
Apply these changes:
+#define FIDO_PREFIX "fid" +#define FIDO_PREFIX_LEN 3 +#define FIDO_DEFAULT_TIMEOUT_MS 1000 + bool i2c_slave_send_fido(uint8_t *data, uint32_t data_len) { + if (!data || data_len > SI2C_BUF_MAX_OUT_LEN - FIDO_PREFIX_LEN) { + return false; + } + i2c_data_out_pos = 0; - i2c_data_outlen = data_len + 3; - memcpy(i2c_data_out, "fid", 3); + i2c_data_outlen = data_len + FIDO_PREFIX_LEN; + memcpy(i2c_data_out, FIDO_PREFIX, FIDO_PREFIX_LEN); memcpy(i2c_data_out + 3, data, data_len); SET_COMBUS_HIGH(); uint32_t start_time = timer_ms(); while (i2c_data_outlen > 0) { - if (timer_ms() - start_time > 1000) { + if (timer_ms() - start_time > FIDO_DEFAULT_TIMEOUT_MS) { SET_COMBUS_LOW(); return false; } } SET_COMBUS_LOW(); return true; }Likely invalid or redundant comment.
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 301-301: The function 'i2c_slave_send_fido' is never used.
(unusedFunction)
legacy/firmware/fido2/resident_credential.h (1)
27-37: Add documentation for credential management functionsPlease include comments for each function, explaining parameters, return values, and error conditions. This improves readability and maintainability.
legacy/timer.c (2)
152-152: Consider thread safety forloop_callbackAccessing the global
loop_callbackvariable without synchronization might cause race conditions. Add synchronization mechanisms to ensure thread safety.
145-175: Verify integration of loop callback functionsThe functions
register_loop_callback,unregister_loop_callback, andloop_callback_handlerare not called. Ensure they're integrated and invoked where necessary.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 154-154: The function 'register_loop_callback' is never used.
(unusedFunction)
[style] 162-162: The function 'unregister_loop_callback' is never used.
(unusedFunction)
[style] 168-168: The function 'loop_callback_handler' is never used.
(unusedFunction)
legacy/firmware/menu_core.c (2)
12-12: Verify usage ofmenu_refreshfunctionThe function
menu_refreshis defined but not used. Confirm if it should be called to refresh the menu or remove it if unnecessary.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 12-12: The function 'menu_refresh' is never used.
(unusedFunction)
37-53: Nice refactoring of menu descriptionsUsing the
descriptionsarray simplifies the code and reduces redundancy. Good job improving maintainability.legacy/firmware/se_chip.h (1)
154-167: Add documentation for FIDO2 functionsThese functions lack documentation. Explain their purpose, parameters, and return values to aid understanding and maintenance.
legacy/firmware/fsm_msg_webauthn.h (6)
20-21: Avoid static variables for stateUsing static variables like
is_protect_button_pressedandlast_indexcan cause issues with concurrent execution. They retain state between calls, leading to unintended behavior. Pass them as parameters or use a dedicated state structure instead.
44-44: Prevent potential buffer overflowThe condition
if (count > sizeof(resp->id_map[0].bytes))might cause an off-by-one error. Ensurecountdoes not exceed the buffer size to prevent overflow.
52-57: Improve error handling for user promptsIf
is_protect_button_pressedis false, returning an error asking the user to get index information first might confuse them. Guide the user through the required steps or reset the state to allow retrying.
147-149: Ensure safe memory operationsUsing
memcpyto copy data could overflowresp->credentials[0].id.bytesiflen - RP_ID_HASH_LENGTHexceeds its size. Verify that the data length fits within the destination buffer.
184-189: Handle NULL pointers carefullyPassing
NULLforrp_id_hashinctap_authenticate_credential_data(NULL, &desc)may lead to undefined behavior. Ensure the function can handle aNULLargument safely or provide a valid pointer.
219-223: Clarify error message for missing indexWhen
msg->has_indexis false, the error message says "Invalid credential index." Make it clearer by informing the user that the credential index is missing from the request.legacy/firmware/fido2/ctap.h (6)
151-153: Clarify size constraints in commentsPhrases like "Must be minimum of 64 bytes but can be more" can be confusing. Specify whether these are minimum required sizes or maximum limits to prevent misunderstandings.
193-203: Use consistent struct packing attributesEnsure all related structures use the same
__attribute__((packed))to prevent alignment issues across different compilers.
298-303: Allocate memory for dynamic arraysIn
CTAP_Credential_ID, the pointercred_idis declared but not initialized. Allocate memory before use to avoid undefined behavior.
464-469: Document error codes clearlyFunctions
ctap_store_keyandctap_load_keyreturnint8_t. The comment mentions "See error codes in storage.h," but the codes aren't specified. Provide explicit documentation or enums for error codes to improve maintainability.
471-471: Explain constant definitionsAdd a comment explaining why
PIN_TOKEN_SIZEis set to 16. This helps future developers understand its significance.
477-485: Clean up debug macrosDebug macros like
DEBUG_CTAP,ctap_printf, anddump_hex1may not be needed in production. Remove or conditionally compile them to streamline the codebase.legacy/firmware/usb.c (3)
321-328: Improved NULL device pointer handlingGreat job handling the NULL
devpointers inu2f_rx_callback. This prevents potential null dereferences and enhances stability.
447-465: Optimize I2C slave polling logicThe repeated calls to
fifo_lockdata_leninside the loop might be inefficient. Consider storingtotal_lenonce per iteration to improve performance.
567-579: Remove unused functionThe function
usb_u2f_data_sendis never used. Based on the static analysis hint, consider removing it to clean up the codebase.🧰 Tools
🪛 cppcheck (2.10-2)
[style] 567-567: The function 'usb_u2f_data_send' is never used.
(unusedFunction)
legacy/firmware/i18n/keys.h (1)
631-631: 🧹 Nitpick (assertive)Correct informal language in message
The message
C__DATA_TRANSFER_MODE_USE_A_CHARGER_IF_YOU_WANNA_FASTER_CHARGINGuses informal language. Replace "wanna" with "want to" for professionalism.Apply this diff to correct the message:
-#define C__DATA_TRANSFER_MODE_USE_A_CHARGER_IF_YOU_WANNA_FASTER_CHARGING 301 +#define C__DATA_TRANSFER_MODE_USE_A_CHARGER_TO_ALLOW_FAST_CHARGING 301Likely invalid or redundant comment.
legacy/firmware/fido2/ctap_parse.c (1)
28-28: Unused functions detectedThe following functions are defined but not used:
cbor_value_get_type_stringctap_parse_make_credentialctap_parse_cred_mgmtctap_parse_get_assertionctap_parse_client_pinctap_parse_credential_idVerify if these functions are intended for future use or if they can be removed to clean up the code.
Also applies to: 624-624, 947-947, 1025-1025, 1238-1238, 1368-1368
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 28-28: The function 'cbor_value_get_type_string' is never used.
(unusedFunction)
legacy/firmware/fido2/ctap_trans.c (2)
1092-1099: Consolidate error handling insend_u2f_errorThe function checks
transport_typemultiple times for similar operations. Simplify error handling to improve readability.
1633-1638: Verify usage of potentially unused functionctap_ble_u2f_errorStatic analysis indicates that
ctap_ble_u2f_errormay not be used. Please verify if this function is necessary, and consider removing it if it's unused.Run the following script to check for usages of
ctap_ble_u2f_error:🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1633-1633: The function 'ctap_ble_u2f_error' is never used.
(unusedFunction)
legacy/firmware/se_chip.c (1)
1658-1666: Verify usage of potentially unused functionsStatic analysis suggests that several functions, such as
se_slip21_fido_node,se_derive_fido_keys,se_fido_hdnode_sign_digest,se_fido_att_sign_digest,se_get_fido2_resident_credentials,se_set_fido2_resident_credentials,se_delete_fido2_resident_credentials,se_delete_all_fido2_credentials, andse_check_fido2_resident_credential_simple, might be unused. Please verify if these functions are necessary, and consider removing them if they're not used.Run the following script to search for usages of these functions:
Also applies to: 1667-1695, 1696-1722, 1748-1775, 1777-1799, 1801-1812, 1813-1818, 1819-1824, 1826-1839
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1658-1658: The function 'se_slip21_fido_node' is never used.
(unusedFunction)
legacy/firmware/fido2/ctap.c (2)
331-393: Handle potential null pointer dereferenceEnsure that
ext->hmac_secret.saltEncis not null before using it in thehmac-secretextension handling. This prevents possible null pointer dereferences.
1892-1899: Avoid potential infinite loop intrailing_zerosfunctionIf
indxis zero andbuf[0]is zero, the while loop might continue indefinitely. Add a condition to preventindxfrom becoming negative.legacy/firmware/protob/messages-webauthn.options (1)
7-9: Review WebAuthnCredentials limitsThe max_count:1 limit on credentials seems very restrictive. This would allow storing only one credential per device.
Please confirm if this is intentional or if it should support multiple credentials.
legacy/firmware/fido2/ctap_parse.h (1)
22-44: Add function documentation.Add documentation for each function describing:
- Purpose
- Parameters
- Return values
- Error conditions
legacy/firmware/fido2/ctap_trans.h (2)
68-77: Group related functions together.Move BLE-related functions next to each other and add section comments to improve code organization:
+// FIDO2 core functions uint8_t ctap_cbor_cmd(const uint8_t *data, const uint32_t len); +void ctap_error(uint8_t err); -// ble +// BLE communication void ctap_ble_cmd(void); void ctap_ble_u2f_send(uint8_t cmd, uint8_t *data, uint16_t len); uint8_t *get_ble_fido_data_ptr(void); void set_ble_fido_data_len(uint16_t len); - -void ctap_error(uint8_t err);
71-75: Standardize error handling for BLE functions.BLE functions should follow consistent error handling patterns. Consider returning error codes instead of void.
-void ctap_ble_cmd(void); -void ctap_ble_u2f_send(uint8_t cmd, uint8_t *data, uint16_t len); +uint8_t ctap_ble_cmd(void); +uint8_t ctap_ble_u2f_send(uint8_t cmd, uint8_t *data, uint16_t len);legacy/firmware/fido2/resident_credential.c (2)
18-24:⚠️ Potential issueAdd null check for UI callback.
The UI callback is used without checking if it's null.
UI_WAIT_CALLBACK ui_callback = se_get_ui_callback(); uint8_t percent = 0; for (uint32_t i = 0; i < FIDO2_RESIDENT_CREDENTIALS_COUNT && count < max_count; i++) { percent = (i + 1) * 100 / FIDO2_RESIDENT_CREDENTIALS_COUNT; - ui_callback(_(C__PROCESSING_ETC), percent * 10); + if (ui_callback) { + ui_callback(_(C__PROCESSING_ETC), percent * 10); + }Likely invalid or redundant comment.
149-151: 🛠️ Refactor suggestionAdd user confirmation for credential deletion.
Credential deletion is irreversible. Add user confirmation.
bool resident_credential_delete(uint8_t index) { + if (!confirm(ButtonRequestType_ButtonRequest_Other, + "Delete Credential", + "Are you sure you want to delete this credential?")) { + return false; + } return se_delete_fido2_resident_credentials(index); }Likely invalid or redundant comment.
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 149-149: The function 'resident_credential_delete' is never used.
(unusedFunction)
legacy/firmware/i18n/locales/zh_tw.inc (1)
356-357: Fix Traditional Chinese character usageThe strings use Simplified Chinese characters "凭证" instead of the correct Traditional Chinese "憑證".
Apply this diff to fix the character usage:
- "列出凭证", - "移除凭证", + "列出憑證", + "移除憑證",legacy/firmware/i18n/locales/zh_cn.inc (1)
350-368: LGTM! Translations are accurate and consistentThe Simplified Chinese translations are properly formatted and use correct characters.
legacy/firmware/i18n/locales/ja.inc (1)
350-368: LGTM! Translations are accurate and consistentThe Japanese translations are properly formatted and use correct characters.
legacy/firmware/i18n/locales/en.inc (2)
303-303: LGTM! Clear improvement in message clarity.The new message "use a charger to allow fast charging" is more direct and clearer than the previous version.
350-368: 🧹 Nitpick (assertive)Enhance string clarity and specificity.
Some strings are too generic and should be more specific:
- "Message" → "FIDO2 Message"
- "Account" → "User Account"
- "App Name" → "Application Name"
- "Message", + "FIDO2 Message", - "Account", + "User Account", - "App Name", + "Application Name",Likely invalid or redundant comment.
legacy/firmware/i18n/locales/pt_br.inc (1)
350-368: LGTM! Accurate translations.The Brazilian Portuguese translations are accurate and maintain consistent capitalization with the English strings.
legacy/firmware/i18n/locales/es.inc (1)
350-368: LGTM! Accurate translations.The Spanish translations are accurate and maintain consistent capitalization with the English strings.
legacy/firmware/protect.c (1)
772-772: Timer mechanism changes look good but need verification.The changes improve the timer implementation by using elapsed time calculation. The addition of
loop_callback_handler()enhances responsiveness.Let's verify the timer behavior:
Also applies to: 800-800, 814-814
✅ Verification successful
Timer implementation changes are safe and well-integrated
The new timer_ms() based elapsed time calculation works alongside existing timeout mechanisms. The loop_callback_handler() addition improves system responsiveness without breaking existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other timer usages that may need similar updates rg "timer_out_get|timer_out_set|timer_out_oper" --type cLength of output: 3195
Script:
#!/bin/bash # Search for timer_ms function usage and implementation rg "timer_ms\(" --type c -A 2 # Look for timer.h to understand the timer interface fd timer.h --type f --exec cat {}Length of output: 9577
legacy/firmware/layout2.c (3)
171-194: Well-structured message formatter.The
short_line_messagefunction handles long text elegantly by:
- Checking if text fits OLED width
- Adding ellipsis for long text
- Preserving start and end portions
210-218: Clean pagination display implementation.The
layout_index_countfunction provides consistent pagination display across the UI.
Line range hint
2576-2643: Dialog layout flexibility improved.The new
centerparameter enhances text positioning control. The changes maintain backward compatibility.core/src/trezor/messages.py (2)
8644-8652: LGTM! Clean implementation of new fields for resident credential management.The new fields
indexandrequest_list_indexare properly typed and handled as optional parameters.
8689-8695: LGTM! Well-structured addition of the id_map field.The new
id_mapfield follows the established pattern for optional list parameters.legacy/vendor/tiny_cbor (1)
1-1: Add documentation for TinyCBOR integrationAdd a README file explaining the FIDO2 integration with TinyCBOR.
vendor/tiny_cbor (1)
1-1: Add TinyCBOR integration docs.Document why we chose TinyCBOR for FIDO2 CBOR encoding.
legacy/firmware/chinese.h (1)
7-8: Verify UTF-8 migration impactChanging from GBK to UTF-8 encoding affects character handling. Ensure all character processing code handles 3-byte UTF-8 sequences correctly.
✅ Verification successful
UTF-8 migration is properly handled
The codebase already supports UTF-8 encoding through proper Unicode conversion functions. The HZ_CODE_LEN change only affects legacy font fallback paths which are designed to handle both encodings correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GBK-specific character handling that might need updates rg -l "GBK|GB2312|0x[89A-F][0-9A-F]" legacy/firmware/Length of output: 1666
Script:
#!/bin/bash # Find direct references to HZ_CODE_LEN rg "HZ_CODE_LEN" legacy/firmware/ -B 2 -A 2 # Examine chinese.c content cat legacy/firmware/chinese.c # Look for potential buffer calculations rg "malloc|alloc|buf|buffer|memcpy|strlen" legacy/firmware/chinese.cLength of output: 12463
legacy/firmware/fido2/u2f_knownapps.h.mako (1)
8-8: Verify include path resolutionThe include path change from
"u2f/u2f.h"to"u2f.h"needs verification. Check if the compiler can locate the header file.✅ Verification successful
Include path change is correct
The header file exists in the same directory as the including file. The new path
"u2f.h"is the proper way to reference it.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if u2f.h exists in include paths fd -t f "u2f.h" legacy/firmware/Length of output: 92
Script:
#!/bin/bash # Search for all u2f.h files in the repository fd -t f "u2f.h"Length of output: 153
.gitmodules (1)
33-35: Update TinyCBOR to latest stable release v0.6.0The current commit is outdated. Use v0.6.0 for better stability and bug fixes.
common/protob/messages-webauthn.proto (1)
63-63: Consider documenting the id_map field.Add a comment explaining the purpose and format of the id_map field.
legacy/firmware/protob/Makefile (1)
Line range hint
5-25: LGTM!The changes correctly integrate WebAuthn message handling into the build system.
legacy/firmware/Makefile (1)
226-230: Verify TinyCBOR version for security updates.Check if the included TinyCBOR version has any known security vulnerabilities.
legacy/firmware/fsm.h (1)
342-349: LGTM!The WebAuthn message handler declarations follow the project's naming conventions and are well-organized.
legacy/firmware/fsm.c (1)
712-712: Clean header inclusion for WebAuthn support.The header is correctly placed within the non-Bitcoin section.
legacy/firmware/menu_list.c (5)
348-362: Well-structured data initialization with security consideration.Good use of secure memory section for sensitive FIDO2 credential data.
390-391: Previous NULL pointer issue remains unfixed.The user_name field could be NULL.
- layout_fido2_resident_credential(0, 0, user_info[index].rp_id, - user_info[index].user_name); + const char *name = user_info[index].user_name[0] ? user_info[index].user_name : "Unknown"; + layout_fido2_resident_credential(0, 0, user_info[index].rp_id, name);
416-420: Fix integer overflow in comparison function.Direct subtraction can cause overflow.
- return a->creation_time - b->creation_time; + if (a->creation_time < b->creation_time) return -1; + if (a->creation_time > b->creation_time) return 1; + return 0;
442-447: Handle NULL account name safely.Protect against NULL pointer dereference.
char *account_name = get_account_name(&cred_desc.credential.user); + if (account_name == NULL) { + account_name = ""; + } strlcpy(user_info[i].user_name, account_name, sizeof(user_info[i].user_name));
472-473: Improve menu item labeling for clarity.Use more descriptive term for FIDO2 credentials.
- {"FIDO Keys", NULL, true, menu_fido2_resident_credential, NULL, false, + {"FIDO2 Credentials", NULL, true, menu_fido2_resident_credential, NULL, false,python/src/trezorlib/messages.py (2)
10709-10719: 🧹 Nitpick (assertive)Remove quotes from type annotations.
Clean up type hints by removing unnecessary quotes.
- self.id_map: Sequence["bytes"] = id_map if id_map is not None else [] + self.id_map: Sequence[bytes] = id_map if id_map is not None else []Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
10718-10718: Remove quotes from type annotation
Remove quotes
(UP037)
10719-10719: Remove quotes from type annotation
Remove quotes
(UP037)
10662-10674: 🧹 Nitpick (assertive)Add typing.ClassVar annotation to FIELDS.
Add type annotation to improve type safety.
- FIELDS = { + FIELDS: typing.ClassVar[dict] = { }Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
10662-10665: Mutable class attributes should be annotated with
typing.ClassVar(RUF012)
legacy/util.h (1)
68-68: Add function documentation.Document the version comparison function:
- Input format requirements
- Return value meaning
- Example usage
+/** + * Compare two version strings (e.g., "1.2.3" vs "1.2.4") + * @param version1 First version string in format "X.Y.Z" + * @param version2 Second version string in format "X.Y.Z" + * @return -1 if version1 < version2, 0 if equal, 1 if version1 > version2 + */ int compare_str_version(const char *version1, const char *version2);legacy/util.c (2)
148-183:⚠️ Potential issueFix potential security and reliability issues.
The version comparison implementation needs several improvements:
- Add NULL pointer checks
- Validate version string format
- Prevent integer overflow
- Add error handling
Apply these improvements:
int compare_str_version(const char *version1, const char *version2) { + if (!version1 || !version2) { + return 0; // Or consider returning an error code + } + int vnum1 = 0, vnum2 = 0; while (*version1 != '\0' || *version2 != '\0') { + // Reset for each component + vnum1 = vnum2 = 0; + while (*version1 != '\0' && *version1 != '.') { + if (*version1 < '0' || *version1 > '9') { + return 0; // Invalid format + } + // Check for overflow + if (vnum1 > INT_MAX/10) { + return 1; // version1 is greater + } vnum1 = vnum1 * 10 + (*version1 - '0'); version1++; } while (*version2 != '\0' && *version2 != '.') { + if (*version2 < '0' || *version2 > '9') { + return 0; // Invalid format + } + // Check for overflow + if (vnum2 > INT_MAX/10) { + return -1; // version2 is greater + } vnum2 = vnum2 * 10 + (*version2 - '0'); version2++; }Likely invalid or redundant comment.
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 148-148: The function 'compare_str_version' is never used.
(unusedFunction)
148-183: Verify function usage in the codebase.Static analysis indicates this function is unused. Let's verify its usage in the FIDO2 implementation.
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 148-148: The function 'compare_str_version' is never used.
(unusedFunction)
| @@ -35,6 +35,7 @@ void i2c_slave_init(void); | |||
| void i2c_set_wait(bool flag); | |||
| bool i2c_slave_send(uint32_t data_len); | |||
| void i2c_slave_send_ex(uint32_t data_len); | |||
| bool i2c_slave_send_fido(uint8_t *data, uint32_t data_len); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document the FIDO2 function purpose and parameters.
Add a function comment explaining:
- Purpose of the FIDO2-specific I2C send function
- Parameter requirements
- Return value meaning
+/**
+ * Sends FIDO2 data over I2C with "fid" prefix
+ * @param data: Pointer to data buffer to send
+ * @param data_len: Length of data to send
+ * @return true if send successful, false on timeout
+ */
bool i2c_slave_send_fido(uint8_t *data, uint32_t data_len);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool i2c_slave_send_fido(uint8_t *data, uint32_t data_len); | |
| /** | |
| * Sends FIDO2 data over I2C with "fid" prefix | |
| * @param data: Pointer to data buffer to send | |
| * @param data_len: Length of data to send | |
| * @return true if send successful, false on timeout | |
| */ | |
| bool i2c_slave_send_fido(uint8_t *data, uint32_t data_len); |
| bool i2c_slave_send_fido(uint8_t *data, uint32_t data_len) { | ||
| i2c_data_out_pos = 0; | ||
| i2c_data_outlen = data_len + 3; | ||
| memcpy(i2c_data_out, "fid", 3); | ||
| memcpy(i2c_data_out + 3, data, data_len); | ||
| SET_COMBUS_HIGH(); | ||
| uint32_t start_time = timer_ms(); | ||
| while (i2c_data_outlen > 0) { | ||
| if (timer_ms() - start_time > 1000) { | ||
| SET_COMBUS_LOW(); | ||
| return false; | ||
| } | ||
| } | ||
| SET_COMBUS_LOW(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding retry mechanism for reliability.
I2C communication can be unreliable. Add a retry mechanism to handle transient failures.
bool i2c_slave_send_fido(uint8_t *data, uint32_t data_len) {
+ const int max_retries = 3;
+ for (int retry = 0; retry < max_retries; retry++) {
// ... existing code ...
+ if (success) return true;
+ delay_ms(50 * (retry + 1)); // Exponential backoff
+ }
return false;
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 301-301: The function 'i2c_slave_send_fido' is never used.
(unusedFunction)
💡 Codebase verification
Warning: Non-standard FIDO2 message format detected
The "fid" string prefix doesn't comply with CTAP protocol specifications. FIDO2/CTAP uses binary command codes (0x01-0xBF), not text prefixes. This custom format might cause compatibility issues.
- Consider replacing string-based prefix with standard CTAP command codes defined in
legacy/firmware/fido2/ctap.h - Review FIDO2 specification for proper message framing
🔗 Analysis chain
Verify FIDO2 message format compliance.
Ensure the "fid" prefix matches FIDO2 protocol requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other FIDO2 protocol implementations to verify message format
rg -l "fid.{0,10}prefix" --type cpp --type c
Length of output: 46
Script:
#!/bin/bash
# Search for FIDO-related definitions and protocol handling
rg -l "FIDO2?[_ ]" --type c --type cpp
rg -l "ctap2?" --type c --type cpp
# Look for similar message handling functions
ast-grep --pattern 'memcpy($_data, "fid", $_)'
Length of output: 811
Script:
#!/bin/bash
# Check CTAP transport and parsing for message format
rg -A 5 "protocol.*prefix|message.*format|frame.*header" legacy/firmware/fido2/
rg -A 5 "^#define.*CTAP" legacy/firmware/fido2/ctap.h
Length of output: 2071
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 301-301: The function 'i2c_slave_send_fido' is never used.
(unusedFunction)
| #define MAX_RESIDENT_CREDENTIALS 60 | ||
| #define RP_ID_HASH_LENGTH 32 | ||
|
|
||
| #define FIDO2_RESIDENT_CREDENTIALS_SIZE (512) | ||
| #define FIDO2_RESIDENT_CREDENTIALS_COUNT 60 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consolidate macros for resident credentials count
You define both MAX_RESIDENT_CREDENTIALS and FIDO2_RESIDENT_CREDENTIALS_COUNT as 60. Use a single macro to avoid duplication and potential inconsistencies.
| int resident_credential_info(uint8_t indexs[FIDO2_RESIDENT_CREDENTIALS_COUNT], | ||
| int progress_ratio); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Correct parameter name indexs to indices
Change indexs to indices for proper grammar and clarity in resident_credential_info.
| #define SE_FIDO2_SLOT_DATA_OK 0 | ||
| #define SE_FIDO2_SLOT_DATA_NULL 1 | ||
| #define SE_FIDO2_SLOT_DATA_BUFFER_TOO_SMALL 2 | ||
| #define SE_FIDO2_SLOT_DATA_INVALID 3 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use an enum for FIDO2 slot data status codes
Instead of #define macros, consider using an enum for SE_FIDO2_SLOT_DATA_* constants. This improves type safety and code readability.
| int oledCharWidthEx(const char text, uint8_t font) { | ||
| char text_array[2] = {0}; | ||
| text_array[0] = text; | ||
| return oledStringWidthEx(text_array, font); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused function oledCharWidthEx
The function is not used anywhere in the codebase. Remove both the implementation in chinese.c and the declaration in chinese.h.
🔗 Analysis chain
Verify if oledCharWidthEx is used.
Static analysis indicates this function is unused. Either:
- Remove the function if it's truly unused
- Add a comment explaining its intended future use
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if oledCharWidthEx is used in the codebase
# Test: Search for function usage
rg "oledCharWidthEx" --type cpp
Length of output: 109
Script:
#!/bin/bash
# Check for potential indirect usage through function pointers or macros
rg -i "oledchar.*width" --type cpp -A 2 -B 2
# Check if it's documented in README or other docs
fd README -e md -X cat {} 2>/dev/null | rg -i "oledchar.*width"
Length of output: 432
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 121-121: The function 'oledCharWidthEx' is never used.
(unusedFunction)
| @@ -175,11 +175,11 @@ static struct menu ble_set_menu = { | |||
| .current = 0, | |||
| .counts = COUNT_OF(ble_set_menu_items), | |||
| .title = "Bluetooth", | |||
| .items = ble_set_menu_items, | |||
| .items = (struct menu_item *)ble_set_menu_items, | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve const correctness in menu item assignments.
The menu structures should use const pointers to const items.
- .items = (struct menu_item *)menu_items,
+ .items = (const struct menu_item *const)menu_items,Also applies to: 195-195, 219-219, 235-235, 248-248, 261-261, 274-274, 296-296, 482-482, 570-570, 598-598, 616-616, 631-631, 650-650
| percent = 30 + ((i + 1) * 100 / count) * 70 / 100; | ||
| layoutProgressAdapter(_(C__PROCESSING_ETC), percent * 10); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify progress calculation.
The progress calculation is unnecessarily complex.
- percent = 30 + ((i + 1) * 100 / count) * 70 / 100;
+ percent = 30 + ((i + 1) * 70 / count);
layoutProgressAdapter(_(C__PROCESSING_ETC), percent * 10);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| percent = 30 + ((i + 1) * 100 / count) * 70 / 100; | |
| layoutProgressAdapter(_(C__PROCESSING_ETC), percent * 10); | |
| percent = 30 + ((i + 1) * 70 / count); | |
| layoutProgressAdapter(_(C__PROCESSING_ETC), percent * 10); |
| bool menu_fido2_remove_credential(const char *title, int index) { | ||
| uint8_t key = KEY_NULL; | ||
| layoutMenuItemsEx(NULL, &bmp_bottom_right_arrow, 0, 0, title, NULL, | ||
| _(ACTION__REMOVE), NULL, NULL, NULL, NULL, NULL, NULL, NULL, | ||
| NULL); | ||
| key = protectWaitKey(0, 1); | ||
| if (key != KEY_CONFIRM) { | ||
| return false; | ||
| } | ||
| layoutDialogCenterAdapterV2(NULL, &bmp_icon_warning, &bmp_bottom_left_arrow, | ||
| &bmp_bottom_right_arrow, NULL, NULL, NULL, | ||
| _(FIDO_REMOVE_KEY_DESC), NULL, NULL, NULL); | ||
| key = protectWaitKey(0, 1); | ||
| if (key != KEY_CONFIRM) { | ||
| return false; | ||
| } | ||
| resident_credential_delete(index); | ||
| layoutDialogCenterAdapterV2(NULL, NULL, NULL, &bmp_bottom_right_confirm, NULL, | ||
| NULL, NULL, NULL, | ||
| _(FIDO_REMOVE_KEY_SUCCESS_TITLE), NULL, NULL); | ||
| protectWaitKey(timer1s * 2, 0); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Extract magic number into a named constant.
Define a constant for the 2-second timer value to improve code maintainability.
+#define FIDO2_SUCCESS_MESSAGE_DISPLAY_TIME (timer1s * 2)
protectWaitKey(timer1s * 2, 0);
+protectWaitKey(FIDO2_SUCCESS_MESSAGE_DISPLAY_TIME, 0);Committable suggestion skipped: line range outside the PR's diff.
| void menu_fido2_resident_credential_display(int index) { | ||
| uint8_t key = KEY_NULL; | ||
| layout_fido2_resident_credential(0, 0, user_info[index].rp_id, | ||
| user_info[index].user_name); | ||
| key = protectWaitKey(0, 1); | ||
| if (key == KEY_CONFIRM) { | ||
| if (menu_fido2_remove_credential(user_info[index].rp_id, | ||
| user_info[index].index)) { | ||
| if (fido_resident_credential_menu.counts > 1) { | ||
| if (index < fido_resident_credential_menu.counts - 1) { | ||
| memset(&user_info[index], 0, sizeof(CTAP_UserInfo)); | ||
| memmove(&user_info[index], &user_info[index + 1], | ||
| (fido_resident_credential_menu.counts - index - 1) * | ||
| sizeof(CTAP_UserInfo)); | ||
| } | ||
| fido_resident_credential_menu.counts--; | ||
| if (fido_resident_credential_menu.current) { | ||
| fido_resident_credential_menu.current--; | ||
| } | ||
| menu_refresh(); | ||
| } else { | ||
| fido_resident_credential_menu_items[0].go_prev = true; | ||
| } | ||
| } | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add bounds checking for array operations.
The array manipulation during credential removal needs bounds checking.
+ if (index >= FIDO2_RESIDENT_CREDENTIALS_COUNT) {
+ return;
+ }
if (menu_fido2_remove_credential(user_info[index].rp_id,
user_info[index].index)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void menu_fido2_resident_credential_display(int index) { | |
| uint8_t key = KEY_NULL; | |
| layout_fido2_resident_credential(0, 0, user_info[index].rp_id, | |
| user_info[index].user_name); | |
| key = protectWaitKey(0, 1); | |
| if (key == KEY_CONFIRM) { | |
| if (menu_fido2_remove_credential(user_info[index].rp_id, | |
| user_info[index].index)) { | |
| if (fido_resident_credential_menu.counts > 1) { | |
| if (index < fido_resident_credential_menu.counts - 1) { | |
| memset(&user_info[index], 0, sizeof(CTAP_UserInfo)); | |
| memmove(&user_info[index], &user_info[index + 1], | |
| (fido_resident_credential_menu.counts - index - 1) * | |
| sizeof(CTAP_UserInfo)); | |
| } | |
| fido_resident_credential_menu.counts--; | |
| if (fido_resident_credential_menu.current) { | |
| fido_resident_credential_menu.current--; | |
| } | |
| menu_refresh(); | |
| } else { | |
| fido_resident_credential_menu_items[0].go_prev = true; | |
| } | |
| } | |
| return; | |
| } | |
| } | |
| void menu_fido2_resident_credential_display(int index) { | |
| uint8_t key = KEY_NULL; | |
| layout_fido2_resident_credential(0, 0, user_info[index].rp_id, | |
| user_info[index].user_name); | |
| key = protectWaitKey(0, 1); | |
| if (key == KEY_CONFIRM) { | |
| if (index >= FIDO2_RESIDENT_CREDENTIALS_COUNT) { | |
| return; | |
| } | |
| if (menu_fido2_remove_credential(user_info[index].rp_id, | |
| user_info[index].index)) { | |
| if (fido_resident_credential_menu.counts > 1) { | |
| if (index < fido_resident_credential_menu.counts - 1) { | |
| memset(&user_info[index], 0, sizeof(CTAP_UserInfo)); | |
| memmove(&user_info[index], &user_info[index + 1], | |
| (fido_resident_credential_menu.counts - index - 1) * | |
| sizeof(CTAP_UserInfo)); | |
| } | |
| fido_resident_credential_menu.counts--; | |
| if (fido_resident_credential_menu.current) { | |
| fido_resident_credential_menu.current--; | |
| } | |
| menu_refresh(); | |
| } else { | |
| fido_resident_credential_menu_items[0].go_prev = true; | |
| } | |
| } | |
| return; | |
| } | |
| } |
Summary by CodeRabbit
Release Notes v3.11.0
New Features
Improvements
Bug Fixes
Chores