Skip to content
This repository was archived by the owner on May 28, 2019. It is now read-only.

Commit a274a36

Browse files
real-or-randomreal-or-random
committed
extmod/modtrezorcrypto: return False or None consistently when a signature verification fails
So far, we either return False (or None for public recovery) or raise a ValueError (e.g., when the length of the signature). This is inconsistent and dangerous because the inputs to signature verification may be attacker-provided and cannot be assumed to be well-formed. This led to issue #422 where a firmware error is raised when an invalid signature is is provided. This has been fixed for the ethereum app but not for the wallet app. This commit addresses the problem at the core of the issue, i.e., at the verification functions in extmod such that all apps are covered.
1 parent c542cb3 commit a274a36

File tree

5 files changed

+28
-31
lines changed

5 files changed

+28
-31
lines changed

‎embed/extmod/modtrezorcrypto/modtrezorcrypto-ed25519.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -147,13 +147,13 @@ STATIC mp_obj_t mod_trezorcrypto_ed25519_verify(mp_obj_t public_key,
147147
mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
148148
mp_get_buffer_raise(message, &msg, MP_BUFFER_READ);
149149
if (pk.len != 32) {
150-
mp_raise_ValueError("Invalid length of public key");
150+
return mp_const_false;
151151
}
152152
if (sig.len != 64) {
153-
mp_raise_ValueError("Invalid length of signature");
153+
return mp_const_false;
154154
}
155155
if (msg.len == 0) {
156-
mp_raise_ValueError("Empty data to verify");
156+
return mp_const_false;
157157
}
158158
return (0 == ed25519_sign_open(msg.buf, msg.len,
159159
*(const ed25519_public_key *)pk.buf,

‎embed/extmod/modtrezorcrypto/modtrezorcrypto-nist256p1.h‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -122,14 +122,14 @@ STATIC mp_obj_t mod_trezorcrypto_nist256p1_verify(mp_obj_t public_key,
122122
mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
123123
mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
124124
if (pk.len != 33 && pk.len != 65) {
125-
mp_raise_ValueError("Invalid length of public key");
125+
return mp_const_false;
126126
}
127127
if (sig.len != 64 && sig.len != 65) {
128-
mp_raise_ValueError("Invalid length of signature");
128+
return mp_const_false;
129129
}
130130
int offset = sig.len - 64;
131131
if (dig.len != 32) {
132-
mp_raise_ValueError("Invalid length of digest");
132+
return mp_const_false;
133133
}
134134
return mp_obj_new_bool(
135135
0 == ecdsa_verify_digest(&nist256p1, (const uint8_t *)pk.buf,
@@ -142,22 +142,22 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_3(mod_trezorcrypto_nist256p1_verify_obj,
142142
/// def verify_recover(signature: bytes, digest: bytes) -> bytes:
143143
/// '''
144144
/// Uses signature of the digest to verify the digest and recover the public
145-
/// key. Returns public key on success, None on failure.
145+
/// key. Returns public key on success, None if the signature is invalid.
146146
/// '''
147147
STATIC mp_obj_t mod_trezorcrypto_nist256p1_verify_recover(mp_obj_t signature,
148148
mp_obj_t digest) {
149149
mp_buffer_info_t sig, dig;
150150
mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
151151
mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
152152
if (sig.len != 65) {
153-
mp_raise_ValueError("Invalid length of signature");
153+
return mp_const_none;
154154
}
155155
if (dig.len != 32) {
156-
mp_raise_ValueError("Invalid length of digest");
156+
return mp_const_none;
157157
}
158158
uint8_t recid = ((const uint8_t *)sig.buf)[0] - 27;
159159
if (recid >= 8) {
160-
mp_raise_ValueError("Invalid recid in signature");
160+
return mp_const_none;
161161
}
162162
bool compressed = (recid >= 4);
163163
recid &= 3;

‎embed/extmod/modtrezorcrypto/modtrezorcrypto-secp256k1.h‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -139,14 +139,14 @@ STATIC mp_obj_t mod_trezorcrypto_secp256k1_verify(mp_obj_t public_key,
139139
mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
140140
mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
141141
if (pk.len != 33 && pk.len != 65) {
142-
mp_raise_ValueError("Invalid length of public key");
142+
return mp_const_false;
143143
}
144144
if (sig.len != 64 && sig.len != 65) {
145-
mp_raise_ValueError("Invalid length of signature");
145+
return mp_const_false;
146146
}
147147
int offset = sig.len - 64;
148148
if (dig.len != 32) {
149-
mp_raise_ValueError("Invalid length of digest");
149+
return mp_const_false;
150150
}
151151
return mp_obj_new_bool(
152152
0 == ecdsa_verify_digest(&secp256k1, (const uint8_t *)pk.buf,
@@ -159,22 +159,22 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_3(mod_trezorcrypto_secp256k1_verify_obj,
159159
/// def verify_recover(signature: bytes, digest: bytes) -> bytes:
160160
/// '''
161161
/// Uses signature of the digest to verify the digest and recover the public
162-
/// key. Returns public key on success, None on failure.
162+
/// key. Returns public key on success, None if the signature is invalid.
163163
/// '''
164164
STATIC mp_obj_t mod_trezorcrypto_secp256k1_verify_recover(mp_obj_t signature,
165165
mp_obj_t digest) {
166166
mp_buffer_info_t sig, dig;
167167
mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
168168
mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
169169
if (sig.len != 65) {
170-
mp_raise_ValueError("Invalid length of signature");
170+
return mp_const_none;
171171
}
172172
if (dig.len != 32) {
173-
mp_raise_ValueError("Invalid length of digest");
173+
return mp_const_none;
174174
}
175175
uint8_t recid = ((const uint8_t *)sig.buf)[0] - 27;
176176
if (recid >= 8) {
177-
mp_raise_ValueError("Invalid recid in signature");
177+
return mp_const_none;
178178
}
179179
bool compressed = (recid >= 4);
180180
recid &= 3;

‎embed/extmod/modtrezorcrypto/modtrezorcrypto-secp256k1_zkp.h‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -170,24 +170,24 @@ STATIC mp_obj_t mod_trezorcrypto_secp256k1_zkp_verify(mp_obj_t public_key,
170170
mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
171171
mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
172172
if (pk.len != 33 && pk.len != 65) {
173-
mp_raise_ValueError("Invalid length of public key");
173+
return mp_const_false;
174174
}
175175
if (sig.len != 64 && sig.len != 65) {
176-
mp_raise_ValueError("Invalid length of signature");
176+
return mp_const_false;
177177
}
178178
int offset = sig.len - 64;
179179
if (dig.len != 32) {
180-
mp_raise_ValueError("Invalid length of digest");
180+
return mp_const_false;
181181
}
182182
secp256k1_ecdsa_signature ec_sig;
183183
if (!secp256k1_ecdsa_signature_parse_compact(
184184
ctx, &ec_sig, (const uint8_t *)sig.buf + offset)) {
185-
mp_raise_ValueError("Invalid signature");
185+
return mp_const_false;
186186
}
187187
secp256k1_pubkey ec_pk;
188188
if (!secp256k1_ec_pubkey_parse(ctx, &ec_pk, (const uint8_t *)pk.buf,
189189
pk.len)) {
190-
mp_raise_ValueError("Invalid public key");
190+
return mp_obj_new_bool(0);
191191
}
192192
return mp_obj_new_bool(1 == secp256k1_ecdsa_verify(ctx, &ec_sig,
193193
(const uint8_t *)dig.buf,
@@ -199,7 +199,7 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_3(mod_trezorcrypto_secp256k1_zkp_verify_obj,
199199
/// def verify_recover(signature: bytes, digest: bytes) -> bytes:
200200
/// '''
201201
/// Uses signature of the digest to verify the digest and recover the public
202-
/// key. Returns public key on success, None on failure.
202+
/// key. Returns public key on success, None if the signature is invalid.
203203
/// '''
204204
STATIC mp_obj_t mod_trezorcrypto_secp256k1_zkp_verify_recover(
205205
mp_obj_t signature, mp_obj_t digest) {
@@ -208,22 +208,22 @@ STATIC mp_obj_t mod_trezorcrypto_secp256k1_zkp_verify_recover(
208208
mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
209209
mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
210210
if (sig.len != 65) {
211-
mp_raise_ValueError("Invalid length of signature");
211+
return mp_const_none;
212212
}
213213
if (dig.len != 32) {
214-
mp_raise_ValueError("Invalid length of digest");
214+
return mp_const_none;
215215
}
216216
int recid = ((const uint8_t *)sig.buf)[0] - 27;
217217
if (recid >= 8) {
218-
mp_raise_ValueError("Invalid recid in signature");
218+
return mp_const_none;
219219
}
220220
bool compressed = (recid >= 4);
221221
recid &= 3;
222222

223223
secp256k1_ecdsa_recoverable_signature ec_sig;
224224
if (!secp256k1_ecdsa_recoverable_signature_parse_compact(
225225
ctx, &ec_sig, (const uint8_t *)sig.buf + 1, recid)) {
226-
mp_raise_ValueError("Invalid signature");
226+
return mp_const_none;
227227
}
228228
secp256k1_pubkey pk;
229229
if (!secp256k1_ecdsa_recover(ctx, &pk, &ec_sig, (const uint8_t *)dig.buf)) {

‎src/apps/ethereum/verify_message.py‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,7 @@ async def verify_message(ctx, msg):
1717
raise wire.DataError("Invalid signature")
1818
sig = bytearray([msg.signature[64]]) + msg.signature[:64]
1919

20-
try:
21-
pubkey = secp256k1.verify_recover(sig, digest)
22-
except ValueError:
23-
raise wire.DataError("Invalid signature")
20+
pubkey = secp256k1.verify_recover(sig, digest)
2421

2522
if not pubkey:
2623
raise wire.DataError("Invalid signature")

0 commit comments

Comments
 (0)