Skip to content

Commit cb40639

Browse files
fanquakefanquake
committed
Merge bitcoin/bitcoin#27165: Make miniscript_{stable,smart} fuzzers avoid too large scripts
56e37e7 Make miniscript fuzzers avoid script size limit (Pieter Wuille) bcec5ab Make miniscript fuzzers avoid ops limit (Pieter Wuille) 213fffa Enforce type consistency in miniscript_stable fuzz test (Pieter Wuille) e1f3041 Simplify miniscript fuzzer NodeInfo struct (Pieter Wuille) 5abb0f5 Do base type propagation in miniscript_stable fuzzer (Pieter Wuille) Pull request description: This adds a number of improvements to the miniscript fuzzers that all amount to rejecting invalid or overly big miniscripts early on: * Base type propagation in the miniscript_stable fuzzers prevents constructing a large portion of miniscripts that would be illegal, with just a little bit of type logic in the fuzzer. The fuzzer input format is unchanged. * Ops and script size tracking in GenNode means that too-large scripts (either due to script size limit or ops limit) will be detected on the fly during fuzz input processing, before actually constructing the scripts. Closes #27147. ACKs for top commit: darosior: re-ACK 56e37e7 dergoegge: tACK 56e37e7 Tree-SHA512: 245584adf9a6644a35fe103bc81b619e5b4f5d467571a761b5809d08b1dec48f7ceaf4d8791ccd8208b45c6b309d2ccca23b3d1ec5399df76cd5bf88f2263280
2 parents 4398cfb + 56e37e7 commit cb40639

File tree

2 files changed

+189
-44
lines changed

2 files changed

+189
-44
lines changed

