aboutsummaryrefslogtreecommitdiff
path: root/src
diff options
context:
space:
mode:
authorAndrew Chow <github@achow101.com>2023-07-17 19:08:56 -0400
committerAndrew Chow <github@achow101.com>2023-07-17 19:16:09 -0400
commitbc88f3ab903f8a748a7570a6e17579dc4e9ba791 (patch)
tree690ff2f9c0a2e7e5096a10a1b422ab64b4a314c7 /src
parent306157ae92f47b36a7ad438cf76969a1ab6ef401 (diff)
parentc7db88af71b3204171f33399aa4f33b40a4f7cd9 (diff)
Merge bitcoin/bitcoin#27997: Descriptors: rule out unspendable miniscript descriptors
c7db88af71b3204171f33399aa4f33b40a4f7cd9 descriptor: assert we never parse a sane miniscript with no pubkey (Antoine Poinsot) a49402a9ec7431c286139b76f8759719a99a8551 qa: make sure we don't let unspendable Miniscript descriptors be imported (Antoine Poinsot) 639e3b6c9759a7a582c5c86fdbfa5ea99cb7bb16 descriptor: refuse to parse unspendable miniscript descriptors (Antoine Poinsot) e3280eae1b53006d74d11f3cf9d7a9dc7ff2c39e miniscript: make GetStackSize() and GetOps() return optionals (Antoine Poinsot) Pull request description: `IsSane()` in Miniscript does not ensure a Script is actually spendable. This is an issue as we would accept any sane Miniscript when parsing a descriptor. Fix this by explicitly checking a Miniscript descriptor is both sane and spendable when parsing it. This bug was exposed due to a check added in #22838 (https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226859880) that triggered a fuzz crash (https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1612510057). ACKs for top commit: sipa: utACK c7db88af71b3204171f33399aa4f33b40a4f7cd9 achow101: ACK c7db88af71b3204171f33399aa4f33b40a4f7cd9 Tree-SHA512: e79bc9f7842e98a4e8f358f05811fca51b15b4b80a171c0d2b17cf4bb1f578a18e4397bc2ece9817d392e0de0196ee6a054b7318441fd3566dd22e1f03eb64a5
Diffstat (limited to 'src')
-rw-r--r--src/script/descriptor.cpp10
-rw-r--r--src/script/miniscript.h23
-rw-r--r--src/test/fuzz/miniscript.cpp7
-rw-r--r--src/test/miniscript_tests.cpp10
4 files changed, 36 insertions, 14 deletions
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index dff6429259..f57f167e5d 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -15,6 +15,7 @@
#include <common/args.h>
#include <span.h>
#include <util/bip32.h>
+#include <util/check.h>
#include <util/spanparsing.h>
#include <util/strencodings.h>
#include <util/vector.h>
@@ -1553,14 +1554,14 @@ std::unique_ptr<DescriptorImpl> ParseScript(uint32_t& key_exp_index, Span<const
error = std::move(parser.m_key_parsing_error);
return nullptr;
}
- if (!node->IsSane()) {
+ if (!node->IsSane() || node->IsNotSatisfiable()) {
// Try to find the first insane sub for better error reporting.
auto insane_node = node.get();
if (const auto sub = node->FindInsaneSub()) insane_node = sub;
if (const auto str = insane_node->ToString(parser)) error = *str;
if (!insane_node->IsValid()) {
error += " is invalid";
- } else {
+ } else if (!node->IsSane()) {
error += " is not sane";
if (!insane_node->IsNonMalleable()) {
error += ": malleable witnesses exist";
@@ -1573,9 +1574,14 @@ std::unique_ptr<DescriptorImpl> ParseScript(uint32_t& key_exp_index, Span<const
} else if (!insane_node->ValidSatisfactions()) {
error += ": needs witnesses that may exceed resource limits";
}
+ } else {
+ error += " is not satisfiable";
}
return nullptr;
}
+ // A signature check is required for a miniscript to be sane. Therefore no sane miniscript
+ // may have an empty list of public keys.
+ CHECK_NONFATAL(!parser.m_keys.empty());
return std::make_unique<MiniscriptDescriptor>(std::move(parser.m_keys), std::move(node));
}
}
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 7c1a87a7dc..b58740a125 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -1134,20 +1134,35 @@ public:
size_t ScriptSize() const { return scriptlen; }
//! Return the maximum number of ops needed to satisfy this script non-malleably.
- uint32_t GetOps() const { return ops.count + ops.sat.value; }
+ std::optional<uint32_t> GetOps() const {
+ if (!ops.sat.valid) return {};
+ return ops.count + ops.sat.value;
+ }
//! Return the number of ops in the script (not counting the dynamic ones that depend on execution).
uint32_t GetStaticOps() const { return ops.count; }
//! Check the ops limit of this script against the consensus limit.
- bool CheckOpsLimit() const { return GetOps() <= MAX_OPS_PER_SCRIPT; }
+ bool CheckOpsLimit() const {
+ if (const auto ops = GetOps()) return *ops <= MAX_OPS_PER_SCRIPT;
+ return true;
+ }
/** Return the maximum number of stack elements needed to satisfy this script non-malleably, including
* the script push. */
- uint32_t GetStackSize() const { return ss.sat.value + 1; }
+ std::optional<uint32_t> GetStackSize() const {
+ if (!ss.sat.valid) return {};
+ return ss.sat.value + 1;
+ }
//! Check the maximum stack size for this script against the policy limit.
- bool CheckStackSize() const { return GetStackSize() - 1 <= MAX_STANDARD_P2WSH_STACK_ITEMS; }
+ bool CheckStackSize() const {
+ if (const auto ss = GetStackSize()) return *ss - 1 <= MAX_STANDARD_P2WSH_STACK_ITEMS;
+ return true;
+ }
+
+ //! Whether no satisfaction exists for this node.
+ bool IsNotSatisfiable() const { return !GetStackSize(); }
//! Return the expression type.
Type GetType() const { return typ; }
diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp
index 08c147af15..56327b9665 100644
--- a/src/test/fuzz/miniscript.cpp
+++ b/src/test/fuzz/miniscript.cpp
@@ -943,7 +943,8 @@ void TestNode(const NodeRef& node, FuzzedDataProvider& provider)
assert(decoded->ToScript(PARSER_CTX) == script);
assert(decoded->GetType() == node->GetType());
- if (provider.ConsumeBool() && node->GetOps() < MAX_OPS_PER_SCRIPT && node->ScriptSize() < MAX_STANDARD_P2WSH_SCRIPT_SIZE) {
+ const auto node_ops{node->GetOps()};
+ if (provider.ConsumeBool() && node_ops && *node_ops < MAX_OPS_PER_SCRIPT && node->ScriptSize() < MAX_STANDARD_P2WSH_SCRIPT_SIZE) {
// Optionally pad the script with OP_NOPs to max op the ops limit of the constructed script.
// This makes the script obviously not actually miniscript-compatible anymore, but the
// signatures constructed in this test don't commit to the script anyway, so the same
@@ -954,7 +955,7 @@ void TestNode(const NodeRef& node, FuzzedDataProvider& provider)
// Do not pad more than what would cause MAX_STANDARD_P2WSH_SCRIPT_SIZE to be reached, however,
// as that also invalidates scripts.
int add = std::min<int>(
- MAX_OPS_PER_SCRIPT - node->GetOps(),
+ MAX_OPS_PER_SCRIPT - *node_ops,
MAX_STANDARD_P2WSH_SCRIPT_SIZE - node->ScriptSize());
for (int i = 0; i < add; ++i) script.push_back(OP_NOP);
}
@@ -972,7 +973,7 @@ void TestNode(const NodeRef& node, FuzzedDataProvider& provider)
if (nonmal_success) {
// Non-malleable satisfactions are bounded by GetStackSize().
- assert(witness_nonmal.stack.size() <= node->GetStackSize());
+ assert(witness_nonmal.stack.size() <= *node->GetStackSize());
// If a non-malleable satisfaction exists, the malleable one must also exist, and be identical to it.
assert(mal_success);
assert(witness_nonmal.stack == witness_mal.stack);
diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index 42e441c41a..2d183c8844 100644
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -297,7 +297,7 @@ void TestSatisfy(const std::string& testcase, const NodeRef& node) {
if (nonmal_success) {
// Non-malleable satisfactions are bounded by GetStackSize().
- BOOST_CHECK(witness_nonmal.stack.size() <= node->GetStackSize());
+ BOOST_CHECK(witness_nonmal.stack.size() <= *node->GetStackSize());
// If a non-malleable satisfaction exists, the malleable one must also exist, and be identical to it.
BOOST_CHECK(mal_success);
BOOST_CHECK(witness_nonmal.stack == witness_mal.stack);
@@ -375,8 +375,8 @@ void Test(const std::string& ms, const std::string& hexscript, int mode, int ops
auto inferred_miniscript = miniscript::FromScript(computed_script, CONVERTER);
BOOST_CHECK_MESSAGE(inferred_miniscript, "Cannot infer miniscript from script: " + ms);
BOOST_CHECK_MESSAGE(inferred_miniscript->ToScript(CONVERTER) == computed_script, "Roundtrip failure: miniscript->script != miniscript->script->miniscript->script: " + ms);
- if (opslimit != -1) BOOST_CHECK_MESSAGE((int)node->GetOps() == opslimit, "Ops limit mismatch: " << ms << " (" << node->GetOps() << " vs " << opslimit << ")");
- if (stacklimit != -1) BOOST_CHECK_MESSAGE((int)node->GetStackSize() == stacklimit, "Stack limit mismatch: " << ms << " (" << node->GetStackSize() << " vs " << stacklimit << ")");
+ if (opslimit != -1) BOOST_CHECK_MESSAGE((int)*node->GetOps() == opslimit, "Ops limit mismatch: " << ms << " (" << *node->GetOps() << " vs " << opslimit << ")");
+ if (stacklimit != -1) BOOST_CHECK_MESSAGE((int)*node->GetStackSize() == stacklimit, "Stack limit mismatch: " << ms << " (" << *node->GetStackSize() << " vs " << stacklimit << ")");
TestSatisfy(ms, node);
}
}
@@ -498,8 +498,8 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
// For CHECKMULTISIG the OP cost is the number of keys, but the stack size is the number of sigs (+1)
const auto ms_multi = miniscript::FromString("multi(1,03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65,03fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556,0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)", CONVERTER);
BOOST_CHECK(ms_multi);
- BOOST_CHECK_EQUAL(ms_multi->GetOps(), 4); // 3 pubkeys + CMS
- BOOST_CHECK_EQUAL(ms_multi->GetStackSize(), 3); // 1 sig + dummy elem + script push
+ BOOST_CHECK_EQUAL(*ms_multi->GetOps(), 4); // 3 pubkeys + CMS
+ BOOST_CHECK_EQUAL(*ms_multi->GetStackSize(), 3); // 1 sig + dummy elem + script push
// The 'd:' wrapper leaves on the stack what was DUP'ed at the beginning of its execution.
// Since it contains an OP_IF just after on the same element, we can make sure that the element
// in question must be OP_1 if OP_IF enforces that its argument must only be OP_1 or the empty
generated by cgit v1.2.3 (git 2.26.2) at 2025-03-17 06:34:34 +0000