Harden input validation in NEAR serialization and Zcash signing

rust/chains/tw_bitcoin/src/modules/compiler.rs

@@ -53,7 +53,7 @@ impl<Context: BitcoinSigningContext> BitcoinCompiler<Context> {
                 TxPlanner::plan(request)?.unsigned_tx
             },
             TransactionType::psbt(ref psbt) => {
-                Context::PsbtRequestHandler::parse_request(&input, psbt)?.unsigned_tx
+                Context::PsbtRequestHandler::parse_request(coin, &input, psbt)?.unsigned_tx
             },
             TransactionType::None => {
                 return SigningError::err(SigningErrorType::Error_invalid_params)
@@ -132,13 +132,13 @@ impl<Context: BitcoinSigningContext> BitcoinCompiler<Context> {
     }
 
     fn compile_psbt(
-        _coin: &dyn CoinContext,
+        coin: &dyn CoinContext,
         input: &Proto::SigningInput,
         psbt: &Proto::Psbt,
         signatures: Vec<SignatureBytes>,
     ) -> SigningResult<Proto::SigningOutput<'static>> {
         let PsbtRequest { unsigned_tx, .. } =
-            Context::PsbtRequestHandler::parse_request(input, psbt)?;
+            Context::PsbtRequestHandler::parse_request(coin, input, psbt)?;
         let fee = unsigned_tx.fee()?;
 
         SighashVerifier::verify_signatures(&unsigned_tx, &signatures)?;

rust/chains/tw_bitcoin/src/modules/planner/psbt_planner.rs

@@ -32,7 +32,7 @@ impl<Context: BitcoinSigningContext> PsbtPlanner<Context> {
     ) -> SigningResult<Proto::TransactionPlan<'static>> {
         let chain_info = chain_info(coin, &input.chain_info)?;
         let PsbtRequest { unsigned_tx, .. } =
-            Context::PsbtRequestHandler::parse_request(input, psbt_input)?;
+            Context::PsbtRequestHandler::parse_request(coin, input, psbt_input)?;
 
         let total_input = unsigned_tx.total_input()?;
         let fee_estimate = unsigned_tx.fee()?;

rust/chains/tw_bitcoin/src/modules/psbt_request/mod.rs

@@ -3,6 +3,7 @@
 // Copyright © 2017 Trust Wallet.
 
 use std::marker::PhantomData;
+use tw_coin_entry::coin_context::CoinContext;
 use tw_coin_entry::error::prelude::*;
 use tw_memory::Data;
 use tw_proto::BitcoinV2::Proto;
@@ -18,6 +19,7 @@ pub mod utxo_psbt;
 pub trait PsbtRequestHandler<Context: UtxoContext> {
     /// Parses a PSBT request from Protobuf.
     fn parse_request(
+        coin: &dyn CoinContext,
         input: &Proto::SigningInput,
         psbt_input: &Proto::Psbt,
     ) -> SigningResult<PsbtRequest<Context>>;
@@ -43,6 +45,7 @@ pub struct NoPsbtRequestBuilder;
 
 impl<Context: UtxoContext> PsbtRequestHandler<Context> for NoPsbtRequestBuilder {
     fn parse_request(
+        _coin: &dyn CoinContext,
         _input: &Proto::SigningInput,
         _psbt_input: &Proto::Psbt,
     ) -> SigningResult<PsbtRequest<Context>> {
@@ -77,8 +80,10 @@ mod tests {
         let psbt_input = Proto::Psbt::default();
 
         // Test parse_request
+        use tw_coin_entry::test_utils::test_context::TestCoinContext;
+        let coin = TestCoinContext::default();
         let result: SigningResult<PsbtRequest<StandardBitcoinContext>> =
-            NoPsbtRequestBuilder::parse_request(&input, &psbt_input);
+            NoPsbtRequestBuilder::parse_request(&coin, &input, &psbt_input);
         assert_eq!(
             *result.err().expect("Expected an error").error_type(),
             SigningErrorType::Error_not_supported

rust/chains/tw_bitcoin/src/modules/psbt_request/standard_psbt_request_handler.rs

@@ -7,6 +7,7 @@ use crate::modules::psbt_request::utxo_psbt::UtxoPsbt;
 use crate::modules::psbt_request::{PsbtRequest, PsbtRequestHandler};
 use crate::modules::signing_request::standard_signing_request::StandardSigningRequestBuilder;
 use std::marker::PhantomData;
+use tw_coin_entry::coin_context::CoinContext;
 use tw_coin_entry::error::prelude::*;
 use tw_memory::Data;
 use tw_proto::BitcoinV2::Proto;
@@ -24,6 +25,7 @@ where
     Context: UtxoContext<Transaction = Transaction, Psbt = Psbt>,
 {
     fn parse_request(
+        _coin: &dyn CoinContext,
         input: &Proto::SigningInput,
         psbt_input: &Proto::Psbt,
     ) -> SigningResult<PsbtRequest<Context>> {

rust/chains/tw_bitcoin/src/modules/signer.rs

@@ -80,15 +80,15 @@ impl<Context: BitcoinSigningContext> BitcoinSigner<Context> {
     }
 
     pub fn sign_psbt(
-        _coin: &dyn CoinContext,
+        coin: &dyn CoinContext,
         input: &Proto::SigningInput,
         psbt_input: &Proto::Psbt,
     ) -> SigningResult<Proto::SigningOutput<'static>> {
         let PsbtRequest {
             mut psbt,
             unsigned_tx,
             ..
-        } = Context::PsbtRequestHandler::parse_request(input, psbt_input)?;
+        } = Context::PsbtRequestHandler::parse_request(coin, input, psbt_input)?;
 
         let fee = unsigned_tx.fee()?;
 

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

@@ -2,19 +2,25 @@
 //
 // Copyright © 2017 Trust Wallet.
 
+use crate::address::ZcashAddress;
+use crate::context::ZcashContext;
 use crate::modules::pczt;
+use std::str::FromStr;
+use tw_bitcoin::modules::tx_builder::BitcoinChainInfo;
 use tw_coin_entry::error::prelude::{MapTWError, ResultContext, SigningErrorType, SigningResult};
+use tw_utxo::context::UtxoContext;
 use tw_utxo::script::Script;
 use tw_utxo::transaction::standard_transaction::TransactionOutput;
 
 /// Currently, we rely on `pczt` crate to build our own [`TransactionOutput`].
 pub struct OutputPczt<'a> {
     output: &'a pczt::transparent::Output,
+    chain_info: &'a BitcoinChainInfo,
 }
 
 impl<'a> OutputPczt<'a> {
-    pub fn new(output: &'a pczt::transparent::Output) -> Self {
-        OutputPczt { output }
+    pub fn new(output: &'a pczt::transparent::Output, chain_info: &'a BitcoinChainInfo) -> Self {
+        OutputPczt { output, chain_info }
     }
 
     pub fn build(self) -> SigningResult<TransactionOutput> {
@@ -25,9 +31,99 @@ 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, self.chain_info.to_address_prefixes())
+                    .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 chain_info() -> BitcoinChainInfo {
+        BitcoinChainInfo {
+            p2pkh_prefix: 0xB8,
+            p2sh_prefix: 0xBD,
+            hrp: None,
+        }
+    }
+
+    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 info = chain_info();
+        let output = make_output(hex_to_bytes(MERCHANT_SCRIPT), None);
+        assert!(OutputPczt::new(&output, &info).build().is_ok());
+    }
+
+    #[test]
+    fn test_matching_user_address_passes() {
+        let info = chain_info();
+        let output = make_output(hex_to_bytes(MERCHANT_SCRIPT), Some(MERCHANT_ADDR));
+        assert!(OutputPczt::new(&output, &info).build().is_ok());
+    }
+
+    #[test]
+    fn test_mismatched_user_address_rejected() {
+        let info = chain_info();
+        let output = make_output(hex_to_bytes(ATTACKER_SCRIPT), Some(MERCHANT_ADDR));
+        let result = OutputPczt::new(&output, &info).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 info = chain_info();
+        let output = make_output(hex_to_bytes(MERCHANT_SCRIPT), Some("not_a_valid_address"));
+        let result = OutputPczt::new(&output, &info).build();
+        assert!(result.is_err());
+    }
+}

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

@@ -9,7 +9,10 @@ use crate::modules::transaction_builder::ZcashTransactionBuilder;
 use crate::transaction::{ZcashTransaction, TRANSACTION_VERSION_4, TRANSACTION_VERSION_GROUP_ID};
 use std::marker::PhantomData;
 use tw_bitcoin::modules::psbt_request::{PsbtRequest, PsbtRequestHandler};
-use tw_bitcoin::modules::signing_request::standard_signing_request::StandardSigningRequestBuilder;
+use tw_bitcoin::modules::signing_request::standard_signing_request::{
+    chain_info, StandardSigningRequestBuilder,
+};
+use tw_coin_entry::coin_context::CoinContext;
 use tw_coin_entry::error::prelude::{ResultContext, SigningError, SigningErrorType, SigningResult};
 use tw_hash::H32;
 use tw_memory::Data;
@@ -24,9 +27,11 @@ where
     Context: UtxoContext<Transaction = ZcashTransaction, Psbt = pczt::Pczt>,
 {
     fn parse_request(
+        coin: &dyn CoinContext,
         input: &Proto::SigningInput,
         psbt_input: &Proto::Psbt,
     ) -> SigningResult<PsbtRequest<Context>> {
+        let chain_info = chain_info(coin, &input.chain_info)?;
         let pczt = pczt::Pczt::deserialize(&psbt_input.psbt)?;
 
         let version = pczt.global.tx_version;
@@ -76,7 +81,7 @@ where
 
         // Add all outputs to the unsigned transaction builder.
         for txout in pczt.transparent.outputs.iter() {
-            let output = OutputPczt::new(txout)
+            let output = OutputPczt::new(txout, &chain_info)
                 .build()
                 .context("Error creating Output from PCZT")?;
             builder.push_output(output);

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,45 @@ 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)
+}
+
+fn sign_pczt_with_user_address(user_address: &str) -> SigningError {
+    const PRIVATE_KEY: &str = "c9d84f11d992c1a527293b468ba67f739f8098c333748493da45b9cf53844ec4";
+    let psbt = pczt_with_user_address(Some(user_address));
+
+    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();
+    signer.sign(CoinType::Zcash, signing).error
+}
+
+#[test]
+fn test_zcash_sign_pczt_matching_user_address() {
+    assert_eq!(
+        sign_pczt_with_user_address("t1X2gmNQRqwWiGincK2UtYwZbWg7ixYFEKr"),
+        SigningError::OK
+    );
+}
+
+#[test]
+fn test_zcash_sign_pczt_mismatched_user_address_rejected() {
+    assert_eq!(
+        sign_pczt_with_user_address("t1gWVE2uyrET2CxSmCaBiKzmWxQdHhnvMSz"),
+        SigningError::Error_invalid_address
+    );
+}