From 0378491e6f9dbfc4fdaafe4a945a0cb82df35c51 Mon Sep 17 00:00:00 2001 From: Jean-Pierre De Jesus DIAZ Date: Thu, 17 Oct 2024 16:05:29 +0200 Subject: [PATCH] SFT-4310: Remove panics. --- extmod/foundation-rust/include/foundation.h | 8 +-- extmod/foundation-rust/src/secp256k1.rs | 33 +++++++--- extmod/foundation-rust/src/ur/decoder.rs | 62 ++++++++++++------- extmod/foundation-rust/src/ur/encoder.rs | 8 ++- extmod/foundation-rust/src/ur/mod.rs | 4 +- extmod/foundation-rust/src/ur/registry.rs | 2 + extmod/foundation/modfoundation-secp56k1.h | 20 +++--- extmod/foundation/modfoundation-ur.h | 4 +- .../Passport/modules/data_codecs/ur2_codec.py | 3 +- 9 files changed, 92 insertions(+), 52 deletions(-) diff --git a/extmod/foundation-rust/include/foundation.h b/extmod/foundation-rust/include/foundation.h index 4f71e66f8..3432afdf3 100644 --- a/extmod/foundation-rust/include/foundation.h +++ b/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); diff --git a/extmod/foundation-rust/src/secp256k1.rs b/extmod/foundation-rust/src/secp256k1.rs index c3ec7b387..09bea5291 100644 --- a/extmod/foundation-rust/src/secp256k1.rs +++ b/extmod/foundation-rust/src/secp256k1.rs @@ -34,11 +34,17 @@ pub static PRE_ALLOCATED_CTX: Lazy>> = 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")] diff --git a/extmod/foundation-rust/src/ur/decoder.rs b/extmod/foundation-rust/src/ur/decoder.rs index 1b643bb8d..3d65b44ae 100644 --- a/extmod/foundation-rust/src/ur/decoder.rs +++ b/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), diff --git a/extmod/foundation-rust/src/ur/encoder.rs b/extmod/foundation-rust/src/ur/encoder.rs index e1974627f..ca02b83b9 100644 --- a/extmod/foundation-rust/src/ur/encoder.rs +++ b/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. diff --git a/extmod/foundation-rust/src/ur/mod.rs b/extmod/foundation-rust/src/ur/mod.rs index fb5170516..49e79ebb3 100644 --- a/extmod/foundation-rust/src/ur/mod.rs +++ b/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 { diff --git a/extmod/foundation-rust/src/ur/registry.rs b/extmod/foundation-rust/src/ur/registry.rs index c40d56636..7e98f2565 100644 --- a/extmod/foundation-rust/src/ur/registry.rs +++ b/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" ), diff --git a/extmod/foundation/modfoundation-secp56k1.h b/extmod/foundation/modfoundation-secp56k1.h index 0f22a1506..63126c7fa 100644 --- a/extmod/foundation/modfoundation-secp56k1.h +++ b/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); diff --git a/extmod/foundation/modfoundation-ur.h b/extmod/foundation/modfoundation-ur.h index f17e2c32f..e3389a9f6 100644 --- a/extmod/foundation/modfoundation-ur.h +++ b/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); diff --git a/ports/stm32/boards/Passport/modules/data_codecs/ur2_codec.py b/ports/stm32/boards/Passport/modules/data_codecs/ur2_codec.py index 9750f8e12..636f30d2d 100644 --- a/ports/stm32/boards/Passport/modules/data_codecs/ur2_codec.py +++ b/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"""