Skip to content

Commit

Permalink
SFT-4310: Remove panics.
Browse files Browse the repository at this point in the history
  • Loading branch information
jeandudey committed Oct 17, 2024
1 parent 457e740 commit 0378491
Show file tree
Hide file tree
Showing 9 changed files with 92 additions and 52 deletions.
8 changes: 4 additions & 4 deletions extmod/foundation-rust/include/foundation.h
Original file line number Diff line number Diff line change
Expand Up @@ -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]);

/**
Expand All @@ -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]);

Expand All @@ -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]);

Expand Down Expand Up @@ -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);

Expand Down
33 changes: 24 additions & 9 deletions extmod/foundation-rust/src/secp256k1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand All @@ -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`.
Expand All @@ -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")]
Expand Down
62 changes: 40 additions & 22 deletions extmod/foundation-rust/src/ur/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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();
}
Expand Down Expand Up @@ -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),
Expand Down
8 changes: 6 additions & 2 deletions extmod/foundation-rust/src/ur/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions extmod/foundation-rust/src/ur/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions extmod/foundation-rust/src/ur/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
),
Expand Down
20 changes: 10 additions & 10 deletions extmod/foundation/modfoundation-secp56k1.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions extmod/foundation/modfoundation-ur.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion ports/stm32/boards/Passport/modules/data_codecs/ur2_codec.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"""
Expand Down

0 comments on commit 0378491

Please sign in to comment.