Conversation
|
Warning Rate limit exceeded@kwsantiago has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
WalkthroughAdds a new ESP-IDF component Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
main/frost_signer.c (1)
44-49: Consider extractinghex_digitto a shared utility.This function is duplicated in
main/storage.c(lines 26-31). Extract to a common header (e.g.,hex_utils.h) to avoid duplication and ensure consistent behavior.components/crypto_asm/include/crypto_asm.h (1)
11-16: Well-structured header, consider adding documentation.The declarations are correct with proper types. For maintainability, consider adding brief documentation explaining the semantics, particularly for
ct_select*functions where the behavior ofcondition(e.g., "returnsawhen condition is 0,botherwise") is critical for correct usage.main/storage.c (3)
47-55: Solid input validation for group names.The allowlist approach (alphanumeric plus
_and-) is appropriate for storage keys.Minor consideration:
isalnum()behavior depends on locale. On ESP-IDF this is typically not an issue (C locale), but for maximum portability in security-critical code, you could use explicit range checks similar tohex_digit.🔎 Locale-independent alternative
static int validate_group_name(const char *group) { size_t len = strnlen(group, STORAGE_GROUP_LEN + 1); if (len == 0 || len > STORAGE_GROUP_LEN) return 0; for (size_t i = 0; i < len; i++) { unsigned char c = (unsigned char)group[i]; - if (!isalnum(c) && c != '_' && c != '-') return 0; + int valid = (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || c == '_' || c == '-'; + if (!valid) return 0; } return 1; }
137-156: Consider zeroingslotafter use for consistency.The
slotvariable contains raw share data read from storage. For consistency with the hardening applied instorage_save_share, consider clearing it before returning.🔎 Suggested change
if (strcmp(slot.group, group) == 0) { if (slot.share_len * 2 + 1 > len) { ESP_LOGE(TAG, "Output buffer too small"); + secure_memzero(&slot, sizeof(slot)); return -1; } bytes_to_hex(slot.share_data, slot.share_len, share_hex); + secure_memzero(&slot, sizeof(slot)); return 0; } }
158-187:sector_bufnot cleared after delete operation.Unlike
storage_save_sharewhich clearssector_bufat line 133, this function leaves the static buffer populated with sector contents (including other slots' share data) after completion.🔎 Suggested change
err = esp_partition_erase_range(storage_partition, sector_offset, SECTOR_SIZE); - if (err != ESP_OK) return -1; + if (err != ESP_OK) { + secure_memzero(sector_buf, SECTOR_SIZE); + return -1; + } err = esp_partition_write(storage_partition, sector_offset, sector_buf, SECTOR_SIZE); + secure_memzero(sector_buf, SECTOR_SIZE); if (err == ESP_OK) { ESP_LOGI(TAG, "Deleted share for group %.16s...", group); return 0; } return -1;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
components/crypto_asm/CMakeLists.txtcomponents/crypto_asm/ct_compare.Scomponents/crypto_asm/ct_select.Scomponents/crypto_asm/include/crypto_asm.hcomponents/crypto_asm/secure_memzero.Scomponents/secp256k1-frost/CMakeLists.txtmain/CMakeLists.txtmain/frost.cmain/frost_signer.cmain/main.cmain/storage.c
🧰 Additional context used
🧬 Code graph analysis (1)
main/frost_signer.c (2)
main/frost.c (1)
secure_zero(45-48)main/storage.c (2)
hex_digit(27-32)hex_to_bytes(34-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (14)
main/main.c (1)
30-30: LGTM - JSON key now matches the semantic intent.The rename from
"groups"to"shares"correctly reflects whathandle_list_sharesreturns.main/CMakeLists.txt (1)
4-4: LGTM - correctly adds crypto_asm dependency.components/crypto_asm/CMakeLists.txt (1)
1-8: LGTM - clean component registration.components/crypto_asm/secure_memzero.S (1)
6-33: LGTM - secure memory zeroing with proper barrier.The assembly implementation correctly:
- Uses word-aligned writes when possible for performance
- Falls back to byte writes for unaligned portions
- Issues
memwbarrier to ensure writes complete before returnBeing pure assembly, the compiler cannot optimize away the stores.
main/frost.c (1)
75-75: LGTM - clean macro alias to assembly implementation.main/frost_signer.c (4)
22-23: LGTM - secure_memzero integration.
55-58: Good hardening - proper hex validation.Replacing
sscanfwith explicit digit validation and returning -1 on invalid characters is a solid security improvement.
94-94: Good practice - securely wiping session data.Using
secure_zeroto wipe the entiresigning_session_tstruct prevents sensitive key material from lingering in memory.
247-250: LGTM - improved input validation for commitments.The bounds checking (
commits_hex_len <= max_commits_hexand divisibility check) prevents buffer overflows and malformed input from being processed.components/secp256k1-frost/CMakeLists.txt (1)
26-27: LGTM - reasonable performance tuning for FROST signing.
ECMULT_GEN_PREC_BITS=4andECMULT_WINDOW_SIZE=4are appropriate optimization parameters for this ESP32-S3 device. With 8MB PSRAM available, the increased precomputed tables for faster EC multiplication are a sensible tradeoff.components/crypto_asm/ct_compare.S (2)
6-25: Solid constant-time comparison implementation.The function correctly accumulates XOR differences without early exits, maintaining constant-time behavior. The use of OR to collect differences ensures timing doesn't leak which byte differs.
31-50: Correct constant-time zero-check.The function ORs all bytes into an accumulator and converts the result to 1/0 at the end, ensuring constant-time execution regardless of input values.
main/storage.c (2)
27-32: Good security improvement replacing sscanf.Manual hex parsing eliminates format string handling overhead and provides explicit validation. The -1 return for invalid characters is clean.
79-134: Thorough secure memory clearing on all exit paths.The consistent use of
secure_memzeroforshare_bytes,work_slot, andsector_bufon both success and failure paths is well done.
kwsantiago
force-pushed
the
security/hardening
branch
from
d1f253d to
1c1678f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
main/storage.c (1)
27-32: Consider extracting shared hex utilities.The
hex_digit,hex_to_bytes, andbytes_to_hexfunctions are duplicated acrossstorage.candfrost_signer.c(see lines 43-67 in frost_signer.c). Consider extracting these into a shared utility module to reduce duplication and maintenance burden.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
README.mdcomponents/crypto_asm/CMakeLists.txtcomponents/crypto_asm/ct_compare.Scomponents/crypto_asm/ct_select.Scomponents/crypto_asm/include/crypto_asm.hcomponents/crypto_asm/secure_memzero.Smain/CMakeLists.txtmain/frost.cmain/frost_signer.cmain/nostr.cmain/nostr_client.hmain/provision.cmain/provision.hmain/session.cmain/session.hmain/storage.ctest_kfp.ctest_native_frost.ctests/Makefile.testtests/test_firmware_flow.ctests/test_frost_native.ctests/test_qemu.pytests/test_rpc.py
💤 Files with no reviewable changes (11)
- main/provision.h
- tests/test_qemu.py
- tests/test_firmware_flow.c
- test_native_frost.c
- test_kfp.c
- tests/test_frost_native.c
- tests/Makefile.test
- main/nostr_client.h
- main/nostr.c
- tests/test_rpc.py
- main/provision.c
🚧 Files skipped from review as they are similar to previous changes (4)
- components/crypto_asm/include/crypto_asm.h
- main/CMakeLists.txt
- components/crypto_asm/ct_compare.S
- components/crypto_asm/CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (2)
main/frost.c (3)
components/secp256k1-frost/esp_random_adapter.c (1)
fill_random(8-14)main/frost_signer.c (1)
secure_zero(25-28)main/session.c (1)
secure_zero(8-11)
main/storage.c (1)
main/frost_signer.c (3)
hex_digit(44-49)hex_to_bytes(51-61)bytes_to_hex(63-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (16)
main/session.h (1)
48-48: LGTM!The
session_destroydeclaration properly complementssession_initand follows the existing API style. This provides a clean interface for secure session cleanup.main/session.c (2)
4-12: LGTM!The platform abstraction for secure zeroing is well-implemented. The volatile pointer approach for non-ESP platforms prevents compiler optimization from eliding the zeroing operation.
40-42: LGTM!The
session_destroyimplementation correctly zeroes the entire session structure, protecting sensitive data like nonces and signature shares.README.md (1)
74-78: LGTM!Documentation correctly reflects the updated test directory structure and cmake-based build workflow for native tests.
main/storage.c (3)
47-55: LGTM!Good input validation for group names. The allowlist approach (alphanumeric, underscore, dash) prevents injection attacks and ensures safe storage key handling.
79-134: LGTM on secure zeroing throughout storage operations.Comprehensive secure zeroing is applied on all error paths and after buffer use in
storage_save_share. This properly protects sensitive share data from lingering in memory.
137-162: LGTM!
storage_load_shareproperly zeroes the slot after each iteration, preventing sensitive data from persisting in the stack-allocated structure.components/crypto_asm/secure_memzero.S (1)
1-35: Well-implemented secure memory zeroing.Key strengths:
- Assembly implementation prevents compiler optimization from eliding the zeroing
- Word-aligned path optimizes performance for larger buffers
memwbarrier at line 32 ensures all writes complete before return, which is critical for security- Proper handling of unaligned addresses and remainders
main/frost.c (2)
53-94: RNG health check implementation looks good, but consider small buffer behavior.The health checks (extreme byte values, monobit test, bit-transition test) provide reasonable entropy validation. However, line 54 returns success for buffers smaller than 8 bytes, bypassing all checks. This is acceptable since statistical tests need sufficient sample size, but ensure callers generating critical secrets (like nonces at lines 173-175) always use 32-byte buffers.
96-106: LGTM - retry logic correctly implemented.The previous review's concern about redundant
esp_fill_randomhas been addressed. The flow is now: fill → check → if failed, refill → recheck → abort on second failure. This provides a single retry for transient RNG issues while still failing fast on persistent problems.main/frost_signer.c (3)
70-77: Good use of constant-time comparison for session lookup.Using
ct_comparefor session ID matching prevents timing side-channels that could leak information about valid session IDs.
91-97: LGTM!The
free_sessionfunction properly chainssession_destroy(which zeros the nested session struct) withsecure_zeroon the entiresigning_session_t, ensuring complete cleanup of sensitive data including thefrost_state.
247-269: Input validation for commitments looks correct.The validation ensures:
- Total hex length doesn't exceed
(KFP_MAX_PARTICIPANTS - 1) * 264- Length is a multiple of 264 (132 bytes per commitment × 2 for hex)
- Loop breaks if
commitment_countreachesKFP_MAX_PARTICIPANTSOne note: if
commits_hex_len == 0, the block is skipped entirely, which is valid for a coordinator-only flow (no peer commitments).components/crypto_asm/ct_select.S (3)
7-19: Constant-time issues from previous review have been addressed.The implementation now uses branchless mask derivation (lines 9-11):
neg a5, a4 # a5 = -a4 or a5, a5, a4 # sign bit set if a4 != 0 srai a5, a5, 31 # arithmetic shift: all 1s or all 0sThis produces the mask without data-dependent branches.
25-48: LGTM - ct_select_bytes now constant-time.The mask derivation (lines 27-31) is branchless. The
beqz a5at line 32 is acceptable since it branches on the public length parameter, not the secret selector value.
54-69: LGTM - ct_cswap32 correctly implements constant-time conditional swap.The branchless mask derivation (lines 58-60) combined with the XOR-swap pattern (lines 61-64) provides a correct constant-time implementation.
kwsantiago
force-pushed
the
security/hardening
branch
from
1c1678f to
45c28e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
main/frost_signer.c (1)
44-61: Consider extracting shared hex utilities.The
hex_digitandhex_to_bytesfunctions are duplicated betweenfrost_signer.candstorage.c. Consider extracting these to a shared utility module to reduce duplication.main/storage.c (1)
27-45: Duplicate hex utilities with frost_signer.c.As noted in the
frost_signer.creview, these hex conversion functions are duplicated. Consider consolidating into a shared module.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/workflows/ci.ymlREADME.mdcomponents/crypto_asm/CMakeLists.txtcomponents/crypto_asm/ct_compare.Scomponents/crypto_asm/ct_select.Scomponents/crypto_asm/include/crypto_asm.hcomponents/crypto_asm/secure_memzero.Smain/CMakeLists.txtmain/frost.cmain/frost_signer.cmain/nostr.cmain/nostr_client.hmain/provision.cmain/provision.hmain/session.cmain/session.hmain/storage.ctest_kfp.ctest_native_frost.ctests/Makefile.testtests/test_firmware_flow.ctests/test_frost_native.ctests/test_qemu.pytests/test_rpc.py
💤 Files with no reviewable changes (11)
- main/provision.c
- tests/Makefile.test
- main/nostr.c
- tests/test_qemu.py
- test_native_frost.c
- tests/test_frost_native.c
- main/nostr_client.h
- test_kfp.c
- main/provision.h
- tests/test_rpc.py
- tests/test_firmware_flow.c
🚧 Files skipped from review as they are similar to previous changes (4)
- components/crypto_asm/ct_select.S
- main/CMakeLists.txt
- components/crypto_asm/ct_compare.S
- main/session.c
🧰 Additional context used
🧬 Code graph analysis (2)
main/session.h (1)
main/session.c (1)
session_destroy(40-42)
main/storage.c (1)
main/frost_signer.c (3)
hex_digit(44-49)hex_to_bytes(51-61)bytes_to_hex(63-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
README.md (1)
74-78: LGTM!The updated testing instructions properly reflect the new CMake-based native test build flow and the updated hardware test path. These align with the CI workflow changes.
components/crypto_asm/secure_memzero.S (1)
1-35: Well-structured secure memory zeroing implementation.The assembly correctly:
- Handles zero-length input with early return
- Optimizes aligned writes using 32-bit stores when possible
- Falls back to byte-wise zeroing for unaligned addresses or trailing bytes
- Includes
memwbarrier to prevent compiler/hardware reordering, ensuring the zeroing is not optimized awayThis is a solid constant-time zeroing primitive suitable for cryptographic use.
main/session.h (1)
48-48: LGTM!Clean API addition for secure session destruction. The implementation in
session.cproperly usessecure_zeroto wipe sensitive session data including nonces and signature shares.components/crypto_asm/CMakeLists.txt (1)
1-8: LGTM!Clean ESP-IDF component registration for the crypto assembly primitives. The include directory setup properly exposes the
crypto_asm.hheader to dependent components.main/frost.c (1)
53-106: Solid RNG health check implementation.The implementation now includes:
- Monobit test (bit count within ±25% of expected)
- Bit transition test (transition count within ±25% of expected)
- Extreme byte value detection (>50% zeros or ones)
- Single retry before aborting
The redundant
esp_fill_randomcall from the previous review has been addressed. The retry logic now correctly: fill → check → (on fail) fill → check → abort.main/frost_signer.c (3)
70-77: Good use of constant-time comparison for session lookup.Using
ct_comparefor session ID matching prevents timing-based side-channel attacks that could leak information about valid session IDs.
91-96: Proper layered secure cleanup.The cleanup correctly:
- Frees FROST cryptographic state
- Securely wipes the session via
session_destroy- Securely wipes the entire
signing_session_tstructureThis ensures no sensitive data (nonces, shares, keys) remains in memory.
247-269: Improved commitment validation logic.The tightened validation now:
- Enforces a maximum commitment count based on
KFP_MAX_PARTICIPANTS - 1- Requires hex length to be a multiple of 264 (132 bytes per commitment)
- Properly bounds the loop with both the calculated count and a safety check
This prevents buffer overflows and malformed input processing.
main/storage.c (4)
47-55: Good input validation for group names.The
validate_group_namefunction properly restricts group names to alphanumeric characters, underscores, and hyphens. This prevents potential injection or path-related vulnerabilities when group names are used in storage operations.
79-134: Comprehensive secure memory handling in storage_save_share.The function now properly zeroes sensitive data in all code paths:
share_bytesafter conversion or on errorwork_slotafter copying to sector buffersector_bufafter write or on errorThis prevents sensitive share data from lingering in memory after operations complete or fail.
137-162: Consistent secure cleanup in storage_load_share.Each
slotstructure read from flash is properly zeroed after use, regardless of whether a match is found. This prevents share data from remaining in stack memory.
164-205: Secure deletion with proper cleanup.The delete operation now zeroes:
- The slot structure after reading/before sector modification
- The sector buffer on error or after write
Combined with writing
0xFFto the slot region, this provides defense-in-depth for secure share deletion..github/workflows/ci.yml (1)
51-57: The CMake configuration intest/native/CMakeLists.txtcorrectly discovers the secp256k1-frost library. It calculates the relative path to the siblingsecp256k1-frostdirectory via${CMAKE_CURRENT_LIST_DIR}/../../../secp256k1-frostand links to the built library at${SECP_DIR}/build/src, which aligns with the CI workflow's build step. No action needed.Likely an incorrect or invalid review comment.
components/crypto_asm/include/crypto_asm.h (2)
1-10: LGTM: Header structure is well-formed.The include guards, standard library includes, and C++ compatibility wrapper are all correctly implemented.
18-22: LGTM: Closing blocks are correct.The C++ compatibility and include guard are properly closed.
kwsantiago
force-pushed
the
security/hardening
branch
from
45c28e7 to
800295b
Compare
kwsantiago
force-pushed
the
security/hardening
branch
from
800295b to
29199dc
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.