From 5abb0f5ac37e8a17072d5989a025227035fdc7e6 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 24 Feb 2023 16:45:01 -0500 Subject: [PATCH 1/5] Do base type propagation in miniscript_stable fuzzer Keep track of which base type (B, K, V, or W) is desired in the miniscript_stable ConsumeStableNode function. This allows aborting early if the constructed node won't have the right type. Note that this does not change the fuzzer format; the meaning of inputs in ConsumeStableNode is unmodified. The only change is that often the fuzzer will abort early. The direct motivation is preventing recursing v: wrappers, which are the only fragment type that does not otherwise increase the overall minimum possible script size. In a later commit this will be exploited to prevent overly-large scripts from being constructed. --- src/test/fuzz/miniscript.cpp | 113 ++++++++++++++++++++++++++--------- 1 file changed, 85 insertions(+), 28 deletions(-) diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp index 73096cd5ca..3d210aa7a6 100644 --- a/src/test/fuzz/miniscript.cpp +++ b/src/test/fuzz/miniscript.cpp @@ -329,27 +329,51 @@ std::optional ConsumeTimeLock(FuzzedDataProvider& provider) { * bytes as the number of keys define the index of each key in the test data. * - For thresh(), the next byte defines the threshold value and the following one the number of subs. */ -std::optional ConsumeNodeStable(FuzzedDataProvider& provider) { +std::optional ConsumeNodeStable(FuzzedDataProvider& provider, Type type_needed) { + bool allow_B = (type_needed == ""_mst) || (type_needed << "B"_mst); + bool allow_K = (type_needed == ""_mst) || (type_needed << "K"_mst); + bool allow_V = (type_needed == ""_mst) || (type_needed << "V"_mst); + bool allow_W = (type_needed == ""_mst) || (type_needed << "W"_mst); + switch (provider.ConsumeIntegral()) { - case 0: return {{Fragment::JUST_0}}; - case 1: return {{Fragment::JUST_1}}; - case 2: return {{Fragment::PK_K, ConsumePubKey(provider)}}; - case 3: return {{Fragment::PK_H, ConsumePubKey(provider)}}; + case 0: + if (!allow_B) return {}; + return {{Fragment::JUST_0}}; + case 1: + if (!allow_B) return {}; + return {{Fragment::JUST_1}}; + case 2: + if (!allow_K) return {}; + return {{Fragment::PK_K, ConsumePubKey(provider)}}; + case 3: + if (!allow_K) return {}; + return {{Fragment::PK_H, ConsumePubKey(provider)}}; case 4: { + if (!allow_B) return {}; const auto k = ConsumeTimeLock(provider); if (!k) return {}; return {{Fragment::OLDER, *k}}; } case 5: { + if (!allow_B) return {}; const auto k = ConsumeTimeLock(provider); if (!k) return {}; return {{Fragment::AFTER, *k}}; } - case 6: return {{Fragment::SHA256, ConsumeSha256(provider)}}; - case 7: return {{Fragment::HASH256, ConsumeHash256(provider)}}; - case 8: return {{Fragment::RIPEMD160, ConsumeRipemd160(provider)}}; - case 9: return {{Fragment::HASH160, ConsumeHash160(provider)}}; + case 6: + if (!allow_B) return {}; + return {{Fragment::SHA256, ConsumeSha256(provider)}}; + case 7: + if (!allow_B) return {}; + return {{Fragment::HASH256, ConsumeHash256(provider)}}; + case 8: + if (!allow_B) return {}; + return {{Fragment::RIPEMD160, ConsumeRipemd160(provider)}}; + case 9: + if (!allow_B) return {}; + return {{Fragment::HASH160, ConsumeHash160(provider)}}; case 10: { + if (!allow_B) return {}; const auto k = provider.ConsumeIntegral(); const auto n_keys = provider.ConsumeIntegral(); if (n_keys > 20 || k == 0 || k > n_keys) return {}; @@ -357,26 +381,59 @@ std::optional ConsumeNodeStable(FuzzedDataProvider& provider) { for (auto& key: keys) key = ConsumePubKey(provider); return {{Fragment::MULTI, k, std::move(keys)}}; } - case 11: return {{3, Fragment::ANDOR}}; - case 12: return {{2, Fragment::AND_V}}; - case 13: return {{2, Fragment::AND_B}}; - case 15: return {{2, Fragment::OR_B}}; - case 16: return {{2, Fragment::OR_C}}; - case 17: return {{2, Fragment::OR_D}}; - case 18: return {{2, Fragment::OR_I}}; + case 11: + if (!(allow_B || allow_K || allow_V)) return {}; + return {{{"B"_mst, type_needed, type_needed}, Fragment::ANDOR}}; + case 12: + if (!(allow_B || allow_K || allow_V)) return {}; + return {{{"V"_mst, type_needed}, Fragment::AND_V}}; + case 13: + if (!allow_B) return {}; + return {{{"B"_mst, "W"_mst}, Fragment::AND_B}}; + case 15: + if (!allow_B) return {}; + return {{{"B"_mst, "W"_mst}, Fragment::OR_B}}; + case 16: + if (!allow_V) return {}; + return {{{"B"_mst, "V"_mst}, Fragment::OR_C}}; + case 17: + if (!allow_B) return {}; + return {{{"B"_mst, "B"_mst}, Fragment::OR_D}}; + case 18: + if (!(allow_B || allow_K || allow_V)) return {}; + return {{{type_needed, type_needed}, Fragment::OR_I}}; case 19: { + if (!allow_B) return {}; auto k = provider.ConsumeIntegral(); auto n_subs = provider.ConsumeIntegral(); if (k == 0 || k > n_subs) return {}; - return {{n_subs, Fragment::THRESH, k}}; + std::vector subtypes; + subtypes.reserve(n_subs); + subtypes.emplace_back("B"_mst); + for (size_t i = 1; i < n_subs; ++i) subtypes.emplace_back("W"_mst); + return {{std::move(subtypes), Fragment::THRESH, k}}; } - case 20: return {{1, Fragment::WRAP_A}}; - case 21: return {{1, Fragment::WRAP_S}}; - case 22: return {{1, Fragment::WRAP_C}}; - case 23: return {{1, Fragment::WRAP_D}}; - case 24: return {{1, Fragment::WRAP_V}}; - case 25: return {{1, Fragment::WRAP_J}}; - case 26: return {{1, Fragment::WRAP_N}}; + case 20: + if (!allow_W) return {}; + return {{{"B"_mst}, Fragment::WRAP_A}}; + case 21: + if (!allow_W) return {}; + return {{{"B"_mst}, Fragment::WRAP_S}}; + case 22: + if (!allow_B) return {}; + return {{{"K"_mst}, Fragment::WRAP_C}}; + case 23: + if (!allow_B) return {}; + return {{{"V"_mst}, Fragment::WRAP_D}}; + case 24: + if (!allow_V) return {}; + return {{{"B"_mst}, Fragment::WRAP_V}}; + case 25: + if (!allow_B) return {}; + return {{{"B"_mst}, Fragment::WRAP_J}}; + case 26: + if (!allow_B) return {}; + return {{{"B"_mst}, Fragment::WRAP_N}}; default: break; } @@ -709,7 +766,7 @@ std::optional ConsumeNodeSmart(FuzzedDataProvider& provider, Type type * a NodeRef whose Type() matches the type fed to ConsumeNode. */ template -NodeRef GenNode(F ConsumeNode, Type root_type = ""_mst, bool strict_valid = false) { +NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { /** A stack of miniscript Nodes being built up. */ std::vector stack; /** The queue of instructions. */ @@ -921,9 +978,9 @@ void FuzzInitSmart() FUZZ_TARGET_INIT(miniscript_stable, FuzzInit) { FuzzedDataProvider provider(buffer.data(), buffer.size()); - TestNode(GenNode([&](Type) { - return ConsumeNodeStable(provider); - }), provider); + TestNode(GenNode([&](Type needed_type) { + return ConsumeNodeStable(provider, needed_type); + }, ""_mst), provider); } /** Fuzz target that runs TestNode on nodes generated using ConsumeNodeSmart. */ From e1f30414c6b9434048e089ccc3ec4f475f980c60 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 24 Feb 2023 16:47:51 -0500 Subject: [PATCH 2/5] Simplify miniscript fuzzer NodeInfo struct Since we now keep track of all expected child node types (even if rudimentary) in both miniscript_stable and miniscript_smart fuzzers, there is no need anymore for the former shortcut NodeInfo constructors without sub types. --- src/test/fuzz/miniscript.cpp | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp index 3d210aa7a6..1e11c3323c 100644 --- a/src/test/fuzz/miniscript.cpp +++ b/src/test/fuzz/miniscript.cpp @@ -261,8 +261,6 @@ template NodeRef MakeNodeRef(Args&&... args) { struct NodeInfo { //! The type of this node Fragment fragment; - //! Number of subs of this node - uint8_t n_subs; //! The timelock value for older() and after(), the threshold value for multi() and thresh() uint32_t k; //! Keys for this node, if it has some @@ -272,15 +270,13 @@ struct NodeInfo { //! The type requirements for the children of this node. std::vector subtypes; - NodeInfo(Fragment frag): fragment(frag), n_subs(0), k(0) {} - NodeInfo(Fragment frag, CPubKey key): fragment(frag), n_subs(0), k(0), keys({key}) {} - NodeInfo(Fragment frag, uint32_t _k): fragment(frag), n_subs(0), k(_k) {} - NodeInfo(Fragment frag, std::vector h): fragment(frag), n_subs(0), k(0), hash(std::move(h)) {} - NodeInfo(uint8_t subs, Fragment frag): fragment(frag), n_subs(subs), k(0), subtypes(subs, ""_mst) {} - NodeInfo(uint8_t subs, Fragment frag, uint32_t _k): fragment(frag), n_subs(subs), k(_k), subtypes(subs, ""_mst) {} - NodeInfo(std::vector subt, Fragment frag): fragment(frag), n_subs(subt.size()), k(0), subtypes(std::move(subt)) {} - NodeInfo(std::vector subt, Fragment frag, uint32_t _k): fragment(frag), n_subs(subt.size()), k(_k), subtypes(std::move(subt)) {} - NodeInfo(Fragment frag, uint32_t _k, std::vector _keys): fragment(frag), n_subs(0), k(_k), keys(std::move(_keys)) {} + NodeInfo(Fragment frag): fragment(frag), k(0) {} + NodeInfo(Fragment frag, CPubKey key): fragment(frag), k(0), keys({key}) {} + NodeInfo(Fragment frag, uint32_t _k): fragment(frag), k(_k) {} + NodeInfo(Fragment frag, std::vector h): fragment(frag), k(0), hash(std::move(h)) {} + NodeInfo(std::vector subt, Fragment frag): fragment(frag), k(0), subtypes(std::move(subt)) {} + NodeInfo(std::vector subt, Fragment frag, uint32_t _k): fragment(frag), k(_k), subtypes(std::move(subt)) {} + NodeInfo(Fragment frag, uint32_t _k, std::vector _keys): fragment(frag), k(_k), keys(std::move(_keys)) {} }; /** Pick an index in a collection from a single byte in the fuzzer's output. */ @@ -795,11 +791,11 @@ NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { NodeInfo& info = *todo.back().second; // Gather children from the back of stack. std::vector sub; - sub.reserve(info.n_subs); - for (size_t i = 0; i < info.n_subs; ++i) { - sub.push_back(std::move(*(stack.end() - info.n_subs + i))); + sub.reserve(info.subtypes.size()); + for (size_t i = 0; i < info.subtypes.size(); ++i) { + sub.push_back(std::move(*(stack.end() - info.subtypes.size() + i))); } - stack.erase(stack.end() - info.n_subs, stack.end()); + stack.erase(stack.end() - info.subtypes.size(), stack.end()); // Construct new NodeRef. NodeRef node; if (info.keys.empty()) { From 213fffa5138229eac2d4a9eda0f643fe90870378 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sat, 25 Feb 2023 15:35:43 -0500 Subject: [PATCH 3/5] Enforce type consistency in miniscript_stable fuzz test Add a self-check to the fuzzer that the constructed types match the expected types in the miniscript_stable fuzzer too. --- src/test/fuzz/miniscript.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp index 1e11c3323c..df55fc0429 100644 --- a/src/test/fuzz/miniscript.cpp +++ b/src/test/fuzz/miniscript.cpp @@ -806,10 +806,13 @@ NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { node = MakeNodeRef(info.fragment, std::move(info.keys), info.k); } // Verify acceptability. - if (!node || !(node->GetType() << type_needed)) { + if (!node || (node->GetType() & "KVWB"_mst) == ""_mst) { assert(!strict_valid); return {}; } + if (!(type_needed == ""_mst)) { + assert(node->GetType() << type_needed); + } if (!node->IsValid()) return {}; // Move it to the stack. stack.push_back(std::move(node)); From bcec5ab4ff1039c0c309dbbb9953adbd0a4f3e88 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sat, 25 Feb 2023 16:05:04 -0500 Subject: [PATCH 4/5] Make miniscript fuzzers avoid ops limit Keep track of the total number of ops the constructed script will have during miniscript_stable and miniscript_smart fuzzers' GenNode, so it can abort early if the 201 ops limit would be exceeded. Also add a self-check that the final constructed node has the predicted ops size limit, so we know the fuzzer's logic for keeping track of this is correct. --- src/script/miniscript.h | 3 ++ src/test/fuzz/miniscript.cpp | 74 ++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index c42b530c4d..bb42bf3c92 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -1136,6 +1136,9 @@ public: //! Return the maximum number of ops needed to satisfy this script non-malleably. uint32_t GetOps() const { 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; } diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp index df55fc0429..9597c289fa 100644 --- a/src/test/fuzz/miniscript.cpp +++ b/src/test/fuzz/miniscript.cpp @@ -767,6 +767,8 @@ NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { std::vector stack; /** The queue of instructions. */ std::vector>> todo{{root_type, {}}}; + /** Predict the number of (static) script ops. */ + uint32_t ops{0}; while (!todo.empty()) { // The expected type we have to construct. @@ -775,6 +777,72 @@ NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { // Fragment/children have not been decided yet. Decide them. auto node_info = ConsumeNode(type_needed); if (!node_info) return {}; + // Update predicted resource limits. + switch (node_info->fragment) { + case Fragment::JUST_0: + case Fragment::JUST_1: + break; + case Fragment::PK_K: + break; + case Fragment::PK_H: + ops += 3; + break; + case Fragment::OLDER: + case Fragment::AFTER: + ops += 1; + break; + case Fragment::RIPEMD160: + case Fragment::SHA256: + case Fragment::HASH160: + case Fragment::HASH256: + ops += 4; + break; + case Fragment::ANDOR: + ops += 3; + break; + case Fragment::AND_V: + break; + case Fragment::AND_B: + case Fragment::OR_B: + ops += 1; + break; + case Fragment::OR_C: + ops += 2; + break; + case Fragment::OR_D: + ops += 3; + break; + case Fragment::OR_I: + ops += 3; + break; + case Fragment::THRESH: + ops += node_info->subtypes.size(); + break; + case Fragment::MULTI: + ops += 1; + break; + case Fragment::WRAP_A: + ops += 2; + break; + case Fragment::WRAP_S: + ops += 1; + break; + case Fragment::WRAP_C: + ops += 1; + break; + case Fragment::WRAP_D: + ops += 3; + break; + case Fragment::WRAP_V: + break; + case Fragment::WRAP_J: + ops += 4; + break; + case Fragment::WRAP_N: + ops += 1; + break; + } + if (ops > MAX_OPS_PER_SCRIPT) return {}; auto subtypes = node_info->subtypes; todo.back().second = std::move(node_info); todo.reserve(todo.size() + subtypes.size()); @@ -814,12 +882,18 @@ NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { assert(node->GetType() << type_needed); } if (!node->IsValid()) return {}; + // Update resource predictions. + if (node->fragment == Fragment::WRAP_V && node->subs[0]->GetType() << "x"_mst) { + ops += 1; + } + if (ops > MAX_OPS_PER_SCRIPT) return {}; // Move it to the stack. stack.push_back(std::move(node)); todo.pop_back(); } } assert(stack.size() == 1); + assert(stack[0]->GetStaticOps() == ops); stack[0]->DuplicateKeyCheck(KEY_COMP); return std::move(stack[0]); } From 56e37e71a2538a240cc360678aeb752d17bd8f45 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sat, 25 Feb 2023 16:19:12 -0500 Subject: [PATCH 5/5] Make miniscript fuzzers avoid script size limit Use the same technique as is using in the FromString miniscript parser to predict the final script size of the miniscript being generated in the miniscript_stable and miniscript_smart fuzzers (by counting every unexplored sub node as 1 script byte, which is possible because every leaf node always adds at least 1 byte). This allows bailing out early if the script being generated would exceed the maximum allowed size (before actually constructing the miniscript, as that may happen only significantly later potentially). Also add a self-check to make sure this predicted script size matches that of generated scripts. --- src/test/fuzz/miniscript.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp index 9597c289fa..f3a48daf2c 100644 --- a/src/test/fuzz/miniscript.cpp +++ b/src/test/fuzz/miniscript.cpp @@ -769,6 +769,9 @@ NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { std::vector>> todo{{root_type, {}}}; /** Predict the number of (static) script ops. */ uint32_t ops{0}; + /** Predict the total script size (every unexplored subnode is counted as one, as every leaf is + * at least one script byte). */ + uint32_t scriptsize{1}; while (!todo.empty()) { // The expected type we have to construct. @@ -777,7 +780,11 @@ NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { // Fragment/children have not been decided yet. Decide them. auto node_info = ConsumeNode(type_needed); if (!node_info) return {}; - // Update predicted resource limits. + // Update predicted resource limits. Since every leaf Miniscript node is at least one + // byte long, we move one byte from each child to their parent. A similar technique is + // used in the miniscript::internal::Parse function to prevent runaway string parsing. + scriptsize += miniscript::internal::ComputeScriptLen(node_info->fragment, ""_mst, node_info->subtypes.size(), node_info->k, node_info->subtypes.size(), node_info->keys.size()) - 1; + if (scriptsize > MAX_STANDARD_P2WSH_SCRIPT_SIZE) return {}; switch (node_info->fragment) { case Fragment::JUST_0: case Fragment::JUST_1: @@ -834,6 +841,8 @@ NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { ops += 3; break; case Fragment::WRAP_V: + // We don't account for OP_VERIFY here; that will be corrected for when the actual + // node is constructed below. break; case Fragment::WRAP_J: ops += 4; @@ -885,8 +894,10 @@ NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { // Update resource predictions. if (node->fragment == Fragment::WRAP_V && node->subs[0]->GetType() << "x"_mst) { ops += 1; + scriptsize += 1; } if (ops > MAX_OPS_PER_SCRIPT) return {}; + if (scriptsize > MAX_STANDARD_P2WSH_SCRIPT_SIZE) return {}; // Move it to the stack. stack.push_back(std::move(node)); todo.pop_back(); @@ -894,6 +905,7 @@ NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) { } assert(stack.size() == 1); assert(stack[0]->GetStaticOps() == ops); + assert(stack[0]->ScriptSize() == scriptsize); stack[0]->DuplicateKeyCheck(KEY_COMP); return std::move(stack[0]); }