Harden filter parser/lexer against malicious search expressions (DoS)#154
Open
MrEchoFi wants to merge 2 commits into
Open
Harden filter parser/lexer against malicious search expressions (DoS)#154MrEchoFi wants to merge 2 commits into
MrEchoFi wants to merge 2 commits into
Conversation
NewLexer spawns a goroutine that emits tokens on an unbuffered channel. When ParseQuery returned early on a parse error, it stopped reading tokens, leaving the lexer goroutine blocked forever on a channel send. Because ParseQuery is reachable from the unauthenticated GraphQL API via search expressions (httpRequestLogFilter / senderRequestFilter), each malformed query leaked one goroutine permanently, allowing remote goroutine/memory exhaustion (DoS). Make the lexer cancellable via a `done` channel and a `Stop` method. ParseQuery now defers `Stop`, releasing the goroutine on every return path. Adds a regression test asserting no goroutines leak across many malformed queries.
The recursive-descent parser had no depth limit. Deeply nested input
(e.g. many "(" or repeated "NOT" prefixes) recursed until the goroutine
stack was exhausted. In Go a stack overflow is a *fatal* runtime error:
it cannot be caught by recover(), so it crashes the entire process.
ParseQuery is reachable from the unauthenticated GraphQL API via search
expressions, so a single small request (a few hundred KB of "(") would
crash the whole Hetty instance — proxy, admin API and all in-flight
intercepts. gqlgen's panic recovery does not help, because this is not a
recoverable panic.
Add a maxParseDepth (256) guard in parseExpression, the single recursion
point reached by grouped, prefix and infix sub-expression parsing.
Legitimate human-written search queries nest far below this limit.
59a19ec to
fad7c41
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
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.
Fixes #153 .
filter.ParseQueryis reachable from the unauthenticated GraphQL API viasearch/filter fields (
setHttpRequestLogFilter,setSenderRequestFilter,updateInterceptSettings). This PR fixes two denial-of-service issues inpkg/filter.1. Stack overflow → whole-process crash (critical)
The recursive-descent parser had no depth limit. Deeply nested input — many
(or repeatedNOT— recurses throughparseExpressionuntil the goroutinestack is exhausted. In Go a stack overflow is a fatal runtime error: it
cannot be caught by
recover(), so gqlgen's panic-recovery middleware doesnot contain it and the entire process crashes (proxy, admin API and all
in-flight intercepts). The parse happens before the "no active project" check,
so no project/auth/setup is required.
Fix: add a
maxParseDepth(256) guard inparseExpression, the singlerecursion point reached by grouped/prefix/infix sub-expression parsing.
Legitimate, human-written queries nest far below this.
2. Lexer goroutine leak (high)
NewLexerruns a goroutine that emits tokens on an unbuffered channel. WhenParseQueryreturned early on a parse error it stopped draining the channel,leaving that goroutine blocked on its send forever. Every malformed query
leaked one goroutine → gradual goroutine/memory exhaustion, also remotely
triggerable.
Fix: make the lexer cancellable (a
donechannel +Stop());ParseQuerydefers
Stop()so the goroutine is released on every return path.How to verify
Crash on current
main:With this PR:
Tests added
TestParseQueryDepthLimit— 1M-deep(/NOT/ nested-group inputs nowreturn
ErrMaxDepthExceededinstead of crashing.TestParseQueryReasonableNestingStillWorks— legitimate nesting still parses.No behavior change for valid expressions; no public API change.