Harden input validation in NEAR serialization and Zcash signing

rust/chains/tw_zcash/src/modules/pczt_request/output_pczt.rs

@@ -2,11 +2,18 @@
 //
 // Copyright © 2017 Trust Wallet.
 
+use crate::address::ZcashAddress;
+use crate::context::ZcashContext;
 use crate::modules::pczt;
+use std::str::FromStr;
 use tw_coin_entry::error::prelude::{MapTWError, ResultContext, SigningErrorType, SigningResult};
+use tw_utxo::context::{AddressPrefixes, UtxoContext};
 use tw_utxo::script::Script;
 use tw_utxo::transaction::standard_transaction::TransactionOutput;
 
+const ZCASH_P2PKH_PREFIX: u8 = 0xB8;
+const ZCASH_P2SH_PREFIX: u8 = 0xBD;
+
 /// Currently, we rely on `pczt` crate to build our own [`TransactionOutput`].
 pub struct OutputPczt<'a> {
     output: &'a pczt::transparent::Output,
@@ -25,9 +32,92 @@ impl<'a> OutputPczt<'a> {
             .tw_err(SigningErrorType::Error_invalid_requested_token_amount)
             .context("PCZT Output amount is too large")?;
         let script_pubkey = Script::from(self.output.script_pubkey.clone());
+
+        if let Some(ref addr_str) = self.output.user_address {
+            let addr = ZcashAddress::from_str(addr_str)
+                .tw_err(SigningErrorType::Error_invalid_address)
+                .context("PCZT Output user_address is not a valid Zcash address")?;
+            let expected_script = ZcashContext::addr_to_script_pubkey(
+                &addr,
+                AddressPrefixes {
+                    p2pkh_prefix: ZCASH_P2PKH_PREFIX,
+                    p2sh_prefix: ZCASH_P2SH_PREFIX,
+                },
+            )
+            .context("PCZT Output user_address cannot be converted to a script")?;
+            if expected_script != script_pubkey {
+                return tw_coin_entry::error::prelude::SigningError::err(
+                    SigningErrorType::Error_invalid_address,
+                )
+                .context("PCZT Output user_address does not match script_pubkey");
+            }
+        }
+
         Ok(TransactionOutput {
             value,
             script_pubkey,
         })
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::collections::BTreeMap;
+
+    fn make_output(
+        script_pubkey: Vec<u8>,
+        user_address: Option<&str>,
+    ) -> pczt::transparent::Output {
+        pczt::transparent::Output {
+            value: 100_000,
+            script_pubkey,
+            redeem_script: None,
+            bip32_derivation: BTreeMap::new(),
+            user_address: user_address.map(str::to_owned),
+            proprietary: BTreeMap::new(),
+        }
+    }
+
+    const ATTACKER_SCRIPT: &str = "76a91449964a736f3713d64283fd0018626ba50091c7e988ac";
+
+    const MERCHANT_ADDR: &str = "t1gWVE2uyrET2CxSmCaBiKzmWxQdHhnvMSz";
+    const MERCHANT_SCRIPT: &str = "76a914f84c7f4dd3c3dc311676444fdead6e6d290d50e388ac";
+
+    fn hex_to_bytes(hex: &str) -> Vec<u8> {
+        (0..hex.len())
+            .step_by(2)
+            .map(|i| u8::from_str_radix(&hex[i..i + 2], 16).unwrap())
+            .collect()
+    }
+
+    #[test]
+    fn test_no_user_address_passes() {
+        let output = make_output(hex_to_bytes(MERCHANT_SCRIPT), None);
+        assert!(OutputPczt::new(&output).build().is_ok());
+    }
+
+    #[test]
+    fn test_matching_user_address_passes() {
+        let output = make_output(hex_to_bytes(MERCHANT_SCRIPT), Some(MERCHANT_ADDR));
+        assert!(OutputPczt::new(&output).build().is_ok());
+    }
+
+    #[test]
+    fn test_mismatched_user_address_rejected() {
+        let output = make_output(hex_to_bytes(ATTACKER_SCRIPT), Some(MERCHANT_ADDR));
+        let result = OutputPczt::new(&output).build();
+        assert!(result.is_err());
+        assert_eq!(
+            *result.unwrap_err().error_type(),
+            tw_coin_entry::error::prelude::SigningErrorType::Error_invalid_address,
+        );
+    }
+
+    #[test]
+    fn test_invalid_user_address_rejected() {
+        let output = make_output(hex_to_bytes(MERCHANT_SCRIPT), Some("not_a_valid_address"));
+        let result = OutputPczt::new(&output).build();
+        assert!(result.is_err());
+    }
+}

rust/tw_tests/Cargo.toml

@@ -24,3 +24,4 @@ tw_solana = { path = "../chains/tw_solana" }
 tw_ton = { path = "../chains/tw_ton" }
 tw_ton_sdk = { path = "../frameworks/tw_ton_sdk" }
 tw_utxo = { path = "../frameworks/tw_utxo" }
+tw_zcash = { path = "../chains/tw_zcash" }

rust/tw_tests/tests/chains/zcash/zcash_sign.rs

