Conversation
Transaction::intervention() hands out a strdup'd copy of the redirect URL, which the connector owns. The early return taken when the response headers have already been sent (the only reachable outcome for a phase 4 RESPONSE_BODY redirect, since ngx_http_header_filter has already run) returned without releasing it, leaking the string on every such request. The other leak of the same string, on the successful redirect path where the Location header value is pointed straight at the strdup'd buffer, is deliberately left alone here: that path needs the URL copied into r->pool before the buffer is released (freeing it in place would leave the Location header pointing at freed memory), and it is the subject of a separate change. Add a test covering both redirect shapes: a phase 1 redirect, asserting the status and the exact Location value, and a phase 4 RESPONSE_BODY redirect, asserting that the rule is audit logged and that the worker keeps serving afterwards.
|
Warning Review limit reachedNext included review available in 6 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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.
what
intervention.urlon the early return taken inngx_http_modsecurity_process_intervention()when the response headers have already been sent. libmodsecurity'sTransaction::intervention()hands out astrdup'd copy that the caller owns; this path returned without releasing it.tests/modsecurity-intervention-redirect.t: a phase 1 redirect (asserts the 302 and the exactLocationvalue) and a phase 4RESPONSE_BODYredirect after headers were sent (asserts the rule is audit-logged and the worker keeps serving). It makes no assertion about how the phase 4 response ends, which fix: fail closed on swallowed WAF return values #384 changes.why
--leak-check=full) showed this record growing linearly with request count: 720 bytes in 20 blocks after 20 phase 4 redirects, 7,200 bytes in 200 blocks after 200. With the fix the record is gone at both counts, and there are no invalid or double frees.redirect:action can reach: by the time the body filter runs,ngx_http_header_filterhas already sent the headers, so every such intervention leaked.references
Locationheader value aliases thestrdup'd buffer, which is never freed) is the subject of fix: free intervention.url on redirect path and check ngx_list_push r… #378 and is intentionally not touched here. Note for that change: the URL has to be copied intor->poolbefore the buffer is freed, otherwise theLocationheader points at freed memory; the exact-Locationassertion in the new test would catch that.mcf == NULLpath) and fix: fail closed on swallowed WAF return values #384 (create_ctxerror handling) are likewise untouched.Origin: this fix comes from a memory-leak audit of the connector done with Claude Fable 5.1 (Anthropic), following its performance review. Verified by building against nginx master with libmodsecurity 3.0.14 (PCRE2) and with upstream CI's flags against libmodsecurity 3.0.9 (PCRE1), running the full
tests/modsecurity*.tsuite in both builds (16 files, 255 tests, all passing), and by valgrind runs of 20 and 200 requests before and after the change.