Merge #9650: Better handle invalid parameters to signrawtransaction

6dbfe08 [qa] test signrawtransaction merge with missing inputs (Matt Corallo)
ec4f7e4 [qa] Add second input to signrawtransaction test case (Matt Corallo)
691710a [qa] Test that decoderawtransaction throws with extra data appended (Matt Corallo)
922bea9 Better handle invalid parameters to signrawtransaction (Matt Corallo)
7ea0ad5 Fail in DecodeHexTx if there is extra data at the end (Matt Corallo)

3 files changed, 36 insertions, 4 deletions

diff --git a/qa/rpc-tests/signrawtransactions.py b/qa/rpc-tests/signrawtransactions.py
index c61a28061..109312bd5 100755
--- a/qa/rpc-tests/signrawtransactions.py
+++ b/qa/rpc-tests/signrawtransactions.py
@@ -26,12 +26,14 @@ class SignRawTransactionsTest(BitcoinTestFramework):
 
         1) The transaction has a complete set of signatures
         2) No script verification error occurred"""
-        privKeys = ['cUeKHd5orzT3mz8P9pxyREHfsWtVfgsfDjiZZBcjUBAaGk1BTj7N']
+        privKeys = ['cUeKHd5orzT3mz8P9pxyREHfsWtVfgsfDjiZZBcjUBAaGk1BTj7N', 'cVKpPfVKSJxKqVpE9awvXNWuLHCa5j5tiE7K6zbUSptFpTEtiFrA']
 
         inputs = [
-            # Valid pay-to-pubkey script
+            # Valid pay-to-pubkey scripts
             {'txid': '9b907ef1e3c26fc71fe4a4b3580bc75264112f95050014157059c736f0202e71', 'vout': 0,
-             'scriptPubKey': '76a91460baa0f494b38ce3c940dea67f3804dc52d1fb9488ac'}
+             'scriptPubKey': '76a91460baa0f494b38ce3c940dea67f3804dc52d1fb9488ac'},
+            {'txid': '83a4f6a6b73660e13ee6cb3c6063fa3759c50c9b7521d0536022961898f4fb02', 'vout': 0,
+             'scriptPubKey': '76a914669b857c03a5ed269d5d85a1ffac9ed5d663072788ac'},
         ]
 
         outputs = {'mpLQjfK79b7CCV4VMJWEWAj5Mpx8Up5zxB': 0.1}
@@ -46,6 +48,22 @@ class SignRawTransactionsTest(BitcoinTestFramework):
         # 2) No script verification error occurred
         assert 'errors' not in rawTxSigned
 
+        # Check that signrawtransaction doesn't blow up on garbage merge attempts
+        dummyTxInconsistent = self.nodes[0].createrawtransaction([inputs[0]], outputs)
+        rawTxUnsigned = self.nodes[0].signrawtransaction(rawTx + dummyTxInconsistent, inputs)
+
+        assert 'complete' in rawTxUnsigned
+        assert_equal(rawTxUnsigned['complete'], False)
+
+        # Check that signrawtransaction properly merges unsigned and signed txn, even with garbage in the middle
+        rawTxSigned2 = self.nodes[0].signrawtransaction(rawTxUnsigned["hex"] + dummyTxInconsistent + rawTxSigned["hex"], inputs)
+
+        assert 'complete' in rawTxSigned2
+        assert_equal(rawTxSigned2['complete'], True)
+
+        assert 'errors' not in rawTxSigned2
+
+
     def script_verification_error_test(self):
         """Creates and signs a raw transaction with valid (vin 0), invalid (vin 1) and one missing (vin 2) input script.
 
@@ -78,6 +96,16 @@ class SignRawTransactionsTest(BitcoinTestFramework):
         outputs = {'mpLQjfK79b7CCV4VMJWEWAj5Mpx8Up5zxB': 0.1}
 
         rawTx = self.nodes[0].createrawtransaction(inputs, outputs)
+
+        # Make sure decoderawtransaction is at least marginally sane
+        decodedRawTx = self.nodes[0].decoderawtransaction(rawTx)
+        for i, inp in enumerate(inputs):
+            assert_equal(decodedRawTx["vin"][i]["txid"], inp["txid"])
+            assert_equal(decodedRawTx["vin"][i]["vout"], inp["vout"])
+
+        # Make sure decoderawtransaction throws if there is extra data
+        assert_raises(JSONRPCException, self.nodes[0].decoderawtransaction, rawTx + "00")
+
         rawTxSigned = self.nodes[0].signrawtransaction(rawTx, scripts, privKeys)
 
         # 3) The transaction has no complete set of signatures
diff --git a/src/core_read.cpp b/src/core_read.cpp
index a5f232c22..a8d667e3b 100644
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -111,6 +111,8 @@ bool DecodeHexTx(CMutableTransaction& tx, const std::string& strHexTx, bool fTry
     CDataStream ssData(txData, SER_NETWORK, PROTOCOL_VERSION);
     try {
         ssData >> tx;
+        if (!ssData.empty())
+            return false;
     }
     catch (const std::exception&) {
         return false;
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index c10de45f8..21396ebb0 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -835,7 +835,9 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
 
         // ... and merge in other signatures:
         BOOST_FOREACH(const CMutableTransaction& txv, txVariants) {
-            sigdata = CombineSignatures(prevPubKey, TransactionSignatureChecker(&txConst, i, amount), sigdata, DataFromTransaction(txv, i));
+            if (txv.vin.size() > i) {
+                sigdata = CombineSignatures(prevPubKey, TransactionSignatureChecker(&txConst, i, amount), sigdata, DataFromTransaction(txv, i));
+            }
         }
 
         UpdateTransaction(mergedTx, i, sigdata);