‎src/script/miniscript.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1136,6 +1136,9 @@ struct Node {
11361136
//! Return the maximum number of ops needed to satisfy this script non-malleably.
11371137
uint32_t GetOps() const { return ops.count + ops.sat.value; }
11381138

1139+
//! Return the number of ops in the script (not counting the dynamic ones that depend on execution).
1140+
uint32_t GetStaticOps() const { return ops.count; }
1141+
11391142
//! Check the ops limit of this script against the consensus limit.
11401143
bool CheckOpsLimit() const { return GetOps() <= MAX_OPS_PER_SCRIPT; }
11411144

‎src/test/fuzz/miniscript.cpp‎

Lines changed: 186 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,6 @@ template<typename... Args> NodeRef MakeNodeRef(Args&&... args) {
261261
struct NodeInfo {
262262
//! The type of this node
263263
Fragment fragment;
264-
//! Number of subs of this node
265-
uint8_t n_subs;
266264
//! The timelock value for older() and after(), the threshold value for multi() and thresh()
267265
uint32_t k;
268266
//! Keys for this node, if it has some
@@ -272,15 +270,13 @@ struct NodeInfo {
272270
//! The type requirements for the children of this node.
273271
std::vector<Type> subtypes;
274272

275-
NodeInfo(Fragment frag): fragment(frag), n_subs(0), k(0) {}
276-
NodeInfo(Fragment frag, CPubKey key): fragment(frag), n_subs(0), k(0), keys({key}) {}
277-
NodeInfo(Fragment frag, uint32_t _k): fragment(frag), n_subs(0), k(_k) {}
278-
NodeInfo(Fragment frag, std::vector<unsigned char> h): fragment(frag), n_subs(0), k(0), hash(std::move(h)) {}
279-
NodeInfo(uint8_t subs, Fragment frag): fragment(frag), n_subs(subs), k(0), subtypes(subs, ""_mst) {}
280-
NodeInfo(uint8_t subs, Fragment frag, uint32_t _k): fragment(frag), n_subs(subs), k(_k), subtypes(subs, ""_mst) {}
281-
NodeInfo(std::vector<Type> subt, Fragment frag): fragment(frag), n_subs(subt.size()), k(0), subtypes(std::move(subt)) {}
282-
NodeInfo(std::vector<Type> subt, Fragment frag, uint32_t _k): fragment(frag), n_subs(subt.size()), k(_k), subtypes(std::move(subt)) {}
283-
NodeInfo(Fragment frag, uint32_t _k, std::vector<CPubKey> _keys): fragment(frag), n_subs(0), k(_k), keys(std::move(_keys)) {}
273+
NodeInfo(Fragment frag): fragment(frag), k(0) {}
274+
NodeInfo(Fragment frag, CPubKey key): fragment(frag), k(0), keys({key}) {}
275+
NodeInfo(Fragment frag, uint32_t _k): fragment(frag), k(_k) {}
276+
NodeInfo(Fragment frag, std::vector<unsigned char> h): fragment(frag), k(0), hash(std::move(h)) {}
277+
NodeInfo(std::vector<Type> subt, Fragment frag): fragment(frag), k(0), subtypes(std::move(subt)) {}
278+
NodeInfo(std::vector<Type> subt, Fragment frag, uint32_t _k): fragment(frag), k(_k), subtypes(std::move(subt)) {}
279+
NodeInfo(Fragment frag, uint32_t _k, std::vector<CPubKey> _keys): fragment(frag), k(_k), keys(std::move(_keys)) {}
284280
};
285281

286282
/** Pick an index in a collection from a single byte in the fuzzer's output. */
@@ -329,54 +325,111 @@ std::optional<uint32_t> ConsumeTimeLock(FuzzedDataProvider& provider) {
329325
* bytes as the number of keys define the index of each key in the test data.
330326
* - For thresh(), the next byte defines the threshold value and the following one the number of subs.
331327
*/
332-
std::optional<NodeInfo> ConsumeNodeStable(FuzzedDataProvider& provider) {
328+
std::optional<NodeInfo> ConsumeNodeStable(FuzzedDataProvider& provider, Type type_needed) {
329+
bool allow_B = (type_needed == ""_mst) || (type_needed << "B"_mst);
330+
bool allow_K = (type_needed == ""_mst) || (type_needed << "K"_mst);
331+
bool allow_V = (type_needed == ""_mst) || (type_needed << "V"_mst);
332+
bool allow_W = (type_needed == ""_mst) || (type_needed << "W"_mst);
333+
333334
switch (provider.ConsumeIntegral<uint8_t>()) {
334-
case 0: return {{Fragment::JUST_0}};
335-
case 1: return {{Fragment::JUST_1}};
336-
case 2: return {{Fragment::PK_K, ConsumePubKey(provider)}};
337-
case 3: return {{Fragment::PK_H, ConsumePubKey(provider)}};
335+
case 0:
336+
if (!allow_B) return {};
337+
return {{Fragment::JUST_0}};
338+
case 1:
339+
if (!allow_B) return {};
340+
return {{Fragment::JUST_1}};
341+
case 2:
342+
if (!allow_K) return {};
343+
return {{Fragment::PK_K, ConsumePubKey(provider)}};
344+
case 3:
345+
if (!allow_K) return {};
346+
return {{Fragment::PK_H, ConsumePubKey(provider)}};
338347
case 4: {
348+
if (!allow_B) return {};
339349
const auto k = ConsumeTimeLock(provider);
340350
if (!k) return {};
341351
return {{Fragment::OLDER, *k}};
342352
}
343353
case 5: {
354+
if (!allow_B) return {};
344355
const auto k = ConsumeTimeLock(provider);
345356
if (!k) return {};
346357
return {{Fragment::AFTER, *k}};
347358
}
348-
case 6: return {{Fragment::SHA256, ConsumeSha256(provider)}};
349-
case 7: return {{Fragment::HASH256, ConsumeHash256(provider)}};
350-
case 8: return {{Fragment::RIPEMD160, ConsumeRipemd160(provider)}};
351-
case 9: return {{Fragment::HASH160, ConsumeHash160(provider)}};
359+
case 6:
360+
if (!allow_B) return {};
361+
return {{Fragment::SHA256, ConsumeSha256(provider)}};
362+
case 7:
363+
if (!allow_B) return {};
364+
return {{Fragment::HASH256, ConsumeHash256(provider)}};
365+
case 8:
366+
if (!allow_B) return {};
367+
return {{Fragment::RIPEMD160, ConsumeRipemd160(provider)}};
368+
case 9:
369+
if (!allow_B) return {};
370+
return {{Fragment::HASH160, ConsumeHash160(provider)}};
352371
case 10: {
372+
if (!allow_B) return {};
353373
const auto k = provider.ConsumeIntegral<uint8_t>();
354374
const auto n_keys = provider.ConsumeIntegral<uint8_t>();
355375
if (n_keys > 20 || k == 0 || k > n_keys) return {};
356376
std::vector<CPubKey> keys{n_keys};
357377
for (auto& key: keys) key = ConsumePubKey(provider);
358378
return {{Fragment::MULTI, k, std::move(keys)}};
359379
}
360-
case 11: return {{3, Fragment::ANDOR}};
361-
case 12: return {{2, Fragment::AND_V}};
362-
case 13: return {{2, Fragment::AND_B}};
363-
case 15: return {{2, Fragment::OR_B}};
364-
case 16: return {{2, Fragment::OR_C}};
365-
case 17: return {{2, Fragment::OR_D}};
366-
case 18: return {{2, Fragment::OR_I}};
380+
case 11:
381+
if (!(allow_B || allow_K || allow_V)) return {};
382+
return {{{"B"_mst, type_needed, type_needed}, Fragment::ANDOR}};
383+
case 12:
384+
if (!(allow_B || allow_K || allow_V)) return {};
385+
return {{{"V"_mst, type_needed}, Fragment::AND_V}};
386+
case 13:
387+
if (!allow_B) return {};
388+
return {{{"B"_mst, "W"_mst}, Fragment::AND_B}};
389+
case 15:
390+
if (!allow_B) return {};
391+
return {{{"B"_mst, "W"_mst}, Fragment::OR_B}};
392+
case 16:
393+
if (!allow_V) return {};
394+
return {{{"B"_mst, "V"_mst}, Fragment::OR_C}};
395+
case 17:
396+
if (!allow_B) return {};
397+
return {{{"B"_mst, "B"_mst}, Fragment::OR_D}};
398+
case 18:
399+
if (!(allow_B || allow_K || allow_V)) return {};
400+
return {{{type_needed, type_needed}, Fragment::OR_I}};
367401
case 19: {
402+
if (!allow_B) return {};
368403
auto k = provider.ConsumeIntegral<uint8_t>();
369404
auto n_subs = provider.ConsumeIntegral<uint8_t>();
370405
if (k == 0 || k > n_subs) return {};
371-
return {{n_subs, Fragment::THRESH, k}};
406+
std::vector<Type> subtypes;
407+
subtypes.reserve(n_subs);
408+
subtypes.emplace_back("B"_mst);
409+
for (size_t i = 1; i < n_subs; ++i) subtypes.emplace_back("W"_mst);
410+
return {{std::move(subtypes), Fragment::THRESH, k}};
372411
}
373-
case 20: return {{1, Fragment::WRAP_A}};
374-
case 21: return {{1, Fragment::WRAP_S}};
375-
case 22: return {{1, Fragment::WRAP_C}};
376-
case 23: return {{1, Fragment::WRAP_D}};
377-
case 24: return {{1, Fragment::WRAP_V}};
378-
case 25: return {{1, Fragment::WRAP_J}};
379-
case 26: return {{1, Fragment::WRAP_N}};
412+
case 20:
413+
if (!allow_W) return {};
414+
return {{{"B"_mst}, Fragment::WRAP_A}};
415+
case 21:
416+
if (!allow_W) return {};
417+
return {{{"B"_mst}, Fragment::WRAP_S}};
418+
case 22:
419+
if (!allow_B) return {};
420+
return {{{"K"_mst}, Fragment::WRAP_C}};
421+
case 23:
422+
if (!allow_B) return {};
423+
return {{{"V"_mst}, Fragment::WRAP_D}};
424+
case 24:
425+
if (!allow_V) return {};
426+
return {{{"B"_mst}, Fragment::WRAP_V}};
427+
case 25:
428+
if (!allow_B) return {};
429+
return {{{"B"_mst}, Fragment::WRAP_J}};
430+
case 26:
431+
if (!allow_B) return {};
432+
return {{{"B"_mst}, Fragment::WRAP_N}};
380433
default:
381434
break;
382435
}
@@ -709,11 +762,16 @@ std::optional<NodeInfo> ConsumeNodeSmart(FuzzedDataProvider& provider, Type type
709762
* a NodeRef whose Type() matches the type fed to ConsumeNode.
710763
*/
711764
template<typename F>
712-
NodeRef GenNode(F ConsumeNode, Type root_type = ""_mst, bool strict_valid = false) {
765+
NodeRef GenNode(F ConsumeNode, Type root_type, bool strict_valid = false) {
713766
/** A stack of miniscript Nodes being built up. */
714767
std::vector<NodeRef> stack;
715768
/** The queue of instructions. */
716769
std::vector<std::pair<Type, std::optional<NodeInfo>>> todo{{root_type, {}}};
770+
/** Predict the number of (static) script ops. */
771+
uint32_t ops{0};
772+
/** Predict the total script size (every unexplored subnode is counted as one, as every leaf is
773+
* at least one script byte). */
774+
uint32_t scriptsize{1};
717775

718776
while (!todo.empty()) {
719777
// The expected type we have to construct.
@@ -722,6 +780,78 @@ NodeRef GenNode(F ConsumeNode, Type root_type = ""_mst, bool strict_valid = fals
722780
// Fragment/children have not been decided yet. Decide them.
723781
auto node_info = ConsumeNode(type_needed);
724782
if (!node_info) return {};
783+
// Update predicted resource limits. Since every leaf Miniscript node is at least one
784+
// byte long, we move one byte from each child to their parent. A similar technique is
785+
// used in the miniscript::internal::Parse function to prevent runaway string parsing.
786+
scriptsize += miniscript::internal::ComputeScriptLen(node_info->fragment, ""_mst, node_info->subtypes.size(), node_info->k, node_info->subtypes.size(), node_info->keys.size()) - 1;
787+
if (scriptsize > MAX_STANDARD_P2WSH_SCRIPT_SIZE) return {};
788+
switch (node_info->fragment) {
789+
case Fragment::JUST_0:
790+
case Fragment::JUST_1:
791+
break;
792+
case Fragment::PK_K:
793+
break;
794+
case Fragment::PK_H:
795+
ops += 3;
796+
break;
797+
case Fragment::OLDER:
798+
case Fragment::AFTER:
799+
ops += 1;
800+
break;
801+
case Fragment::RIPEMD160:
802+
case Fragment::SHA256:
803+
case Fragment::HASH160:
804+
case Fragment::HASH256:
805+
ops += 4;
806+
break;
807+
case Fragment::ANDOR:
808+
ops += 3;
809+
break;
810+
case Fragment::AND_V:
811+
break;
812+
case Fragment::AND_B:
813+
case Fragment::OR_B:
814+
ops += 1;
815+
break;
816+
case Fragment::OR_C:
817+
ops += 2;
818+
break;
819+
case Fragment::OR_D:
820+
ops += 3;
821+
break;
822+
case Fragment::OR_I:
823+
ops += 3;
824+
break;
825+
case Fragment::THRESH:
826+
ops += node_info->subtypes.size();
827+
break;
828+
case Fragment::MULTI:
829+
ops += 1;
830+
break;
831+
case Fragment::WRAP_A:
832+
ops += 2;
833+
break;
834+
case Fragment::WRAP_S:
835+
ops += 1;
836+
break;
837+
case Fragment::WRAP_C:
838+
ops += 1;
839+
break;
840+
case Fragment::WRAP_D:
841+
ops += 3;
842+
break;
843+
case Fragment::WRAP_V:
844+
// We don't account for OP_VERIFY here; that will be corrected for when the actual
845+
// node is constructed below.
846+
break;
847+
case Fragment::WRAP_J:
848+
ops += 4;
849+
break;
850+
case Fragment::WRAP_N:
851+
ops += 1;
852+
break;
853+
}
854+
if (ops > MAX_OPS_PER_SCRIPT) return {};
725855
auto subtypes = node_info->subtypes;
726856
todo.back().second = std::move(node_info);
727857
todo.reserve(todo.size() + subtypes.size());
@@ -738,11 +868,11 @@ NodeRef GenNode(F ConsumeNode, Type root_type = ""_mst, bool strict_valid = fals
738868
NodeInfo& info = *todo.back().second;
739869
// Gather children from the back of stack.
740870
std::vector<NodeRef> sub;
741-
sub.reserve(info.n_subs);
742-
for (size_t i = 0; i < info.n_subs; ++i) {
743-
sub.push_back(std::move(*(stack.end() - info.n_subs + i)));
871+
sub.reserve(info.subtypes.size());
872+
for (size_t i = 0; i < info.subtypes.size(); ++i) {
873+
sub.push_back(std::move(*(stack.end() - info.subtypes.size() + i)));
744874
}
745-
stack.erase(stack.end() - info.n_subs, stack.end());
875+
stack.erase(stack.end() - info.subtypes.size(), stack.end());
746876
// Construct new NodeRef.
747877
NodeRef node;
748878
if (info.keys.empty()) {
@@ -753,17 +883,29 @@ NodeRef GenNode(F ConsumeNode, Type root_type = ""_mst, bool strict_valid = fals
753883
node = MakeNodeRef(info.fragment, std::move(info.keys), info.k);
754884
}
755885
// Verify acceptability.
756-
if (!node || !(node->GetType() << type_needed)) {
886+
if (!node || (node->GetType() & "KVWB"_mst) == ""_mst) {
757887
assert(!strict_valid);
758888
return {};
759889
}
890+
if (!(type_needed == ""_mst)) {
891+
assert(node->GetType() << type_needed);
892+
}
760893
if (!node->IsValid()) return {};
894+
// Update resource predictions.
895+
if (node->fragment == Fragment::WRAP_V && node->subs[0]->GetType() << "x"_mst) {
896+
ops += 1;
897+
scriptsize += 1;
898+
}
899+
if (ops > MAX_OPS_PER_SCRIPT) return {};
900+
if (scriptsize > MAX_STANDARD_P2WSH_SCRIPT_SIZE) return {};
761901
// Move it to the stack.
762902
stack.push_back(std::move(node));
763903
todo.pop_back();
764904
}
765905
}
766906
assert(stack.size() == 1);
907+
assert(stack[0]->GetStaticOps() == ops);
908+
assert(stack[0]->ScriptSize() == scriptsize);
767909
stack[0]->DuplicateKeyCheck(KEY_COMP);
768910
return std::move(stack[0]);
769911
}
@@ -921,9 +1063,9 @@ void FuzzInitSmart()
9211063
FUZZ_TARGET_INIT(miniscript_stable, FuzzInit)
9221064
{
9231065
FuzzedDataProvider provider(buffer.data(), buffer.size());
924-
TestNode(GenNode([&](Type) {
925-
return ConsumeNodeStable(provider);
926-
}), provider);
1066+
TestNode(GenNode([&](Type needed_type) {
1067+
return ConsumeNodeStable(provider, needed_type);
1068+
}, ""_mst), provider);
9271069
}
9281070

9291071
/** Fuzz target that runs TestNode on nodes generated using ConsumeNodeSmart. */

0 commit comments

Comments
 (0)