From 3c481f8921bbc587cf287329f39243abe703b868 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 20 Jun 2019 18:01:23 +1000 Subject: [PATCH 1/2] signrawtransactionwithkey: better error messages for bad redeemScript/witnessScript This adds checks to ensure the redeemScript/witnessScript actually correspond to the provided scriptPubKey, and, if both are provided, that they are sensibly related to each other. Thanks to github user passionofvc for raising this issue. --- src/rpc/rawtransaction_util.cpp | 73 +++++++++++++++++++++------ test/functional/rpc_createmultisig.py | 21 ++++++++ 2 files changed, 78 insertions(+), 16 deletions(-) diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index 697c6d45c4a..e05f28347f4 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -196,32 +196,73 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst } // if redeemScript and private keys were given, add redeemScript to the keystore so it can be signed - if (keystore && (scriptPubKey.IsPayToScriptHash() || scriptPubKey.IsPayToWitnessScriptHash())) { + const bool is_p2sh = scriptPubKey.IsPayToScriptHash(); + const bool is_p2wsh = scriptPubKey.IsPayToWitnessScriptHash(); + if (keystore && (is_p2sh || is_p2wsh)) { RPCTypeCheckObj(prevOut, { {"redeemScript", UniValueType(UniValue::VSTR)}, {"witnessScript", UniValueType(UniValue::VSTR)}, }, true); UniValue rs = find_value(prevOut, "redeemScript"); - if (!rs.isNull()) { - std::vector rsData(ParseHexV(rs, "redeemScript")); - CScript redeemScript(rsData.begin(), rsData.end()); - keystore->AddCScript(redeemScript); - // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH). - // This is only for compatibility, it is encouraged to use the explicit witnessScript field instead. - keystore->AddCScript(GetScriptForWitness(redeemScript)); - } UniValue ws = find_value(prevOut, "witnessScript"); - if (!ws.isNull()) { - std::vector wsData(ParseHexV(ws, "witnessScript")); - CScript witnessScript(wsData.begin(), wsData.end()); - keystore->AddCScript(witnessScript); - // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH). - keystore->AddCScript(GetScriptForWitness(witnessScript)); - } if (rs.isNull() && ws.isNull()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Missing redeemScript/witnessScript"); } + + // work from witnessScript when possible + std::vector scriptData(!ws.isNull() ? ParseHexV(ws, "witnessScript") : ParseHexV(rs, "redeemScript")); + CScript script(scriptData.begin(), scriptData.end()); + keystore->AddCScript(script); + // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH). + // This is done for redeemScript only for compatibility, it is encouraged to use the explicit witnessScript field instead. + CScript witness_output_script{GetScriptForWitness(script)}; + keystore->AddCScript(witness_output_script); + + if (!ws.isNull() && !rs.isNull()) { + // if both witnessScript and redeemScript are provided, + // they should either be the same (for backwards compat), + // or the redeemScript should be the encoded form of + // the witnessScript (ie, for p2sh-p2wsh) + if (ws.get_str() != rs.get_str()) { + std::vector redeemScriptData(ParseHexV(rs, "redeemScript")); + CScript redeemScript(redeemScriptData.begin(), redeemScriptData.end()); + if (redeemScript != witness_output_script) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "redeemScript does not correspond to witnessScript"); + } + } + } + + if (is_p2sh) { + const CTxDestination p2sh{ScriptHash(script)}; + const CTxDestination p2sh_p2wsh{ScriptHash(witness_output_script)}; + if (scriptPubKey == GetScriptForDestination(p2sh)) { + // traditional p2sh; arguably an error if + // we got here with rs.IsNull(), because + // that means the p2sh script was specified + // via witnessScript param, but for now + // we'll just quietly accept it + } else if (scriptPubKey == GetScriptForDestination(p2sh_p2wsh)) { + // p2wsh encoded as p2sh; ideally the witness + // script was specified in the witnessScript + // param, but also support specifying it via + // redeemScript param for backwards compat + // (in which case ws.IsNull() == true) + } else { + // otherwise, can't generate scriptPubKey from + // either script, so we got unusable parameters + throw JSONRPCError(RPC_INVALID_PARAMETER, "redeemScript/witnessScript does not match scriptPubKey"); + } + } else if (is_p2wsh) { + // plain p2wsh; could throw an error if script + // was specified by redeemScript rather than + // witnessScript (ie, ws.IsNull() == true), but + // accept it for backwards compat + const CTxDestination p2wsh{WitnessV0ScriptHash(script)}; + if (scriptPubKey != GetScriptForDestination(p2wsh)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "redeemScript/witnessScript does not match scriptPubKey"); + } + } } } } diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index 62f38437567..056e193d55a 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -134,6 +134,27 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): assert_raises_rpc_error(-8, "Missing redeemScript/witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + # if witnessScript specified, all ok + prevtx_err["witnessScript"] = prevtxs[0]["redeemScript"] + node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + + # both specified, also ok + prevtx_err["redeemScript"] = prevtxs[0]["redeemScript"] + node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + + # redeemScript mismatch to witnessScript + prevtx_err["redeemScript"] = "6a" # OP_RETURN + assert_raises_rpc_error(-8, "redeemScript does not correspond to witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + + # redeemScript does not match scriptPubKey + del prevtx_err["witnessScript"] + assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + + # witnessScript does not match scriptPubKey + prevtx_err["witnessScript"] = prevtx_err["redeemScript"] + del prevtx_err["redeemScript"] + assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + rawtx2 = node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs - 1], prevtxs) rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [self.priv[-1]], prevtxs) From ec4c79326bb670c2cc1757ecfb1900f8460c5257 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 20 Jun 2019 19:40:29 +1000 Subject: [PATCH 2/2] signrawtransaction*: improve error for partial signing Thanks to Danial Jaffy (tipu) for reporting this issue. --- src/rpc/rawtransaction_util.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index e05f28347f4..f1d176ba4d1 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -309,6 +309,9 @@ UniValue SignTransaction(CMutableTransaction& mtx, const SigningProvider* keysto if (serror == SCRIPT_ERR_INVALID_STACK_OPERATION) { // Unable to sign input and verification failed (possible attempt to partially sign). TxInErrorToJSON(txin, vErrors, "Unable to sign input, invalid stack size (possibly missing key)"); + } else if (serror == SCRIPT_ERR_SIG_NULLFAIL) { + // Verification failed (possibly due to insufficient signatures). + TxInErrorToJSON(txin, vErrors, "CHECK(MULTI)SIG failing with non-zero signature (possibly need more signatures)"); } else { TxInErrorToJSON(txin, vErrors, ScriptErrorString(serror)); }