exp/api/remote: dont panic on malformed remote-write label refs - #2087
Open
samarth70 wants to merge 1 commit into
Open
exp/api/remote: dont panic on malformed remote-write label refs#2087samarth70 wants to merge 1 commit into
samarth70 wants to merge 1 commit into
Conversation
DesymbolizeLabels stepped through labelRefs two at a time and indexed symbols directly, without checking that the slice has an even length or that each reference is within the symbols table. Both are attacker or peer controlled: they come from a remote-write v2 request decoded by a receiver, so a malformed request panicked the decoding goroutine with an index out of range error. Return an error for both cases instead, matching the reference implementation in prometheus/prometheus, which rejects an odd labelRefs length and references outside the symbols table. Signed-off-by: Sam Agarwal <samarthagrawal526@gmail.com>
5 tasks
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.
Problem
DesymbolizeLabelswalkslabelRefstwo entries at a time and indexes intosymbolsdirectly, with no validation of either:Both inputs come off the wire.
NewWriteHandleraccepts remote-write v2 requests and hands them to awriteStorageimplementation, and this is the helper that implementation uses to turnTimeSeries.LabelsRefsinto labels. A sender that emits an odd number of references, or a reference past the end of the symbols table, panics the decoding goroutine:Neither condition is checked anywhere before this point, so a malformed or malicious request reaches it unfiltered.
Fix
Return an error for both cases. This matches the reference implementation of the same spec in
prometheus/prometheus(prompb/io/prometheus/write/v2/symbols.go), which already guards exactly these two conditions:Prometheus's own
appendV2treats this as a fallible operation and turns a failure into a bad-request error rather than a crash, which seems like the behaviour a receiver built on this package would want too.This does change the signature to return an error. That is a breaking change, which I took to be acceptable here because the module documents itself as experimental ("This package is experimental and may contain breaking changes or be removed in the future"), and because silently dropping the malformed pairs would discard metric data without telling the caller. Happy to take a non-breaking variant instead if you would rather not change the signature in this module yet.
I left
SymbolizeLabelsalone. It has the samei += 2shape, but its input is the caller's own label slice rather than wire data, so an odd length there is a programming error in the caller and out of scope for this change. Glad to include it if you would prefer them consistent.Testing
Added
TestDesymbolizeLabelsInvalidInput, covering an odd reference count (two lengths), an out-of-range name reference, an out-of-range value reference, and references against an empty symbols table. It fails on currentmainwith the panics above and passes with the fix.The existing
TestSymbolsTableround trip is updated for the new signature and still passes. Fullexpmodule suite is green (go test ./...), andgofmtandgo vetare clean.