What the developer wrote
Authored by aaron
resolve the code review comments
What changed, and why it matters
This commit changes two things in a cryptocurrency hardware wallet's firmware. First, it makes several low-level secure-chip operations crash or halt the device if the chip reports any unexpected failure, rather than returning the error to the caller. Second, it adds a startup check that tries to detect partially-written account data (for example, after a sudden power loss during a factory wipe) and erases the affected account. The commit title says only 'resolve the code review comments' and does not describe any security issue.
Review whether ASSERT on secure-element failures is safe for a hardware wallet (a failed crypto operation should propagate an error, not necessarily halt the firmware). Verify that AccountsDataCheck() checks all pages required for account integrity and that the erase loop handles write failures safely. Treat this as a hardening/reliability patch unless additional context shows it fixes an exploitable vulnerability.
Security signals we found
Secure-element error codes are now swallowed by ASSERT macros, potentially turning recoverable faults into device crashes or silent failures
New account-data integrity check detects inconsistent IV/key-piece state and erases the account
Defensive change appears aimed at power-loss / partial-write corruption during wipe operations
Commit message gives no security context
Evidence from the diff
se_interface.c now wraps ATECC608B/DS28S60 calls with ASSERT on success codes and returns a fixed SUCCESS_CODE. This converts recoverable hardware errors into fatal assertions and may hide real error values from upper layers. account_manager.c’s AccountsDataCheck() now reads the IV page and key-piece page for each account, counts how many look entropic, and if exactly one of the two is entropic it treats the account as corrupt and overwrites all pages for that account with zeros. The change is defensive but partial: it only checks two of several pages per account, uses a simple entropy heuristic, and the erase loop does not appear to verify success before continuing. No CVE, advisory, or vendor security statement is present in the supplied materials.
Changed components
src/hardware_interface/se_interface.csrc/managers/account_manager.cInspect captured patch +36 / −6
diff --git a/src/hardware_interface/se_interface.c b/src/hardware_interface/se_interface.c
index c846189..7d2cb5c 100644
--- a/src/hardware_interface/se_interface.c
+++ b/src/hardware_interface/se_interface.c
@@ -11,19 +11,25 @@
int32_t SE_EncryptWrite(uint8_t slot, uint8_t block, const uint8_t *data)
{
int32_t ret = Atecc608bEncryptWrite(slot, block, data);
- return ret;
+ ASSERT(ret == ATCA_SUCCESS);
+ return SUCCESS_CODE;
}
int32_t SE_Kdf(uint8_t slot, const uint8_t *authKey, const uint8_t *inData, uint32_t inLen, uint8_t *outData)
{
int32_t ret = Atecc608bKdf(slot, authKey, inData, inLen, outData);
- return ret;
+ if (ret == ATCA_CHECKMAC_VERIFY_FAILED) {
+ return ret;
+ }
+ ASSERT(ret == ATCA_SUCCESS);
+ return SUCCESS_CODE;
}
int32_t SE_DeriveKey(uint8_t slot, const uint8_t *authKey)
{
int32_t ret = Atecc608bDeriveKey(slot, authKey);
- return ret;
+ ASSERT(ret == ATCA_SUCCESS);
+ return SUCCESS_CODE;
}
//END
@@ -31,7 +37,8 @@ int32_t SE_DeriveKey(uint8_t slot, const uint8_t *authKey)
int32_t SE_HmacEncryptRead(uint8_t *data, uint8_t page)
{
int32_t ret = DS28S60_HmacEncryptRead(data, page);
- return ret;
+ ASSERT(ret == DS28S60_SUCCESS);
+ return SUCCESS_CODE;
}
int32_t SE_GetDS28S60Rng(uint8_t *rngArray, uint32_t num)
@@ -56,7 +63,8 @@ int32_t SE_GetAtecc608bRng(uint8_t *rngArray, uint32_t num)
int32_t SE_HmacEncryptWrite(const uint8_t *data, uint8_t page)
{
int32_t ret = DS28S60_HmacEncryptWrite(data, page);
- return ret;
+ ASSERT(ret == DS28S60_SUCCESS);
+ return SUCCESS_CODE;
}
//END
diff --git a/src/managers/account_manager.c b/src/managers/account_manager.c
index 5da84b7..73b9794 100644
--- a/src/managers/account_manager.c
+++ b/src/managers/account_manager.c
@@ -597,15 +597,37 @@ int32_t DestroyAccount(uint8_t accountIndex)
return ret;
}
-
+// wipe device may power lose, check the account status.
+// check whether the account data is valid, if not, erase the account data.
void AccountsDataCheck(void)
{
int32_t ret;
uint8_t data[32], accountIndex;
for (accountIndex = 0; accountIndex < 3; accountIndex++) {
+ // for se gen1, check each account start
ret = SE_HmacEncryptRead(data, accountIndex * PAGE_NUM_PER_ACCOUNT + PAGE_INDEX_IV);
CHECK_ERRCODE_BREAK("read iv", ret);
+ if (CheckEntropy(data, 32)) {
+ validCount++;
+ }
+
+ // for se gen1, check each account key to check validity
+ ret = SE_HmacEncryptRead(data, accountIndex * PAGE_NUM_PER_ACCOUNT + PAGE_INDEX_KEY_PIECE);
+ CHECK_ERRCODE_BREAK("read key piece", ret);
+ if (CheckEntropy(data, 32)) {
+ validCount++;
+ }
+ // if start disconsistent with keypieces, consider the account data is illegal, erase the account data.
+ if (validCount == 1) {
+ printf("illegal data:%d\n", accountIndex);
+ memset_s(data, sizeof(data), 0, sizeof(data));
+ for (i = 0; i < PAGE_NUM_PER_ACCOUNT; i++) {
+ printf("erase index=%d\n", i);
+ ret = SE_HmacEncryptWrite(data, accountIndex * PAGE_NUM_PER_ACCOUNT + i);
+ CHECK_ERRCODE_BREAK("ds28s60 write", ret);
+ }
+ }
}
CLEAR_ARRAY(data);
}
Why this scored 44/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.