fix(subtreevalidation): bound HTTP body reads when fetching subtree data#772
Merged
ordishs merged 3 commits intoMay 19, 2026
Merged
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete Summary: Security fix correctly addresses HIGH-severity unbounded memory allocation vulnerability. Implementation is sound with comprehensive testing. Current Review: Key strengths:
Minor observation (not blocking): History:
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-19 10:10 UTC |
2 tasks
oskarszoon
approved these changes
May 14, 2026
|
|
||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| assert.Equal(t, http.MethodPost, r.Method) | ||
| assert.Equal(t, "application/json", r.Header.Get("Content-Type")) |
Contributor
There was a problem hiding this comment.
[Minor] Test assertion mismatch
The test expects Content-Type: application/json but DoHTTPRequestBounded sets Content-Type: application/octet-stream for POST requests (util/http.go:288).
This matches existing DoHTTPRequest behavior. Per util/http.go:279-284, binary payloads use application/octet-stream to avoid WAF issues.
Fix: Change line 534 to check for application/octet-stream instead.
freemans13
approved these changes
May 19, 2026
…efs #4571) Add TestCheckBlockSubtrees_OversizedBody to cover the third peer-fetch call site at check_block_subtrees.go:218, mirroring the existing fetchSubtreeFromPeer and getSubtreeTxHashes coverage so all three peer- fetch paths fail closed on oversized responses. Align TestDoHTTPRequestBounded_POST content-type assertion with the application/octet-stream default introduced upstream by bsv-blockchain#829, surfaced after rebasing onto main.
021641a to
23da07e
Compare
|
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
3 participants
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.
Closes #4571.
Summary
util.DoHTTPRequestusesio.ReadAllon the response body with no size limit. Two subtree-fetching callers inservices/subtreevalidation/(getSubtreeTxHashesat SubtreeValidation.go:893 and the missing-subtree path at check_block_subtrees.go:214) accept peer-provided URLs, so a malicious peer can stream up to 60 seconds of unbounded memory allocation per request. Severity HIGH because the attack is remote-triggerable.Fix
Adds
util.DoHTTPRequestBounded(ctx, url, maxBytes, body...)that mirrorsDoHTTPRequestbut wraps the body inio.LimitReader(body, maxBytes+1)and returns a typed error if the response exceedsmaxBytes. Both subtree-fetching call sites now passmaxSubtreeBytes = MaximumMerkleItemsPerSubtree * chainhash.HashSize(32 MB by default).Other
DoHTTPRequestcall sites (block fetches, header fetches inservices/blockvalidation/) are out of scope — they have different size profiles and may be addressed in separate audit follow-ups.Test plan
DoHTTPRequestBounded: happy path, body equal to limit, body exceeds limit (typed error returned, no over-allocation).go test -race ./util/...passes.go test -race -tags testtxmetacache ./services/subtreevalidation/...passes.