Conversation
The parameter is optional and typed *bool, so a client may send an explicit JSON null for it. The declared jsonrpcdefault is only applied when the field is absent, which leaves the pointer nil, and the handler dereferenced it as its first effective statement. The handler runs on a goroutine with no recover, so the result is a process crash rather than a failed request, and the limited tier of RPC credential is enough to trigger it. Nil-guard the dereference the same way the other optional pointer parameters in this command set are guarded at their dereference sites. Absent and explicit false then behave identically, which is what the declared default already implies: a request that does not opt in to high fees declines them. No signature or wire change, so clients are unaffected. The existing table test missed this because every case constructs the command with a non-nil pointer, as one naturally does. The nil state is only reachable through a parsed request. The new case is first in the table so the panic is the first thing exercised, and it fails on the unfixed handler with a SIGSEGV at this line before the guard is added. Fixes decred#3813
The SR blame path indexes the submitted matrix as DCMix[j][k], with j bounded by the peer's MessageCount and k by the width of the mix being compared. The sizes of MessageCount and SlotReserveMsgs are checked when the run is set up, but the matrix the peer published is trusted to have those dimensions. The DC-net path immediately below already validates its own dimensions and uses the length-safe Vec.Equals. The SR path does neither, so a peer that publishes fewer rows, or a row shorter than the mix, is read out of bounds here rather than blamed. Add the two length checks the loop depends on, blaming the peer in each case the way every other dimension mismatch in this file is handled. Placing the width check before the comparison loop means a short row is reported with a message naming the problem instead of panicking on the index. Tests cover both directions, plus the exact-match case, and pin the invariant that the mix width is the pads width.
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
1 participant
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.
Problem
The SR blame path indexes the peer's submitted matrix as
p.sr.DCMix[j][k], wherejis boundedby the peer's
MessageCountandkby the width of the mix being compared against it.MessageCountandlen(SlotReserveMsgs)are both checked when the run is set up(
blame.go:274), but the matrix the peer actually published is trusted to have those dimensions.So a peer that publishes too few rows, or a row narrower than the mix, is read out of bounds here.
The DC-net path immediately below already gets this right: it checks
uint32(len(p.dc.DCNet)) != mcountand blames the peer, and it compares via the length-safeVec.Equals. The SR path does neither.Change
Two length checks in the SR loop, each blaming the peer and moving on, which is how every other
dimension mismatch in this file is handled:
len(p.sr.DCMix) != len(p.srMsg)before the loop, sincep.srMsgis sized to the alreadyvalidated
MessageCountlen(p.sr.DCMix[j]) != len(srMix)inside it, before the comparisonThe second check also converts an out-of-bounds panic into a message naming the problem, so a
short row is reported as misbehaviour rather than crashing the node.
Test
srdimensions_test.gocovers both directions for the row count and the row width, plus the exactmatch and empty cases. It also pins the invariant the width check relies on:
SRMixreturnsexactly
len(SRMixPads(...))entries, which is what makes a narrower committed row anout-of-bounds read.
Verification
Run from the
mixingmodule, which is separate from the root module:go test ./... -count=1(inmixing/)mixing,mixclient,mixpoolall passgo test ./mixclient/ -run TestSR -count=1go vet ./mixclient/gofmt -lon both filesTestSRDisruptionin the same package skips, as it requires thecsppsolverbinary which is noton this machine; that is pre-existing and unrelated.
Note on scope
The panic this guards is reachable by a peer that completes a MixPartners handshake, so it is
behind the same access control as the rest of the misbehaviour handling in this file, which blames
rather than errors. I have not added a CVE or opened a disclosure, since the report is public;
flagging it here in case a maintainer wants it handled differently.
Fixes #3788