What the developer wrote
Authored by Salvatore Ingala
Harden crypto_derive_symmetric_key
The function would cause a buffer overflow passed a buffer longer
than 32 bytes. This is not an issue today as it was only ever used
for a fixed short string (WALLET_SLIP0021_LABEL).
The workaround to copy into a local buffer is still needed, so we
defensively forbid longer strings.
What changed, and why it matters
This commit fixes a defensive coding issue in the Ledger Bitcoin app. A function that derives a secret key from a text label could overflow an internal 32-byte buffer if given a label longer than 32 bytes. In practice, the app only ever passed one fixed short label, so the overflow was not reachable from normal use. The patch now rejects oversized labels and also handles the rare failure case when generating a wallet security code (HMAC).
Treat as a hardening fix with low immediate risk because the only known caller passes a fixed short label. Review all call sites of crypto_derive_symmetric_key to confirm no new callers can pass attacker-controlled long labels, and ensure downstream callers now check the new false return value. Consider adding unit tests for label_len > 32 and for HMAC failure paths.
Security signals we found
Stack buffer overflow potential in crypto_derive_symmetric_key
Missing input length validation on label_len before memcpy
Failure to check return value of compute_wallet_hmac in register_wallet
Defensive hardening rather than currently exploitable vulnerability
SLIP-0021 key derivation with alignment workaround
Evidence from the diff
crypto_derive_symmetric_key() copies the caller-supplied label into a 32-byte, 4-byte-aligned local buffer before passing it to os_derive_bip32_with_seed_no_throw() for SLIP-0021 key derivation. Previously, the function performed no length check, so a label_len greater than 32 would cause a stack buffer overflow. The commit adds an explicit label_len > sizeof(label_copy) guard that returns false. It also updates the header comment and makes compute_wallet_hmac() in register_wallet.c propagate that failure by sending SW_BAD_STATE. The original code comment’s TODO is removed and the alignment workaround is retained and documented.
Changed components
src/crypto.csrc/crypto.hsrc/handler/register_wallet.ccrypto_derive_symmetric_keycompute_wallet_hmacInspect captured patch +15 / −6
diff --git a/src/crypto.c b/src/crypto.c
index cc34979..1cf7ca5 100644
--- a/src/crypto.c
+++ b/src/crypto.c
@@ -275,12 +275,18 @@ uint32_t crypto_get_master_key_fingerprint() {
}
bool crypto_derive_symmetric_key(const char *label, size_t label_len, uint8_t key[static 32]) {
- // TODO: is there a better way?
- // The label is a byte string in SLIP-0021, but os_derive_bip32_with_seed_no_throw
- // accesses the `path` argument as an array of uint32_t, causing a device freeze if memory
- // is not aligned.
+ // The label is a byte string in SLIP-0021, but os_derive_bip32_with_seed_no_throw
+ // accesses the `path` argument as an array of uint32_t, causing a device freeze if memory
+ // is not aligned.
+ // As a workaround, we copy the label into a local buffer aligned to 4 bytes.
+
uint8_t label_copy[32] __attribute__((aligned(4)));
+ // Fail if the length of the buffer is longer than the local buffer
+ if (label_len > sizeof(label_copy)) {
+ return false;
+ }
+
memcpy(label_copy, label, label_len);
if (os_derive_bip32_with_seed_no_throw(HDW_SLIP21,
diff --git a/src/crypto.h b/src/crypto.h
index a172b23..2248365 100644
--- a/src/crypto.h
+++ b/src/crypto.h
@@ -300,7 +300,7 @@ int get_extended_pubkey_at_path(const uint32_t bip32_path[],
* @param[in] label
* Pointer to the label. The first byte of the label must be 0x00 to comply with SLIP-0021.
* @param[in] label_len
- * Length of the label.
+ * Length of the label. It must be at most 32 bytes.
* @param[out] key
* Pointer to a 32-byte output buffer that will contain the generated key.
*/
diff --git a/src/handler/register_wallet.c b/src/handler/register_wallet.c
index c3350e7..1734312 100644
--- a/src/handler/register_wallet.c
+++ b/src/handler/register_wallet.c
@@ -250,7 +250,10 @@ void handler_register_wallet(dispatcher_context_t *dc, uint8_t protocol_version)
// And the signature would be on the concatenation of the wallet id and the metadata.
// The client must persist the metadata, together with the signature.
- compute_wallet_hmac(wallet_id, response.hmac);
+ if (!compute_wallet_hmac(wallet_id, response.hmac)) {
+ SEND_SW(dc, SW_BAD_STATE); // this should never fail
+ return;
+ }
SEND_RESPONSE(dc, &response, sizeof(response), SW_OK);
}
Why this scored 45/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.