Merge #18512: Improve asmap checks and add sanity check

748977690e0519110cda9628162a7ccf73a5934b Add asmap_direct fuzzer that tests Interpreter directly (Pieter Wuille)
7cf97fda154ba837933eb05be5aeecfb69a06641 Make asmap Interpreter errors fatal and fuzz test it (Pieter Wuille)
c81aefc5377888c7ac4f29f570249fd6c2fdb352 Add additional effiency checks to sanity checker (Pieter Wuille)
fffd8dca2de39ad4a683f0dce57cdca55ed2f600 Add asmap sanity checker (Pieter Wuille)
5feefbe6e7b6cdd809eba4074d41dc95a7035f7e Improve asmap Interpret checks and document failures (Pieter Wuille)
2b3dbfa5a63cb5a6625ec00294ebd933800f0255 Deal with decoding failures explicitly in asmap Interpret (Pieter Wuille)
1479007a335ab43af46f527d0543e254fc2a8e86 Introduce Instruction enum in asmap (Pieter Wuille)

Pull request description:

  This improves/documents the failure cases inside the asmap interpreter. None of the changes are bug fixes (they only change behavior for corrupted asmap files), but they may make things easier to follow.

  In a second step, a sanity checker is added that effectively executes every potential code path through the asmap file, checking the same failure cases as the interpreter, and more. It takes around 30 ms to run for me for a 1.2 MB asmap file.

  I've verified that this accepts asmap files constructed by https://github.com/sipa/asmap/blob/master/buildmap.py with a large dataset, and no longer accepts it with 1 bit changed in it.

ACKs for top commit:
  practicalswift:
    ACK 748977690e0519110cda9628162a7ccf73a5934b modulo feedback below.
  jonatack:
    ACK 748977690e0519110cda9628162a7ccf73a5934b code review, regular build/tests/ran bitcoin with -asmap, fuzz build/ran both fuzzers overnight.
  fjahr:
    ACK 748977690e0519110cda9628162a7ccf73a5934b

Tree-SHA512: d876df3859735795c857c83e7155ba6851ce839bdfa10c18ce2698022cc493ce024b5578c1828e2a94bcdf2552c2f46c392a251ed086691b41959e62a6970821

8 files changed, 198 insertions, 26 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 68ba1bc68e..3a0d4fdc15 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -9,6 +9,7 @@ FUZZ_TARGETS = \
   test/fuzz/address_deserialize \
   test/fuzz/addrman_deserialize \
   test/fuzz/asmap \
+  test/fuzz/asmap_direct \
   test/fuzz/banentry_deserialize \
   test/fuzz/base_encode_decode \
   test/fuzz/bech32 \
@@ -332,6 +333,12 @@ test_fuzz_asmap_LDADD = $(FUZZ_SUITE_LD_COMMON)
 test_fuzz_asmap_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
 test_fuzz_asmap_SOURCES = test/fuzz/asmap.cpp
 
+test_fuzz_asmap_direct_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
+test_fuzz_asmap_direct_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
+test_fuzz_asmap_direct_LDADD = $(FUZZ_SUITE_LD_COMMON)
+test_fuzz_asmap_direct_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
+test_fuzz_asmap_direct_SOURCES = test/fuzz/asmap_direct.cpp
+
 test_fuzz_banentry_deserialize_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -DBANENTRY_DESERIALIZE=1
 test_fuzz_banentry_deserialize_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 test_fuzz_banentry_deserialize_LDADD = $(FUZZ_SUITE_LD_COMMON)
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 02fb2fd491..7aba340d9d 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -644,5 +644,9 @@ std::vector<bool> CAddrMan::DecodeAsmap(fs::path path)
             bits.push_back((cur_byte >> bit) & 1);
         }
     }
+    if (!SanityCheckASMap(bits)) {
+        LogPrintf("Sanity check of asmap file %s failed\n", path);
+        return {};
+    }
     return bits;
 }
diff --git a/src/netaddress.cpp b/src/netaddress.cpp
index 6adb171abd..f79425a52e 100644
--- a/src/netaddress.cpp
+++ b/src/netaddress.cpp
@@ -894,3 +894,8 @@ bool operator<(const CSubNet& a, const CSubNet& b)
 {
     return (a.network < b.network || (a.network == b.network && memcmp(a.netmask, b.netmask, 16) < 0));
 }
+
+bool SanityCheckASMap(const std::vector<bool>& asmap)
+{
+    return SanityCheckASMap(asmap, 128); // For IP address lookups, the input is 128 bits
+}
diff --git a/src/netaddress.h b/src/netaddress.h
index d0ab770379..d8f19deffe 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
@@ -180,4 +180,6 @@ class CService : public CNetAddr
         }
 };
 
