fix(evo): bound DomainPort string deserialization and stop swallowing deser failures - #7487
Draft
PastaPastaPasta wants to merge 2 commits into
Draft
fix(evo): bound DomainPort string deserialization and stop swallowing deser failures#7487PastaPastaPasta wants to merge 2 commits into
PastaPastaPasta wants to merge 2 commits into
Conversation
Add regression coverage: DomainPort accepts attacker-chosen string lengths up to MAX_SIZE, and NetInfoEntry swallows ios_base::failure so ExtNetInfo vectors can force repeated large allocate-and-zero work from a small payload. These cases expect rejection (throw) and fail on the unfixed tree.
DomainPort serialized m_addr as a bare std::string, so CompactSize claims up to MAX_SIZE (32 MiB) allocated before any body was read. NetInfoEntry::Unserialize caught ios_base::failure and Clear()ed, letting ExtNetInfo vector loops retry from an unadvanced stream position and amplify a ~10 KB ProTx payload into tens of GB of allocate-and-zero under cs_main. Cap the domain with LIMITED_STRING(..., DOMAIN_MAX_LEN=253) at the serialization layer (matching ValidateDomain and RFC 1035 FQDN limits; hard-fork-safe because valid domains were already rejected above 253) and rethrow DomainPort deser failures so the whole ProTx payload fails instead of looping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
DomainPortdeserializes an unboundedstd::string, andNetInfoEntry::Unserialize()swallows the resulting
ios_base::failure. Together these give a large allocate-and-zeroamplification factor from a small ProTx payload.
Mechanism:
DomainPort's serializer readsm_addras a plainstd::string, so an attacker-chosenCompactSizedrivesresize()before any length check.ValidateDomain()enforces the253-byte RFC 1035 ceiling, but only after the allocation has already happened.
NetInfoEntry::Unserialize()catches
ios_base::failureand callsClear(). The stream position is left unmoved (oradvanced only by the
CompactSize), so the enclosing vector loop keeps going and repeatsthe allocation.
The result is that a few attacker-controlled bytes turn into repeated multi-megabyte
allocate-and-zero cycles while the payload is parsed.
What was done?
DomainPort's serializer now usesLIMITED_STRING(obj.m_addr, DOMAIN_MAX_LEN), so anoversized
CompactSizeclaim is rejected beforeresize()rather than after.LimitedStringFormatterchecks the length and throws without allocating.DOMAIN_MAX_LENmoved fromnetinfo.cpptonetinfo.h(anduint8_t→size_tfor thetemplate parameter) so the serialization layer and
ValidateDomain()share one constantand cannot drift apart.
Domainbranch ofNetInfoEntry::Unserialize()no longer swallowsios_base::failure. It propagates, so a malformed domain entry rejects the whole ProTxpayload instead of leaving the stream desynchronised for the rest of the loop.
The
Servicebranch deliberately still catches. ACServiceis fixed-size, so a malformedone cannot desynchronise the stream or amplify allocation — there is nothing to defend
against there, and changing it would alter behaviour for no benefit.
Note on deployment status
This tightens a serialization rule, so the natural question is whether it needs gating.
It does not:
DEPLOYMENT_V24isNEVER_ACTIVEon both mainnet (chainparams.cpp:213) andtestnet (
chainparams.cpp:415), soExtNetInfo— and thereforeDomainPort— cannotappear in any mainnet or testnet chain data. There is nothing already mined for the stricter
rule to retroactively invalidate.
How Has This Been Tested?
Unit tests in
src/test/evo_netinfo_tests.cpp, added in the commit preceding the fix:domainport_deser_rejects_oversized_stringnetinfoentry_domain_deser_rejects_oversized_stringextnetinfo_domain_amplification_rejectedWithout the fix all three fail with
exception std::ios_base::failure expected but not raised. With the fix the fullevo_netinfo_testssuite passes (14 cases), confirmingwell-formed domains up to the 253-byte ceiling still round-trip.
Built and run on macOS/arm64 against current
develop.Breaking Changes
None reachable on mainnet or testnet, per the deployment note above. On devnet/regtest,
a ProTx carrying a domain entry longer than 253 bytes is now rejected at deserialization
instead of being cleared and then rejected by netinfo validation — same outcome, earlier
and cheaper.
Checklist: