From 9d6ebcbbb6190c0ec91fa7476956d8ffa0467d7c Mon Sep 17 00:00:00 2001 From: neithanmo Date: Thu, 10 Oct 2024 05:20:16 -0600 Subject: [PATCH] Some improvements to avoid panicking and enhance remainder handling in case of multisig transactions --- app/rust/src/parser/ffi.rs | 3 + app/rust/src/parser/parser_common.rs | 3 +- app/rust/src/parser/spending_condition.rs | 1 + app/rust/src/parser/structured_msg.rs | 5 +- app/rust/src/parser/transaction.rs | 120 ++++++++++++++------- app/rust/src/parser/transaction_payload.rs | 19 +++- app/rust/src/zxformat.rs | 7 +- 7 files changed, 106 insertions(+), 52 deletions(-) diff --git a/app/rust/src/parser/ffi.rs b/app/rust/src/parser/ffi.rs index d5cc62ca..2d17fd91 100644 --- a/app/rust/src/parser/ffi.rs +++ b/app/rust/src/parser/ffi.rs @@ -225,9 +225,12 @@ pub unsafe extern "C" fn _last_block_ptr( ) -> u16 { if let Some(tx) = parsed_obj_from_state(tx_t as _).and_then(|obj| obj.transaction()) { let block = tx.last_transaction_block(); + *block_ptr = block.as_ptr(); return block.len() as _; } + + *block_ptr = core::ptr::null_mut(); 0 } diff --git a/app/rust/src/parser/parser_common.rs b/app/rust/src/parser/parser_common.rs index 05b42dbe..859b0c88 100644 --- a/app/rust/src/parser/parser_common.rs +++ b/app/rust/src/parser/parser_common.rs @@ -20,7 +20,8 @@ pub const C32_ENCODED_ADDRS_LENGTH: usize = 48; pub const NUM_SUPPORTED_POST_CONDITIONS: usize = 16; pub const SIGNATURE_LEN: usize = 65; pub const PUBKEY_LEN: usize = 33; -pub const TOKEN_TRANSFER_MEMO_LEN: usize = 34; +pub const MEMO_LEN: usize = 34; +pub const AMOUNT_LEN: usize = 8; // A recursion limit use to control ram usage when parsing // contract-call arguments that comes in a transaction diff --git a/app/rust/src/parser/spending_condition.rs b/app/rust/src/parser/spending_condition.rs index 341b19cf..cb32b9ca 100644 --- a/app/rust/src/parser/spending_condition.rs +++ b/app/rust/src/parser/spending_condition.rs @@ -404,6 +404,7 @@ impl<'a> MultisigSpendingCondition<'a> { fn clear_as_singlesig(&mut self) { // TODO: check if it involves shrinking // the general transaction buffer + // function is not being called anywhere todo!(); } } diff --git a/app/rust/src/parser/structured_msg.rs b/app/rust/src/parser/structured_msg.rs index b5812a9f..9a8601db 100644 --- a/app/rust/src/parser/structured_msg.rs +++ b/app/rust/src/parser/structured_msg.rs @@ -82,13 +82,12 @@ impl<'a> Domain<'a> { let id = value.value_id(); if id == ValueId::UInt { - // wont panic as this was checked by the parser - let chain_id = value.uint().unwrap(); + let chain_id = value.uint().ok_or(ParserError::UnexpectedValue)?; let num = chain_id.numtoa_str(10, &mut buff).as_bytes(); pageString(out_value, num, page_idx) } else { - let string = value.string_ascii().unwrap(); + let string = value.string_ascii().ok_or(ParserError::UnexpectedValue)?; pageString(out_value, string.content(), page_idx) } diff --git a/app/rust/src/parser/transaction.rs b/app/rust/src/parser/transaction.rs index f95a819d..d63280d6 100644 --- a/app/rust/src/parser/transaction.rs +++ b/app/rust/src/parser/transaction.rs @@ -219,6 +219,11 @@ pub type TxTuple<'a> = ( impl<'a> From<(&'a [u8], TxTuple<'a>)> for Transaction<'a> { fn from(raw: (&'a [u8], TxTuple<'a>)) -> Self { + let mut remainder = None; + if !raw.0.is_empty() { + remainder = Some(raw.0); + } + Self { version: (raw.1).0, chain_id: (raw.1).1, @@ -228,7 +233,7 @@ impl<'a> From<(&'a [u8], TxTuple<'a>)> for Transaction<'a> { payload: (raw.1).5, // At this point the signer is unknown signer: SignerId::Invalid, - remainder: raw.0, + remainder, } } } @@ -249,23 +254,26 @@ pub struct Transaction<'a> { // with them, we can construct the pre_sig_hash for the current signer // we would ideally verify it, but we can lend such responsability to the application // which has more resources - // If this is not a multisig transaction, this field should be an empty array - pub remainder: &'a [u8], + // If this is not a multisig transaction, this field should be None + pub remainder: Option<&'a [u8]>, } impl<'a> Transaction<'a> { fn update_remainder(&mut self, data: &'a [u8]) { - self.remainder = data; + if !data.is_empty() { + self.remainder = Some(data); + } else { + self.remainder = None; + } } #[inline(never)] pub fn read(&mut self, data: &'a [u8]) -> Result<(), ParserError> { - self.update_remainder(data); - self.read_header()?; - self.read_auth()?; - self.read_transaction_modes()?; - self.read_post_conditions()?; - self.read_payload()?; + let rem = self.read_header(data)?; + let rem = self.read_auth(rem)?; + let rem = self.read_transaction_modes(rem)?; + let rem = self.read_post_conditions(rem)?; + let rem = self.read_payload(rem)?; let is_token_transfer = self.payload.is_token_transfer_payload(); let is_standard_auth = self.transaction_auth.is_standard_auth(); @@ -276,13 +284,21 @@ impl<'a> Transaction<'a> { // At this point we do not know who the signer is self.signer = SignerId::Invalid; + + self.remainder = None; + + // set the remainder if this is mutltisig + if self.is_multisig() && !rem.is_empty() { + self.update_remainder(rem); + } + Ok(()) } #[inline(never)] - fn read_header(&mut self) -> Result<(), ParserError> { - let (next_data, version) = TransactionVersion::from_bytes(self.remainder) - .map_err(|_| ParserError::UnexpectedValue)?; + fn read_header(&mut self, data: &'a [u8]) -> Result<&'a [u8], ParserError> { + let (next_data, version) = + TransactionVersion::from_bytes(data).map_err(|_| ParserError::UnexpectedValue)?; let (next_data, chain_id) = be_u32::<_, ParserError>(next_data).map_err(|_| ParserError::UnexpectedValue)?; @@ -291,52 +307,46 @@ impl<'a> Transaction<'a> { self.chain_id = chain_id; check_canary!(); - self.update_remainder(next_data); - - Ok(()) + Ok(next_data) } #[inline(never)] - fn read_auth(&mut self) -> Result<(), ParserError> { - let (next_data, auth) = TransactionAuth::from_bytes(self.remainder) - .map_err(|_| ParserError::InvalidAuthType)?; + fn read_auth(&mut self, data: &'a [u8]) -> Result<&'a [u8], ParserError> { + let (next_data, auth) = + TransactionAuth::from_bytes(data).map_err(|_| ParserError::InvalidAuthType)?; self.transaction_auth = auth; - self.update_remainder(next_data); check_canary!(); - Ok(()) + Ok(next_data) } #[inline(never)] - fn read_transaction_modes(&mut self) -> Result<(), ParserError> { + fn read_transaction_modes(&mut self, data: &'a [u8]) -> Result<&'a [u8], ParserError> { // two modes are included here, // anchor mode and postcondition mode - let (raw, _) = take::<_, _, ParserError>(2usize)(self.remainder) + let (raw, _) = take::<_, _, ParserError>(2usize)(data) .map_err(|_| ParserError::UnexpectedBufferEnd)?; - let modes = arrayref::array_ref!(self.remainder, 0, 2); + let modes = arrayref::array_ref!(data, 0, 2); self.transaction_modes = modes; - self.update_remainder(raw); check_canary!(); - Ok(()) + Ok(raw) } #[inline(never)] - fn read_post_conditions(&mut self) -> Result<(), ParserError> { - let (raw, conditions) = PostConditions::from_bytes(self.remainder) - .map_err(|_| ParserError::PostConditionFailed)?; + fn read_post_conditions(&mut self, data: &'a [u8]) -> Result<&'a [u8], ParserError> { + let (raw, conditions) = + PostConditions::from_bytes(data).map_err(|_| ParserError::PostConditionFailed)?; self.post_conditions = conditions; - self.update_remainder(raw); check_canary!(); - Ok(()) + Ok(raw) } #[inline(never)] - fn read_payload(&mut self) -> Result<(), ParserError> { - let (raw, payload) = TransactionPayload::from_bytes(self.remainder) + fn read_payload(&mut self, data: &'a [u8]) -> Result<&'a [u8], ParserError> { + let (raw, payload) = TransactionPayload::from_bytes(data) .map_err(|_| ParserError::InvalidTransactionPayload)?; self.payload = payload; - self.update_remainder(raw); check_canary!(); - Ok(()) + Ok(raw) } pub fn from_bytes(bytes: &'a [u8]) -> Result { @@ -532,16 +542,44 @@ impl<'a> Transaction<'a> { // returns a slice of the last block to be used in the presighash calculation pub fn last_transaction_block(&self) -> &[u8] { - unsafe { - let len = - (self.remainder.as_ptr() as usize - self.transaction_modes.as_ptr() as usize) as _; - core::slice::from_raw_parts(self.transaction_modes.as_ptr(), len) + match self.remainder { + Some(remainder) => { + let remainder_ptr = remainder.as_ptr() as usize; + let tx_modes_ptr = self.transaction_modes.as_ptr() as usize; + + unsafe { + let len = remainder_ptr - tx_modes_ptr; + core::slice::from_raw_parts(self.transaction_modes.as_ptr(), len) + } + } + None => { + // If there's no remainder, return everything from transaction_modes to the end of payload + let payload = self.payload.raw_payload(); + unsafe { + let payload_end = payload.as_ptr().add(payload.len()); + let len = payload_end as usize - self.transaction_modes.as_ptr() as usize; + core::slice::from_raw_parts(self.transaction_modes.as_ptr(), len) + } + } } } + // pub fn last_transaction_block(&self) -> Option<&[u8]> { + // self.remainder.map(|remainder| { + // let remainder_ptr = remainder.as_ptr() as usize; + // let tx_modes_ptr = self.transaction_modes.as_ptr() as usize; + // + // unsafe { + // let len = remainder_ptr - tx_modes_ptr; + // core::slice::from_raw_parts(self.transaction_modes.as_ptr(), len) + // } + // }) + // } pub fn previous_signer_data(&self) -> Option<&[u8]> { - if self.is_multisig() && self.remainder.len() >= MULTISIG_PREVIOUS_SIGNER_DATA_LEN { - return Some(&self.remainder[..MULTISIG_PREVIOUS_SIGNER_DATA_LEN]); + let remainder = self.remainder?; + + if self.is_multisig() && remainder.len() >= MULTISIG_PREVIOUS_SIGNER_DATA_LEN { + return Some(&remainder[..MULTISIG_PREVIOUS_SIGNER_DATA_LEN]); } None } diff --git a/app/rust/src/parser/transaction_payload.rs b/app/rust/src/parser/transaction_payload.rs index 3e85613a..f7f32757 100644 --- a/app/rust/src/parser/transaction_payload.rs +++ b/app/rust/src/parser/transaction_payload.rs @@ -11,8 +11,8 @@ use arrayvec::ArrayVec; use numtoa::NumToA; use super::{ - utils::ApduPanic, ClarityName, ContractName, PrincipalData, StacksAddress, - C32_ENCODED_ADDRS_LENGTH, HASH160_LEN, TX_DEPTH_LIMIT, + utils::ApduPanic, ClarityName, ContractName, PrincipalData, StacksAddress, AMOUNT_LEN, + C32_ENCODED_ADDRS_LENGTH, HASH160_LEN, MEMO_LEN, TX_DEPTH_LIMIT, }; use crate::parser::error::ParserError; @@ -64,19 +64,19 @@ impl<'a> StxTokenTransfer<'a> { TokenTranferPrincipal::Contract => PrincipalData::contract_principal_from_bytes(id.0)?, }; // Besides principal we take 34-bytes being the MEMO message + 8-bytes amount of stx - let len = bytes.len() - raw.len() + 34 + 8; + let len = bytes.len() - raw.len() + MEMO_LEN + AMOUNT_LEN; let (raw, data) = take(len)(bytes)?; Ok((raw, Self(data))) } pub fn memo(&self) -> &[u8] { - let at = self.0.len() - 34; + let at = self.0.len() - MEMO_LEN; // safe to unwrap as parser checked for proper len self.0.get(at..).apdu_unwrap() } pub fn amount(&self) -> Result { - let at = self.0.len() - 34 - 8; + let at = self.0.len() - MEMO_LEN - AMOUNT_LEN; let amount = self.0.get(at..).ok_or(ParserError::NoData)?; be_u64::<_, ParserError>(amount) .map(|res| res.1) @@ -753,6 +753,15 @@ impl<'a> TransactionPayload<'a> { } } } + + pub fn raw_payload(&self) -> &'a [u8] { + match self { + Self::TokenTransfer(token) => token.0, + Self::SmartContract(contract) => contract.0, + Self::ContractCall(call) => call.0, + Self::VersionedSmartContract(deploy) => deploy.0, + } + } } #[cfg(test)] diff --git a/app/rust/src/zxformat.rs b/app/rust/src/zxformat.rs index 6f78ea2a..b7178639 100644 --- a/app/rust/src/zxformat.rs +++ b/app/rust/src/zxformat.rs @@ -177,8 +177,11 @@ pub(crate) fn fpstr_to_str( } let fp = in_len - decimals as usize; - let left = str.get(0..fp).unwrap(); - let right = str.get(fp..in_len).unwrap(); + let left = str.get(0..fp).ok_or(ParserError::UnexpectedBufferEnd)?; + let right = str + .get(fp..in_len) + .ok_or(ParserError::UnexpectedBufferEnd)?; + write!(&mut writer, "{}.{}", left, right) .map(|_| writer.offset) .map_err(|_| ParserError::UnexpectedBufferEnd)