From 34621a6b6d340a42e917cad346ec527c94066712 Mon Sep 17 00:00:00 2001
From: Andrew Kozlik <andrew.kozlik@gmail.com>
Date: Fri, 9 Jul 2021 10:30:47 +0200
Subject: [PATCH] fix(crypto,core,legacy): Check private key validity when
 deriving public key.

[no changelog]
---
 .../modtrezorcrypto-nist256p1.h               | 12 +++++++----
 crypto/ecdsa.c                                | 21 +++++++++++++++++--
 crypto/schnorr.c                              |  4 +++-
 crypto/tests/test_check.c                     |  2 +-
 crypto/tests/test_openssl.c                   | 11 ++++++++--
 legacy/firmware/u2f.c                         |  7 +++++--
 6 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-nist256p1.h b/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-nist256p1.h
index 09bfe214c..ef5877ad8 100644
--- a/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-nist256p1.h
+++ b/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-nist256p1.h
@@ -70,12 +70,16 @@ STATIC mp_obj_t mod_trezorcrypto_nist256p1_publickey(size_t n_args,
   bool compressed = n_args < 2 || args[1] == mp_const_true;
   if (compressed) {
     vstr_init_len(&pk, 33);
-    ecdsa_get_public_key33(&nist256p1, (const uint8_t *)sk.buf,
-                           (uint8_t *)pk.buf);
+    if (ecdsa_get_public_key33(&nist256p1, (const uint8_t *)sk.buf,
+                               (uint8_t *)pk.buf) != 0) {
+      mp_raise_ValueError("Invalid secret key");
+    }
   } else {
     vstr_init_len(&pk, 65);
-    ecdsa_get_public_key65(&nist256p1, (const uint8_t *)sk.buf,
-                           (uint8_t *)pk.buf);
+    if (ecdsa_get_public_key65(&nist256p1, (const uint8_t *)sk.buf,
+                               (uint8_t *)pk.buf) != 0) {
+      mp_raise_ValueError("Invalid secret key");
+    }
   }
   return mp_obj_new_str_from_vstr(&mp_type_bytes, &pk);
 }
diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
index 114f34a21..d5d8bd8aa 100644
--- a/crypto/ecdsa.c
+++ b/crypto/ecdsa.c
@@ -641,6 +641,11 @@ int ecdh_multiply(const ecdsa_curve *curve, const uint8_t *priv_key,
 
   bignum256 k = {0};
   bn_read_be(priv_key, &k);
+  if (bn_is_zero(&k) || !bn_is_less(&k, &curve->order)) {
+    // Invalid private key.
+    return 2;
+  }
+
   point_multiply(curve, &k, &point, &point);
   memzero(&k, sizeof(k));
 
@@ -721,8 +726,8 @@ int ecdsa_sign_digest(const ecdsa_curve *curve, const uint8_t *priv_key,
     }
 
     bn_read_be(priv_key, s);
-    if (bn_is_zero(s)) {
-      // Using an all-zero private key is most likely an indication of a bug.
+    if (bn_is_zero(s) || !bn_is_less(s, &curve->order)) {
+      // Invalid private key.
       return 2;
     }
 
@@ -783,6 +788,12 @@ int ecdsa_get_public_key33(const ecdsa_curve *curve, const uint8_t *priv_key,
   bignum256 k = {0};
 
   bn_read_be(priv_key, &k);
+  if (bn_is_zero(&k) || !bn_is_less(&k, &curve->order)) {
+    // Invalid private key.
+    memzero(pub_key, 33);
+    return -1;
+  }
+
   // compute k*G
   if (scalar_multiply(curve, &k, &R) != 0) {
     memzero(&k, sizeof(k));
@@ -802,6 +813,12 @@ int ecdsa_get_public_key65(const ecdsa_curve *curve, const uint8_t *priv_key,
   bignum256 k = {0};
 
   bn_read_be(priv_key, &k);
+  if (bn_is_zero(&k) || !bn_is_less(&k, &curve->order)) {
+    // Invalid private key.
+    memzero(pub_key, 65);
+    return -1;
+  }
+
   // compute k*G
   if (scalar_multiply(curve, &k, &R) != 0) {
     memzero(&k, sizeof(k));
diff --git a/crypto/schnorr.c b/crypto/schnorr.c
index 54fd7157b..ac704fba7 100644
--- a/crypto/schnorr.c
+++ b/crypto/schnorr.c
@@ -135,7 +135,9 @@ int schnorr_sign_digest(const ecdsa_curve *curve, const uint8_t *priv_key,
   curve_point R = {0};
   bignum256 e = {0}, s = {0}, k = {0};
 
-  ecdsa_get_public_key33(curve, priv_key, pub_key);
+  if (ecdsa_get_public_key33(curve, priv_key, pub_key) != 0) {
+    return 1;
+  }
 
   // Compute k
   if (generate_k_schnorr(curve, priv_key, digest, &k) != 0) {
diff --git a/crypto/tests/test_check.c b/crypto/tests/test_check.c
index 4ce746653..ef3ff24c9 100644
--- a/crypto/tests/test_check.c
+++ b/crypto/tests/test_check.c
@@ -8959,7 +8959,7 @@ START_TEST(test_schnorr_sign_verify_digest) {
     memcpy(priv_key, fromhex(tests[i].priv_key), 32);
     memcpy(expected, fromhex(tests[i].sig), SCHNORR_SIG_LENGTH);
 
-    ecdsa_get_public_key33(curve, priv_key, pub_key);
+    ck_assert_int_eq(ecdsa_get_public_key33(curve, priv_key, pub_key), 0);
 
     schnorr_sign_digest(curve, priv_key, digest, result);
 
diff --git a/crypto/tests/test_openssl.c b/crypto/tests/test_openssl.c
index c6ce47894..3095d96d0 100644
--- a/crypto/tests/test_openssl.c
+++ b/crypto/tests/test_openssl.c
@@ -79,8 +79,15 @@ void openssl_check(unsigned int iterations, int nid, const ecdsa_curve *curve) {
     }
 
     // generate public key from private key
-    ecdsa_get_public_key33(curve, priv_key, pub_key33);
-    ecdsa_get_public_key65(curve, priv_key, pub_key65);
+    if (ecdsa_get_public_key33(curve, priv_key, pub_key33) != 0) {
+      printf("ecdsa_get_public_key33 failed\n");
+      return;
+    }
+
+    if (ecdsa_get_public_key65(curve, priv_key, pub_key65) != 0) {
+      printf("ecdsa_get_public_key65 failed\n");
+      return;
+    }
 
     // use our ECDSA verifier to verify the message signature
     if (ecdsa_verify(curve, HASHER_SHA2, pub_key65, sig, msg, msg_len) != 0) {
diff --git a/legacy/firmware/u2f.c b/legacy/firmware/u2f.c
index cd15213cb..1dbfb7a17 100644
--- a/legacy/firmware/u2f.c
+++ b/legacy/firmware/u2f.c
@@ -604,8 +604,11 @@ void u2f_register(const APDU *a) {
       return;
     }
 
-    ecdsa_get_public_key65(node->curve->params, node->private_key,
-                           (uint8_t *)&resp->pubKey);
+    if (ecdsa_get_public_key65(node->curve->params, node->private_key,
+                               (uint8_t *)&resp->pubKey) != 0) {
+      send_u2f_error(U2F_SW_WRONG_DATA);
+      return;
+    }
 
     memcpy(resp->keyHandleCertSig + resp->keyHandleLen, U2F_ATT_CERT,
            sizeof(U2F_ATT_CERT));