Add PIN attempt limiting for share storage#135
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
WalkthroughIntroduces persistent PIN attempt limiting with authenticated secure-element/NVS state, PBKDF2-HMAC-SHA256 plus HKDF key derivation, lockout delays, bricking behavior, new accessors, and native tests. ChangesPIN protection and storage cryptography
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant StorageCrypto
participant SecureElementNVS
participant MbedTLS
User->>StorageCrypto: Provide PIN
StorageCrypto->>SecureElementNVS: Load pin state
SecureElementNVS-->>StorageCrypto: Attempts, deadline, bricked flag, HMAC
StorageCrypto->>MbedTLS: Verify HMAC and derive PBKDF2/HKDF key
MbedTLS-->>StorageCrypto: Validation result or derived key
StorageCrypto->>SecureElementNVS: Save updated state
StorageCrypto-->>User: Success, delay, or bricked result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@main/storage_crypto.c`:
- Around line 343-368: The rate-limit can be bypassed after reboot because
storage_crypto_check_rate_limit reads pin_state.last_failure_time (set from a
monotonic timer) but compares it to get_time_ms which resets on reboot; change
the logic in storage_crypto_check_rate_limit and the pin state handling so you
persist a lockout_deadline (absolute expiry) instead of last_failure_time or
detect clock/monotonic resets: on failure set pin_state.lockout_deadline =
get_time_ms() + get_delay_ms(...), persist it, and in
storage_crypto_check_rate_limit compare current time to lockout_deadline (treat
missing/earlier current time than last stored time as still locked until
deadline) ensuring functions load_pin_state, get_delay_ms, and get_time_ms are
used consistently to compute and validate the absolute deadline.
- Around line 79-112: compute_state_hmac currently uses the device_id as the
HMAC key; replace that with a true secret key from a secure source (secure
element, eFuse, or flash-encryption-derived key). Modify compute_state_hmac to
accept or retrieve a secret key (e.g., a 32-byte key argument or call a secure
retrieval function like secure_element_get_key()), use that secret to build
key_padded/ipad/opad instead of device_id, and ensure the secret is never logged
and is wiped with secure_memzero after use; update callers to provide the secret
or ensure the function retrieves it from the secure element.
- Around line 305-319: The backoff function get_delay_ms currently returns
UINT32_MAX for attempts > 12, which prematurely triggers ERR_PIN_BRICKED before
PIN_MAX_ATTEMPTS (21); change the final condition so attempts
13–(PIN_MAX_ATTEMPTS-1) return the 15-minute backoff and only return UINT32_MAX
when attempts >= PIN_MAX_ATTEMPTS. Concretely, keep the existing branches for
<=3, <=6, <=9, then replace the <=12 branch with a branch that returns
15*60*1000 for attempts <= (PIN_MAX_ATTEMPTS - 1) (or explicitly for attempts <=
20) and return UINT32_MAX only when attempts >= PIN_MAX_ATTEMPTS; reference
get_delay_ms and the PIN_MAX_ATTEMPTS constant when making this change.
🧹 Nitpick comments (1)
test/native/CMakeLists.txt (1)
169-180: Consider using MbedTLS::mbedcrypto when available for consistency.Lines 95-117 use
find_package(MbedTLS)and theMbedTLS::mbedcryptotarget when available. This new target only usesfind_library, which could fail on systems where mbedtls is found via package config but not in standard library paths. For consistency:♻️ Suggested improvement for consistent mbedtls handling
-find_library(MBEDCRYPTO_LIB mbedcrypto) -if(MBEDCRYPTO_LIB) +if(MbedTLS_FOUND OR MBEDCRYPTO_LIB) add_executable(test_pin_attempt_limit test_pin_attempt_limit.c ${MAIN_DIR}/random_utils.c ${MAIN_DIR}/hw_entropy.c ${MAIN_DIR}/error_codes.c) target_include_directories(test_pin_attempt_limit PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/mocks ${MAIN_DIR} ) target_compile_definitions(test_pin_attempt_limit PRIVATE NATIVE_TEST=1 UNIT_TEST=1 MOCK_MBEDTLS=1) - target_link_libraries(test_pin_attempt_limit ${MBEDCRYPTO_LIB}) + if(MbedTLS_FOUND) + target_link_libraries(test_pin_attempt_limit MbedTLS::mbedcrypto) + else() + target_link_libraries(test_pin_attempt_limit ${MBEDCRYPTO_LIB}) + endif() else() message(STATUS "Skipping test_pin_attempt_limit (mbedtls not found)") endif()
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@main/storage_crypto.c`:
- Around line 50-55: The macro block defining NVS_NAMESPACE and NVS_KEY_*
constants violates clang-format style; run clang-format on main/storage_crypto.c
or reformat the block so the define names and string literals follow the
project's alignment rules (e.g. consistent spacing/tabs between macro names and
values) for NVS_NAMESPACE, NVS_KEY_FAILURES, NVS_KEY_LOCKOUT, NVS_KEY_BRICKED,
NVS_KEY_SALT and NVS_KEY_HMAC so the static analysis/CICD checks pass.
- Around line 153-192: compute_state_hmac currently swallows HMAC-key retrieval
failures by zeroing hmac_out and returning void; change its signature to return
an int (0 on success, non-zero on failure), propagate the error from
get_hmac_secret_key to the caller instead of writing a zero HMAC, and ensure
callers (e.g., the state load/save functions that call compute_state_hmac) check
the return value and abort/skip accepting or writing state when the HMAC cannot
be computed; keep the existing secure_memzero calls for secret_key,
ipad/opad/inner_hash on both success and failure paths and ensure hmac_out is
not used when compute_state_hmac returns an error.
- Around line 429-433: The lockout deadline overflows because get_time_ms(),
pin_state.lockout_deadline, and local now are 32-bit; change get_time_ms() to
return uint64_t, change pin_state.lockout_deadline to uint64_t, update any local
uint32_t now variables to uint64_t, and replace NVS calls
nvs_get_u32()/nvs_set_u32() with nvs_get_u64()/nvs_set_u64() where the deadline
is persisted (ensure reading/writing uses the 64-bit key). Update any related
comparisons and assignments in the functions that reference get_time_ms() and
pin_state.lockout_deadline (e.g., the lockout check and setting code) so all
time math uses 64-bit to prevent wrap-around.
♻️ Duplicate comments (1)
main/storage_crypto.c (1)
83-149: Fallback HMAC key is still predictable when SE is absent.
The non‑SE path hashes device_id/serial, which isn’t secret, so an attacker can recompute the HMAC and tamper with the NVS state. Please use a true secret source (e.g., SE-stored key, flash‑encryption/eFuse/NVS‑encryption derived key) or treat state as untrusted in this mode.
df32a34 to
48ca375
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
48ca375 to
2d42539
Compare
kwsantiago
changed the title
2 participants
Summary
Adds a PIN attempt limiter for share storage: a persisted failure counter, a progressive lockout schedule (15s / 1min / 15min), and a device brick after 21 consecutive failures that clears the in-RAM key and NVS key material, with HMAC-protected state. Also lands the PBKDF2 key-stretching plumbing, staged (see Scope below).
Partially addresses #68 (attempt limiting lands active; key stretching is staged, see Scope).
Scope and threat model
This is a best-effort online brute-force limiter. It is not a defense against a physical attacker with flash read/write access, and the code and header now say so plainly.
On shipping hardware the secure element is a not-provisioned stub, so the counter, its HMAC key, and the storage key all live in flash the attacker controls. An attacker with flash access can reset the counter or brute-force the PIN offline regardless of the online limit. Making this a real control requires a hardware root of trust (SE-held secret or eFUSE key + secure boot + flash encryption) backing both the storage key and a monotonic counter. That work, plus fail-closed-on-erased-state and write-ahead counter persistence, is tracked as follow-up issues (#141, #142) rather than claimed here.
Also note the brick path clears the in-RAM key and the NVS key material but does not erase the encrypted shares from their storage partition, so it is not a full data wipe. Whether brick should irreversibly erase the share partition is deliberately left as a separate decision (tracked as a follow-up) rather than bolted onto this PR.
Fixes over the initial revision
storage_load_sharenow records a success (resetting the counter) on a good decrypt. Covered by a newtest_storagecase.kdf_versionmarker that defaults to legacy, so existing devices keep decrypting. The legacy path is re-added byte-for-byte and pinned by a golden-vector test. PBKDF2 is present but not yet activated at provisioning time (staged, tracked separately) — no device silently changes keys.compute_state_hmacignored the SHA-256 return codes and could return a valid-looking MAC over garbage. Replaced the hand-rolled ipad/opad withmbedtls_md_hmac(verified byte-identical, so stored HMACs still validate) which returns an error that both callers handle.ERR_PIN_BRICKED.Test coverage (this was the biggest gap)
The suite was built but never executed (absent from CI's native-test list and the Justfile), and
MOCK_MBEDTLScompiled out the only two crypto tests while the mocks stubbed SHA-256 to a constant, so PBKDF2 and HMAC tamper detection were never actually exercised.test_pin_attempt_limitnow builds and runs against real mbedtls (SHA-256 / HMAC / GCM / HKDF / PBKDF2) viapin_test_compat.c, which supplies the nativecrypto_asmprimitives and anmbedtls_pkcs5_pbkdf2_hmac_extshim for host mbedtls < 3.x.MOCK_MBEDTLSand the stub path are gone.libmbedtls-devadded; the binary is run) andjust test.Verification
just test— full native suite passes, including the real-crypto PIN testscppcheck,clang-format— cleanidf.py buildfor esp32s3 — clean, no warnings (this module is linked into the firmware)