Merge pull request #219 from Zondax/fix/improvements

App improvements: input validation, dispatcher hardening, and parser robustness

Author: abenso

app/Makefile.version

@@ -1,3 +1,3 @@
 APPVERSION_M=0
 APPVERSION_N=26
-APPVERSION_P=4
+APPVERSION_P=5

app/rust/src/parser/ffi.rs

@@ -17,6 +17,12 @@ pub unsafe extern "C" fn _read(
     context: *const parser_context_t,
     parser_state: *mut parse_tx_t,
 ) -> u32 {
+    if context.is_null() || parser_state.is_null() {
+        return ParserError::NoData as u32;
+    }
+    if (*context).buffer.is_null() || (*context).bufferLen == 0 {
+        return ParserError::NoData as u32;
+    }
     let data = core::slice::from_raw_parts((*context).buffer, (*context).bufferLen as _);
 
     if let Some(obj) = parsed_obj_from_state(parser_state) {
@@ -63,13 +69,21 @@ pub unsafe extern "C" fn _getItem(
     pageCount: *mut u8,
     tx_t: *const parse_tx_t,
 ) -> u32 {
+    if pageCount.is_null()
+        || outKey.is_null()
+        || outValue.is_null()
+        || outKeyLen == 0
+        || outValueLen == 0
+    {
+        return ParserError::NoData as _;
+    }
+    if tx_t.is_null() || (*tx_t).state.is_null() {
+        return ParserError::ContextMismatch as _;
+    }
     *pageCount = 0u8;
     let page_count = &mut *pageCount;
     let key = core::slice::from_raw_parts_mut(outKey as *mut u8, outKeyLen as usize);
     let value = core::slice::from_raw_parts_mut(outValue as *mut u8, outValueLen as usize);
-    if tx_t.is_null() || (*tx_t).state.is_null() {
-        return ParserError::ContextMismatch as _;
-    }
     if let Some(obj) = parsed_obj_from_state(tx_t as _) {
         match obj.get_item(displayIdx, key, value, pageIdx) {
             Ok(page) => {
@@ -154,6 +168,9 @@ pub unsafe extern "C" fn _presig_hash_data(
     buf: *mut u8,
     bufLen: u16,
 ) -> u16 {
+    if tx_t.is_null() || buf.is_null() || bufLen == 0 {
+        return 0;
+    }
     let buffer = core::slice::from_raw_parts_mut(buf, bufLen as usize);
 
     if let Some(tx) = parsed_obj_from_state(tx_t as _).and_then(|obj| obj.transaction()) {
@@ -273,6 +290,9 @@ pub unsafe extern "C" fn _structured_msg_hash(
     out: *mut u8,
     out_len: u16,
 ) -> u32 {
+    if tx_t.is_null() || out.is_null() || out_len == 0 {
+        return ParserError::NoData as _;
+    }
     if let Some(tx) = parsed_obj_from_state(tx_t as _).and_then(|obj| obj.structured_msg()) {
         let output = core::slice::from_raw_parts_mut(out, out_len as _);
         if tx.get_hash(output).is_ok() {

app/rust/src/parser/message.rs

@@ -79,9 +79,13 @@ impl<'a> ByteString<'a> {
 
         let (rem, len) = read_varint(data).map_err(|_| ParserError::InvalidBytestrMessage)?;
 
-        let (_, message_content) = take::<_, _, ParserError>(len as usize)(rem)
+        let (tail, message_content) = take::<_, _, ParserError>(len as usize)(rem)
             .map_err(|_| ParserError::InvalidBytestrMessage)?;
 
+        if !tail.is_empty() {
+            return Err(ParserError::InvalidBytestrMessage);
+        }
+
         if !message_content.is_ascii() {
             return Err(ParserError::InvalidBytestrMessage);
         }
@@ -227,4 +231,23 @@ mod test {
         let msg = ByteString::from_bytes("\x17Stacks Signed Message:\n".as_bytes());
         assert!(msg.is_err());
     }
+
+    #[test]
+    fn test_reject_trailing_bytes_after_message() {
+        let visible = "benign";
+        let hidden = b"\xDEADBEEF-hidden-suffix";
+        let mut m = built_message(visible.len(), visible);
+        m.extend_from_slice(hidden);
+        let msg = ByteString::from_bytes(&m);
+        assert!(msg.is_err());
+    }
+
+    #[test]
+    fn test_reject_single_trailing_byte() {
+        let visible = "abc";
+        let mut m = built_message(visible.len(), visible);
+        m.push(0x00);
+        let msg = ByteString::from_bytes(&m);
+        assert!(msg.is_err());
+    }
 }

app/rust/src/parser/parsed_obj.rs

@@ -846,4 +846,32 @@ mod test {
 
         std::println!("tx: {:?}", msg);
     }
+    
+    const MODE_FIXTURE_HEX: &str = "000000000104009ef3889fd070159edcd8ef88a0ec87cea1592c83000000000000000000000000000f42400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000302000000060002169ef3889fd070159edcd8ef88a0ec87cea1592c830100000000000027100003167c5f674a8fd08efa61dd9b11121e046dd2c892730a756e6976322d636f72650300000000000000000103167c5f674a8fd08efa61dd9b11121e046dd2c892730a756e6976322d636f7265168c5e2f8d25627d6edebeb6d10fa3300f5acc8441086c6f6e67636f696e086c6f6e67636f696e0300000000000000000103167c5f674a8fd08efa61dd9b11121e046dd2c892730a756e6976322d636f7265168c5e2f8d25627d6edebeb6d10fa3300f5acc8441086c6f6e67636f696e086c6f6e67636f696e0300000000000000000102169ef3889fd070159edcd8ef88a0ec87cea1592c83168c5e2f8d25627d6edebeb6d10fa3300f5acc8441086c6f6e67636f696e086c6f6e67636f696e030000000000000000010316402da2c079e5d31d58b9cfc7286d1b1eb2f7834e0f616d6d2d7661756c742d76322d303116402da2c079e5d31d58b9cfc7286d1b1eb2f7834e0a746f6b656e2d616c657804616c65780300000000011c908a02162ec1a2dc2904ebc8b408598116c75e42c51afa2617726f757465722d76656c61722d616c65782d762d312d320d737761702d68656c7065722d6100000007010000000000000000000000000000271001000000000000000000000000011c908a040c00000002016106167c5f674a8fd08efa61dd9b11121e046dd2c892730477737478016206168c5e2f8d25627d6edebeb6d10fa3300f5acc8441086c6f6e67636f696e06167c5f674a8fd08efa61dd9b11121e046dd2c8927312756e6976322d73686172652d6665652d746f0c0000000201610616402da2c079e5d31d58b9cfc7286d1b1eb2f7834e0b746f6b656e2d776c6f6e6701620616402da2c079e5d31d58b9cfc7286d1b1eb2f7834e0a746f6b656e2d616c65780c0000000101610100000000000000000000000005f5e100";
+
+    #[test]
+    fn test_anchor_mode_invalid_rejected() {
+        let mut bytes = hex::decode(MODE_FIXTURE_HEX).unwrap();
+        assert_eq!(bytes[109], 0x03);
+        bytes[109] = 0xFF;
+        let mut msg = ParsedObj::from_bytes(&bytes).unwrap();
+        assert!(msg.read(&bytes).is_err());
+    }
+
+    #[test]
+    fn test_postcondition_mode_invalid_rejected() {
+        let mut bytes = hex::decode(MODE_FIXTURE_HEX).unwrap();
+        assert_eq!(bytes[110], 0x02);
+        bytes[110] = 0xFF;
+        let mut msg = ParsedObj::from_bytes(&bytes).unwrap();
+        assert!(msg.read(&bytes).is_err());
+    }
+
+    #[test]
+    fn test_postcondition_mode_zero_rejected() {
+        let mut bytes = hex::decode(MODE_FIXTURE_HEX).unwrap();
+        bytes[110] = 0x00;
+        let mut msg = ParsedObj::from_bytes(&bytes).unwrap();
+        assert!(msg.read(&bytes).is_err());
+    }
 }

app/rust/src/parser/transaction.rs

@@ -17,7 +17,7 @@ use crate::{
 
 use crate::{check_canary, zxformat};
 
-use super::PostConditions;
+use super::{PostConditions, TransactionPostConditionMode};
 
 // In multisig transactions the remainder should contain:
 // 32-byte previous signer post_sig_hash
@@ -44,7 +44,7 @@ pub enum TransactionAnchorMode {
 
 impl TransactionAnchorMode {
     #[inline(never)]
-    fn from_u8(v: u8) -> Option<Self> {
+    pub fn from_u8(v: u8) -> Option<Self> {
         match v {
             1 => Some(Self::OnChainOnly),
             2 => Some(Self::OffChainOnly),
@@ -181,10 +181,12 @@ impl<'a> Transaction<'a> {
     fn read_transaction_modes(&mut self, data: &'a [u8]) -> Result<&'a [u8], ParserError> {
         c_zemu_log_stack("Transaction::read_transaction_modes\x00");
         // two modes are included here,
-        // anchor mode and postcondition mode
+        // anchor mode and postcondition mode — both must be valid enum variants
         let (rem, _) = take::<_, _, ParserError>(2usize)(data)
             .map_err(|_| ParserError::UnexpectedBufferEnd)?;
         let modes = arrayref::array_ref!(data, 0, 2);
+        TransactionAnchorMode::from_u8(modes[0]).ok_or(ParserError::UnexpectedValue)?;
+        TransactionPostConditionMode::from_u8(modes[1]).ok_or(ParserError::UnexpectedValue)?;
         self.transaction_modes = modes;
         check_canary!();
         Ok(rem)

app/rust/src/parser/transaction_payload/contract_call.rs

@@ -636,7 +636,11 @@ impl<'a> TransactionContractCall<'a> {
     pub fn num_items(&self, hide_sip10_details: bool) -> Result<u8, ParserError> {
         // contract-address, contract-name, function-name
         // + the number of arguments
-        let num_args = self.num_args()? as u8;
+        let raw_args = self.num_args()?;
+        if raw_args > u8::MAX as u32 {
+            return Err(ParserError::ValueOutOfRange);
+        }
+        let num_args = raw_args as u8;
         if hide_sip10_details {
             Ok(num_args)
         } else {

app/rust/src/parser/tx_post_conditions.rs

@@ -15,7 +15,7 @@ pub enum TransactionPostConditionMode {
 
 impl TransactionPostConditionMode {
     #[inline(never)]
-    fn from_u8(v: u8) -> Option<Self> {
+    pub fn from_u8(v: u8) -> Option<Self> {
         match v {
             1 => Some(Self::Allow),
             2 => Some(Self::Deny),

app/src/apdu_handler.c

@@ -31,6 +31,8 @@
 
 static bool tx_initialized = false;
 
+bool review_pending = false;
+
 __Z_INLINE void extractHDPath(uint32_t rx, uint32_t offset, uint32_t path_len) {
     if ((rx - offset) < sizeof(uint32_t) * path_len) {
         THROW(APDU_CODE_WRONG_LENGTH);
@@ -54,12 +56,12 @@ __Z_INLINE bool process_chunk(uint32_t rx) {
 
     uint32_t added = 0;
     switch (payloadType) {
-        case 0:
+        case P1_INIT:
             tx_initialize();
             tx_reset();
             tx_initialized = true;
             return false;
-        case 1:
+        case P1_ADD:
             if (!tx_initialized) {
                 THROW(APDU_CODE_TX_NOT_INITIALIZED);
             }
@@ -69,7 +71,7 @@ __Z_INLINE bool process_chunk(uint32_t rx) {
                 THROW(APDU_CODE_OUTPUT_BUFFER_TOO_SMALL);
             }
             return false;
-        case 2:
+        case P1_LAST:
             if (!tx_initialized) {
                 THROW(APDU_CODE_TX_NOT_INITIALIZED);
             }
@@ -78,6 +80,7 @@ __Z_INLINE bool process_chunk(uint32_t rx) {
                 tx_initialized = false;
                 THROW(APDU_CODE_OUTPUT_BUFFER_TOO_SMALL);
             }
+            tx_initialized = false;
             return true;
         default:
             tx_initialized = false;
@@ -148,6 +151,7 @@ __Z_INLINE void handleGetAddrSecp256K1(volatile uint32_t *flags, volatile uint32
     }
 
     if (requireConfirmation) {
+        review_pending = true;
         app_fill_address(addr_secp256k1);
 
         view_review_init(addr_getItem, addr_getNumItems, app_reply_address);
@@ -186,6 +190,7 @@ __Z_INLINE void SignSecp256K1(volatile uint32_t *flags, volatile uint32_t *tx, u
     zemu_log_stack("tx_parse done\n");
 
     CHECK_APP_CANARY()
+    review_pending = true;
     view_review_init(tx_getItem, tx_getNumItems, app_sign);
     view_review_show(REVIEW_TXN);
     *flags |= IO_ASYNCH_REPLY;
@@ -236,6 +241,10 @@ void handleApdu(volatile uint32_t *flags, volatile uint32_t *tx, uint32_t rx) {
                 THROW(APDU_CODE_WRONG_LENGTH);
             }
 
+            if (review_pending) {
+                THROW(APDU_CODE_COMMAND_NOT_ALLOWED);
+            }
+
             switch (G_io_apdu_buffer[OFFSET_INS]) {
                 case INS_GET_VERSION: {
                     handle_getversion(flags, tx, rx);
@@ -287,6 +296,7 @@ void handleApdu(volatile uint32_t *flags, volatile uint32_t *tx, uint32_t rx) {
             }
         }
         CATCH(EXCEPTION_IO_RESET) {
+            review_pending = false;
             THROW(EXCEPTION_IO_RESET);
         }
         CATCH_OTHER(err) {

app/src/common/actions.h

@@ -16,6 +16,7 @@
 #pragma once
 
 #include <os_io_seproxyhal.h>
+#include <stdbool.h>
 #include <stdint.h>
 
 #include "apdu_codes.h"
@@ -67,6 +68,16 @@
 
 extern uint8_t action_addr_len;
 
+extern bool review_pending;
+
+__Z_INLINE void set_review_pending(bool val) {
+    review_pending = val;
+}
+
+__Z_INLINE bool is_review_pending(void) {
+    return review_pending;
+}
+
 // helper function to get the presig_hash of the transaction being signed
 __Z_INLINE zxerr_t get_presig_hash(uint8_t *hash, uint16_t hashLen);
 
@@ -128,6 +139,7 @@ __Z_INLINE void app_sign() {
     }
 
     if (err != zxerr_ok) {
+        set_review_pending(false);
         set_code(G_io_apdu_buffer, 0, APDU_CODE_SIGN_VERIFY_ERROR);
         io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
         return;
@@ -139,6 +151,7 @@ __Z_INLINE void app_sign() {
     uint16_t replyLen = 0;
     err = crypto_sign(G_io_apdu_buffer, IO_APDU_BUFFER_SIZE - 3, presig_hash, CX_SHA256_SIZE, &replyLen);
     if (err != zxerr_ok) {
+        set_review_pending(false);
         set_code(G_io_apdu_buffer, 0, APDU_CODE_SIGN_VERIFY_ERROR);
         io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
         return;
@@ -180,23 +193,27 @@ __Z_INLINE void app_sign() {
             break;
         }
         default: {
+            set_review_pending(false);
             set_code(G_io_apdu_buffer, 0, APDU_CODE_SIGN_VERIFY_ERROR);
             io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
             return;
         }
     }
 
     if (replyLen == 0) {
+        set_review_pending(false);
         set_code(G_io_apdu_buffer, 0, APDU_CODE_SIGN_VERIFY_ERROR);
         io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
         return;
     }
 
+    set_review_pending(false);
     set_code(G_io_apdu_buffer, replyLen, APDU_CODE_OK);
     io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, replyLen + 2);
 }
 
 __Z_INLINE void app_reject() {
+    set_review_pending(false);
     tx_reset_state();
 
     set_code(G_io_apdu_buffer, 0, APDU_CODE_COMMAND_NOT_ALLOWED);
@@ -236,11 +253,13 @@ __Z_INLINE uint8_t app_fill_auth_pubkey(address_kind_e kind) {
 }
 
 __Z_INLINE void app_reply_address() {
+    set_review_pending(false);
     set_code(G_io_apdu_buffer, action_addr_len, APDU_CODE_OK);
     io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, action_addr_len + 2);
 }
 
 __Z_INLINE void app_reply_error() {
+    set_review_pending(false);
     set_code(G_io_apdu_buffer, 0, APDU_CODE_DATA_INVALID);
     io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
 }

app/src/crypto.c

@@ -240,7 +240,8 @@ zxerr_t crypto_sign(uint8_t *buffer, uint16_t signatureMaxlen, const uint8_t *me
     err_convert_e err = convertDERtoRSV(signature->der_signature, info, signature->r, signature->s, &signature->v);
 
     if (err != no_error) {
-        return zxerr_encoding_failed;
+        zxerr = zxerr_encoding_failed;
+        goto catch_cx_error;
     }
 
     // return actual size using value from signatureLength