fix(crypto): add missing memzero to `ecdsa.c`
What the developer wrote
Authored by M1nd3r
fix(crypto): add missing memzero to `ecdsa.c`
[no changelog]
What changed, and why it matters
This commit fixes a cleanup issue in the code that creates cryptocurrency signatures on Trezor hardware wallets. In several error paths, secret intermediate numbers (including a copy of the private key and random nonce values) were left in memory instead of being securely erased. The patch makes sure those values are wiped even when the function exits early due to an invalid key or other failure. This reduces the risk that sensitive signing material could leak if memory is later read by another process or attacker.
Treat as a low-to-moderate security hardening fix. Review whether any released firmware versions shipped with these uncleared error paths and assess whether local memory exposure is practical on the target hardware. Apply the patch and consider adding static analysis or runtime tests to verify that all secret-bearing local variables are memzero'd before return.
Security signals we found
Missing secure wipe (memzero) of bignum256 secrets on error paths
Private-key copy (bn_read_be into local bignum) not cleared on invalid-key returns
ECDSA nonce (k) and blinding factor (randk) not cleared on all exit paths
Pointer alias s = &R.y prevented independent secure erasure of signature component
Consolidated cleanup label introduced to reduce future missing-wipe bugs
Evidence from the diff
The change modifies crypto/ecdsa.c to add missing memzero() calls and consolidate cleanup in tc_ecdsa_sign_digest() via a single cleanup label. Previously, early returns on invalid private key (ret=1/2) left bignum256 variables k, z, randk, R, and s (formerly aliased to R.y) uncleared. The patch also converts the s pointer alias into a standalone variable so it can be explicitly zeroed. Additional memzero() calls were added to tc_ecdh_multiply(), tc_ecdsa_get_public_key33(), and tc_ecdsa_get_public_key65() on their invalid-key error paths. This is a defense-in-depth fix against local information disclosure of sensitive intermediate state.
Changed components
crypto/ecdsa.ctc_ecdsa_sign_digest()tc_ecdh_multiply()tc_ecdsa_get_public_key33()tc_ecdsa_get_public_key65()Inspect captured patch +27 / −23
diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
index e8a63c97..42e6b909 100644
--- a/crypto/ecdsa.c
+++ b/crypto/ecdsa.c
@@ -646,6 +646,7 @@ int tc_ecdh_multiply(const ecdsa_curve *curve, const uint8_t *priv_key,
bn_read_be(priv_key, &k);
if (bn_is_zero(&k) || !bn_is_less(&k, &curve->order)) {
// Invalid private key.
+ memzero(&k, sizeof(k));
return 2;
}
@@ -682,10 +683,10 @@ int ecdsa_sign(const ecdsa_curve *curve, HasherType hasher_sign,
int tc_ecdsa_sign_digest(const ecdsa_curve *curve, const uint8_t *priv_key,
const uint8_t *digest, uint8_t *sig, uint8_t *pby,
int (*is_canonical)(uint8_t by, uint8_t sig[64])) {
+ int ret = -1;
int i = 0;
curve_point R = {0};
- bignum256 k = {0}, z = {0}, randk = {0};
- bignum256 *s = &R.y;
+ bignum256 k = {0}, z = {0}, randk = {0}, s = {0};
uint8_t by; // signature recovery byte
#if USE_RFC6979
@@ -699,7 +700,8 @@ int tc_ecdsa_sign_digest(const ecdsa_curve *curve, const uint8_t *priv_key,
// so this is most likely an indication of a bug. Furthermore, the signature
// has no value, because in this case it can be easily forged for any public
// key, see ecdsa_verify_digest().
- return 1;
+ ret = 1;
+ goto cleanup;
}
for (i = 0; i < 10000; i++) {
@@ -728,34 +730,35 @@ int tc_ecdsa_sign_digest(const ecdsa_curve *curve, const uint8_t *priv_key,
continue;
}
- bn_read_be(priv_key, s);
- if (bn_is_zero(s) || !bn_is_less(s, &curve->order)) {
+ bn_read_be(priv_key, &s);
+ if (bn_is_zero(&s) || !bn_is_less(&s, &curve->order)) {
// Invalid private key.
- return 2;
+ ret = 2;
+ goto cleanup;
}
// randomize operations to counter side-channel attacks
generate_k_random(&randk, &curve->order);
bn_multiply(&randk, &k, &curve->order); // k*rand
bn_inverse(&k, &curve->order); // (k*rand)^-1
- bn_multiply(&R.x, s, &curve->order); // R.x*priv
- bn_add(s, &z); // R.x*priv + z
- bn_multiply(&k, s, &curve->order); // (k*rand)^-1 (R.x*priv + z)
- bn_multiply(&randk, s, &curve->order); // k^-1 (R.x*priv + z)
- bn_mod(s, &curve->order);
+ bn_multiply(&R.x, &s, &curve->order); // R.x*priv
+ bn_add(&s, &z); // R.x*priv + z
+ bn_multiply(&k, &s, &curve->order); // (k*rand)^-1 (R.x*priv + z)
+ bn_multiply(&randk, &s, &curve->order); // k^-1 (R.x*priv + z)
+ bn_mod(&s, &curve->order);
// if s is zero, we retry
- if (bn_is_zero(s)) {
+ if (bn_is_zero(&s)) {
continue;
}
// if S > order/2 => S = -S
- if (bn_is_less(&curve->order_half, s)) {
- bn_subtract(&curve->order, s, s);
+ if (bn_is_less(&curve->order_half, &s)) {
+ bn_subtract(&curve->order, &s, &s);
by ^= 1;
}
// we are done, R.x and s is the result signature
bn_write_be(&R.x, sig);
- bn_write_be(s, sig + 32);
+ bn_write_be(&s, sig + 32);
// check if the signature is acceptable or retry
if (is_canonical && !is_canonical(by, sig)) {
@@ -765,23 +768,22 @@ int tc_ecdsa_sign_digest(const ecdsa_curve *curve, const uint8_t *priv_key,
if (pby) {
*pby = by;
}
-
- memzero(&k, sizeof(k));
- memzero(&randk, sizeof(randk));
-#if USE_RFC6979
- memzero(&rng, sizeof(rng));
-#endif
- return 0;
+ ret = 0;
+ goto cleanup;
}
// Too many retries without a valid signature
// -> fail with an error
+cleanup:
+ memzero(&R, sizeof(R));
memzero(&k, sizeof(k));
memzero(&randk, sizeof(randk));
+ memzero(&z, sizeof(z));
+ memzero(&s, sizeof(s));
#if USE_RFC6979
memzero(&rng, sizeof(rng));
#endif
- return -1;
+ return ret;
}
// returns 0 on success
@@ -793,6 +795,7 @@ int tc_ecdsa_get_public_key33(const ecdsa_curve *curve, const uint8_t *priv_key,
bn_read_be(priv_key, &k);
if (bn_is_zero(&k) || !bn_is_less(&k, &curve->order)) {
// Invalid private key.
+ memzero(&k, sizeof(k));
memzero(pub_key, 33);
return -1;
}
@@ -818,6 +821,7 @@ int tc_ecdsa_get_public_key65(const ecdsa_curve *curve, const uint8_t *priv_key,
bn_read_be(priv_key, &k);
if (bn_is_zero(&k) || !bn_is_less(&k, &curve->order)) {
// Invalid private key.
+ memzero(&k, sizeof(k));
memzero(pub_key, 65);
return -1;
}
Why this scored 51/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.