diff --git a/src/evo/netinfo.cpp b/src/evo/netinfo.cpp index 48eae4e11232..62aa1a5e2a59 100644 --- a/src/evo/netinfo.cpp +++ b/src/evo/netinfo.cpp @@ -20,8 +20,6 @@ static std::once_flag g_main_params_flag; /** Maximum length of a label in a domain per RFC 1035 */ static constexpr uint8_t DOMAIN_LABEL_MAX_LEN{63}; -/** Maximum possible length of a ASCII FQDN */ -static constexpr uint8_t DOMAIN_MAX_LEN{253}; /** Minimum length of a FQDN */ static constexpr uint8_t DOMAIN_MIN_LEN{3}; diff --git a/src/evo/netinfo.h b/src/evo/netinfo.h index 9c75cd4376aa..6d157e16ca86 100644 --- a/src/evo/netinfo.h +++ b/src/evo/netinfo.h @@ -19,6 +19,11 @@ class UniValue; /** Maximum entries that can be stored in an ExtNetInfo per purpose code */ static constexpr uint8_t MAX_ENTRIES_EXTNETINFO{4}; +/** Maximum possible length of an ASCII FQDN (RFC 1035). Applied at the + * serialization layer so oversized CompactSize claims are rejected before + * allocate-and-zero; ValidateDomain enforces the same ceiling after decode. */ +static constexpr size_t DOMAIN_MAX_LEN{253}; + enum class NetInfoStatus : uint8_t { // Managing entries BadInput, @@ -170,7 +175,7 @@ class DomainPort SERIALIZE_METHODS(DomainPort, obj) { - READWRITE(obj.m_addr); + READWRITE(LIMITED_STRING(obj.m_addr, DOMAIN_MAX_LEN)); READWRITE(Using>(obj.m_port)); } @@ -246,11 +251,14 @@ class NetInfoEntry if (!service.IsValid()) { Clear(); } // Invalid CService, mark as invalid } catch (const std::ios_base::failure&) { Clear(); } // Deser failed, mark as invalid } else if (m_type == NetInfoType::Domain) { - try { - auto& domain{m_data.emplace()}; - s >> domain; - if (!domain.IsValid()) { Clear(); } // Invalid DomainPort, mark as invalid - } catch (const std::ios_base::failure&) { Clear(); } // Deser failed, mark as invalid + // Do not swallow deserialization failures: a short/oversized domain + // body would otherwise leave the stream position unmoved (or only + // advanced by CompactSize) while the enclosing vector loop continues, + // amplifying a few attacker bytes into repeated allocations. + // Propagate ios_base::failure so the whole ProTx payload is rejected. + auto& domain{m_data.emplace()}; + s >> domain; + if (!domain.IsValid()) { Clear(); } // Invalid DomainPort, mark as invalid } else { Clear(); } // Invalid type code, mark as invalid } diff --git a/src/test/evo_netinfo_tests.cpp b/src/test/evo_netinfo_tests.cpp index 4455456ac9a2..866680c7b391 100644 --- a/src/test/evo_netinfo_tests.cpp +++ b/src/test/evo_netinfo_tests.cpp @@ -497,6 +497,92 @@ BOOST_AUTO_TEST_CASE(domainport_rules) } } +// DomainPort used a bare std::string unserialize path (bounded only by +// MAX_SIZE == 32 MiB) and NetInfoEntry::Unserialize swallowed ios_base::failure. +// That combination allowed a tiny ProTx ExtNetInfo payload to force repeated +// allocate-and-zero of multi-megabyte strings. These tests pin the serialization +// layer to reject oversized domain claims without performing the large allocation. +static CDataStream MakeOversizedDomainPortStream(const size_t claimed_len) +{ + CDataStream ds(SER_DISK, CLIENT_VERSION); + WriteCompactSize(ds, claimed_len); + // Provide a full body so pre-fix code can complete the string read; the + // bound must reject based on the CompactSize alone, not on a short read. + const std::string body(claimed_len, 'a'); + if (!body.empty()) { + ds.write(MakeByteSpan(body)); + } + ser_writedata16be(ds, 443); + return ds; +} + +BOOST_AUTO_TEST_CASE(domainport_deser_rejects_oversized_string) +{ + // DOMAIN_MAX_LEN is 253; a fully-formed 1000-byte domain must be rejected at + // deserialization time (LIMITED_STRING), not accepted then failed later in + // ValidateDomain. + constexpr size_t kOversized{1000}; + CDataStream ds{MakeOversizedDomainPortStream(kOversized)}; + + DomainPort domain; + BOOST_CHECK_THROW(ds >> domain, std::ios_base::failure); +} + +BOOST_AUTO_TEST_CASE(netinfoentry_domain_deser_rejects_oversized_string) +{ + // NetInfoEntry must not swallow the length-limit failure: rethrow so the + // enclosing ProTx payload deserialization aborts and the peer is penalised. + constexpr size_t kOversized{1000}; + CDataStream ds(SER_DISK, CLIENT_VERSION); + ds << uint8_t{NetInfoEntry::NetInfoType::Domain}; + { + CDataStream body{MakeOversizedDomainPortStream(kOversized)}; + ds.write(MakeByteSpan(body)); + } + + NetInfoEntry entry; + BOOST_CHECK_THROW(ds >> entry, std::ios_base::failure); +} + +BOOST_AUTO_TEST_CASE(domainport_deser_rejects_huge_claimed_length) +{ + // CompactSize claims 1 MiB with no body. Pre-fix this would allocate 1 MiB + // then throw on short-read; post-fix LIMITED_STRING rejects before resize. + // Either way the operation must throw; the important property is that a + // declared length above DOMAIN_MAX_LEN never produces a successful DomainPort. + CDataStream ds(SER_DISK, CLIENT_VERSION); + WriteCompactSize(ds, size_t{1} << 20); + + DomainPort domain; + BOOST_CHECK_THROW(ds >> domain, std::ios_base::failure); +} + +BOOST_AUTO_TEST_CASE(extnetinfo_domain_amplification_rejected) +{ + // Simulate the attack shape: ExtNetInfo vector of Domain entries each + // claiming a large string length from a small stream. Pre-fix, + // NetInfoEntry swallows the short-read and the vector loop continues + // (stream position does not advance on the failed body read), producing + // repeated large allocations. Post-fix the first oversized claim throws + // and aborts the whole ExtNetInfo unserialize. + constexpr size_t kClaimed{1 << 16}; // 64 KiB — large enough to prove, small enough to not OOM pre-fix + constexpr size_t kEntries{8}; + + CDataStream ds(SER_DISK, CLIENT_VERSION); + ds << uint8_t{1}; // ExtNetInfo version + WriteCompactSize(ds, 1); // one purpose + ds << NetInfoPurpose::PLATFORM_HTTPS; + WriteCompactSize(ds, kEntries); + for (size_t i = 0; i < kEntries; ++i) { + ds << uint8_t{NetInfoEntry::NetInfoType::Domain}; + // Claim kClaimed bytes but supply none — CompactSize only. + WriteCompactSize(ds, kClaimed); + } + + ExtNetInfo netInfo; + BOOST_CHECK_THROW(ds >> netInfo, std::ios_base::failure); +} + BOOST_FIXTURE_TEST_CASE(extnetinfo_validate_deser, RegTestingSetup) { // The in-memory builder (AddEntry/ProcessCandidate) refuses duplicates, domains