Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: Add fuzzing harness for miniscript::FromScript(...) #17129

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 13, 2019

Add fuzzing harness for miniscript::FromScript(...).

Based on sipa's PR #16800 (rebased on current master).

Testing this PR

Run:

$ CC=clang CXX=clang++ ./configure --enable-fuzz \
      --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/from_script -max_len=3
…
==1011==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000007d30 at pc 
    0x564149ae8922 bp 0x7ffdc121bb90 sp 0x7ffdc121b340
…
$ src/test/fuzz/from_script -max_len=3
…
from_script: script/miniscript.cpp:51: miniscript::Type 
    miniscript::internal::ComputeType(miniscript::NodeType, miniscript::Type, 
    miniscript::Type, miniscript::Type, const std::vector<Type> &, uint32_t, size_t,
    size_t, size_t): Assertion `k > 1 && k < n_subs' failed.
…

The fix to those two issues demonstrated above was submitted a month ago to the upstream repo sipa/miniscript#18 :)

Here is my suggested patch if you want to try the fuzzer against the fixed version:

diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index d5bc80f98..9bd3f8426 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -912,6 +912,9 @@ inline NodeRef<Key> DecodeSingle(I& in, I last, const Ctx& ctx) {
     }
     subs.clear();
     if (last - in >= 3 && in[0].first == OP_EQUAL && ParseScriptNumber(in[1], k)) {
+        if (k < 2) {
+            return {};
+        }
         in += 2;
         while (last - in >= 2 && in[0].first == OP_ADD) {
             ++in;
@@ -922,6 +925,9 @@ inline NodeRef<Key> DecodeSingle(I& in, I last, const Ctx& ctx) {
         auto sub = DecodeSingle<Key>(in, last, ctx);
         if (!sub) return {};
         subs.push_back(std::move(sub));
+        if (k >= subs.size()) {
+            return {};
+        }
         std::reverse(subs.begin(), subs.end());
         return MakeNodeRef<Key>(NodeType::THRESH, std::move(subs), k);
     }

@fanquake fanquake added the Tests label Oct 13, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 14, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17229 (tests: Add fuzzing harnesses for various Base{32,58,64} and hex related functions by practicalswift)
  • #17225 (tests: Test serialisation as part of deserialisation fuzzing. Test round-trip equality where possible. by practicalswift)
  • #17136 (tests: Add fuzzing harness for various PSBT related functions by practicalswift)
  • #17109 (tests: Add fuzzing harness for various functions consuming only integrals by practicalswift)
  • #17093 (tests: Add fuzzing harness for various CTx{In,Out} related functions by practicalswift)
  • #17071 (tests: Add fuzzing harness for CheckBlock(...) and other CBlock related functions by practicalswift)
  • #17051 (tests: Add deserialization fuzzing harnesses by practicalswift)
  • #17050 (tests: Add fuzzing harnesses for functions parsing scripts, numbers, JSON and HD keypaths (bip32) by practicalswift)
  • #16710 (build: Enable -Wsuggest-override if available by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sipa and others added 10 commits October 23, 2019 11:53
Added are:

* Vector(arg1,arg2,arg3,...) constructs a vector with the specified
  arguments as elements. The vector's type is derived from the
  arguments. If some of the arguments are rvalue references, they
  will be moved into place rather than copied (which can't be achieved
  using list initialization).

* Cat(vector1,vector2) returns a concatenation of the two vectors,
  efficiently moving elements when relevant.

Vector generalizes (and replaces) the Singleton function in
src/descriptor.cpp, and Cat replaces the Cat function in bech32.cpp
@practicalswift practicalswift deleted the fuzzers-miniscript-fromscript branch April 10, 2021 19:39
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants