Support FIDO2 credential storage && write private key in factory mode. #212
Conversation
Walkthrough此次更改引入了多个与安全元素（SE）私钥管理相关的新功能和消息定义。新增的 Changes
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.45.0)common/protob/messages-management.protoConfig file JSON parsing error: invalid character 'l' looking for beginning of value. Please check your buf configuration file for JSON syntax errors. common/protob/messages.protoConfig file JSON parsing error: invalid character 'l' looking for beginning of value. Please check your buf configuration file for JSON syntax errors. core/embed/bootloader/protob/messages.protoConfig file JSON parsing error: invalid character 'l' looking for beginning of value. Please check your buf configuration file for JSON syntax errors. 🪧 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: 31
🧹 Outside diff range comments (4)
core/src/storage/resident_credentials.py (1)
Line range hint
1-41: 架构设计建议当前实现通过条件分支来支持不同的硬件能力，这种方式虽然可行，但建议考虑以下改进：
- 考虑引入抽象接口来处理不同的存储实现
- 将硬件特定的实现逻辑移至专门的适配器类中
- 添加配置验证和日志记录机制
这些改进将使代码更容易维护和扩展。
🧰 Tools
🪛 Ruff (0.7.0)
1-1:
micropython.constimported but unusedRemove unused import:
micropython.const(F401)
core/embed/extmod/modtrezorcrypto/modtrezorcrypto-nist256p1.h (1)
需要增加工厂模式的访问控制
代码分析显示：
- 工厂模式的控制仅通过简单的布尔值开关实现
inner_secret_key参数直接暴露在公共API中- 缺少运行时环境检测和审计日志
建议：
- 在
device.h中增加工厂模式的访问控制机制- 将
inner_secret_key功能移至独立的受保护接口- 添加工厂模式操作的日志记录
🔗 Analysis chain
Line range hint
206-260: 安全建议此次更改引入了直接处理私钥的新功能，需要注意：
- 确保
inner_secret_key=True的调用仅限于工厂模式- 添加运行时环境检测
- 增加日志记录以便审计
- 考虑添加额外的访问控制机制
建议在更高层级添加以下保护措施：
- 工厂模式的独立配置管理
- 关键操作的审计日志
- 运行时环境的完整性校验
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 检查是否有其他地方使用了 inner_secret_key 参数 rg -l "inner_secret_key" # 检查工厂模式的相关实现 rg -l "factory.?mode"Length of output: 280
Script:
#!/bin/bash # Check the implementation of factory mode in device.h cat core/embed/trezorhal/device.h # Check how inner_secret_key is used in the mock cat core/mocks/generated/trezorcrypto/nist256p1.pyi # Check factory mode usage in bootloader rg -A 5 "factory.?mode" core/embed/bootloader/Length of output: 3705
core/embed/bootloader/main.c (2)
Line range hint
1-513: 建议优化工厂模式的错误处理bootloader_usb_loop_factory 函数中的消息处理缺少统一的错误处理机制。建议添加:
- 错误日志记录
- 失败重试机制
- 异常状态恢复
Line range hint
517-700: 建议增强消息处理的安全性消息处理流程中建议增加:
- 消息完整性校验
- 会话超时机制
- 异常消息过滤
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
core/mocks/generated/trezorcrypto/nist256p1.pyiis excluded by!**/generated/**core/mocks/generated/trezorcrypto/se_thd89.pyiis excluded by!**/generated/**
📒 Files selected for processing (25)
common/protob/messages-management.proto(1 hunks)common/protob/messages.proto(1 hunks)core/embed/bootloader/main.c(1 hunks)core/embed/bootloader/messages.c(3 hunks)core/embed/bootloader/messages.h(1 hunks)core/embed/bootloader/protob/messages.options(1 hunks)core/embed/bootloader/protob/messages.pb.c(1 hunks)core/embed/bootloader/protob/messages.pb.h(9 hunks)core/embed/bootloader/protob/messages.proto(2 hunks)core/embed/extmod/modtrezorconfig/modtrezorconfig.c(3 hunks)core/embed/extmod/modtrezorcrypto/modtrezorcrypto-nist256p1.h(3 hunks)core/embed/extmod/modtrezorcrypto/modtrezorcrypto-se-thd89.h(2 hunks)core/embed/trezorhal/se_thd89.c(2 hunks)core/embed/trezorhal/se_thd89.h(3 hunks)core/src/apps/webauthn/add_resident_credential.py(1 hunks)core/src/apps/webauthn/fido2.py(4 hunks)core/src/apps/webauthn/list_resident_credentials.py(1 hunks)core/src/apps/webauthn/remove_resident_credential.py(1 hunks)core/src/apps/workflow_handlers.py(1 hunks)core/src/storage/resident_credentials.py(2 hunks)core/src/trezor/enums/MessageType.py(1 hunks)core/src/trezor/enums/__init__.py(1 hunks)core/src/trezor/messages.py(1 hunks)core/src/trezor/ui/layouts/lvgl/webauthn.py(1 hunks)python/src/trezorlib/messages.py(2 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
core/embed/bootloader/messages.c
[style] 675-675: The function 'process_msg_WriteSEPrivateKey' is never used.
(unusedFunction)
core/embed/trezorhal/se_thd89.c
[style] 1037-1037: The function 'se_sign_message_with_write_key' is never used.
(unusedFunction)
[style] 1053-1053: The function 'se_set_private_key_extern' is never used.
(unusedFunction)
[style] 2352-2352: The function 'se_get_fido2_resident_credentials' is never used.
(unusedFunction)
[style] 2376-2376: The function 'se_set_fido2_resident_credentials' is never used.
(unusedFunction)
[style] 2388-2388: The function 'se_delete_fido2_resident_credentials' is never used.
(unusedFunction)
[style] 2394-2394: The function 'se_delete_all_fido2_credentials' is never used.
(unusedFunction)
🪛 Ruff (0.7.0)
core/src/apps/webauthn/add_resident_credential.py
45-45: Unnecessary else after break statement
Remove unnecessary else
(RET508)
48-48: Do not catch blind exception: Exception
(BLE001)
49-49: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
core/src/apps/webauthn/fido2.py
1815-1815: Boolean positional value in function call
(FBT003)
core/src/apps/webauthn/list_resident_credentials.py
24-24: Unnecessary else after break statement
Remove unnecessary else
(RET508)
27-27: Do not catch blind exception: Exception
(BLE001)
core/src/apps/webauthn/remove_resident_credential.py
45-45: Unnecessary else after break statement
Remove unnecessary else
(RET508)
48-48: Do not catch blind exception: Exception
(BLE001)
49-49: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
core/src/trezor/messages.py
3513-3513: Missing type annotation for self in method
(ANN101)
3520-3520: Missing type annotation for cls in classmethod
(ANN102)
3520-3520: Dynamically typed expressions (typing.Any) are disallowed in msg
(ANN401)
python/src/trezorlib/messages.py
5008-5010: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
5013-5013: Missing type annotation for self in method
(ANN101)
🔇 Additional comments (31)
core/src/storage/resident_credentials.py (1)
4-12: 验证起始索引值的安全性影响
在 THD89 模式下使用索引 0，而在非 THD89 模式下使用索引 1，这种差异需要确认是否会带来安全隐患。
✅ Verification successful
索引值差异不会带来安全隐患
代码分析显示：
- _RESIDENT_CREDENTIAL_START_KEY 仅在 resident_credentials.py 文件内部使用
- 所有凭证操作都正确使用了 _RESIDENT_CREDENTIAL_START_KEY 作为偏移量
- 没有发现直接使用固定索引值的情况
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 搜索代码库中是否有其他地方依赖了特定的起始索引值
# 查找所有引用了 _RESIDENT_CREDENTIAL_START_KEY 的代码
rg "_RESIDENT_CREDENTIAL_START_KEY"
# 查找可能直接使用索引 0 或 1 的凭证操作
ast-grep --pattern 'common.get(common.APP_WEBAUTHN, $_)'
ast-grep --pattern 'common.set(common.APP_WEBAUTHN, $_)'
ast-grep --pattern 'common.delete(common.APP_WEBAUTHN, $_)'
Length of output: 1243
core/src/trezor/ui/layouts/lvgl/webauthn.py (1)
48-48: 类型转换确保了返回值的安全性
显式的布尔类型转换增强了代码的类型安全性，与函数签名声明的返回类型保持一致。这是一个很好的防御性编程实践。
运行以下脚本验证 FIDO2 凭证重置功能的使用情况：
✅ Verification successful
布尔类型转换是安全且合理的修改
代码验证显示，confirm_webauthn_reset 函数在 FIDO2 凭证重置流程中被正确使用：
- 在
core/src/apps/webauthn/fido2.py中直接使用返回值作为条件判断 - 所有实现（tt、tt_v2、lvgl）都声明了
-> bool返回类型
显式的布尔转换确保了类型安全，不会影响现有的 FIDO2 凭证管理功能。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：检查 confirm_webauthn_reset 函数的调用点，确保正确处理布尔返回值
# 搜索函数调用
rg -A 3 "confirm_webauthn_reset\("
# 搜索相关的 FIDO2 重置处理逻辑
ast-grep --pattern 'if $_ = await confirm_webauthn_reset(): $$$'
Length of output: 1216
core/src/apps/webauthn/list_resident_credentials.py (1)
16-17: 导入语句正确且必要！
新增的导入语句清晰地表明了对安全元素和延时功能的依赖。
core/embed/bootloader/protob/messages.pb.c (2)
102-103: 请注意私钥操作的安全性
新增的 WriteSEPrivateKey 绑定涉及敏感的私钥操作。建议：
- 确保仅在工厂模式下可用
- 验证访问控制机制
- 确保密钥传输过程的安全性
✅ Verification successful
私钥写入操作已正确限制在工厂模式
代码分析显示:
- 设备在未设置序列号或无证书时会进入工厂模式 (
device_set_factory_mode(true)) WriteSEPrivateKey消息仅在工厂模式下可用- 实现了32字节私钥大小的验证
- 通过
se_set_private_key_extern安全地写入私钥
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查 WriteSEPrivateKey 消息的使用限制
# 预期结果：仅在工厂模式相关代码中使用
# 搜索 WriteSEPrivateKey 的使用场景
rg "WriteSEPrivateKey" -A 5 -B 5
# 检查相关的安全检查代码
ast-grep --pattern 'if ($CHECK) {
$$$
WriteSEPrivateKey
$$$
}'
Length of output: 20200
Script:
#!/bin/bash
# 检查工厂模式相关的代码和安全限制
# 1. 搜索工厂模式相关的代码
rg "factory.*mode" -A 5 -B 5
# 2. 检查 WriteSEPrivateKey 的具体实现
rg "process_msg_WriteSEPrivateKey" -A 15 -B 5
# 3. 检查 bootloader 中的安全检查
rg "check.*factory" -A 5 -B 5
Length of output: 8283
102-104: 验证消息流程的完整性
请确认以下几点：
WriteSEPrivateKey与ReadSEPublicKey的交互逻辑- 错误处理机制
- 与现有 SE 操作的兼容性
✅ Verification successful
消息流程已正确集成
代码分析显示：
WriteSEPrivateKey已正确实现错误处理机制，包括密钥大小验证- 与
ReadSEPublicKey等其他SE操作完全兼容 - 错误处理遵循统一的
FailureType_Failure_ProcessError模式
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查相关的错误处理代码
ast-grep --pattern 'if ($ERROR) {
$$$
Failure
$$$
}'
# 检查消息处理函数
rg "process.*WriteSEPrivateKey" -A 10
Length of output: 2642
Script:
#!/bin/bash
# 检查SE相关的错误处理和消息流程
rg "FailureType.*SE" -A 2
# 检查SE操作的实现
ast-grep --pattern 'if ($SE_ERROR) {
$$$
send_failure
$$$
}'
# 查找SE相关的消息处理函数定义
rg "MSG_RECV.*SE" -A 5
Length of output: 4038
core/src/apps/webauthn/remove_resident_credential.py (1)
40-50: 🧹 Nitpick (assertive)
建议加强安全监控机制
在处理FIDO2凭证删除这样的敏感操作时，建议：
- 添加失败尝试的日志记录
- 考虑实现速率限制机制
- 添加操作审计跟踪
🧰 Tools
🪛 Ruff (0.7.0)
45-45: Unnecessary else after break statement
Remove unnecessary else
(RET508)
48-48: Do not catch blind exception: Exception
(BLE001)
49-49: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
core/embed/bootloader/messages.h (2)
71-72: 函数声明格式正确
函数声明的格式和参数对齐与文件中其他函数保持一致。
71-72: 🧹 Nitpick (assertive)
需要验证私钥写入的安全控制措施
这是一个高度敏感的安全操作，建议：
- 确保该函数只能在工厂模式下执行
- 添加详细的安全文档说明使用场景和限制条件
- 验证是否有访问控制机制防止未授权调用
建议在实现中添加以下安全控制:
- 工厂模式状态检查
- 访问令牌验证
- 操作日志记录
core/embed/bootloader/protob/messages.options (2)
Line range hint 116-117: 检查消息签名参数设置
消息和签名大小设置合理：
- 消息大小1024字节足够日常使用
- 签名大小64字节符合常见签名算法要求
Line range hint 112-114: 验证证书大小限制是否充分
public_cert 的416字节限制可能不足以容纳某些X.509证书。建议：
- 验证目标使用场景的证书大小
- 考虑增加缓冲区大小以适应将来的需求
✅ Verification successful
证书大小限制416字节是合理的
从代码实现来看：
- SE设备使用THD89协议处理证书，命令缓冲区限制在1024字节
- 实际证书读写操作使用简单的APDU命令，不需要过大的缓冲区
- 其他安全芯片实现(如lite_card)的证书大小限制为256字节
416字节的限制完全满足当前SE设备的证书需求。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 搜索证书相关代码，验证大小限制的合理性
rg "public_cert"
ast-grep --pattern 'struct $_ { $$$ public_cert $$$ }'
Length of output: 2552
Script:
#!/bin/bash
# 搜索证书相关的实现代码，了解证书的具体使用场景
rg -A 5 "se_write_certificate|se_read_cert"
# 搜索证书大小相关的定义或常量
rg "CERT.*SIZE|cert.*size" -i
Length of output: 10334
core/embed/trezorhal/se_thd89.h (1)
112-113: 私钥签名函数安全性评审
新增的写入密钥签名函数涉及敏感操作，建议：
- 添加函数说明文档，明确使用场景
- 确认是否需要权限检查机制
- 考虑添加日志记录关键操作
core/embed/extmod/modtrezorcrypto/modtrezorcrypto-nist256p1.h (1)
258-260: 函数定义更新正确
函数定义的修改符合预期，支持了新增的可选参数。
core/src/trezor/enums/MessageType.py (1)
408-408: 需要验证私钥写入的安全控制
新增的 WriteSEPrivateKey 消息类型涉及敏感的私钥操作。建议：
- 确认此消息仅在工厂模式/引导加载程序模式下可用
- 验证访问控制机制是否完善
✅ Verification successful
私钥写入操作已正确限制在引导加载程序中
代码分析显示:
WriteSEPrivateKey消息仅在引导加载程序中处理，通过wire_bootloader = true标志限制- 实现包含了适当的安全检查:
- 私钥大小限制为32字节
- 处理失败时会返回错误消息
- 该功能与其他安全元件相关操作(如
ReadSEPublicKey,WriteSEPublicCert)一起正确分组
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：验证 WriteSEPrivateKey 消息的使用限制
# 检查引导加载程序中的消息处理
rg -A 5 "WriteSEPrivateKey" "core/embed/bootloader"
# 检查是否存在其他地方使用此消息
rg -g '!core/embed/bootloader/*' "WriteSEPrivateKey"
Length of output: 9572
core/embed/bootloader/protob/messages.proto (1)
529-535: 🧹 Nitpick (assertive)
建议补充安全相关文档说明
处理私钥数据需要特别注意安全性：
- 请补充注释说明以下内容：
- 私钥的预期格式和大小
- 使用场景和访问限制
- 安全存储机制
- 建议添加私钥数据的大小限制
运行以下脚本检查相关实现：
core/src/trezor/enums/__init__.py (1)
426-426: 新增消息类型需要确保安全性
新增的 WriteSEPrivateKey 消息类型遵循了现有的命名规范。由于涉及写入私钥这样的敏感操作，请确保：
- 此消息类型仅在工厂模式下可用
- 实现相应的访问控制机制
✅ Verification successful
已确认 WriteSEPrivateKey 消息类型的安全性
代码分析显示该消息类型已正确实现工厂模式限制：
- 消息处理仅在 bootloader 中实现
- 通过
device_is_factory_mode()函数进行工厂模式检查 - 写入私钥操作有长度限制（32字节）和错误处理
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查是否有相关的访问控制实现
rg -A 5 "WriteSEPrivateKey.*factory.*mode"
Length of output: 44
Script:
#!/bin/bash
# 搜索 WriteSEPrivateKey 的使用场景
rg -A 10 "WriteSEPrivateKey"
# 搜索工厂模式相关的代码
rg -A 10 "factory.*mode"
# 搜索 SE 相关的访问控制实现
ast-grep --pattern 'if $_ {
$$$
WriteSEPrivateKey
$$$
}'
Length of output: 22201
core/embed/bootloader/main.c (1)
514-516:
需要验证私钥写入的安全性
新增的 WriteSEPrivateKey 消息处理涉及敏感的私钥操作，建议：
- 确保仅在工厂模式下可用
- 添加私钥数据验证
- 实现写入操作的原子性
- 添加访问控制机制
✅ Verification successful
私钥写入功能已实现足够的安全控制
代码分析显示:
- 私钥写入功能仅在工厂模式下可用，通过
device_is_factory_mode()控制 - 已实现私钥大小验证(32字节)
- 写入失败时有适当的错误处理
- 消息处理流程符合标准协议规范
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 验证 WriteSEPrivateKey 消息处理的访问控制
# 检查是否仅在工厂模式下可用
# 搜索 process_msg_WriteSEPrivateKey 的实现
ast-grep --pattern 'process_msg_WriteSEPrivateKey($_) {
$$$
}'
# 搜索相关的访问控制检查
rg -A 5 "device_is_factory_mode"
Length of output: 1392
Script:
#!/bin/bash
# 搜索 WriteSEPrivateKey 消息处理的具体实现
rg -A 10 "process_msg_WriteSEPrivateKey"
# 检查工厂模式相关的代码
ast-grep --pattern 'bootloader_usb_loop_factory($$$) {
$$$
}'
# 检查私钥写入的验证逻辑
rg -A 5 "WriteSEPrivateKey"
Length of output: 13777
core/embed/bootloader/messages.c (1)
751-756: 🛠️ Refactor suggestion
签名功能的回退逻辑需要优化
当前实现在写入密钥签名失败后直接尝试普通签名，建议添加错误处理和日志记录。
if (se_sign_message_with_write_key((uint8_t *)msg_recv.message.bytes,
msg_recv.message.size, sign)) {
+ log_message_sign_with_write_key();
MSG_SEND_ASSIGN_REQUIRED_BYTES(signature, sign, 64);
MSG_SEND(SEMessageSignature);
return;
+ } else {
+ log_write_key_sign_failure();
}Likely invalid or redundant comment.
common/protob/messages.proto (1)
586-586:
请注意私钥写入操作的安全性
新增的 MessageType_WriteSEPrivateKey 消息类型涉及敏感的私钥操作。建议：
- 确保此操作仅在工厂模式下可用
- 添加适当的访问控制机制
- 实现私钥数据的安全传输
- 考虑添加验证步骤以确保私钥的完整性
✅ Verification successful
安全检查已得到适当实现
代码分析显示该功能已实现了必要的安全措施：
- 该操作仅在工厂模式下可用，通过
device_is_factory_mode()进行控制 - 在 bootloader 中实现了私钥大小验证(32字节)
- 操作被限制在 bootloader 环境中执行(
wire_bootloader = true) - 工厂模式需要特定的启动条件才能激活
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 验证其他文件中是否有相应的安全检查实现
# 检查 bootloader 中的实现
rg -A 5 "WriteSEPrivateKey"
# 检查是否有工厂模式相关的限制
rg -A 5 "factory.*mode"
Length of output: 13333
core/embed/extmod/modtrezorcrypto/modtrezorcrypto-se-thd89.h (2)
929-937: 函数实现简洁明了
函数fido_delete_all_credentials的实现符合以下要点:
- 函数签名清晰
- 文档注释完整
- 实现逻辑简单直接
1009-1012: 全局表注册项正确
新增的函数和常量已正确注册到模块的全局命名空间。
core/embed/bootloader/protob/messages.pb.h (2)
1074-1077: 字段定义符合安全要求
- 使用 REQUIRED 标记确保私钥字段必须存在
- 默认初始化为 NULL 可防止意外泄露
1233-1233: 消息大小定义合理
WriteSEPrivateKey_size 被设置为34字节，包含：
- 32字节私钥数据
- 2字节协议开销
大小定义准确且符合预期。
core/src/apps/webauthn/fido2.py (3)
175-175: 证书更新：验证新的认证证书
新的认证证书包含了正确的 OneKey 特定信息。建议:
- 验证证书的有效期
- 确保私钥安全存储
运行以下脚本分析证书内容:
✅ Verification successful
证书更新已确认安全有效
从代码分析可见：
- 新证书有效期为 2024-10-18 至 2029-10-18
- 证书包含完整的 OneKey 身份信息和正确的用途（认证器认证）
- 证书仅在非模拟器环境中使用，模拟器环境使用独立的测试证书
- 证书被正确应用于 FIDO2 认证流程中的凭证生成和签名操作
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 提取并验证证书信息
echo "$_FIDO_ATT_CERT" | openssl x509 -inform der -text
Length of output: 140
Script:
#!/bin/bash
# Search for any tests or documentation related to the attestation certificate
rg -l "FIDO_ATT_CERT" --type py
# Look for any configuration or constant files that might contain certificate info
fd -e py -e json -e yaml -e conf | xargs rg -l "attestation.*cert"
# Check if there are any other similar certificates in the codebase
rg -l "\x30\x82.*\x30\x82" --type py
Length of output: 171
Script:
#!/bin/bash
# Get the content and context of the FIDO_ATT_CERT usage
rg -A 5 -B 5 "_FIDO_ATT_CERT" core/src/apps/webauthn/fido2.py
# Check for any test files related to FIDO2 attestation
fd -e py | grep -i "test.*fido\|fido.*test"
Length of output: 5123
1815-1815:
加密更新：ECDH 计算增加参数
在 ECDH 密钥计算中添加验证参数增强了安全性。建议:
- 添加注释说明参数的用途
- 考虑添加单元测试验证新的行为
运行以下脚本检查相关调用:
✅ Verification successful
ECDH 计算中的点验证参数已正确添加
在 nist256p1.multiply() 的其他调用点中，只有 get_ecdh_session_key.py 中的调用不需要点验证，因为它使用的是可信输入。FIDO2 中添加验证参数是正确且必要的安全增强。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查 nist256p1.multiply 的其他调用点
ast-grep --pattern 'nist256p1.multiply($$$)'
Length of output: 277
Script:
#!/bin/bash
# Check the implementation and usage context of nist256p1.multiply
rg "def multiply" -A 5 --glob "*.py"
# Check imports and usage of nist256p1
rg "nist256p1" -B 2 -A 2
Length of output: 57565
🧰 Tools
🪛 Ruff (0.7.0)
1815-1815: Boolean positional value in function call
(FBT003)
208-208: 安全相关：验证 SE01 版本依赖关系
将常驻凭证支持与 SE01 版本绑定是一个很好的安全实践。建议添加注释说明版本要求的原因。
运行以下脚本验证 SE01 版本检查的一致性:
✅ Verification successful
SE01 版本检查实现正确且安全
SE01 版本检查仅用于控制常驻凭证功能,实现简单清晰。代码中没有其他依赖此版本检查的安全敏感功能。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查代码库中其他依赖 SE01 版本的功能
rg "get_se01_version\(\)"
Length of output: 460
Script:
#!/bin/bash
# Check implementation of get_se01_version
ast-grep --pattern 'def get_se01_version() { $$$ }'
# Check usage context of SE01 version in deviceinfo
rg -B 2 -A 2 "onekey_se01_version" core/src/trezor/lvglui/scrs/deviceinfo.py
# Check base.py usage context
rg -B 2 -A 2 "onekey_se01_version" core/src/apps/base.py
Length of output: 1551
Script:
#!/bin/bash
# Check storage/device.py implementation
cat core/src/storage/device.py
# Check for any SE01 version comparisons
rg "get_se01_version\(\) *[><=]"
Length of output: 48006
core/embed/trezorhal/se_thd89.c (6)
2376-2387: 请确认函数 'se_set_fido2_resident_credentials' 的必要性
该函数未在代码中被调用。请检查是否有遗漏的调用，或者如果该函数暂未使用，考虑添加相应的调用或移除。
运行以下脚本以查找函数的使用情况：
✅ Verification successful
函数 'se_set_fido2_resident_credentials' 已被正确使用
该函数在 modtrezorconfig.c 中被调用，用于设置 FIDO2 凭证。函数声明和实现都存在且正确关联。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 查找 'se_set_fido2_resident_credentials' 的使用情况
rg -F 'se_set_fido2_resident_credentials' --type-add 'c:h,c' --stats
Length of output: 582
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 2376-2376: The function 'se_set_fido2_resident_credentials' is never used.
(unusedFunction)
2352-2374: 请确认函数 'se_get_fido2_resident_credentials' 的必要性
该函数未在代码中被调用。请检查是否有遗漏的调用，或者如果该函数暂未使用，考虑添加相应的调用或移除。
运行以下脚本以查找函数的使用情况：
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 2352-2352: The function 'se_get_fido2_resident_credentials' is never used.
(unusedFunction)
1053-1058: 请确认函数 'se_set_private_key_extern' 的必要性
该函数未在代码中被调用。请检查是否遗漏了对该函数的调用，或者如果不需要，考虑移除以保持代码整洁。
运行以下脚本以查找函数的使用情况：
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1053-1053: The function 'se_set_private_key_extern' is never used.
(unusedFunction)
2388-2393: 请确认函数 'se_delete_fido2_resident_credentials' 的必要性
该函数未在代码中被调用。请检查是否有遗漏的调用，或者如果该函数暂未使用，考虑添加相应的调用或移除。
运行以下脚本以查找函数的使用情况：
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 2388-2388: The function 'se_delete_fido2_resident_credentials' is never used.
(unusedFunction)
1037-1051: 请确认函数 'se_sign_message_with_write_key' 的必要性
该函数未在代码中被调用。请检查是否遗漏了对该函数的调用，或者如果不需要，考虑移除以保持代码简洁。
运行以下脚本以查找函数的使用情况：
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 1037-1037: The function 'se_sign_message_with_write_key' is never used.
(unusedFunction)
2394-2400: 请确认函数 'se_delete_all_fido2_credentials' 的必要性
该函数未在代码中被调用。请检查是否有遗漏的调用，或者如果该函数暂未使用，考虑添加相应的调用或移除。
运行以下脚本以查找函数的使用情况：
✅ Verification successful
函数 'se_delete_all_fido2_credentials' 已得到正确使用
该函数在 Python 接口层被调用，用于实现 fido_delete_all_credentials() 功能。这是一个完全合理且必要的实现。
- 在
core/embed/extmod/modtrezorcrypto/modtrezorcrypto-se-thd89.h中作为 Python 函数fido_delete_all_credentials()的底层实现 - 该功能用于删除所有 FIDO2 凭证，是设备安全功能的重要组成部分
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 查找 'se_delete_all_fido2_credentials' 的使用情况
rg -F 'se_delete_all_fido2_credentials' --type-add 'c:h,c' --stats
Length of output: 485
Script:
#!/bin/bash
# 查找 modtrezorcrypto-se-thd89.h 中函数的调用情况
rg -A 5 -B 5 "se_delete_all_fido2_credentials\(\)" core/embed/extmod/modtrezorcrypto/modtrezorcrypto-se-thd89.h
# 查找可能的相关FIDO2凭证删除操作
rg -i "fido2.*delete|delete.*fido2" --type c
Length of output: 1241
🧰 Tools
🪛 cppcheck (2.10-2)
[style] 2394-2394: The function 'se_delete_all_fido2_credentials' is never used.
(unusedFunction)
| if utils.USE_THD89: | ||
| se_thd89.fido_delete_all_credentials() | ||
| else: | ||
| for i in range(MAX_RESIDENT_CREDENTIALS): | ||
| common.delete(common.APP_WEBAUTHN, i + _RESIDENT_CREDENTIAL_START_KEY) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议添加错误处理机制
在调用 se_thd89.fido_delete_all_credentials() 时需要添加错误处理，以确保操作失败时能够正确处理异常情况。
建议修改如下：
def delete_all() -> None:
if utils.USE_THD89:
- se_thd89.fido_delete_all_credentials()
+ try:
+ se_thd89.fido_delete_all_credentials()
+ except Exception as e:
+ # 记录错误并进行适当的错误处理
+ raise ValueError("删除凭证失败") from e
else:
for i in range(MAX_RESIDENT_CREDENTIALS):
common.delete(common.APP_WEBAUTHN, i + _RESIDENT_CREDENTIAL_START_KEY)Committable suggestion skipped: line range outside the PR's diff.
| else: | ||
| sleep_ms(100) | ||
| continue | ||
| except Exception: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
需要处理静态分析工具发现的问题！
- 第24行：
break后的else子句是多余的 - 第27行：不应该捕获泛型
Exception，应该明确指定预期的异常类型
建议修改如下：
- else:
- sleep_ms(100)
- continue
+ sleep_ms(100)
- except Exception:
+ except (se_thd89.SEError, ValueError) as e:Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.7.0)
24-24: Unnecessary else after break statement
Remove unnecessary else
(RET508)
27-27: Do not catch blind exception: Exception
(BLE001)
| while True: | ||
| try: | ||
| ret = se_thd89.fido_seed() | ||
| if ret: | ||
| break | ||
| else: | ||
| sleep_ms(100) | ||
| continue | ||
| except Exception: | ||
| return WebAuthnCredentials(credentials=[]) |
There was a problem hiding this comment.
建议优化重试机制的实现！
当前实现存在以下问题：
- 无限重试可能导致系统挂起
- 异常处理过于宽泛
- 缺少失败日志
- 缺少重试机制的文档说明
建议按如下方式修改：
+ MAX_RETRIES = 10 # 最大重试次数
+ retry_count = 0
while True:
try:
ret = se_thd89.fido_seed()
if ret:
break
- else:
- sleep_ms(100)
- continue
+ sleep_ms(100)
+ retry_count += 1
+ if retry_count >= MAX_RETRIES:
+ return WebAuthnCredentials(credentials=[])
- except Exception:
+ except Exception as e:
+ from trezor import log
+ log.error(__name__, "获取FIDO种子失败: %s", str(e))
return WebAuthnCredentials(credentials=[])📝 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.
| while True: | |
| try: | |
| ret = se_thd89.fido_seed() | |
| if ret: | |
| break | |
| else: | |
| sleep_ms(100) | |
| continue | |
| except Exception: | |
| return WebAuthnCredentials(credentials=[]) | |
| MAX_RETRIES = 10 # 最大重试次数 | |
| retry_count = 0 | |
| while True: | |
| try: | |
| ret = se_thd89.fido_seed() | |
| if ret: | |
| break | |
| sleep_ms(100) | |
| retry_count += 1 | |
| if retry_count >= MAX_RETRIES: | |
| return WebAuthnCredentials(credentials=[]) | |
| except Exception as e: | |
| from trezor import log | |
| log.error(__name__, "获取FIDO种子失败: %s", str(e)) | |
| return WebAuthnCredentials(credentials=[]) |
🧰 Tools
🪛 Ruff (0.7.0)
24-24: Unnecessary else after break statement
Remove unnecessary else
(RET508)
27-27: Do not catch blind exception: Exception
(BLE001)
| from trezor.crypto import se_thd89 | ||
| from utime import sleep_ms |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
建议将导入语句移至文件顶部
为了保持代码整洁性和一致性，建议将新增的导入语句移至文件开头的导入区域。
from trezor import wire
from trezor.lvglui.i18n import gettext as _, keys as i18n_keys
from trezor.messages import Success, WebAuthnRemoveResidentCredential
+from trezor.crypto import se_thd89
+from utime import sleep_ms
from trezor.ui.components.common.webauthn import ConfirmInfoCommittable suggestion skipped: line range outside the PR's diff.
| while True: | ||
| try: | ||
| ret = se_thd89.fido_seed() | ||
| if ret: | ||
| break | ||
| else: | ||
| sleep_ms(100) | ||
| continue | ||
| except Exception: | ||
| raise wire.ProcessError("Failed to generate seed.") |
There was a problem hiding this comment.
需要改进种子生成的错误处理和循环结构
当前实现存在以下问题：
- 使用了过于宽泛的异常捕获
- 缺少超时机制，可能导致无限循环
- 循环结构可以简化
- 异常信息传递不完整
建议按如下方式修改：
+ MAX_RETRIES = 10 # 添加最大重试次数
+ retry_count = 0
while True:
try:
ret = se_thd89.fido_seed()
if ret:
break
- else:
- sleep_ms(100)
- continue
+ sleep_ms(100)
+ retry_count += 1
+ if retry_count >= MAX_RETRIES:
+ raise wire.ProcessError("种子生成超时")
- except Exception:
- raise wire.ProcessError("Failed to generate seed.")
+ except se_thd89.SEError as err:
+ raise wire.ProcessError("种子生成失败") from err📝 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.
| while True: | |
| try: | |
| ret = se_thd89.fido_seed() | |
| if ret: | |
| break | |
| else: | |
| sleep_ms(100) | |
| continue | |
| except Exception: | |
| raise wire.ProcessError("Failed to generate seed.") | |
| MAX_RETRIES = 10 # 添加最大重试次数 | |
| retry_count = 0 | |
| while True: | |
| try: | |
| ret = se_thd89.fido_seed() | |
| if ret: | |
| break | |
| sleep_ms(100) | |
| retry_count += 1 | |
| if retry_count >= MAX_RETRIES: | |
| raise wire.ProcessError("种子生成超时") | |
| except se_thd89.SEError as err: | |
| raise wire.ProcessError("种子生成失败") from err |
🧰 Tools
🪛 Ruff (0.7.0)
45-45: Unnecessary else after break statement
Remove unnecessary else
(RET508)
48-48: Do not catch blind exception: Exception
(BLE001)
49-49: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
| except Exception: | ||
| raise wire.ProcessError("Failed to generate seed.") |
There was a problem hiding this comment.
避免捕获通用的 Exception
捕获 Exception 过于宽泛，可能掩盖其他错误。建议捕获更具体的异常类型，或自定义异常。
建议修改如下：
- except Exception:
+ except SpecificException as e: # 请替换为实际的异常类型
raise wire.ProcessError("Failed to generate seed.") from eCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.7.0)
48-48: Do not catch blind exception: Exception
(BLE001)
49-49: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
| uint8_t app = trezor_obj_get_uint8(args[0]); | ||
| // webauthn resident credentials, FIDO2 | ||
| if (app == 4) { | ||
| uint32_t index = trezor_obj_get_uint(args[1]); | ||
| uint16_t len = sizeof(CTAP_credential_id_storage) - | ||
| FIDO2_RESIDENT_CREDENTIALS_HEADER_LEN; | ||
| CTAP_credential_id_storage cred_id = {0}; | ||
|
|
||
| if (!se_get_fido2_resident_credentials(index, cred_id.rp_id_hash, &len)) { | ||
| return mp_const_none; | ||
| } | ||
| return mp_obj_new_bytes(cred_id.rp_id_hash, len); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议使用常量替代魔数 '4'，并验证 'index' 参数的有效性
为了提高代码的可读性和可维护性，建议将 '4' 替换为具有明确含义的常量，例如 APP_ID_FIDO2。
另外，为了防止无效的 index 值导致潜在的运行时错误或安全问题，建议在调用 se_get_fido2_resident_credentials 之前验证 index 的范围。
| uint8_t app = trezor_obj_get_uint8(args[0]); | ||
| // webauthn resident credentials, FIDO2 | ||
| if (app == 4) { | ||
| uint32_t index = trezor_obj_get_uint(args[1]); | ||
|
|
||
| mp_buffer_info_t cred_id; | ||
| mp_get_buffer_raise(args[2], &cred_id, MP_BUFFER_READ); | ||
| if (cred_id.len > sizeof(CTAP_credential_id_storage) - | ||
| FIDO2_RESIDENT_CREDENTIALS_HEADER_LEN) { | ||
| mp_raise_msg(&mp_type_RuntimeError, "Credential ID too long"); | ||
| } | ||
| if (!se_set_fido2_resident_credentials(index, cred_id.buf, cred_id.len)) { | ||
| mp_raise_msg(&mp_type_RuntimeError, "Could not save value"); | ||
| } | ||
| return mp_const_none; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议使用常量替代魔数 '4'，并验证 'index' 参数的有效性
为了提高代码的可读性和可维护性，建议将 '4' 替换为具有明确含义的常量，例如 APP_ID_FIDO2。
另外，为了防止无效的 index 值导致潜在的运行时错误或安全问题，建议在调用 se_set_fido2_resident_credentials 之前验证 index 的范围。
| uint8_t app = trezor_obj_get_uint8(args[0]); | ||
| // webauthn resident credentials, FIDO2 | ||
| if (app == 4) { | ||
| uint32_t index = trezor_obj_get_uint(args[1]); | ||
| if (!se_delete_fido2_resident_credentials(index)) { | ||
| mp_raise_msg(&mp_type_RuntimeError, "Could not delete value"); | ||
| } | ||
| return mp_const_true; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议使用常量替代魔数 '4'，并验证 'index' 参数的有效性
为了提高代码的可读性和可维护性，建议将 '4' 替换为具有明确含义的常量，例如 APP_ID_FIDO2。
另外，为了防止无效的 index 值导致潜在的运行时错误或安全问题，建议在调用 se_delete_fido2_resident_credentials 之前验证 index 的范围。
| secbool se_get_fido2_data(uint16_t offset, uint8_t *dest, uint16_t len) { | ||
| uint8_t cmd[4] = {0}; | ||
| uint16_t recv_len = len; | ||
| cmd[0] = (offset >> 8) & 0xFF; | ||
| cmd[1] = offset & 0xFF; | ||
| cmd[2] = (len >> 8) & 0xFF; | ||
| cmd[3] = len & 0xFF; | ||
| if (!se_transmit_mac(SE_INS_READ_DATA, 0x00, 0x03, cmd, sizeof(cmd), dest, | ||
| &recv_len)) { | ||
| return secfalse; | ||
| } | ||
| return sectrue; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
完善 'se_get_fido2_data' 函数的错误处理
在 'se_get_fido2_data' 中，未验证 'se_transmit_mac' 返回后的 'recv_len' 是否与期望的 'len' 匹配。建议在成功调用后，检查 'recv_len' 是否等于预期值，确保接收到的数据完整。
Summary by CodeRabbit
新功能
WriteSEPrivateKey，增强了密钥管理功能。错误修复
文档