fix: header registration visibility and body-loop edge case - #386
Open
fzipi wants to merge 1 commit into
Open
Conversation
msc_add_n_request_header() and msc_add_n_response_header() signal failure via their return value, but every call site discarded it -- both the resolver dispatch loop and the raw header_out/header_in list loops. A failure (OOM inside libmodsecurity) meant that header simply never reached the WAF, with no visibility into it. Log a warning on failure. Unlike the request/response body phase, dropping a single header is not equivalent to skipping inspection outright, so this logs rather than fails the request closed. ngx_str_to_char() returns NULL for a zero-length ngx_str_t (not an error, just "empty"), and most callers check for it, but client_addr and server_addr in the connection-info block didn't. An empty addr_text (e.g. some unix-socket configurations) would pass NULL straight into msc_process_connection(), which expects a C string. The response-body loop in the body filter never broke out after running msc_process_response_body() on the buffer carrying last_buf. A filter upstream of us (sub_filter, gzip, ...) can append a trailing buffer after the last_buf link in the same chain, which would loop back around and call msc_append_response_body() again on an already-finalized transaction. The full chain is still forwarded to the next filter regardless of where this loop stops inspecting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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.



Summary
msc_add_n_request_header()andmsc_add_n_response_header()signal failure via their return value, but every call site discarded it — the resolver dispatch loop inngx_http_modsecurity_header_filter(), the rawheaders_out/headers_inlist loops, and the nestedKeep-Aliveadd insidengx_http_modsecurity_resolv_header_connection(). A failure (OOM inside libmodsecurity) meant that header simply never reached the WAF with no visibility into it. These now log a warning on failure. Unlike the request/response body phase (handled in a separate PR), dropping a single header isn't equivalent to skipping inspection outright, so this logs rather than fails the request closed.ngx_str_to_char()returnsNULLfor a zero-lengthngx_str_t(not an error, just "empty"), and most callers already check for it (e.g.n_uri,host_name), butclient_addrandserver_addrin the connection-info block didn't. An emptyaddr_text(e.g. some unix-socket configurations) would passNULLstraight intomsc_process_connection(), which expects a C string.ngx_http_modsecurity_body_filter()never broke out after runningmsc_process_response_body()on the buffer carryinglast_buf. A filter upstream of us (sub_filter, gzip, ...) can append a trailing buffer after thelast_buflink in the same chain, which would loop back around and callmsc_append_response_body()again on an already-finalized transaction. The full chain is still forwarded to the next filter regardless of where this loop stops inspecting, so nothing is dropped from the response.Test plan
tests/*.tsuite (prove modsecurity*.t) to confirm no regression.