Merge pull request #564 from Foundation-Devices/jeandudey/sft-4310-re…

…move-panics-from-rust-code

SFT-4310: Remove panics.

extmod/foundation-rust/include/foundation.h

@@ -497,7 +497,7 @@ void foundation_firmware_verify_update_signatures(const uint8_t *header,
  *
  * - https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#user-content-Public_Key_Conversion
  */
-void foundation_secp256k1_public_key_schnorr(const uint8_t (*secret_key)[32],
+bool foundation_secp256k1_public_key_schnorr(const uint8_t (*secret_key)[32],
                                              uint8_t (*public_key)[32]);
 
 /**
@@ -507,7 +507,7 @@ void foundation_secp256k1_public_key_schnorr(const uint8_t (*secret_key)[32],
  * - `secret_key` is the secret key used to sign the message.
  * - `signature` is the output of the resulting signature.
  */
-void foundation_secp256k1_sign_ecdsa(const uint8_t (*data)[32],
+bool foundation_secp256k1_sign_ecdsa(const uint8_t (*data)[32],
                                      const uint8_t (*secret_key)[32],
                                      uint8_t (*signature)[64]);
 
@@ -518,7 +518,7 @@ void foundation_secp256k1_sign_ecdsa(const uint8_t (*data)[32],
  * - `secret_key` is the secret key used to sign the message.
  * - `signature` is the output of the resulting signature.
  */
-void foundation_secp256k1_sign_schnorr(const uint8_t (*data)[32],
+bool foundation_secp256k1_sign_schnorr(const uint8_t (*data)[32],
                                        const uint8_t (*secret_key)[32],
                                        uint8_t (*signature)[64]);
 
@@ -597,7 +597,7 @@ bool ur_decode_single_part(const uint8_t *ur,
  * This function assumes that is called on the same thread and its not used
  * concurrently.
  */
-void ur_encoder_start(UR_Encoder *encoder,
+bool ur_encoder_start(UR_Encoder *encoder,
                       const UR_Value *value,
                       size_t max_chars);
 

extmod/foundation-rust/src/secp256k1.rs

@@ -34,11 +34,17 @@ pub static PRE_ALLOCATED_CTX: Lazy<Secp256k1<AllPreallocated<'static>>> =
 pub extern "C" fn secp256k1_public_key_schnorr(
     secret_key: &[u8; 32],
     public_key: &mut [u8; 32],
-) {
-    let keypair = Keypair::from_seckey_slice(&PRE_ALLOCATED_CTX, secret_key)
-        .expect("invalid secret key");
+) -> bool {
+    let keypair =
+        match Keypair::from_seckey_slice(&PRE_ALLOCATED_CTX, secret_key) {
+            Ok(v) => v,
+            Err(_) => return false,
+        };
+
     let compressed_key = keypair.public_key().serialize();
     public_key.copy_from_slice(&compressed_key[1..]);
+
+    true
 }
 
 /// Computes a ECDSA signature over the message `data`.
@@ -51,13 +57,17 @@ pub extern "C" fn secp256k1_sign_ecdsa(
     data: &[u8; 32],
     secret_key: &[u8; 32],
     signature: &mut [u8; 64],
-) {
-    let secret_key =
-        SecretKey::from_slice(secret_key).expect("invalid secret key");
+) -> bool {
+    let secret_key = match SecretKey::from_slice(secret_key) {
+        Ok(v) => v,
+        Err(_) => return false,
+    };
 
     let msg = Message::from_digest_slice(data).unwrap();
     let sig = PRE_ALLOCATED_CTX.sign_ecdsa(&msg, &secret_key);
     signature.copy_from_slice(&sig.serialize_compact());
+
+    true
 }
 
 /// Computes a Schnorr signature over the message `data`.
@@ -70,14 +80,19 @@ pub extern "C" fn secp256k1_sign_schnorr(
     data: &[u8; 32],
     secret_key: &[u8; 32],
     signature: &mut [u8; 64],
-) {
-    let keypair = Keypair::from_seckey_slice(&PRE_ALLOCATED_CTX, secret_key)
-        .expect("invalid secret key");
+) -> bool {
+    let keypair =
+        match Keypair::from_seckey_slice(&PRE_ALLOCATED_CTX, secret_key) {
+            Ok(v) => v,
+            Err(_) => return false,
+        };
 
     let msg = Message::from_digest_slice(data).unwrap();
     let sig =
         PRE_ALLOCATED_CTX.sign_schnorr_with_rng(&msg, &keypair, &mut rng());
     signature.copy_from_slice(sig.as_ref());
+
+    true
 }
 
 #[cfg(target_arch = "arm")]

extmod/foundation-rust/src/ur/decoder.rs

@@ -179,15 +179,19 @@ pub extern "C" fn ur_decoder_decode_message(
         }
     };
 
-    let result = unsafe {
-        UR_Value::from_ur(
-            decoder
-                .inner
-                .ur_type()
-                .expect("decoder should already contain the UR type"),
-            message,
-        )
+    // NOTE: This should be unreachable because after
+    // decoder.inner.message() == Ok(_) the ur_type should
+    // exist.
+    let ur_type = match decoder.inner.ur_type() {
+        Some(v) => v,
+        None => {
+            *error = unsafe { UR_Error::other(&"UR type is not set") };
+            return false;
+        }
     };
+
+    let result = unsafe { UR_Value::from_ur(ur_type, message) };
+
     *value = match result {
         Ok(v) => v,
         Err(e) => {
@@ -209,9 +213,12 @@ pub unsafe extern "C" fn ur_validate(ur: *const u8, ur_len: usize) -> bool {
     // SAFETY: ur and ur_len are assumed to be valid.
     let ur = unsafe { slice::from_raw_parts(ur, ur_len) };
     if let Ok(Ok(ur)) = str::from_utf8(ur).map(UR::parse) {
-        let bytewords = ur
-            .as_bytewords()
-            .expect("Parsed URs shouldn't be in a deserialized variant");
+        // NOTE: UR::parse will never return a deserialized variant when parsing
+        // a UR string.
+        let bytewords = match ur.as_bytewords() {
+            Some(v) => v,
+            None => return false,
+        };
 
         return bytewords::validate(bytewords, Style::Minimal).is_ok();
     }
@@ -249,17 +256,28 @@ pub unsafe extern "C" fn ur_decode_single_part(
     let message =
         unsafe { &mut *ptr::addr_of_mut!(UR_DECODER_SINGLE_PART_MESSAGE) };
     message.clear();
-    message
-        .resize(UR_DECODER_MAX_SINGLE_PART_MESSAGE_LEN, 0)
-        .expect("Message buffer should have the same size as in the constant");
-
-    let result = bytewords::decode_to_slice(
-        ur.as_bytewords()
-            .expect("Uniform Resource should contain bytewords"),
-        message,
-        Style::Minimal,
-    )
-    .map_err(|e| unsafe { UR_Error::other(&e) });
+
+    // NOTE: This should never happen as we are resizing to it's capacity
+    // defined as a constant.
+    if let Err(_) = message.resize(UR_DECODER_MAX_SINGLE_PART_MESSAGE_LEN, 0) {
+        *error = unsafe { UR_Error::other(&"Could not resize message buffer") };
+        return false;
+    }
+
+    // NOTE: This should never happen as when a UR is parsed it should always
+    // contain the bytewords.
+    let bytewords = match ur.as_bytewords() {
+        Some(v) => v,
+        None => {
+            *error = unsafe {
+                UR_Error::other(&"Could not retrieve bytewords from UR")
+            };
+            return false;
+        }
+    };
+
+    let result = bytewords::decode_to_slice(bytewords, message, Style::Minimal)
+        .map_err(|e| unsafe { UR_Error::other(&e) });
 
     match result {
         Ok(len) => message.truncate(len),

extmod/foundation-rust/src/ur/encoder.rs

@@ -98,7 +98,7 @@ pub unsafe extern "C" fn ur_encoder_start(
     encoder: &mut UR_Encoder,
     value: &UR_Value,
     max_chars: usize,
-) {
+) -> bool {
     // SAFETY: The UR_Value can contain some raw pointers which need to be
     // accessed in order to convert it to a `ur::registry::BaseValue` which
     // is then encoded below, so the pointers lifetime only need to be valid
@@ -110,13 +110,17 @@ pub unsafe extern "C" fn ur_encoder_start(
 
     message.clear();
     let mut e = Encoder::new(Writer(message));
-    value.encode(&mut e, &mut ()).expect("Couldn't encode UR");
+    if let Err(_) = value.encode(&mut e, &mut ()) {
+        return false;
+    }
 
     encoder.inner.start(
         value.ur_type(),
         message,
         max_fragment_len(UR_MAX_TYPE, usize::MAX, max_chars),
     );
+
+    true
 }
 
 /// Returns the UR corresponding to the next fountain encoded part.

extmod/foundation-rust/src/ur/mod.rs

@@ -48,8 +48,8 @@ impl UR_Error {
         error.clear();
 
         if write!(error, "{}", message).is_err() {
-            write!(error, "Error is too long to display.")
-                .expect("This error string should fit");
+            // NOTE: Make sure UR_ERROR always can fit this string.
+            write!(error, "Error is too long to display.").ok();
         }
 
         Self {

extmod/foundation-rust/src/ur/registry.rs

@@ -90,6 +90,8 @@ impl UR_Value {
                 Value::Psbt(buf)
             }
             UR_Value::HDKey(v) => Value::HDKey(v.into()),
+            // NOTE: This is unreachable because the firmware should never
+            // create this value as this is only created by Envoy.
             UR_Value::PassportRequest(_) => panic!(
                 "Not implemented as it isn't needed. Should be unreachable"
             ),

extmod/foundation/modfoundation-secp56k1.h

@@ -14,8 +14,8 @@ STATIC mp_obj_t mod_foundation_secp256k1_public_key_schnorr(mp_obj_t secret_key_
         mp_raise_msg(&mp_type_ValueError, MP_ERROR_TEXT("secret key should be 32 bytes"));
     }
 
-    foundation_secp256k1_public_key_schnorr(secret_key.buf, &public_key);
-    return mp_obj_new_bytes(public_key, 32);
+    bool ret = foundation_secp256k1_public_key_schnorr(secret_key.buf, &public_key);
+    return ret ? mp_obj_new_bytes(public_key, 32) : mp_const_none;
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_1(mod_foundation_secp256k1_public_key_schnorr_obj,
                                  mod_foundation_secp256k1_public_key_schnorr);
@@ -39,11 +39,11 @@ STATIC mp_obj_t mod_foundation_secp256k1_sign_ecdsa(mp_obj_t data_obj,
         mp_raise_msg(&mp_type_ValueError, MP_ERROR_TEXT("secret key should be 32 bytes"));
     }
 
-    foundation_secp256k1_sign_ecdsa(data.buf,
-                                    secret_key.buf,
-                                    &signature);
+    bool ret = foundation_secp256k1_sign_ecdsa(data.buf,
+                                               secret_key.buf,
+                                               &signature);
 
-    return mp_obj_new_bytes(signature, sizeof(signature));
+    return ret ? mp_obj_new_bytes(signature, sizeof(signature)) : mp_const_none;
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_2(mod_foundation_secp256k1_sign_ecdsa_obj,
                                  mod_foundation_secp256k1_sign_ecdsa);
@@ -69,11 +69,11 @@ STATIC mp_obj_t mod_foundation_secp256k1_sign_schnorr(mp_obj_t data_obj,
         mp_raise_msg(&mp_type_ValueError, MP_ERROR_TEXT("secret key should be 32 bytes"));
     }
 
-    foundation_secp256k1_sign_schnorr(data.buf,
-                                      secret_key.buf,
-                                      &signature);
+    bool ret = foundation_secp256k1_sign_schnorr(data.buf,
+                                                 secret_key.buf,
+                                                 &signature);
 
-    return mp_obj_new_bytes(signature, sizeof(signature));
+    return ret ? mp_obj_new_bytes(signature, sizeof(signature)) : mp_const_none;
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_2(mod_foundation_secp256k1_sign_schnorr_obj,
                                  mod_foundation_secp256k1_sign_schnorr);

extmod/foundation/modfoundation-ur.h

@@ -526,9 +526,9 @@ STATIC mp_obj_t mod_foundation_ur_encoder_start(mp_obj_t value_in,
 
     value = MP_OBJ_TO_PTR(value_in);
     max_fragment_len = mp_obj_int_get_uint_checked(max_fragment_len_in);
-    ur_encoder_start(&UR_ENCODER, &value->value, max_fragment_len);
+    bool ret = ur_encoder_start(&UR_ENCODER, &value->value, max_fragment_len);
 
-    return mp_const_none;
+    return ret ? mp_const_true : mp_const_false;
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_2(mod_foundation_ur_encoder_start_obj,
                                  mod_foundation_ur_encoder_start);

ports/stm32/boards/Passport/modules/data_codecs/ur2_codec.py

@@ -79,7 +79,8 @@ class UR2Encoder(DataEncoder):
     def encode(self, value, max_fragment_len=200):
         """Initialize the encoder using the given UR data value"""
 
-        ur.encoder_start(value, max_fragment_len)
+        if not ur.encoder_start(value, max_fragment_len):
+            raise Exception('Failed to encode UR')
 
     def next_part(self):
         """Returns the next part of the UR encoder"""