Merge bitcoin/bitcoin#31676: fuzz: add targets for PCP and NAT-PMP port mapping requests

c73b59d fuzz: implement targets for PCP and NAT-PMP port mapping requests (Antoine Poinsot)
1695c8a fuzz: in FuzzedSock::GetSockName(), return a random-length name (Antoine Poinsot)
0d472c1 fuzz: never return an uninitialized sockaddr in FuzzedSock::GetSockName (Antoine Poinsot)
39b7e2b fuzz: add steady clock mocking to FuzzedSock (Antoine Poinsot)
6fe1c35 pcp: make NAT-PMP error codes uint16_t (Antoine Poinsot)
01906ce pcp: make the ToString method const (Antoine Poinsot)

Pull request description:

  Based on bitcoin/bitcoin#31022, this introduces a fuzz target for `PCPRequestPortMap` and `NATPMPRequestPortMap`.

  Like in #31022 we set `CreateSock` to return a `Sock` which mocks the responses from the server and uses a mocked steady clock for the `Wait`s. Except here we simply respond with fuzzer-provided data until the client stop sending requests. We also sometimes inject errors and connection failures based on fuzzer-provided data.

  We reuse the existing `FuzzedSock`, so a preparatory commit is included that adds steady clock mocking to it. This may be useful for other harnesses as well.

ACKs for top commit:
  laanwj:
    re-ACK c73b59d
  marcofleon:
    ACK c73b59d
  dergoegge:
    utACK c73b59d

Tree-SHA512: 24cd4d958a0999946a0c3d164a242fc3f0a0b66770630252b881423ad0065d29fdaab765014d193b705d3eff397f201d51a88a3ca80c63fd3867745e6f21bb2b

Author: fanquake

src/common/pcp.cpp

@@ -84,7 +84,7 @@ constexpr uint8_t NATPMP_RESULT_UNSUPP_VERSION = 1;
 constexpr uint8_t NATPMP_RESULT_NO_RESOURCES = 4;
 
 //! Mapping of NATPMP result code to string (RFC6886 3.5). Result codes <=2 match PCP.