+bool SanityCheckASMap(const std::vector<bool>& asmap);
+
 #endif // BITCOIN_NETADDRESS_H
diff --git a/src/test/fuzz/asmap.cpp b/src/test/fuzz/asmap.cpp
index 7f3eef79a1..ea56277eac 100644
--- a/src/test/fuzz/asmap.cpp
+++ b/src/test/fuzz/asmap.cpp
@@ -3,26 +3,47 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <netaddress.h>
-#include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 
 #include <cstdint>
 #include <vector>
 
+//! asmap code that consumes nothing
+static const std::vector<bool> IPV6_PREFIX_ASMAP = {};
+
+//! asmap code that consumes the 96 prefix bits of ::ffff:0/96 (IPv4-in-IPv6 map)
+static const std::vector<bool> IPV4_PREFIX_ASMAP = {
+    true, true, false, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, // Match 0x00
+    true, true, false, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, // Match 0x00
+    true, true, false, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, // Match 0x00
+    true, true, false, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, // Match 0x00
+    true, true, false, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, // Match 0x00
+    true, true, false, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, // Match 0x00
+    true, true, false, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, // Match 0x00
+    true, true, false, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, // Match 0x00
+    true, true, false, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, // Match 0x00
+    true, true, false, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, // Match 0x00
+    true, true, false, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, // Match 0xFF
+    true, true, false, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true // Match 0xFF
+};
+
 void test_one_input(const std::vector<uint8_t>& buffer)
 {
-    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
-    const Network network = fuzzed_data_provider.PickValueInArray({NET_IPV4, NET_IPV6});
-    if (fuzzed_data_provider.remaining_bytes() < 16) {
-        return;
-    }
-    CNetAddr net_addr;
-    net_addr.SetRaw(network, fuzzed_data_provider.ConsumeBytes<uint8_t>(16).data());
-    std::vector<bool> asmap;
-    for (const char cur_byte : fuzzed_data_provider.ConsumeRemainingBytes<char>()) {
-        for (int bit = 0; bit < 8; ++bit) {
-            asmap.push_back((cur_byte >> bit) & 1);
+    // Encoding: [7 bits: asmap size] [1 bit: ipv6?] [3-130 bytes: asmap] [4 or 16 bytes: addr]
+    if (buffer.size() < 1 + 3 + 4) return;
+    int asmap_size = 3 + (buffer[0] & 127);
+    bool ipv6 = buffer[0] & 128;
+    int addr_size = ipv6 ? 16 : 4;
+    if (buffer.size() < size_t(1 + asmap_size + addr_size)) return;
+    std::vector<bool> asmap = ipv6 ? IPV6_PREFIX_ASMAP : IPV4_PREFIX_ASMAP;
+    asmap.reserve(asmap.size() + 8 * asmap_size);
+    for (int i = 0; i < asmap_size; ++i) {
+        for (int j = 0; j < 8; ++j) {
+            asmap.push_back((buffer[1 + i] >> j) & 1);
         }
     }
+    if (!SanityCheckASMap(asmap)) return;
+    CNetAddr net_addr;
+    net_addr.SetRaw(ipv6 ? NET_IPV6 : NET_IPV4, buffer.data() + 1 + asmap_size);
     (void)net_addr.GetMappedAS(asmap);
 }
diff --git a/src/test/fuzz/asmap_direct.cpp b/src/test/fuzz/asmap_direct.cpp
new file mode 100644
index 0000000000..790f80237d
--- /dev/null
+++ b/src/test/fuzz/asmap_direct.cpp
@@ -0,0 +1,46 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <util/asmap.h>
+#include <test/fuzz/fuzz.h>
+
+#include <cstdint>
+#include <vector>
+
+#include <assert.h>
+
+void test_one_input(const std::vector<uint8_t>& buffer)
+{
+    // Encoding: [asmap using 1 bit / byte] 0xFF [addr using 1 bit / byte]
+    bool have_sep = false;
+    size_t sep_pos;
+    for (size_t pos = 0; pos < buffer.size(); ++pos) {
+        uint8_t x = buffer[pos];
+        if ((x & 0xFE) == 0) continue;
+        if (x == 0xFF) {
+            if (have_sep) return;
+            have_sep = true;
+            sep_pos = pos;
+        } else {
+            return;
+        }
+    }
+    if (!have_sep) return; // Needs exactly 1 separator
+    if (buffer.size() - sep_pos - 1 > 128) return; // At most 128 bits in IP address
+
+    // Checks on asmap
+    std::vector<bool> asmap(buffer.begin(), buffer.begin() + sep_pos);
+    if (SanityCheckASMap(asmap, buffer.size() - 1 - sep_pos)) {
+        // Verify that for valid asmaps, no prefix (except up to 7 zero padding bits) is valid.
+        std::vector<bool> asmap_prefix = asmap;
+        while (!asmap_prefix.empty() && asmap_prefix.size() + 7 > asmap.size() && asmap_prefix.back() == false) asmap_prefix.pop_back();
+        while (!asmap_prefix.empty()) {
+            asmap_prefix.pop_back();
+            assert(!SanityCheckASMap(asmap_prefix, buffer.size() - 1 - sep_pos));
+        }
+        // No address input should trigger assertions in interpreter
+        std::vector<bool> addr(buffer.begin() + sep_pos + 1, buffer.end());
+        (void)Interpret(asmap, addr);
+    }
+}
diff --git a/src/util/asmap.cpp b/src/util/asmap.cpp
index 5354cdb962..b4090482b9 100644
--- a/src/util/asmap.cpp
+++ b/src/util/asmap.cpp
@@ -2,12 +2,15 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <map>
 #include <vector>
 #include <assert.h>
 #include <crypto/common.h>
 
 namespace {
 
+constexpr uint32_t INVALID = 0xFFFFFFFF;
+
 uint32_t DecodeBits(std::vector<bool>::const_iterator& bitpos, const std::vector<bool>::const_iterator& endpos, uint8_t minval, const std::vector<uint8_t> &bit_sizes)
 {
     uint32_t val = minval;
@@ -25,7 +28,7 @@ uint32_t DecodeBits(std::vector<bool>::const_iterator& bitpos, const std::vector
             val += (1 << *bit_sizes_it);
         } else {
             for (int b = 0; b < *bit_sizes_it; b++) {
-                if (bitpos == endpos) break;
+                if (bitpos == endpos) return INVALID; // Reached EOF in mantissa
                 bit = *bitpos;
                 bitpos++;
                 val += bit << (*bit_sizes_it - 1 - b);
@@ -33,13 +36,21 @@ uint32_t DecodeBits(std::vector<bool>::const_iterator& bitpos, const std::vector
             return val;
         }
     }
-    return -1;
+    return INVALID; // Reached EOF in exponent
 }
 
+enum class Instruction : uint32_t
+{
+    RETURN = 0,
+    JUMP = 1,
+    MATCH = 2,
+    DEFAULT = 3,
+};
+
 const std::vector<uint8_t> TYPE_BIT_SIZES{0, 0, 1};
-uint32_t DecodeType(std::vector<bool>::const_iterator& bitpos, const std::vector<bool>::const_iterator& endpos)
+Instruction DecodeType(std::vector<bool>::const_iterator& bitpos, const std::vector<bool>::const_iterator& endpos)
 {
-    return DecodeBits(bitpos, endpos, 0, TYPE_BIT_SIZES);
+    return Instruction(DecodeBits(bitpos, endpos, 0, TYPE_BIT_SIZES));
 }
 
 const std::vector<uint8_t> ASN_BIT_SIZES{15, 16, 17, 18, 19, 20, 21, 22, 23, 24};
@@ -70,34 +81,105 @@ uint32_t Interpret(const std::vector<bool> &asmap, const std::vector<bool> &ip)
     const std::vector<bool>::const_iterator endpos = asmap.end();
     uint8_t bits = ip.size();
     uint32_t default_asn = 0;
-    uint32_t opcode, jump, match, matchlen;
+    uint32_t jump, match, matchlen;
+    Instruction opcode;
     while (pos != endpos) {
         opcode = DecodeType(pos, endpos);
-        if (opcode == 0) {
-            return DecodeASN(pos, endpos);
-        } else if (opcode == 1) {
+        if (opcode == Instruction::RETURN) {
+            default_asn = DecodeASN(pos, endpos);
+            if (default_asn == INVALID) break; // ASN straddles EOF
+            return default_asn;
+        } else if (opcode == Instruction::JUMP) {
             jump = DecodeJump(pos, endpos);
-            if (bits == 0) break;
+            if (jump == INVALID) break; // Jump offset straddles EOF
+            if (bits == 0) break; // No input bits left
+            if (jump >= endpos - pos) break; // Jumping past EOF
             if (ip[ip.size() - bits]) {
-                if (jump >= endpos - pos) break;
                 pos += jump;
             }
             bits--;
-        } else if (opcode == 2) {
+        } else if (opcode == Instruction::MATCH) {
             match = DecodeMatch(pos, endpos);
+            if (match == INVALID) break; // Match bits straddle EOF
             matchlen = CountBits(match) - 1;
+            if (bits < matchlen) break; // Not enough input bits
             for (uint32_t bit = 0; bit < matchlen; bit++) {
-                if (bits == 0) break;
                 if ((ip[ip.size() - bits]) != ((match >> (matchlen - 1 - bit)) & 1)) {
                     return default_asn;
                 }
                 bits--;
             }
-        } else if (opcode == 3) {
+        } else if (opcode == Instruction::DEFAULT) {
             default_asn = DecodeASN(pos, endpos);
+            if (default_asn == INVALID) break; // ASN straddles EOF
         } else {
-            break;
+            break; // Instruction straddles EOF
         }
     }
+    assert(false); // Reached EOF without RETURN, or aborted (see any of the breaks above) - should have been caught by SanityCheckASMap below
     return 0; // 0 is not a valid ASN
 }
+
+bool SanityCheckASMap(const std::vector<bool>& asmap, int bits)
+{
+    const std::vector<bool>::const_iterator begin = asmap.begin(), endpos = asmap.end();
+    std::vector<bool>::const_iterator pos = begin;
+    std::vector<std::pair<uint32_t, int>> jumps; // All future positions we may jump to (bit offset in asmap -> bits to consume left)
+    jumps.reserve(bits);
+    Instruction prevopcode = Instruction::JUMP;
+    bool had_incomplete_match = false;
+    while (pos != endpos) {
+        uint32_t offset = pos - begin;
+        if (!jumps.empty() && offset >= jumps.back().first) return false; // There was a jump into the middle of the previous instruction
+        Instruction opcode = DecodeType(pos, endpos);
+        if (opcode == Instruction::RETURN) {
+            if (prevopcode == Instruction::DEFAULT) return false; // There should not be any RETURN immediately after a DEFAULT (could be combined into just RETURN)
+            uint32_t asn = DecodeASN(pos, endpos);
+            if (asn == INVALID) return false; // ASN straddles EOF
+            if (jumps.empty()) {
+                // Nothing to execute anymore
+                if (endpos - pos > 7) return false; // Excessive padding
+                while (pos != endpos) {
+                    if (*pos) return false; // Nonzero padding bit
+                    ++pos;
+                }
+                return true; // Sanely reached EOF
+            } else {
+                // Continue by pretending we jumped to the next instruction
+                offset = pos - begin;
+                if (offset != jumps.back().first) return false; // Unreachable code
+                bits = jumps.back().second; // Restore the number of bits we would have had left after this jump
+                jumps.pop_back();
+                prevopcode = Instruction::JUMP;
+            }
+        } else if (opcode == Instruction::JUMP) {
+            uint32_t jump = DecodeJump(pos, endpos);
+            if (jump == INVALID) return false; // Jump offset straddles EOF
+            if (jump > endpos - pos) return false; // Jump out of range
+            if (bits == 0) return false; // Consuming bits past the end of the input
+            --bits;
+            uint32_t jump_offset = pos - begin + jump;
+            if (!jumps.empty() && jump_offset >= jumps.back().first) return false; // Intersecting jumps
+            jumps.emplace_back(jump_offset, bits);
+            prevopcode = Instruction::JUMP;
+        } else if (opcode == Instruction::MATCH) {
+            uint32_t match = DecodeMatch(pos, endpos);
+            if (match == INVALID) return false; // Match bits straddle EOF
+            int matchlen = CountBits(match) - 1;
+            if (prevopcode != Instruction::MATCH) had_incomplete_match = false;
+            if (matchlen < 8 && had_incomplete_match) return false; // Within a sequence of matches only at most one should be incomplete
+            had_incomplete_match = (matchlen < 8);
+            if (bits < matchlen) return false; // Consuming bits past the end of the input
+            bits -= matchlen;
+            prevopcode = Instruction::MATCH;
+        } else if (opcode == Instruction::DEFAULT) {
+            if (prevopcode == Instruction::DEFAULT) return false; // There should not be two successive DEFAULTs (they could be combined into one)
+            uint32_t asn = DecodeASN(pos, endpos);
+            if (asn == INVALID) return false; // ASN straddles EOF
+            prevopcode = Instruction::DEFAULT;
+        } else {
+            return false; // Instruction straddles EOF
+        }
+    }
+    return false; // Reached EOF without RETURN instruction
+}
diff --git a/src/util/asmap.h b/src/util/asmap.h
index a0e14013c5..b31e639bb5 100644
--- a/src/util/asmap.h
+++ b/src/util/asmap.h
@@ -5,6 +5,11 @@
 #ifndef BITCOIN_UTIL_ASMAP_H
 #define BITCOIN_UTIL_ASMAP_H
 
+#include <stdint.h>
+#include <vector>
+
 uint32_t Interpret(const std::vector<bool> &asmap, const std::vector<bool> &ip);
 
+bool SanityCheckASMap(const std::vector<bool>& asmap, int bits);
+
 #endif // BITCOIN_UTIL_ASMAP_H