extmod/modtrezorcrypto: return False or None consistently when a sign…

…ature 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.

embed/extmod/modtrezorcrypto/modtrezorcrypto-ed25519.h

@@ -147,13 +147,13 @@ STATIC mp_obj_t mod_trezorcrypto_ed25519_verify(mp_obj_t public_key,
   mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
   mp_get_buffer_raise(message, &msg, MP_BUFFER_READ);
   if (pk.len != 32) {
-    mp_raise_ValueError("Invalid length of public key");
+    return mp_const_false;
   }
   if (sig.len != 64) {
-    mp_raise_ValueError("Invalid length of signature");
+    return mp_const_false;
   }
   if (msg.len == 0) {
-    mp_raise_ValueError("Empty data to verify");
+    return mp_const_false;
   }
   return (0 == ed25519_sign_open(msg.buf, msg.len,
                                  *(const ed25519_public_key *)pk.buf,

embed/extmod/modtrezorcrypto/modtrezorcrypto-nist256p1.h

@@ -122,14 +122,14 @@ STATIC mp_obj_t mod_trezorcrypto_nist256p1_verify(mp_obj_t public_key,
   mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
   mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
   if (pk.len != 33 && pk.len != 65) {
-    mp_raise_ValueError("Invalid length of public key");
+    return mp_const_false;
   }
   if (sig.len != 64 && sig.len != 65) {
-    mp_raise_ValueError("Invalid length of signature");
+    return mp_const_false;
   }
   int offset = sig.len - 64;
   if (dig.len != 32) {
-    mp_raise_ValueError("Invalid length of digest");
+    return mp_const_false;
   }
   return mp_obj_new_bool(
       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,
 /// def verify_recover(signature: bytes, digest: bytes) -> bytes:
 ///     '''
 ///     Uses signature of the digest to verify the digest and recover the public
-///     key. Returns public key on success, None on failure.
+///     key. Returns public key on success, None if the signature is invalid.
 ///     '''
 STATIC mp_obj_t mod_trezorcrypto_nist256p1_verify_recover(mp_obj_t signature,
                                                           mp_obj_t digest) {
   mp_buffer_info_t sig, dig;
   mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
   mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
   if (sig.len != 65) {
-    mp_raise_ValueError("Invalid length of signature");
+    return mp_const_none;
   }
   if (dig.len != 32) {
-    mp_raise_ValueError("Invalid length of digest");
+    return mp_const_none;
   }
   uint8_t recid = ((const uint8_t *)sig.buf)[0] - 27;
   if (recid >= 8) {
-    mp_raise_ValueError("Invalid recid in signature");
+    return mp_const_none;
   }
   bool compressed = (recid >= 4);
   recid &= 3;

embed/extmod/modtrezorcrypto/modtrezorcrypto-secp256k1.h

@@ -139,14 +139,14 @@ STATIC mp_obj_t mod_trezorcrypto_secp256k1_verify(mp_obj_t public_key,
   mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
   mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
   if (pk.len != 33 && pk.len != 65) {
-    mp_raise_ValueError("Invalid length of public key");
+    return mp_const_false;
   }
   if (sig.len != 64 && sig.len != 65) {
-    mp_raise_ValueError("Invalid length of signature");
+    return mp_const_false;
   }
   int offset = sig.len - 64;
   if (dig.len != 32) {
-    mp_raise_ValueError("Invalid length of digest");
+    return mp_const_false;
   }
   return mp_obj_new_bool(
       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,
 /// def verify_recover(signature: bytes, digest: bytes) -> bytes:
 ///     '''
 ///     Uses signature of the digest to verify the digest and recover the public
-///     key. Returns public key on success, None on failure.
+///     key. Returns public key on success, None if the signature is invalid.
 ///     '''
 STATIC mp_obj_t mod_trezorcrypto_secp256k1_verify_recover(mp_obj_t signature,
                                                           mp_obj_t digest) {
   mp_buffer_info_t sig, dig;
   mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
   mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
   if (sig.len != 65) {
-    mp_raise_ValueError("Invalid length of signature");
+    return mp_const_none;
   }
   if (dig.len != 32) {
-    mp_raise_ValueError("Invalid length of digest");
+    return mp_const_none;
   }
   uint8_t recid = ((const uint8_t *)sig.buf)[0] - 27;
   if (recid >= 8) {
-    mp_raise_ValueError("Invalid recid in signature");
+    return mp_const_none;
   }
   bool compressed = (recid >= 4);
   recid &= 3;

embed/extmod/modtrezorcrypto/modtrezorcrypto-secp256k1_zkp.h

@@ -170,24 +170,24 @@ STATIC mp_obj_t mod_trezorcrypto_secp256k1_zkp_verify(mp_obj_t public_key,
   mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
   mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
   if (pk.len != 33 && pk.len != 65) {
-    mp_raise_ValueError("Invalid length of public key");
+    return mp_const_false;
   }
   if (sig.len != 64 && sig.len != 65) {
-    mp_raise_ValueError("Invalid length of signature");
+    return mp_const_false;
   }
   int offset = sig.len - 64;
   if (dig.len != 32) {
-    mp_raise_ValueError("Invalid length of digest");
+    return mp_const_false;
   }
   secp256k1_ecdsa_signature ec_sig;
   if (!secp256k1_ecdsa_signature_parse_compact(
           ctx, &ec_sig, (const uint8_t *)sig.buf + offset)) {
-    mp_raise_ValueError("Invalid signature");
+    return mp_const_false;
   }
   secp256k1_pubkey ec_pk;
   if (!secp256k1_ec_pubkey_parse(ctx, &ec_pk, (const uint8_t *)pk.buf,
                                  pk.len)) {
-    mp_raise_ValueError("Invalid public key");
+    return mp_obj_new_bool(0);
   }
   return mp_obj_new_bool(1 == secp256k1_ecdsa_verify(ctx, &ec_sig,
                                                      (const uint8_t *)dig.buf,
@@ -199,7 +199,7 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_3(mod_trezorcrypto_secp256k1_zkp_verify_obj,
 /// def verify_recover(signature: bytes, digest: bytes) -> bytes:
 ///     '''
 ///     Uses signature of the digest to verify the digest and recover the public
-///     key. Returns public key on success, None on failure.
+///     key. Returns public key on success, None if the signature is invalid.
 ///     '''
 STATIC mp_obj_t mod_trezorcrypto_secp256k1_zkp_verify_recover(
     mp_obj_t signature, mp_obj_t digest) {
@@ -208,22 +208,22 @@ STATIC mp_obj_t mod_trezorcrypto_secp256k1_zkp_verify_recover(
   mp_get_buffer_raise(signature, &sig, MP_BUFFER_READ);
   mp_get_buffer_raise(digest, &dig, MP_BUFFER_READ);
   if (sig.len != 65) {
-    mp_raise_ValueError("Invalid length of signature");
+    return mp_const_none;
   }
   if (dig.len != 32) {
-    mp_raise_ValueError("Invalid length of digest");
+    return mp_const_none;
   }
   int recid = ((const uint8_t *)sig.buf)[0] - 27;
   if (recid >= 8) {
-    mp_raise_ValueError("Invalid recid in signature");
+    return mp_const_none;
   }
   bool compressed = (recid >= 4);
   recid &= 3;
 
   secp256k1_ecdsa_recoverable_signature ec_sig;
   if (!secp256k1_ecdsa_recoverable_signature_parse_compact(
           ctx, &ec_sig, (const uint8_t *)sig.buf + 1, recid)) {
-    mp_raise_ValueError("Invalid signature");
+    return mp_const_none;
   }
   secp256k1_pubkey pk;
   if (!secp256k1_ecdsa_recover(ctx, &pk, &ec_sig, (const uint8_t *)dig.buf)) {

src/apps/ethereum/verify_message.py

@@ -17,10 +17,7 @@ async def verify_message(ctx, msg):
         raise wire.DataError("Invalid signature")
     sig = bytearray([msg.signature[64]]) + msg.signature[:64]
 
-    try:
-        pubkey = secp256k1.verify_recover(sig, digest)
-    except ValueError:
-        raise wire.DataError("Invalid signature")
+    pubkey = secp256k1.verify_recover(sig, digest)
 
     if not pubkey:
         raise wire.DataError("Invalid signature")