-const std::map<uint8_t, std::string> NATPMP_RESULT_STR{
+const std::map<uint16_t, std::string> NATPMP_RESULT_STR{
     {0,  "SUCCESS"},
     {1,  "UNSUPP_VERSION"},
     {2,  "NOT_AUTHORIZED"},
@@ -165,7 +165,7 @@ const std::map<uint8_t, std::string> PCP_RESULT_STR{
 };
 
 //! Return human-readable string from NATPMP result code.
-std::string NATPMPResultString(uint8_t result_code)
+std::string NATPMPResultString(uint16_t result_code)
 {
     auto result_i = NATPMP_RESULT_STR.find(result_code);
     return strprintf("%s (code %d)", result_i == NATPMP_RESULT_STR.end() ? "(unknown)" : result_i->second,  result_code);
@@ -512,7 +512,7 @@ std::variant<MappingResult, MappingError> PCPRequestPortMap(const PCPMappingNonc
     return MappingResult(PCP_VERSION, CService(internal, port), CService(external_addr, external_port), lifetime_ret);
 }
 
-std::string MappingResult::ToString()
+std::string MappingResult::ToString() const
 {
     Assume(version == NATPMP_VERSION || version == PCP_VERSION);
     return strprintf("%s:%s -> %s (for %ds)",

src/common/pcp.h

@@ -40,7 +40,7 @@ struct MappingResult {
     uint32_t lifetime;
 
     //! Format mapping as string for logging.
-    std::string ToString();
+    std::string ToString() const;
 };
 
 //! Try to open a port using RFC 6886 NAT-PMP. IPv4 only.

src/test/fuzz/CMakeLists.txt

@@ -75,6 +75,7 @@ add_executable(fuzz
   p2p_handshake.cpp
   p2p_headers_presync.cpp
   p2p_transport_serialization.cpp
+  pcp.cpp
   package_eval.cpp
   parse_hd_keypath.cpp
   parse_iso8601.cpp

src/test/fuzz/pcp.cpp

@@ -0,0 +1,80 @@
+// Copyright (c) 2024 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 <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/util.h>
+#include <test/fuzz/util/net.h>
+
+#include <common/pcp.h>
+#include <util/check.h>
+
+using namespace std::literals;
+
+//! Fixed nonce to use in PCP port mapping requests.
+constexpr PCPMappingNonce PCP_NONCE{0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc};
+
+//! Number of attempts to request a NAT-PMP or PCP port mapping to the gateway.
+constexpr int NUM_TRIES{5};
+
+//! Timeout for each attempt to request a port mapping.
+constexpr std::chrono::duration TIMEOUT{100ms};
+
+void port_map_target_init()
+{
+    LogInstance().DisableLogging();
+}
+
+FUZZ_TARGET(pcp_request_port_map, .init = port_map_target_init)
+{
+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+
+    // Create a mocked socket between random (and potentially invalid) client and gateway addresses.
+    CreateSock = [&](int domain, int type, int protocol) {
+        if ((domain == AF_INET || domain == AF_INET6) && type == SOCK_DGRAM && protocol == IPPROTO_UDP) {
+            return std::make_unique<FuzzedSock>(fuzzed_data_provider);
+        }
+        return std::unique_ptr<FuzzedSock>();
+    };
+
+    // Perform the port mapping request. The mocked socket will return fuzzer-provided data.
+    const auto gateway_addr{ConsumeNetAddr(fuzzed_data_provider)};
+    const auto local_addr{ConsumeNetAddr(fuzzed_data_provider)};
+    const auto port{fuzzed_data_provider.ConsumeIntegral<uint16_t>()};
+    const auto lifetime{fuzzed_data_provider.ConsumeIntegral<uint32_t>()};
+    const auto res{PCPRequestPortMap(PCP_NONCE, gateway_addr, local_addr, port, lifetime, NUM_TRIES, TIMEOUT)};
+
+    // In case of success the mapping must be consistent with the request.
+    if (const MappingResult* mapping = std::get_if<MappingResult>(&res)) {
+        Assert(mapping);
+        Assert(mapping->internal.GetPort() == port);
+        mapping->ToString();
+    }
+}
+
+FUZZ_TARGET(natpmp_request_port_map, .init = port_map_target_init)
+{
+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+
+    // Create a mocked socket between random (and potentially invalid) client and gateway addresses.
+    CreateSock = [&](int domain, int type, int protocol) {
+        if (domain == AF_INET && type == SOCK_DGRAM && protocol == IPPROTO_UDP) {
+            return std::make_unique<FuzzedSock>(fuzzed_data_provider);
+        }
+        return std::unique_ptr<FuzzedSock>();
+    };
+
+    // Perform the port mapping request. The mocked socket will return fuzzer-provided data.
+    const auto gateway_addr{ConsumeNetAddr(fuzzed_data_provider)};
+    const auto port{fuzzed_data_provider.ConsumeIntegral<uint16_t>()};
+    const auto lifetime{fuzzed_data_provider.ConsumeIntegral<uint32_t>()};
+    const auto res{NATPMPRequestPortMap(gateway_addr, port, lifetime, NUM_TRIES, TIMEOUT)};
+
+    // In case of success the mapping must be consistent with the request.
+    if (const MappingResult* mapping = std::get_if<MappingResult>(&res)) {
+        Assert(mapping);
+        Assert(mapping->internal.GetPort() == port);
+        mapping->ToString();
+    }
+}

src/test/fuzz/util/net.cpp

@@ -113,8 +113,10 @@ template CAddress::SerParams ConsumeDeserializationParams(FuzzedDataProvider&) n
 FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
     : Sock{fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET)},
       m_fuzzed_data_provider{fuzzed_data_provider},
-      m_selectable{fuzzed_data_provider.ConsumeBool()}
+      m_selectable{fuzzed_data_provider.ConsumeBool()},
+      m_time{MockableSteadyClock::INITIAL_MOCK_TIME}
 {
+    ElapseTime(std::chrono::seconds(0)); // start mocking the steady clock.
 }
 
 FuzzedSock::~FuzzedSock()
@@ -126,6 +128,12 @@ FuzzedSock::~FuzzedSock()
     m_socket = INVALID_SOCKET;
 }
 
+void FuzzedSock::ElapseTime(std::chrono::milliseconds duration) const
+{
+    m_time += duration;
+    MockableSteadyClock::SetMockTime(m_time);
+}
+
 FuzzedSock& FuzzedSock::operator=(Sock&& other)
 {
     assert(false && "Move of Sock into FuzzedSock not allowed.");
@@ -349,7 +357,11 @@ int FuzzedSock::GetSockName(sockaddr* name, socklen_t* name_len) const
         SetFuzzedErrNo(m_fuzzed_data_provider, getsockname_errnos);
         return -1;
     }
-    *name_len = m_fuzzed_data_provider.ConsumeData(name, *name_len);
+    assert(name_len);
+    const auto bytes{ConsumeRandomLengthByteVector(m_fuzzed_data_provider, *name_len)};
+    if (bytes.size() < (int)sizeof(sockaddr)) return -1;
+    std::memcpy(name, bytes.data(), bytes.size());
+    *name_len = bytes.size();
     return 0;
 }
 
@@ -388,6 +400,7 @@ bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, Event*
         // FuzzedDataProvider runs out of data.
         *occurred = m_fuzzed_data_provider.ConsumeBool() ? 0 : requested;
     }
+    ElapseTime(timeout);
     return true;
 }
 
@@ -400,6 +413,7 @@ bool FuzzedSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& even
         // FuzzedDataProvider runs out of data.
         events.occurred = m_fuzzed_data_provider.ConsumeBool() ? 0 : events.requested;
     }
+    ElapseTime(timeout);
     return true;
 }
 

src/test/fuzz/util/net.h

@@ -157,6 +157,16 @@ class FuzzedSock : public Sock
      */
     const bool m_selectable;
 
+    /**
+     * Used to mock the steady clock in methods waiting for a given duration.
+     */
+    mutable std::chrono::milliseconds m_time;
+
+    /**
+     * Set the value of the mocked steady clock such as that many ms have passed.
+     */
+    void ElapseTime(std::chrono::milliseconds duration) const;
+
 public:
     explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider);