From 6d0e8e56f7304afb41591d435c404f7fe3a52ff7 Mon Sep 17 00:00:00 2001 From: Marko Bencun Date: Thu, 9 Sep 2021 01:17:02 +0200 Subject: [PATCH] rust: add better error messages The protobuf Error response has a message field, but it is hardcoded to generic strings depending on the error type, e.g. "generic", "invalid input", etc. When a user/developer reports this error, better information is useful. This patch adds error messages based on the error condition. Unfortunately, for now we still have to stick to static strings with no runtime information, e.g. "invalid keypath" over "keypath invalid: {}" cotaining the actual keypath. I experimented with dynamic strings, but this immediatelly added another ~7kB in binary bloat due the usage of String and formatting. Only changing the type from `&'static str` to `String` adds a few kB. Alternatives considered: - use String+format!() to be able to return runtime info (e.g. actual keypaths used), as well as chain context strings together, but that resulted in too many kilobytes of additional binary bloat. - error deps like anyhow, snafu, etc. They all add a lot of binary and code bloat. - using only numeric error codes instead of static strings to save binary space. Can still do in the future if needed. - using `ufmt` crate hoping it produces smaller binaries than `format!()` for dynamic strings. I tried it and it was about the same. --- src/rust/bitbox02-rust/src/hww.rs | 13 +- src/rust/bitbox02-rust/src/hww/api.rs | 21 +- src/rust/bitbox02-rust/src/hww/api/backup.rs | 36 ++- src/rust/bitbox02-rust/src/hww/api/bitcoin.rs | 58 +++- .../src/hww/api/bitcoin/signmsg.rs | 88 ++++-- .../bitbox02-rust/src/hww/api/device_info.rs | 11 +- .../bitbox02-rust/src/hww/api/electrum.rs | 32 ++- src/rust/bitbox02-rust/src/hww/api/error.rs | 263 ++++++++++++++++-- .../bitbox02-rust/src/hww/api/ethereum.rs | 20 +- .../src/hww/api/ethereum/keypath.rs | 7 +- .../src/hww/api/ethereum/pubrequest.rs | 87 ++++-- .../src/hww/api/ethereum/sign.rs | 113 +++++--- .../src/hww/api/ethereum/signmsg.rs | 40 ++- src/rust/bitbox02-rust/src/hww/api/reset.rs | 9 +- src/rust/bitbox02-rust/src/hww/api/restore.rs | 19 +- .../src/hww/api/rootfingerprint.rs | 6 +- .../src/hww/api/set_device_name.rs | 41 ++- .../api/set_mnemonic_passphrase_enabled.rs | 13 +- .../bitbox02-rust/src/hww/api/set_password.rs | 17 +- .../src/hww/api/show_mnemonic.rs | 13 +- src/rust/bitbox02-rust/src/hww/api/system.rs | 7 +- 21 files changed, 699 insertions(+), 215 deletions(-) diff --git a/src/rust/bitbox02-rust/src/hww.rs b/src/rust/bitbox02-rust/src/hww.rs index 93a6ae2c2..2fe32beea 100644 --- a/src/rust/bitbox02-rust/src/hww.rs +++ b/src/rust/bitbox02-rust/src/hww.rs @@ -18,6 +18,8 @@ pub mod noise; extern crate alloc; use alloc::vec::Vec; +use api::error::{Error, ErrorKind}; + const OP_UNLOCK: u8 = b'u'; const OP_ATTESTATION: u8 = b'a'; @@ -30,17 +32,20 @@ const OP_STATUS_FAILURE_UNINITIALIZED: u8 = 2; /// message, `Err(Error::InvalidInput)` is returned. pub async fn next_request( response: crate::pb::response::Response, -) -> Result { +) -> Result { let mut out = [OP_STATUS_SUCCESS].to_vec(); - noise::encrypt(&api::encode(response), &mut out).or(Err(api::error::Error::NoiseEncrypt))?; + noise::encrypt(&api::encode(response), &mut out).map_err(Error::err_noise_encrypt)?; let request = crate::async_usb::next_request(out).await; match request.split_first() { Some((&noise::OP_NOISE_MSG, encrypted_request)) => { let decrypted_request = - noise::decrypt(&encrypted_request).or(Err(api::error::Error::NoiseDecrypt))?; + noise::decrypt(&encrypted_request).map_err(Error::err_noise_decrypt)?; api::decode(&decrypted_request[..]) } - _ => Err(api::error::Error::InvalidInput), + _ => Err(Error { + msg: None, + kind: ErrorKind::InvalidInput, + }), } } diff --git a/src/rust/bitbox02-rust/src/hww/api.rs b/src/rust/bitbox02-rust/src/hww/api.rs index 56792bcbc..61ff84410 100644 --- a/src/rust/bitbox02-rust/src/hww/api.rs +++ b/src/rust/bitbox02-rust/src/hww/api.rs @@ -37,7 +37,7 @@ mod system; use alloc::vec::Vec; -use error::{make_error, Error}; +use error::{make_error, Error, ErrorKind}; use pb::request::Request; use pb::response::Response; use prost::Message; @@ -58,7 +58,14 @@ pub fn decode(input: &[u8]) -> Result { Ok(pb::Request { request: Some(request), }) => Ok(request), - _ => Err(Error::InvalidInput), + Ok(pb::Request { request: None }) => Err(Error { + msg: Some("request missing".into()), + kind: ErrorKind::InvalidInput, + }), + Err(_) => Err(Error { + msg: Some("protobuf decode error".into()), + kind: ErrorKind::InvalidInput, + }), } } @@ -110,7 +117,10 @@ async fn process_api_btc(request: &Request) -> Option> { #[cfg(not(any(feature = "app-bitcoin", feature = "app-litecoin")))] async fn process_api_btc(_request: &Request) -> Option> { - Some(Err(Error::Disabled)) + Some(Err(Error { + msg: None, + kind: ErrorKind::Disabled, + })) } /// Handle a protobuf api call. @@ -164,7 +174,10 @@ pub async fn process(input: Vec) -> Vec { Err(err) => return encode(make_error(err)), }; if !bitbox02::commander::states_can_call(request_tag(&request) as u16) { - return encode(make_error(Error::InvalidState)); + return encode(make_error(Error { + msg: None, + kind: ErrorKind::InvalidState, + })); } // Since we will process the call now, so can clear the 'force next' info. diff --git a/src/rust/bitbox02-rust/src/hww/api/backup.rs b/src/rust/bitbox02-rust/src/hww/api/backup.rs index 516ecd119..586da4748 100644 --- a/src/rust/bitbox02-rust/src/hww/api/backup.rs +++ b/src/rust/bitbox02-rust/src/hww/api/backup.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use super::Error; +use super::error::{Context, Error, ErrorKind}; use crate::pb; use pb::response::Response; @@ -24,7 +24,10 @@ pub async fn check( &pb::CheckBackupRequest { silent }: &pb::CheckBackupRequest, ) -> Result { if !bitbox02::sdcard_inserted() { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("sdcard not inserted".into()), + kind: ErrorKind::InvalidInput, + }); } match backup::check() { Ok(backup::CheckData { id, name, .. }) => { @@ -55,12 +58,18 @@ pub async fn check( if !silent { status::status("Backup missing\nor invalid", false).await; } - Err(Error::Generic) + Err(Error { + msg: Some("backup missing or invalid".into()), + kind: ErrorKind::Generic, + }) } Err(err) => { let msg = format!("Could not check\nbackup\n{:?}", err).replace("BACKUP_ERR_", ""); status::status(&msg, false).await; - Err(Error::Generic) + Err(Error { + msg: None, + kind: ErrorKind::Generic, + }) } } } @@ -83,7 +92,10 @@ pub async fn create( const MAX_WEST_UTC_OFFSET: i32 = -43200; // 12 hours in seconds if timezone_offset < MAX_WEST_UTC_OFFSET || timezone_offset > MAX_EAST_UTC_OFFSET { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("invalid timezone_offset".into()), + kind: ErrorKind::InvalidInput, + }); } // Wait for sd card @@ -95,7 +107,10 @@ pub async fn create( let is_initialized = bitbox02::memory::is_initialized(); if is_initialized { - unlock::unlock_keystore("Unlock device", unlock::CanCancel::Yes).await?; + unlock::unlock_keystore("Unlock device", unlock::CanCancel::Yes) + .await + .map_err(Error::err) + .context("unlock_keystore failed")?; } let seed_birthdate = if !is_initialized { @@ -106,9 +121,7 @@ pub async fn create( ..Default::default() }; confirm::confirm(¶ms).await?; - if bitbox02::memory::set_seed_birthdate(timestamp).is_err() { - return Err(Error::Memory); - } + bitbox02::memory::set_seed_birthdate(timestamp).map_err(Error::err_memory)?; timestamp } else if let Ok(backup::CheckData { birthdate, .. }) = backup::check() { // If adding new backup after initialized, we do not know the seed birthdate. @@ -133,7 +146,10 @@ pub async fn create( let msg = format!("Backup not created\nPlease contact\nsupport ({:?})", err) .replace("BACKUP_ERR_", ""); status::status(&msg, false).await; - Err(Error::Generic) + Err(Error { + msg: None, + kind: ErrorKind::Generic, + }) } } } diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs index c8911f698..b27309637 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs @@ -19,8 +19,8 @@ mod params; mod script; pub mod signmsg; +use super::error::{Context, Error, ErrorKind}; use super::pb; -use super::Error; use crate::apps::bitcoin; use crate::workflow::confirm; @@ -48,7 +48,10 @@ pub async fn next_request(response: pb::btc_response::Response) -> Result Ok(request), - _ => Err(Error::InvalidState), + _ => Err(Error { + msg: Some("expected Btc request, got".into()), + kind: ErrorKind::InvalidState, + }), } } @@ -68,9 +71,13 @@ pub async fn antiklepto_get_host_nonce( Ok(host_nonce .as_slice() .try_into() - .or(Err(Error::InvalidInput))?) + .map_err(Error::err_invalid_input) + .context("could not parse host nonce")?) } - _ => Err(Error::InvalidState), + _ => Err(Error { + msg: Some("expected AntikleptoSignature".into()), + kind: ErrorKind::InvalidState, + }), } } @@ -85,7 +92,10 @@ fn coin_enabled(coin: pb::BtcCoin) -> Result<(), Error> { if let Ltc | Tltc = coin { return Ok(()); } - Err(Error::Disabled) + Err(Error { + msg: None, + kind: ErrorKind::Disabled, + }) } /// Processes an xpub api call. @@ -96,7 +106,9 @@ async fn xpub( display: bool, ) -> Result { let params = params::get(coin); - bitcoin::keypath::validate_xpub(keypath, params.bip44_coin)?; + bitcoin::keypath::validate_xpub(keypath, params.bip44_coin) + .map_err(Error::err_generic) + .context("invalid keypath")?; let xpub_type = match xpub_type { XPubType::Tpub => xpub_type_t::TPUB, XPubType::Xpub => xpub_type_t::XPUB, @@ -109,7 +121,9 @@ async fn xpub( XPubType::CapitalUpub => xpub_type_t::CAPITAL_UPUB, XPubType::CapitalYpub => xpub_type_t::CAPITAL_YPUB, }; - let xpub = encode_xpub_at_keypath(keypath, xpub_type).or(Err(Error::InvalidInput))?; + let xpub = encode_xpub_at_keypath(keypath, xpub_type) + .map_err(Error::err_invalid_input) + .context("encode_xpub_at_keypath failed")?; if display { let title = format!("{}\naccount #{}", params.name, keypath[2] - HARDENED + 1,); let confirm_params = confirm::Params { @@ -130,7 +144,9 @@ async fn address_simple( keypath: &[u32], display: bool, ) -> Result { - let address = bitbox02::app_btc::address_simple(coin as _, simple_type as _, keypath)?; + let address = bitbox02::app_btc::address_simple(coin as _, simple_type as _, keypath) + .map_err(Error::err) + .context("address_simple failed")?; if display { let confirm_params = confirm::Params { title: params::get(coin).name, @@ -150,17 +166,30 @@ async fn address_simple( pub async fn process_pub(request: &pb::BtcPubRequest) -> Option> { let coin = match BtcCoin::from_i32(request.coin) { Some(coin) => coin, - None => return Some(Err(Error::InvalidInput)), + None => { + return Some(Err(Error { + msg: Some("invalid coin".into()), + kind: ErrorKind::InvalidInput, + })) + } }; if let Err(err) = coin_enabled(coin) { return Some(Err(err)); } match request.output { - None => Some(Err(Error::InvalidInput)), + None => Some(Err(Error { + msg: Some("request.output missing".into()), + kind: ErrorKind::InvalidInput, + })), Some(Output::XpubType(xpub_type)) => { let xpub_type = match XPubType::from_i32(xpub_type) { Some(xpub_type) => xpub_type, - None => return Some(Err(Error::InvalidInput)), + None => { + return Some(Err(Error { + msg: Some("invalid xpub_type".into()), + kind: ErrorKind::InvalidInput, + })) + } }; Some(xpub(coin, xpub_type, &request.keypath, request.display).await) } @@ -169,7 +198,12 @@ pub async fn process_pub(request: &pb::BtcPubRequest) -> Option { let simple_type = match SimpleType::from_i32(simple_type) { Some(simple_type) => simple_type, - None => return Some(Err(Error::InvalidInput)), + None => { + return Some(Err(Error { + msg: Some("invalid simple_type".into()), + kind: ErrorKind::InvalidInput, + })) + } }; Some(address_simple(coin, simple_type, &request.keypath, request.display).await) } diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/signmsg.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/signmsg.rs index 166fc49ab..ce3d46096 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/signmsg.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/signmsg.rs @@ -18,7 +18,6 @@ use core::convert::TryInto; use sha2::{Digest, Sha256}; use super::pb; -use super::Error; use pb::btc_script_config::{Config, SimpleType}; use pb::BtcCoin; @@ -27,6 +26,7 @@ use pb::btc_response::Response; use bitbox02::keystore; +use crate::hww::api::error::{Context, Error, ErrorKind}; use crate::workflow::{confirm, verify_message}; const MAX_MESSAGE_SIZE: usize = 1024; @@ -36,9 +36,15 @@ const MAX_MESSAGE_SIZE: usize = 1024; /// The result contains a 65 byte signature. The first 64 bytes are the secp256k1 signature in /// compact format (R and S values), and the last byte is the recoverable id (recid). pub async fn process(request: &pb::BtcSignMessageRequest) -> Result { - let coin = BtcCoin::from_i32(request.coin).ok_or(Error::InvalidInput)?; + let coin = BtcCoin::from_i32(request.coin).ok_or(Error { + msg: Some("invalid coin".into()), + kind: ErrorKind::InvalidInput, + })?; if coin != BtcCoin::Btc { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("only BTC mainnet supported".into()), + kind: ErrorKind::InvalidInput, + }); } let (keypath, simple_type) = match &request.script_config { Some(pb::BtcScriptConfigWithKeypath { @@ -49,17 +55,29 @@ pub async fn process(request: &pb::BtcSignMessageRequest) -> Result ( keypath, - SimpleType::from_i32(*simple_type).ok_or(Error::InvalidInput)?, + SimpleType::from_i32(*simple_type).ok_or(Error { + msg: Some("invalid simple_type".into()), + kind: ErrorKind::InvalidInput, + })?, ), - _ => return Err(Error::InvalidInput), + _ => { + return Err(Error { + msg: Some("script config not supported".into()), + kind: ErrorKind::InvalidInput, + }) + } }; if request.msg.len() > MAX_MESSAGE_SIZE { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("message too large".into()), + kind: ErrorKind::InvalidInput, + }); } // Keypath and script_config are validated in address_simple(). let address = bitbox02::app_btc::address_simple(coin as _, simple_type as _, keypath) - .or(Err(Error::InvalidInput))?; + .map_err(Error::err_invalid_input) + .context("address_simple failed")?; let basic_info = format!("Coin: {}", super::params::get(coin).name); let confirm_params = confirm::Params { @@ -104,8 +122,11 @@ pub async fn process(request: &pb::BtcSignMessageRequest) -> Result Result [0; 32], }; - let sign_result = bitbox02::keystore::secp256k1_sign(keypath, &sighash, &host_nonce)?; + let sign_result = bitbox02::keystore::secp256k1_sign(keypath, &sighash, &host_nonce) + .map_err(Error::err) + .context("secp256k1_sign failed")?; let mut signature: Vec = sign_result.signature.to_vec(); signature.push(sign_result.recid); @@ -229,7 +252,10 @@ mod tests { ..Default::default() }); mock_unlocked(); - assert_eq!(block_on(process(&request)), Err(Error::UserAbort)); + assert_eq!( + block_on(process(&request)).unwrap_err().kind, + ErrorKind::UserAbort + ); // Address verification aborted. unsafe { @@ -253,7 +279,10 @@ mod tests { ..Default::default() }); mock_unlocked(); - assert_eq!(block_on(process(&request)), Err(Error::UserAbort)); + assert_eq!( + block_on(process(&request)).unwrap_err().kind, + ErrorKind::UserAbort + ); // Message verification aborted. unsafe { @@ -277,7 +306,10 @@ mod tests { ..Default::default() }); mock_unlocked(); - assert_eq!(block_on(process(&request)), Err(Error::UserAbort)); + assert_eq!( + block_on(process(&request)).unwrap_err().kind, + ErrorKind::UserAbort + ); } #[test] @@ -296,8 +328,10 @@ mod tests { }), msg: MESSAGE.to_vec(), host_nonce_commitment: None, - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput ); // Invalid script type (invalid simple type) @@ -312,8 +346,10 @@ mod tests { }), msg: MESSAGE.to_vec(), host_nonce_commitment: None, - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput ); // Invalid script type (multisig not supported) @@ -330,8 +366,10 @@ mod tests { }), msg: MESSAGE.to_vec(), host_nonce_commitment: None, - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput ); // Message too long @@ -346,8 +384,10 @@ mod tests { }), msg: [0; 1025].to_vec(), host_nonce_commitment: None, - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput ); // Invalid keypath @@ -366,8 +406,10 @@ mod tests { }), msg: MESSAGE.to_vec(), host_nonce_commitment: None, - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput ); } } diff --git a/src/rust/bitbox02-rust/src/hww/api/device_info.rs b/src/rust/bitbox02-rust/src/hww/api/device_info.rs index 5153f2f8c..b112b5ad0 100644 --- a/src/rust/bitbox02-rust/src/hww/api/device_info.rs +++ b/src/rust/bitbox02-rust/src/hww/api/device_info.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use super::Error; +use super::error::{Context, Error}; use crate::pb; use pb::response::Response; @@ -25,8 +25,13 @@ pub fn process() -> Result { initialized: memory::is_initialized(), version: bitbox02::version_short().into(), mnemonic_passphrase_enabled: memory::is_mnemonic_passphrase_enabled(), - monotonic_increments_remaining: securechip::monotonic_increments_remaining()?, - securechip_model: match securechip::model()? { + monotonic_increments_remaining: securechip::monotonic_increments_remaining() + .map_err(Error::err) + .context("securechip::monotonic_increments_remaining failed")?, + securechip_model: match securechip::model() + .map_err(Error::err) + .context("securechip::model failed")? + { securechip::Model::SECURECHIP_ATECC608A => "ATECC608A".into(), securechip::Model::SECURECHIP_ATECC608B => "ATECC608B".into(), }, diff --git a/src/rust/bitbox02-rust/src/hww/api/electrum.rs b/src/rust/bitbox02-rust/src/hww/api/electrum.rs index 0b99ab8a9..48bfabd14 100644 --- a/src/rust/bitbox02-rust/src/hww/api/electrum.rs +++ b/src/rust/bitbox02-rust/src/hww/api/electrum.rs @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use super::error::{Error, ErrorKind}; use super::pb; -use super::Error; use pb::response::Response; @@ -30,16 +30,18 @@ const ELECTRUM_WALLET_ENCRYPTION_KEYPATH_LEVEL_TWO: u32 = 1112098098 + HARDENED; pub async fn process( pb::ElectrumEncryptionKeyRequest { keypath }: &pb::ElectrumEncryptionKeyRequest, ) -> Result { - if *keypath - != [ - ELECTRUM_WALLET_ENCRYPTION_KEYPATH_LEVEL_ONE, - ELECTRUM_WALLET_ENCRYPTION_KEYPATH_LEVEL_TWO, - ] - { - return Err(Error::InvalidInput); + let expected_keypath = [ + ELECTRUM_WALLET_ENCRYPTION_KEYPATH_LEVEL_ONE, + ELECTRUM_WALLET_ENCRYPTION_KEYPATH_LEVEL_TWO, + ]; + if *keypath != expected_keypath { + return Err(Error { + msg: Some("invalid keypath".into()), + kind: ErrorKind::InvalidInput, + }); } let xpub = keystore::encode_xpub_at_keypath(keypath, keystore::xpub_type_t::XPUB) - .or(Err(Error::InvalidInput))?; + .map_err(Error::err_invalid_input)?; Ok(Response::ElectrumEncryptionKey( pb::ElectrumEncryptionKeyResponse { key: xpub }, @@ -80,8 +82,10 @@ mod tests { assert_eq!( block_on(process(&pb::ElectrumEncryptionKeyRequest { keypath: vec![ELECTRUM_WALLET_ENCRYPTION_KEYPATH_LEVEL_ONE, 0] - })), - Err(Error::InvalidInput), + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput, ); // Invalid keypath (wrong length). @@ -92,8 +96,10 @@ mod tests { ELECTRUM_WALLET_ENCRYPTION_KEYPATH_LEVEL_TWO, 0 ] - })), - Err(Error::InvalidInput), + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput, ); } } diff --git a/src/rust/bitbox02-rust/src/hww/api/error.rs b/src/rust/bitbox02-rust/src/hww/api/error.rs index 2521aa659..7e5ca18b6 100644 --- a/src/rust/bitbox02-rust/src/hww/api/error.rs +++ b/src/rust/bitbox02-rust/src/hww/api/error.rs @@ -16,9 +16,12 @@ use crate::pb; use crate::workflow::unlock::UnlockError; +extern crate alloc; +use alloc::string::String; + #[allow(dead_code)] #[derive(Debug, PartialEq)] -pub enum Error { +pub enum ErrorKind { InvalidInput, Memory, Generic, @@ -30,50 +33,126 @@ pub enum Error { NoiseDecrypt, } -impl core::convert::From<()> for Error { +// See the unit tests below for usage patterns. +#[derive(Debug, PartialEq)] +pub struct Error { + // Ideally we want this to be `Option` so the messages can + // be dynamic and contain runtime info, but that quickly leads to + // uncontrolled binary bloat. For now we only do static error + // context strings. + pub msg: Option<&'static str>, + pub kind: ErrorKind, +} + +impl Error { + pub fn err>(e: E) -> Error { + Error { + msg: None, + kind: e.into(), + } + } + pub fn err_generic(_e: E) -> Error { + Error { + msg: None, + kind: ErrorKind::Generic, + } + } + + pub fn err_memory(_e: E) -> Error { + Error { + msg: None, + kind: ErrorKind::Memory, + } + } + + pub fn err_invalid_input(_e: E) -> Error { + Error { + msg: None, + kind: ErrorKind::InvalidInput, + } + } + + pub fn err_noise_encrypt(_e: E) -> Error { + Error { + msg: None, + kind: ErrorKind::NoiseEncrypt, + } + } + + pub fn err_noise_decrypt(_e: E) -> Error { + Error { + msg: None, + kind: ErrorKind::NoiseDecrypt, + } + } +} + +pub trait Context { + fn context(self, msg: &'static str) -> Result; +} + +impl Context for Result { + fn context(self, msg: &'static str) -> Result { + self.map_err(|e| Error { + msg: Some(msg), + kind: e.kind, + }) + } +} + +impl core::convert::From<()> for ErrorKind { fn from(_error: ()) -> Self { - Error::Generic + ErrorKind::Generic } } -impl core::convert::From for Error { +impl core::convert::From for ErrorKind { fn from(_error: bitbox02::memory::Error) -> Self { - Error::Memory + ErrorKind::Memory } } -impl core::convert::From for Error { +impl core::convert::From for ErrorKind { fn from(_error: crate::workflow::cancel::Error) -> Self { - Error::UserAbort + ErrorKind::UserAbort } } -impl core::convert::From for Error { +impl core::convert::From for ErrorKind { fn from(_error: crate::workflow::confirm::UserAbort) -> Self { - Error::UserAbort + ErrorKind::UserAbort } } -impl core::convert::From for Error { +impl core::convert::From for ErrorKind { fn from(_error: crate::workflow::transaction::UserAbort) -> Self { - Error::UserAbort + ErrorKind::UserAbort } } -impl core::convert::From for Error { +impl core::convert::From for ErrorKind { fn from(error: crate::workflow::verify_message::Error) -> Self { match error { - crate::workflow::verify_message::Error::InvalidInput => Error::InvalidInput, - crate::workflow::verify_message::Error::UserAbort => Error::UserAbort, + crate::workflow::verify_message::Error::InvalidInput => ErrorKind::InvalidInput, + crate::workflow::verify_message::Error::UserAbort => ErrorKind::UserAbort, } } } -impl core::convert::From for Error { +impl core::convert::From for ErrorKind { fn from(error: UnlockError) -> Self { match error { - UnlockError::UserAbort => Error::UserAbort, - UnlockError::IncorrectPassword | UnlockError::Generic => Error::Generic, + UnlockError::UserAbort => ErrorKind::UserAbort, + UnlockError::IncorrectPassword | UnlockError::Generic => ErrorKind::Generic, + } + } +} + +impl> core::convert::From for Error { + fn from(error: A) -> Self { + Error { + msg: None, + kind: error.into(), } } } @@ -82,44 +161,172 @@ use pb::response::Response; /// Creates an Error response. Corresponds to commander.c:_report_error(). pub fn make_error(err: Error) -> Response { - use Error::*; - let err = match err { + fn format_err(kind_str: &str, msg: Option<&str>) -> String { + match msg { + Some(msg) => format!("{}: {}", kind_str, msg), + None => kind_str.into(), + } + } + + use ErrorKind::*; + let err = match err.kind { InvalidInput => pb::Error { code: 101, - message: "invalid input".into(), + message: format_err("invalid input", err.msg), }, Memory => pb::Error { code: 102, - message: "memory".into(), + message: format_err("memory", err.msg), }, Generic => pb::Error { code: 103, - message: "generic error".into(), + message: format_err("generic error", err.msg), }, UserAbort => pb::Error { code: 104, - message: "aborted by the user".into(), + message: format_err("aborted by the user", err.msg), }, InvalidState => pb::Error { code: 105, - message: "can't call this endpoint: wrong state".into(), + message: format_err("can't call this endpoint: wrong state", err.msg), }, Disabled => pb::Error { code: 106, - message: "function disabled".into(), + message: format_err("function disabled", err.msg), }, Duplicate => pb::Error { code: 107, - message: "duplicate entry".into(), + message: format_err("duplicate entry", err.msg), }, NoiseEncrypt => pb::Error { code: 108, - message: "noise encryption failed".into(), + message: format_err("noise encryption failed", err.msg), }, NoiseDecrypt => pb::Error { code: 109, - message: "noise decryption failed".into(), + message: format_err("noise decryption failed", err.msg), }, }; Response::Error(err) } + +#[cfg(test)] +mod tests { + use super::*; + + fn err_generic() -> Result<(), ()> { + Err(()) + } + + fn converted_err_generic() -> Result<(), Error> { + Ok(err_generic()?) + } + + fn err_generic_with_context() -> Result<(), Error> { + Ok(err_generic() + .map_err(Error::err) + .context("generic context")?) + } + + fn err_replaced_context() -> Result<(), Error> { + Ok(err_generic_with_context().context("new generic context")?) + } + + fn err_adhoc_kind() -> Result<(), Error> { + Ok(err_generic() + .map_err(Error::err_invalid_input) + .context("invalid input context")?) + } + + fn err_user_abort() -> Result<(), crate::workflow::confirm::UserAbort> { + Err(crate::workflow::confirm::UserAbort {}) + } + + fn converted_err_user_abort() -> Result<(), Error> { + Ok(err_user_abort()?) + } + + fn err_user_abort_with_context() -> Result<(), Error> { + Ok(err_user_abort() + .map_err(Error::err) + .context("user abort context")?) + } + + #[test] + pub fn test_errors() { + assert_eq!( + err_generic().map_err(Error::err), + Err(Error { + msg: None, + kind: ErrorKind::Generic + }) + ); + + assert_eq!( + converted_err_generic(), + Err(Error { + msg: None, + kind: ErrorKind::Generic + }) + ); + + assert_eq!( + err_generic_with_context(), + Err(Error { + msg: Some("generic context".into()), + kind: ErrorKind::Generic + }) + ); + + assert_eq!( + err_replaced_context(), + Err(Error { + msg: Some("new generic context".into()), + kind: ErrorKind::Generic + }) + ); + + assert_eq!( + err_adhoc_kind(), + Err(Error { + msg: Some("invalid input context".into()), + kind: ErrorKind::InvalidInput + }) + ); + + assert_eq!( + converted_err_user_abort(), + Err(Error { + msg: None, + kind: ErrorKind::UserAbort + }) + ); + + assert_eq!( + err_user_abort_with_context(), + Err(Error { + msg: Some("user abort context"), + kind: ErrorKind::UserAbort + }) + ); + } + + #[test] + pub fn test_make_error() { + assert_eq!( + make_error(converted_err_user_abort().unwrap_err()), + Response::Error(pb::Error { + code: 104, + message: "aborted by the user".into(), + }) + ); + + assert_eq!( + make_error(err_user_abort_with_context().unwrap_err()), + Response::Error(pb::Error { + code: 104, + message: "aborted by the user: user abort context".into(), + }) + ); + } +} diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum.rs index c37017c38..42469e51a 100644 --- a/src/rust/bitbox02-rust/src/hww/api/ethereum.rs +++ b/src/rust/bitbox02-rust/src/hww/api/ethereum.rs @@ -24,8 +24,8 @@ mod pubrequest; mod sign; mod signmsg; +use super::error::{Context, Error, ErrorKind}; use super::pb; -use super::Error; use pb::eth_request::Request; use pb::eth_response::Response; @@ -42,7 +42,10 @@ pub async fn next_request(response: Response) -> Result { pb::request::Request::Eth(pb::EthRequest { request: Some(request), }) => Ok(request), - _ => Err(Error::InvalidState), + _ => Err(Error { + msg: Some("expected Eth request".into()), + kind: ErrorKind::InvalidState, + }), } } @@ -62,9 +65,13 @@ pub async fn antiklepto_get_host_nonce( Ok(host_nonce .as_slice() .try_into() - .or(Err(Error::InvalidInput))?) + .map_err(Error::err_invalid_input) + .context("could not parse host nonce")?) } - _ => Err(Error::InvalidState), + _ => Err(Error { + msg: Some("expected AntikleptoSignature".into()), + kind: ErrorKind::InvalidState, + }), } } @@ -77,6 +84,9 @@ pub async fn process_api(request: &Request) -> Option> { Request::Pub(ref request) => Some(pubrequest::process(request).await), Request::SignMsg(ref request) => Some(signmsg::process(request).await), Request::Sign(ref request) => Some(sign::process(request).await), - Request::AntikleptoSignature(_) => Some(Err(Error::InvalidInput)), + Request::AntikleptoSignature(_) => Some(Err(Error { + msg: Some("unexpected AntikleptoSignature request".into()), + kind: ErrorKind::InvalidInput, + })), } } diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum/keypath.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum/keypath.rs index 05bf59e59..0d1ed702b 100644 --- a/src/rust/bitbox02-rust/src/hww/api/ethereum/keypath.rs +++ b/src/rust/bitbox02-rust/src/hww/api/ethereum/keypath.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use super::Error; +use crate::hww::api::error::{Error, ErrorKind}; use crate::workflow::confirm; use bitbox02::app_eth::Params; @@ -35,7 +35,10 @@ pub async fn warn_unusual_keypath( keypath: &[u32], ) -> Result<(), Error> { if keypath.len() < 2 { - return Err(Error::InvalidInput); + return Err(Error { + msg: None, + kind: ErrorKind::InvalidInput, + }); } if keypath[1] != params.bip44_coin { let body = format!( diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs index 664f7d496..fa2ebd6e9 100644 --- a/src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs +++ b/src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs @@ -13,19 +13,23 @@ // limitations under the License. use super::pb; -use super::Error; use pb::eth_pub_request::OutputType; use pb::eth_response::Response; +use crate::hww::api::error::{Context, Error, ErrorKind}; use crate::workflow::confirm; + use bitbox02::keystore; extern crate alloc; use core::convert::TryInto; async fn process_address(request: &pb::EthPubRequest) -> Result { - let params = bitbox02::app_eth::params_get(request.coin as _).ok_or(Error::InvalidInput)?; + let params = bitbox02::app_eth::params_get(request.coin as _).ok_or(Error { + msg: Some("invalid coin".into()), + kind: ErrorKind::InvalidInput, + })?; // If a contract_address is provided, it has to be a supported ERC20-token. let erc20_params: Option = if request.contract_address.is_empty() { @@ -35,18 +39,25 @@ async fn process_address(request: &pb::EthPubRequest) -> Result .contract_address .as_slice() .try_into() - .or(Err(Error::InvalidInput))?; + .map_err(Error::err_invalid_input) + .context("could not parse address")?; Some( - bitbox02::app_eth::erc20_params_get(request.coin as _, address) - .ok_or(Error::InvalidInput)?, + bitbox02::app_eth::erc20_params_get(request.coin as _, address).ok_or(Error { + msg: Some("invalid coin".into()), + kind: ErrorKind::InvalidInput, + })?, ) }; if !super::keypath::is_valid_keypath_address(&request.keypath) { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("invalid keypath".into()), + kind: ErrorKind::InvalidInput, + }); } let pubkey = bitbox02::keystore::secp256k1_pubkey_uncompressed(&request.keypath) - .or(Err(Error::InvalidInput))?; + .map_err(Error::err_invalid_input) + .context("secp256k1_pubkey_uncompressed failed")?; let address = super::address::from_pubkey(&pubkey); if request.display { @@ -71,21 +82,34 @@ async fn process_address(request: &pb::EthPubRequest) -> Result fn process_xpub(request: &pb::EthPubRequest) -> Result { if request.display { // No xpub user verification for now. - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("display must be false".into()), + kind: ErrorKind::InvalidInput, + }); } - bitbox02::app_eth::params_get(request.coin as _).ok_or(Error::InvalidInput)?; + bitbox02::app_eth::params_get(request.coin as _).ok_or(Error { + msg: Some("invalid coin".into()), + kind: ErrorKind::InvalidInput, + })?; if !super::keypath::is_valid_keypath_xpub(&request.keypath) { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("invalid keypath".into()), + kind: ErrorKind::InvalidInput, + }); } let xpub = keystore::encode_xpub_at_keypath(&request.keypath, keystore::xpub_type_t::XPUB) - .or(Err(Error::InvalidInput))?; + .map_err(Error::err_invalid_input) + .context("encode_xpub_at_keypath failed")?; Ok(Response::Pub(pb::PubResponse { r#pub: xpub })) } pub async fn process(request: &pb::EthPubRequest) -> Result { - let output_type = OutputType::from_i32(request.output_type).ok_or(Error::InvalidInput)?; + let output_type = OutputType::from_i32(request.output_type).ok_or(Error { + msg: Some("invalid output_type".into()), + kind: ErrorKind::InvalidInput, + })?; match output_type { OutputType::Address => process_address(request).await, OutputType::Xpub => process_xpub(request), @@ -131,8 +155,8 @@ mod tests { ..Default::default() }); assert_eq!( - block_on(process(&invalid_request)), - Err(Error::InvalidInput) + block_on(process(&invalid_request)).unwrap_err().kind, + ErrorKind::InvalidInput, ); // Wrong keypath (wrong expected coin) @@ -142,15 +166,18 @@ mod tests { ..Default::default() }); assert_eq!( - block_on(process(&invalid_request)), - Err(Error::InvalidInput) + block_on(process(&invalid_request)).unwrap_err().kind, + ErrorKind::InvalidInput, ); // xpub fetching/encoding failed. mock(Data { ..Default::default() }); - assert_eq!(block_on(process(&request)), Err(Error::InvalidInput)); + assert_eq!( + block_on(process(&request)).unwrap_err().kind, + ErrorKind::InvalidInput + ); } #[test] @@ -252,8 +279,10 @@ mod tests { coin: pb::EthCoin::Eth as _, display: true, contract_address: b"".to_vec(), - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput, ); // Params not found. @@ -263,8 +292,8 @@ mod tests { ..Default::default() }); assert_eq!( - block_on(process(&invalid_request)), - Err(Error::InvalidInput) + block_on(process(&invalid_request)).unwrap_err().kind, + ErrorKind::InvalidInput, ); // Wrong keypath (wrong expected coin) @@ -274,8 +303,8 @@ mod tests { ..Default::default() }); assert_eq!( - block_on(process(&invalid_request)), - Err(Error::InvalidInput) + block_on(process(&invalid_request)).unwrap_err().kind, + ErrorKind::InvalidInput, ); // Wrong keypath (account too high) @@ -289,8 +318,10 @@ mod tests { coin: pb::EthCoin::Eth as _, display: false, contract_address: b"".to_vec(), - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput, ); } @@ -356,8 +387,10 @@ mod tests { coin: pb::EthCoin::Eth as _, display: false, contract_address: b"aaaaaaaaaaaaaaaaaaaa".to_vec(), - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput, ); } } diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum/sign.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum/sign.rs index 007bcf434..f39e60179 100644 --- a/src/rust/bitbox02-rust/src/hww/api/ethereum/sign.rs +++ b/src/rust/bitbox02-rust/src/hww/api/ethereum/sign.rs @@ -14,10 +14,10 @@ use super::amount::Amount; use super::pb; -use super::Error; use bitbox02::keystore; +use crate::hww::api::error::{Context, Error, ErrorKind}; use crate::workflow::{confirm, transaction}; use bitbox02::app_eth::{params_get, sighash, Params, SighashParams}; @@ -34,7 +34,10 @@ const WEI_DECIMALS: usize = 18; /// Converts `recipient` to an array of 20 chars. If `recipient` is /// not exactly 20 elements, `InvalidInput` is returned. fn parse_recipient(recipient: &[u8]) -> Result<[u8; 20], Error> { - recipient.try_into().or(Err(Error::InvalidInput)) + recipient.try_into().or(Err(Error { + msg: Some("failed parsing recipient".into()), + kind: ErrorKind::InvalidInput, + })) } /// Checks if the transaction is an ERC20 transaction. @@ -132,7 +135,10 @@ async fn verify_standard_transaction( ) -> Result<(), Error> { if request.data.is_empty() && request.value.is_empty() { // Must transfer non-zero value, unless there is data (contract invocation). - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("value and data can't both be empty".into()), + kind: ErrorKind::InvalidInput, + }); } let recipient = parse_recipient(&request.recipient)?; @@ -184,10 +190,16 @@ async fn verify_standard_transaction( /// Verify and sign an Ethereum transaction. pub async fn process(request: &pb::EthSignRequest) -> Result { - let params = params_get(request.coin as _).ok_or(Error::InvalidInput)?; + let params = params_get(request.coin as _).ok_or(Error { + msg: Some("invalid coin".into()), + kind: ErrorKind::InvalidInput, + })?; if !super::keypath::is_valid_keypath_address(&request.keypath) { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("invalid keypath".into()), + kind: ErrorKind::InvalidInput, + }); } super::keypath::warn_unusual_keypath(¶ms, params.name, &request.keypath).await?; @@ -198,27 +210,45 @@ pub async fn process(request: &pb::EthSignRequest) -> Result { || request.value.len() > 32 || request.data.len() > 1024 { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("field sizes too big".into()), + kind: ErrorKind::InvalidInput, + }); } // No zero prefix in the big endian numbers. if let [0, ..] = &request.nonce[..] { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("nonce can't be zero prefixed".into()), + kind: ErrorKind::InvalidInput, + }); } if let [0, ..] = &request.gas_price[..] { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("gas_price can't be zero prefixed".into()), + kind: ErrorKind::InvalidInput, + }); } if let [0, ..] = &request.gas_limit[..] { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("gas_limit can't be zero prefixed".into()), + kind: ErrorKind::InvalidInput, + }); } if let [0, ..] = &request.value[..] { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("value can't be zero prefixed".into()), + kind: ErrorKind::InvalidInput, + }); } let recipient = parse_recipient(&request.recipient)?; if recipient == [0; 20] { // Reserved for contract creation. - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("recipient can't be zero (contract creation not supported)".into()), + kind: ErrorKind::InvalidInput, + }); } if let Some((erc20_recipient, erc20_value)) = parse_erc20(request) { @@ -236,7 +266,8 @@ pub async fn process(request: &pb::EthSignRequest) -> Result { data: &request.data, chain_id: params.chain_id, }) - .or(Err(Error::InvalidInput))?; + .map_err(Error::err_invalid_input) + .context("sighash failed")?; let host_nonce = match request.host_nonce_commitment { // Engage in the anti-klepto protocol if the host sends a host nonce commitment. @@ -247,8 +278,11 @@ pub async fn process(request: &pb::EthSignRequest) -> Result { commitment .as_slice() .try_into() - .or(Err(Error::InvalidInput))?, - )?; + .map_err(Error::err_invalid_input) + .context("could not parse host nonce commitment")?, + ) + .map_err(Error::err) + .context("secp256k1_nonce_commit failed")?; // Send signer commitment to host and wait for the host nonce from the host. super::antiklepto_get_host_nonce(signer_commitment).await? @@ -257,7 +291,9 @@ pub async fn process(request: &pb::EthSignRequest) -> Result { // Return signature directly without the anti-klepto protocol, for backwards compatibility. None => [0; 32], }; - let sign_result = keystore::secp256k1_sign(&request.keypath, &hash, &host_nonce)?; + let sign_result = keystore::secp256k1_sign(&request.keypath, &hash, &host_nonce) + .map_err(Error::err) + .context("secp256k1_sign failed")?; let mut signature: Vec = sign_result.signature.to_vec(); signature.push(sign_result.recid); @@ -283,12 +319,12 @@ mod tests { ); assert_eq!( - parse_recipient(b"0123456789012345678"), - Err(Error::InvalidInput), + parse_recipient(b"0123456789012345678").unwrap_err().kind, + ErrorKind::InvalidInput, ); assert_eq!( - parse_recipient(b"012345678901234567890"), - Err(Error::InvalidInput), + parse_recipient(b"012345678901234567890").unwrap_err().kind, + ErrorKind::InvalidInput, ); } @@ -589,8 +625,8 @@ mod tests { let mut invalid_request = valid_request.clone(); invalid_request.coin = 100; assert_eq!( - block_on(process(&invalid_request)), - Err(Error::InvalidInput) + block_on(process(&invalid_request)).unwrap_err().kind, + ErrorKind::InvalidInput, ); } @@ -599,8 +635,8 @@ mod tests { let mut invalid_request = valid_request.clone(); invalid_request.keypath = vec![44 + HARDENED, 0 + HARDENED, 0 + HARDENED, 0, 0]; assert_eq!( - block_on(process(&invalid_request)), - Err(Error::InvalidInput) + block_on(process(&invalid_request)).unwrap_err().kind, + ErrorKind::InvalidInput, ); } @@ -609,8 +645,8 @@ mod tests { let mut invalid_request = valid_request.clone(); invalid_request.keypath = vec![44 + HARDENED, 60 + HARDENED, 0 + HARDENED, 0, 100]; assert_eq!( - block_on(process(&invalid_request)), - Err(Error::InvalidInput) + block_on(process(&invalid_request)).unwrap_err().kind, + ErrorKind::InvalidInput, ); } @@ -619,8 +655,8 @@ mod tests { let mut invalid_request = valid_request.clone(); invalid_request.data = vec![0; 1025]; assert_eq!( - block_on(process(&invalid_request)), - Err(Error::InvalidInput) + block_on(process(&invalid_request)).unwrap_err().kind, + ErrorKind::InvalidInput, ); } @@ -629,8 +665,8 @@ mod tests { let mut invalid_request = valid_request.clone(); invalid_request.recipient = vec![b'a'; 21]; assert_eq!( - block_on(process(&invalid_request)), - Err(Error::InvalidInput) + block_on(process(&invalid_request)).unwrap_err().kind, + ErrorKind::InvalidInput, ); } @@ -639,8 +675,8 @@ mod tests { let mut invalid_request = valid_request.clone(); invalid_request.recipient = vec![0; 20]; assert_eq!( - block_on(process(&invalid_request)), - Err(Error::InvalidInput) + block_on(process(&invalid_request)).unwrap_err().kind, + ErrorKind::InvalidInput, ); } @@ -654,7 +690,10 @@ mod tests { })), ..Default::default() }); - assert_eq!(block_on(process(&valid_request)), Err(Error::UserAbort)); + assert_eq!( + block_on(process(&valid_request)).unwrap_err().kind, + ErrorKind::UserAbort + ); } { // User rejects total/fee. @@ -671,7 +710,10 @@ mod tests { })), ..Default::default() }); - assert_eq!(block_on(process(&valid_request)), Err(Error::UserAbort)); + assert_eq!( + block_on(process(&valid_request)).unwrap_err().kind, + ErrorKind::UserAbort + ); } { // Keystore locked. @@ -680,7 +722,10 @@ mod tests { ui_transaction_fee_create: Some(Box::new(|_, _| true)), ..Default::default() }); - assert_eq!(block_on(process(&valid_request)), Err(Error::Generic)); + assert_eq!( + block_on(process(&valid_request)).unwrap_err().kind, + ErrorKind::Generic + ); } } } diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum/signmsg.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum/signmsg.rs index 88ef3491f..241727fb2 100644 --- a/src/rust/bitbox02-rust/src/hww/api/ethereum/signmsg.rs +++ b/src/rust/bitbox02-rust/src/hww/api/ethereum/signmsg.rs @@ -13,10 +13,10 @@ // limitations under the License. use super::pb; -use super::Error; use bitbox02::keystore; +use crate::hww::api::error::{Context, Error, ErrorKind}; use crate::workflow::verify_message; use pb::eth_response::Response; @@ -32,7 +32,10 @@ use sha3::digest::Digest; /// compact format (R and S values), and the last byte is the recoverable id (recid). pub async fn process(request: &pb::EthSignMessageRequest) -> Result { if request.msg.len() > 9999 { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("message length larger than 9999 bytes".into()), + kind: ErrorKind::InvalidInput, + }); } let pub_request = pb::EthPubRequest { output_type: pb::eth_pub_request::OutputType::Address as _, @@ -67,8 +70,11 @@ pub async fn process(request: &pb::EthSignMessageRequest) -> Result Result [0; 32], }; - let sign_result = bitbox02::keystore::secp256k1_sign(&request.keypath, &sighash, &host_nonce)?; + let sign_result = bitbox02::keystore::secp256k1_sign(&request.keypath, &sighash, &host_nonce) + .map_err(Error::err) + .context("secp256k1_sign failed")?; let mut signature: Vec = sign_result.signature.to_vec(); signature.push(sign_result.recid); @@ -220,7 +228,10 @@ mod tests { ..Default::default() }); mock_unlocked(); - assert_eq!(block_on(process(&request)), Err(Error::UserAbort)); + assert_eq!( + block_on(process(&request)).unwrap_err().kind, + ErrorKind::UserAbort + ); // User abort message verification. unsafe { @@ -244,7 +255,10 @@ mod tests { ..Default::default() }); mock_unlocked(); - assert_eq!(block_on(process(&request)), Err(Error::UserAbort)); + assert_eq!( + block_on(process(&request)).unwrap_err().kind, + ErrorKind::UserAbort + ); } #[test] @@ -260,8 +274,10 @@ mod tests { keypath: KEYPATH.to_vec(), msg: [0; 10000].to_vec(), host_nonce_commitment: None, - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput ); // Keystore locked. @@ -274,8 +290,10 @@ mod tests { keypath: KEYPATH.to_vec(), msg: b"message".to_vec(), host_nonce_commitment: None, - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput ); } } diff --git a/src/rust/bitbox02-rust/src/hww/api/reset.rs b/src/rust/bitbox02-rust/src/hww/api/reset.rs index 41f9ce5cf..a4745f1bf 100644 --- a/src/rust/bitbox02-rust/src/hww/api/reset.rs +++ b/src/rust/bitbox02-rust/src/hww/api/reset.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use super::Error; +use super::error::Error; use crate::pb; use pb::response::Response; @@ -27,7 +27,9 @@ pub async fn process() -> Result { ..Default::default() }; - confirm::confirm(¶ms).await.or(Err(Error::Generic))?; + confirm::confirm(¶ms) + .await + .map_err(Error::err_generic)?; bitbox02::reset(true); @@ -40,6 +42,7 @@ mod tests { use super::*; use crate::bb02_async::block_on; + use crate::hww::api::error::ErrorKind; use bitbox02::testing::{mock, Data, MUTEX}; use std::boxed::Box; @@ -68,6 +71,6 @@ mod tests { })), ..Default::default() }); - assert_eq!(block_on(process()), Err(Error::Generic)); + assert_eq!(block_on(process()).unwrap_err().kind, ErrorKind::Generic); } } diff --git a/src/rust/bitbox02-rust/src/hww/api/restore.rs b/src/rust/bitbox02-rust/src/hww/api/restore.rs index aac34226e..fc73a5612 100644 --- a/src/rust/bitbox02-rust/src/hww/api/restore.rs +++ b/src/rust/bitbox02-rust/src/hww/api/restore.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use super::Error; +use super::error::{Context, Error, ErrorKind}; use crate::pb; use pb::response::Response; @@ -37,12 +37,18 @@ pub async fn from_mnemonic( confirm::confirm(¶ms).await?; } - let mnemonic = mnemonic::get().await?; + let mnemonic = mnemonic::get() + .await + .map_err(Error::err) + .context("mnemonic::get failed")?; let seed = match bitbox02::keystore::bip39_mnemonic_to_seed(&mnemonic) { Ok(seed) => seed, Err(()) => { status::status("Recovery words\ninvalid", false).await; - return Err(Error::Generic); + return Err(Error { + msg: Some("recovery words invalid".into()), + kind: ErrorKind::Generic, + }); } }; status::status("Recovery words\nvalid", true).await; @@ -65,7 +71,10 @@ pub async fn from_mnemonic( if bitbox02::keystore::encrypt_and_store_seed(&seed, password.as_str()).is_err() { status::status("Could not\nrestore backup", false).await; - return Err(Error::Generic); + return Err(Error { + msg: Some("encrypt_and_store_seed failed".into()), + kind: ErrorKind::Generic, + }); }; #[cfg(feature = "app-u2f")] @@ -75,7 +84,7 @@ pub async fn from_mnemonic( let _ = bitbox02::securechip::u2f_counter_set(timestamp); } - bitbox02::memory::set_initialized().or(Err(Error::Memory))?; + bitbox02::memory::set_initialized().map_err(Error::err_memory)?; // This should never fail. bitbox02::keystore::unlock(&password).expect("restore_from_mnemonic: unlock failed"); diff --git a/src/rust/bitbox02-rust/src/hww/api/rootfingerprint.rs b/src/rust/bitbox02-rust/src/hww/api/rootfingerprint.rs index e7b46f886..e6b85d6aa 100644 --- a/src/rust/bitbox02-rust/src/hww/api/rootfingerprint.rs +++ b/src/rust/bitbox02-rust/src/hww/api/rootfingerprint.rs @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use super::error::{Context, Error}; use super::pb; -use super::Error; use pb::response::Response; @@ -23,7 +23,9 @@ use bitbox02::keystore; /// bits of the hash160 of the pubkey at the keypath m/. /// https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#key-identifiers pub fn process() -> Result { - let fingerprint = keystore::root_fingerprint()?; + let fingerprint = keystore::root_fingerprint() + .map_err(Error::err) + .context("root_fingerprint failed")?; Ok(Response::Fingerprint(pb::RootFingerprintResponse { fingerprint: fingerprint.to_vec(), })) diff --git a/src/rust/bitbox02-rust/src/hww/api/set_device_name.rs b/src/rust/bitbox02-rust/src/hww/api/set_device_name.rs index 041b9c2ad..5875701d1 100644 --- a/src/rust/bitbox02-rust/src/hww/api/set_device_name.rs +++ b/src/rust/bitbox02-rust/src/hww/api/set_device_name.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use super::Error; +use super::error::{Context, Error, ErrorKind}; use crate::pb; use pb::response::Response; @@ -23,7 +23,10 @@ pub async fn process( pb::SetDeviceNameRequest { name }: &pb::SetDeviceNameRequest, ) -> Result { if !util::name::validate(name, bitbox02::memory::DEVICE_NAME_MAX_LEN) { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("invalid name".into()), + kind: ErrorKind::InvalidInput, + }); } let params = confirm::Params { @@ -35,7 +38,9 @@ pub async fn process( confirm::confirm(¶ms).await?; - bitbox02::memory::set_device_name(&name)?; + bitbox02::memory::set_device_name(&name) + .map_err(Error::err) + .context("set_device_name failed")?; Ok(Response::Success(pb::Success {})) } @@ -85,8 +90,10 @@ mod tests { assert_eq!( block_on(process(&pb::SetDeviceNameRequest { name: SOME_NAME.into() - })), - Err(Error::UserAbort) + })) + .unwrap_err() + .kind, + ErrorKind::UserAbort ); // Memory write error. @@ -101,32 +108,40 @@ mod tests { assert_eq!( block_on(process(&pb::SetDeviceNameRequest { name: SOME_NAME.into() - })), - Err(Error::Memory) + })) + .unwrap_err() + .kind, + ErrorKind::Memory ); // Non-ascii character. assert_eq!( block_on(process(&pb::SetDeviceNameRequest { name: "emoji are 😃, 😭, and 😈".into() - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput ); // Non-printable character. assert_eq!( block_on(process(&pb::SetDeviceNameRequest { name: "foo\nbar".into() - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput ); // Too long. assert_eq!( block_on(process(&pb::SetDeviceNameRequest { name: core::str::from_utf8(&[b'a'; 500]).unwrap().into() - })), - Err(Error::InvalidInput) + })) + .unwrap_err() + .kind, + ErrorKind::InvalidInput ); } } diff --git a/src/rust/bitbox02-rust/src/hww/api/set_mnemonic_passphrase_enabled.rs b/src/rust/bitbox02-rust/src/hww/api/set_mnemonic_passphrase_enabled.rs index 17b7fb79e..a71f0b2b6 100644 --- a/src/rust/bitbox02-rust/src/hww/api/set_mnemonic_passphrase_enabled.rs +++ b/src/rust/bitbox02-rust/src/hww/api/set_mnemonic_passphrase_enabled.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use super::Error; +use super::error::Error; use crate::pb; use pb::response::Response; @@ -31,9 +31,7 @@ pub async fn process( confirm::confirm(¶ms).await?; - if bitbox02::memory::set_mnemonic_passphrase_enabled(enabled).is_err() { - return Err(Error::Memory); - } + bitbox02::memory::set_mnemonic_passphrase_enabled(enabled).map_err(Error::err_memory)?; Ok(Response::Success(pb::Success {})) } @@ -44,6 +42,7 @@ mod tests { use super::*; use crate::bb02_async::block_on; + use crate::hww::api::error::ErrorKind; use bitbox02::testing::{mock, Data, MUTEX}; use std::boxed::Box; @@ -81,8 +80,10 @@ mod tests { assert_eq!( block_on(process(&pb::SetMnemonicPassphraseEnabledRequest { enabled: true - })), - Err(Error::UserAbort) + })) + .unwrap_err() + .kind, + ErrorKind::UserAbort ); } } diff --git a/src/rust/bitbox02-rust/src/hww/api/set_password.rs b/src/rust/bitbox02-rust/src/hww/api/set_password.rs index 05769c8ea..5fc46df2d 100644 --- a/src/rust/bitbox02-rust/src/hww/api/set_password.rs +++ b/src/rust/bitbox02-rust/src/hww/api/set_password.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use super::Error; +use super::error::{Context, Error, ErrorKind}; use crate::pb; use crate::workflow::password; @@ -29,11 +29,20 @@ pub async fn process( pb::SetPasswordRequest { entropy }: &pb::SetPasswordRequest, ) -> Result { if entropy.len() != 16 && entropy.len() != 32 { - return Err(Error::InvalidInput); + return Err(Error { + msg: Some("host entropy len should be 16 or 32".into()), + kind: ErrorKind::InvalidInput, + }); } - let password = password::enter_twice().await?; + let password = password::enter_twice() + .await + .map_err(Error::err) + .context("password::enter_twice")?; if !bitbox02::keystore::create_and_store_seed(&password, &entropy) { - return Err(Error::Generic); + return Err(Error { + msg: Some("create_and_store_seed failed".into()), + kind: ErrorKind::Generic, + }); } if bitbox02::keystore::unlock(&password).is_err() { panic!("Unexpected error during restore: unlock failed."); diff --git a/src/rust/bitbox02-rust/src/hww/api/show_mnemonic.rs b/src/rust/bitbox02-rust/src/hww/api/show_mnemonic.rs index 8ce897324..f84add2ca 100644 --- a/src/rust/bitbox02-rust/src/hww/api/show_mnemonic.rs +++ b/src/rust/bitbox02-rust/src/hww/api/show_mnemonic.rs @@ -16,7 +16,7 @@ extern crate alloc; use alloc::string::String; use alloc::vec::Vec; -use super::Error; +use super::error::{Context, Error}; use crate::pb; use crate::workflow::confirm; @@ -72,9 +72,14 @@ fn create_random_unique_words(word: &str, length: u8) -> (u8, Vec Result { - unlock::unlock_keystore("Unlock device", unlock::CanCancel::Yes).await?; - - let mnemonic_sentence = keystore::get_bip39_mnemonic()?; + unlock::unlock_keystore("Unlock device", unlock::CanCancel::Yes) + .await + .map_err(Error::err) + .context("unlock_keystore failed")?; + + let mnemonic_sentence = keystore::get_bip39_mnemonic() + .map_err(Error::err) + .context("get_bip39_mnemonic failed")?; let words: Vec<&str> = mnemonic_sentence.split(' ').collect(); diff --git a/src/rust/bitbox02-rust/src/hww/api/system.rs b/src/rust/bitbox02-rust/src/hww/api/system.rs index 595bfee1d..93a2f117d 100644 --- a/src/rust/bitbox02-rust/src/hww/api/system.rs +++ b/src/rust/bitbox02-rust/src/hww/api/system.rs @@ -42,6 +42,7 @@ mod tests { use super::*; use crate::bb02_async::block_on; + use crate::hww::api::error::ErrorKind; use bitbox02::testing::{mock, Data, MUTEX}; use std::boxed::Box; @@ -76,8 +77,10 @@ mod tests { assert_eq!( block_on(reboot(&pb::RebootRequest { purpose: Purpose::Upgrade as _ - })), - Err(Error::UserAbort), + })) + .unwrap_err() + .kind, + ErrorKind::UserAbort, ); } }