Fix XDR stream DoS via memory exhaustion and decouple hash validation from Close - #5900
Conversation
… from Close Add a record size limit (default 64MB) to ReadOne to prevent an attacker from triggering massive allocations with a small 4-byte header claiming up to 2GB. Replace SetExpectedHash/Close hash validation with an explicit ValidateHash method so Close only releases resources and early close never triggers unnecessary stream draining. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical DoS vulnerability in XDR stream handling and improves the API design for hash validation. The vulnerability allowed an attacker to trigger up to 2GB memory allocations with just 4 bytes of malicious input. The fix introduces a configurable record size limit (default 64MB) that is checked before buffer allocation. Additionally, the PR decouples hash validation from the Close() method by introducing an explicit ValidateHash() method, ensuring that early close operations (via defer) don't unnecessarily drain entire decompressed streams.
Changes:
- Adds DoS protection with a default 64MB record size limit checked before buffer allocation in ReadOne
- Replaces SetExpectedHash/Close pattern with explicit ValidateHash method for clearer separation of concerns
- Updates all callers to use the new ValidateHash pattern
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| xdr/xdrstream.go | Adds record size validation, new ValidateHash method, removes hash validation from Close, adds SetMaxRecordSize configuration |
| xdr/xdrstream_test.go | Updates tests to use ValidateHash, adds comprehensive tests for record size limits and boundaries |
| ingest/checkpoint_change_reader.go | Simplifies newXDRStream by removing SetExpectedHash, adds explicit ValidateHash call in streamBucket after successful iteration |
| ingest/checkpoint_change_reader_test.go | Removes tests for deprecated ExpectedHash method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Verify that ValidateHash returns an error when the stream reader has already been closed, using a custom closedReader that mimics real file/network close behavior. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add a record size limit (default 64MB) to ReadOne to prevent an attacker from triggering massive allocations with a small 4-byte header claiming up to 2GB. Replace SetExpectedHash/Close hash validation with an explicit ValidateHash method so Close only releases resources and early close never triggers unnecessary stream draining.
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
CHANGELOG.mdwithin the component folder structure. For example, if I changed horizon, then I updated (services/horizon/CHANGELOG.md. I add a new line item describing the change and reference to this PR. If I don't update a CHANGELOG, I acknowledge this PR's change may not be mentioned in future release notes.semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
allocations with just 4 bytes of input
decompressed stream