From 4c2e1a5ebf43c63a3dd3ab38110ea99a0a0bda60 Mon Sep 17 00:00:00 2001
From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com>
Date: Thu, 21 Jan 2021 09:55:54 +0000
Subject: [PATCH] Sanitize transactions more (bp #14716) (#14734)

* `solana decode-transaction` no longer panics on unsanitary transactions

(cherry picked from commit e9b5d65f40aa607ec6e67afb49ca101cda5f7ce0)

* Ensure sanitary transactions

(cherry picked from commit 04ce33a04e0314c794dc1773d65c3b302abefe3e)

Co-authored-by: Michael Vines <mvines@gmail.com>
---
 core/src/rpc.rs               | 38 +++++++++++++++++++++++++++++------
 transaction-status/src/lib.rs | 20 ++++++++++++++++--
 2 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/core/src/rpc.rs b/core/src/rpc.rs
index 0cbad4d166..61d41c0a40 100644
--- a/core/src/rpc.rs
+++ b/core/src/rpc.rs
@@ -54,6 +54,7 @@ use solana_sdk::{
     epoch_schedule::EpochSchedule,
     hash::Hash,
     pubkey::Pubkey,
+    sanitize::Sanitize,
     signature::Signature,
     stake_history::StakeHistory,
     system_instruction,
@@ -2868,6 +2869,16 @@ fn deserialize_transaction(
             info!("transaction deserialize error: {:?}", err);
             Error::invalid_params(&err.to_string())
         })
+        .and_then(|transaction: Transaction| {
+            if let Err(err) = transaction.sanitize() {
+                Err(Error::invalid_params(format!(
+                    "invalid transaction: {}",
+                    err
+                )))
+            } else {
+                Ok(transaction)
+            }
+        })
         .map(|transaction| (wire_transaction, transaction))
 }
 
@@ -4513,7 +4524,7 @@ pub mod tests {
         assert_eq!(
             res,
             Some(
-                r#"{"jsonrpc":"2.0","error":{"code":-32002,"message":"Transaction simulation failed: Transaction failed to sanitize accounts offsets correctly","data":{"err":"SanitizeFailure","logs":[]}},"id":1}"#.to_string(),
+                r#"{"jsonrpc":"2.0","error":{"code":-32602,"message":"invalid transaction: index out of bounds"},"id":1}"#.to_string(),
             )
         );
         let mut bad_transaction = system_transaction::transfer(
@@ -4567,18 +4578,17 @@ pub mod tests {
             )
         );
 
-        // sendTransaction will fail due to no signer. Skip preflight so signature verification
-        // doesn't catch it
+        // sendTransaction will fail due to sanitization failure
         bad_transaction.signatures.clear();
         let req = format!(
-            r#"{{"jsonrpc":"2.0","id":1,"method":"sendTransaction","params":["{}", {{"skipPreflight": true}}]}}"#,
+            r#"{{"jsonrpc":"2.0","id":1,"method":"sendTransaction","params":["{}"]}}"#,
             bs58::encode(serialize(&bad_transaction).unwrap()).into_string()
         );
         let res = io.handle_request_sync(&req, meta);
         assert_eq!(
             res,
             Some(
-                r#"{"jsonrpc":"2.0","error":{"code":-32003,"message":"Transaction signature verification failure"},"id":1}"#.to_string(),
+                r#"{"jsonrpc":"2.0","error":{"code":-32602,"message":"invalid transaction: index out of bounds"},"id":1}"#.to_string(),
             )
         );
     }
@@ -6099,7 +6109,7 @@ pub mod tests {
     }
 
     #[test]
-    fn test_deserialize_transacion_too_large_payloads_fail() {
+    fn test_deserialize_transaction_too_large_payloads_fail() {
         // +2 because +1 still fits in base64 encoded worst-case
         let too_big = PACKET_DATA_SIZE + 2;
         let tx_ser = vec![0xffu8; too_big];
@@ -6140,4 +6150,20 @@ pub mod tests {
             expect
         );
     }
+
+    #[test]
+    fn test_deserialize_transaction_unsanitary() {
+        let unsanitary_tx58 = "ju9xZWuDBX4pRxX2oZkTjxU5jB4SSTgEGhX8bQ8PURNzyzqKMPPpNvWihx8zUe\
+             FfrbVNoAaEsNKZvGzAnTDy5bhNT9kt6KFCTBixpvrLCzg4M5UdFUQYrn1gdgjX\
+             pLHxcaShD81xBNaFDgnA2nkkdHnKtZt4hVSfKAmw3VRZbjrZ7L2fKZBx21CwsG\
+             hD6onjM2M3qZW5C8J6d1pj41MxKmZgPBSha3MyKkNLkAGFASK"
+            .to_string();
+
+        let expect58 =
+            Error::invalid_params("invalid transaction: index out of bounds".to_string());
+        assert_eq!(
+            deserialize_transaction(unsanitary_tx58, UiTransactionEncoding::Base58).unwrap_err(),
+            expect58
+        );
+    }
 }
diff --git a/transaction-status/src/lib.rs b/transaction-status/src/lib.rs
index bffb753144..445dc5a5b9 100644
--- a/transaction-status/src/lib.rs
+++ b/transaction-status/src/lib.rs
@@ -23,6 +23,7 @@ use solana_sdk::{
     instruction::CompiledInstruction,
     message::{Message, MessageHeader},
     pubkey::Pubkey,
+    sanitize::Sanitize,
     signature::Signature,
     transaction::{Result, Transaction, TransactionError},
 };
@@ -507,7 +508,7 @@ impl EncodedTransaction {
         }
     }
     pub fn decode(&self) -> Option<Transaction> {
-        match self {
+        let transaction: Option<Transaction> = match self {
             EncodedTransaction::Json(_) => None,
             EncodedTransaction::LegacyBinary(blob) => bs58::decode(blob)
                 .into_vec()
@@ -525,7 +526,8 @@ impl EncodedTransaction {
                 | UiTransactionEncoding::Json
                 | UiTransactionEncoding::JsonParsed => None,
             },
-        }
+        };
+        transaction.filter(|transaction| transaction.sanitize().is_ok())
     }
 }
 
@@ -533,6 +535,20 @@ impl EncodedTransaction {
 mod test {
     use super::*;
 
+    #[test]
+    fn test_decode_invalid_transaction() {
+        // This transaction will not pass sanitization
+        let unsanitary_transaction = EncodedTransaction::Binary(
+            "ju9xZWuDBX4pRxX2oZkTjxU5jB4SSTgEGhX8bQ8PURNzyzqKMPPpNvWihx8zUe\
+             FfrbVNoAaEsNKZvGzAnTDy5bhNT9kt6KFCTBixpvrLCzg4M5UdFUQYrn1gdgjX\
+             pLHxcaShD81xBNaFDgnA2nkkdHnKtZt4hVSfKAmw3VRZbjrZ7L2fKZBx21CwsG\
+             hD6onjM2M3qZW5C8J6d1pj41MxKmZgPBSha3MyKkNLkAGFASK"
+                .to_string(),
+            UiTransactionEncoding::Base58,
+        );
+        assert!(unsanitary_transaction.decode().is_none());
+    }
+
     #[test]
     fn test_satisfies_commitment() {
         let status = TransactionStatus {