@@ -9,10 +9,12 @@ use crate::chains::common::bitcoin::{
 use crate::chains::zcash::{zcash_extra_data, zec_info, NU6_BRANCH_ID, SAPLING_BRANCH_ID};
 use tw_any_coin::test_utils::sign_utils::AnySignerHelper;
 use tw_coin_registry::coin_type::CoinType;
+use tw_encoding::base64::{self, STANDARD};
 use tw_encoding::hex::DecodeHex;
 use tw_proto::BitcoinV2::Proto;
 use tw_proto::Common::Proto::SigningError;
 use tw_proto::Zcash::Proto as ZcashProto;
+use tw_zcash::modules::pczt::Pczt;
 
 /// Successfully broadcasted:
 /// https://explorer.zcha.in/transactions/ec9033381c1cc53ada837ef9981c03ead1c7c41700ff3a954389cfaddc949256
@@ -314,3 +316,48 @@ fn test_zcash_sign_pczt_unsupported_tx_version() {
     let output = signer.sign(CoinType::Zcash, signing);
     assert_eq!(output.error, SigningError::Error_not_supported);
 }
+
+/// Builds a modified PCZT from the base PCZT used in `test_zcash_sign_pczt`,
+/// setting `user_address` on the first output.
+fn pczt_with_user_address(user_address: Option<&str>) -> String {
+    const BASE_PSBT: &str = "UENaVAEAAAAEhcG8yQjVoJzHDAEAz+i/AYUBgwACD/xF9t6uGgnrn6HPk4TTxoTB42mxjfZ2E8dQ7UzfodcAAf////8PAAAAgIl6GXapFFWjZeeDsUiGjDI8hm1k+WCt0MtqiKwAAAEAAAAAAACg+XQ96jWHJsJWFA3/kk+6ZaHhUI5wi57Hf3ZM6xQzRQEB/////w8AAAC1lhgZdqkUVaNl54OxSIaMMjyGbWT5YK3Qy2qIrAAAAQAAAAAAAALD8X4ZdqkUkFFaBL/xtThv5Hnc0F1hHGFf2sSIrAAAASN0MVgyZ21OUVJxd1dpR2luY0syVXRZd1piV2c3aXhZRkVLcgDCwxEZdqkUVaNl54OxSIaMMjyGbWT5YK3Qy2qIrAAAASN0MVJnUkJqam54WFNnMXB0TERya2FNTmlWNHRKVlh1N2RXVgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAwABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==";
+    let bytes = base64::decode(BASE_PSBT, STANDARD).unwrap();
+    let mut pczt = Pczt::deserialize(&bytes).unwrap();
+    pczt.transparent.outputs[0].user_address = user_address.map(str::to_owned);
+    let serialized = pczt.serialize().unwrap();
+    base64::encode(&serialized, STANDARD)
+}
+
+#[test]
+fn test_zcash_sign_pczt_matching_user_address() {
+    const PRIVATE_KEY: &str = "c9d84f11d992c1a527293b468ba67f739f8098c333748493da45b9cf53844ec4";
+    let psbt = pczt_with_user_address(Some("t1X2gmNQRqwWiGincK2UtYwZbWg7ixYFEKr"));
+
+    let signing = Proto::SigningInput {
+        private_keys: vec![PRIVATE_KEY.decode_hex().unwrap().into()],
+        chain_info: zec_info(),
+        transaction: transaction_psbt_b64(&psbt),
+        ..Default::default()
+    };
+
+    let mut signer = AnySignerHelper::<Proto::SigningOutput>::default();
+    let output = signer.sign(CoinType::Zcash, signing);
+    assert_eq!(output.error, SigningError::OK);
+}
+
+#[test]
+fn test_zcash_sign_pczt_mismatched_user_address_rejected() {
+    const PRIVATE_KEY: &str = "c9d84f11d992c1a527293b468ba67f739f8098c333748493da45b9cf53844ec4";
+    let psbt = pczt_with_user_address(Some("t1gWVE2uyrET2CxSmCaBiKzmWxQdHhnvMSz"));
+
+    let signing = Proto::SigningInput {
+        private_keys: vec![PRIVATE_KEY.decode_hex().unwrap().into()],
+        chain_info: zec_info(),
+        transaction: transaction_psbt_b64(&psbt),
+        ..Default::default()
+    };
+
+    let mut signer = AnySignerHelper::<Proto::SigningOutput>::default();
+    let output = signer.sign(CoinType::Zcash, signing);
+    assert_eq!(output.error, SigningError::Error_invalid_address);
+}

src/NEAR/Serialization.cpp

@@ -8,6 +8,7 @@
 #include "../PrivateKey.h"
 
 #include <nlohmann/json.hpp>
+#include <stdexcept>
 
 namespace TW::NEAR {
 
@@ -28,7 +29,9 @@ static void writeU64(Data& data, uint64_t number) {
 }
 
 static void writeU128(Data& data, const std::string& numberData) {
-    assert(numberData.size() == 16 && "U128 number should be 16 bytes long");
+    if (numberData.size() != 16) {
+        throw std::invalid_argument("U128 number should be exactly 16 bytes long");
+    }
     data.insert(std::end(data), std::begin(numberData), std::end(numberData));
 }