fix: fail closed on swallowed WAF return values

[fzipi]

Summary

• msc_process_request_body() and msc_process_response_body() signal failure via their return value (1 = success, 0 = failure per libmodsecurity's C API), but every call site in ngx_http_modsecurity_access.c / ngx_http_modsecurity_body_filter.c discarded it and kept forwarding the request/response as if it had been fully inspected. They now check the return value and fail closed with a 500.
• ngx_http_modsecurity_process_intervention() can return -1 (NGX_ERROR) when it wants to block or redirect but headers were already sent. Most call sites only checked for a positive return, silently letting the request/response through uninspected. All call sites in ngx_http_modsecurity_access.c, ngx_http_modsecurity_body_filter.c and ngx_http_modsecurity_header_filter.c now handle it.
• ctx->intervention_triggered was only set on some of the blocking paths, so a later filter invocation for the same request could re-run the WAF on an already-finished transaction. It's now set consistently on every early-return path.
• ngx_http_modsecurity_create_ctx() never checked whether msc_new_transaction()/msc_new_transaction_with_id() returned NULL, and three of its own error paths returned NGX_CONF_ERROR ((void*)-1) instead of NULL — the only caller only checks for NULL, so that sentinel was never caught and would have dereferenced a bogus pointer instead of cleanly failing with a 500.

Added: phase-4 RESPONSE_BODY blocking always corrupted the response

While building a local libmodsecurity + nginx stack to actually verify the ret < 0 handling above (rather than just reasoning about the code), I found that the -1 case added above was itself broken on arrival.

process_intervention() returns -1 specifically when r->header_sent is already true — it wants to intervene but can't safely rewrite headers. By the time the body filter runs, header_sent is always true (this connector doesn't delay response headers until body inspection finishes), so every phase-4/5 RESPONSE_BODY deny/block/redirect hits this branch. Both ret < 0 sites in ngx_http_modsecurity_body_filter.c called ngx_http_filter_finalize_request(), which calls nginx's own ngx_http_clean_header() and tries to send a fresh status line and headers regardless — since headers had already gone out, this produced nginx's own [alert] header already sent and delivered a corrupted response instead of a clean block.

Confirmed with an instrumented build: ret was -1 with header_sent=1 on every phase-4 deny I triggered — the positive/redirect case those branches were apparently also written to cover can't actually occur here, since process_intervention() can only return a positive status when header_sent is false, which never holds in the body filter.

Fix: return NGX_ERROR instead. nginx's own ngx_http_finalize_request() treats NGX_ERROR as "abort the connection, don't try to build a response" (ngx_http_terminate_request()) — the correct behavior once headers can no longer be rewritten. For a response that fits in one buffer this drops the connection before anything goes out; for a larger, multi-buffer response the client sees however much had already been forwarded, then the connection closes — never anything resembling a complete, successfully-served response. Added tests/modsecurity-response-body-deny.t, confirmed to fail on the pre-fix code ([alert] header already sent) and pass with this fix.

Fixed: CI caught a real regression — reverted the fail-closed check on the append_*/*_from_file calls

CI (modsecurity-config-merge.t, modsecurity-request-body.t) failed on every SecRequestBodyLimitAction ProcessPartial / SecResponseBodyLimitAction ProcessPartial scenario, always returning 500 instead of passing.

Root cause: msc_append_request_body() / msc_append_response_body() / msc_request_body_from_file() return 0 not only on a genuine failure but also — indistinguishably, from the caller's side — whenever the relevant *BodyLimitAction is ProcessPartial and the body exceeds the configured limit. Transaction::appendRequestBody()/appendResponseBody() in libmodsecurity deliberately truncate at the limit and report it via the exact same return value as a real failure (appendResponseBody()'s own docstring is literally @retval false Operation failed, process partial demanded). requestBodyFromFile() delegates to appendRequestBody(), so it inherits the same ambiguity. This is normal, by-design behavior — the truncated content is still evaluated by the subsequent process_request_body()/process_response_body() call — not an inspection bypass, so my original "fail closed on any non-1 return" was wrong for these three specific functions. Reverted to the original behavior for just those three (ignore the return value); msc_process_request_body()/msc_process_response_body() don't have this ambiguity (the reference implementation always returns success there regardless of body limit) and keep their fail-closed check, as does the process_intervention() ret < 0 handling.

Test plan

• Built libmodsecurity v3 + nginx locally (macOS) against both the pre-fix and fixed code; confirmed with curl -v that the pre-fix build corrupts the response (200 headers already sent, then "header already sent" alert, then truncated/duplicated body) and the fixed build cleanly aborts the connection instead, for both a single-buffer (small) and multi-buffer (large) response.
• tests/modsecurity-response-body-deny.t added; ran it directly against both binaries via nginx-tests — fails on the pre-fix code ([alert] header already sent), passes with the fix.
• CI caught the ProcessPartial regression on linux-gcc/linux-clang across all three nginx versions; reproduced locally, fixed, and re-ran modsecurity-config-merge.t + modsecurity-request-body.t + modsecurity-response-body-deny.t against a rebuilt binary — all pass.
• CI: full green run across all matrix jobs (Linux passing as of the latest push; the unrelated windows-nginx-* failures on this and other open PRs are a flaky third-party tarball download in test_new.yml's "Set up third-party libraries" step, not caused by this change).

[fzipi]

Summary

• msc_append_request_body(), msc_process_request_body(), msc_request_body_from_file(), msc_append_response_body() and msc_process_response_body() all signal failure via their return value (1 = success, 0 = failure per libmodsecurity's C API), but every call site in ngx_http_modsecurity_access.c / ngx_http_modsecurity_body_filter.c discarded it and kept forwarding the request/response as if it had been fully inspected. They now check the return value and fail closed with a 500.
• ngx_http_modsecurity_process_intervention() can return -1 (NGX_ERROR) when it wants to block or redirect but headers were already sent. Most call sites only checked for a positive return, silently letting the request/response through uninspected. All call sites in ngx_http_modsecurity_access.c, ngx_http_modsecurity_body_filter.c and ngx_http_modsecurity_header_filter.c now handle it.
• ctx->intervention_triggered was only set on some of the blocking paths, so a later filter invocation for the same request could re-run the WAF on an already-finished transaction. It's now set consistently on every early-return path.
• ngx_http_modsecurity_create_ctx() never checked whether msc_new_transaction()/msc_new_transaction_with_id() returned NULL, and three of its own error paths returned NGX_CONF_ERROR ((void*)-1) instead of NULL — the only caller only checks for NULL, so that sentinel was never caught and would have dereferenced a bogus pointer instead of cleanly failing with a 500.

Added: phase-4 RESPONSE_BODY blocking always corrupted the response

While building a local libmodsecurity + nginx stack to actually verify the ret < 0 handling above (rather than just reasoning about the code), I found that the -1 case added above was itself broken on arrival.

process_intervention() returns -1 specifically when r->header_sent is already true — it wants to intervene but can't safely rewrite headers. By the time the body filter runs, header_sent is always true (this connector doesn't delay response headers until body inspection finishes), so every phase-4/5 RESPONSE_BODY deny/block/redirect hits this branch. Both ret < 0 sites in ngx_http_modsecurity_body_filter.c called ngx_http_filter_finalize_request(), which calls nginx's own ngx_http_clean_header() and tries to send a fresh status line and headers regardless — since headers had already gone out, this produced nginx's own [alert] header already sent and delivered a corrupted response instead of a clean block.

Confirmed with an instrumented build: ret was -1 with header_sent=1 on every phase-4 deny I triggered — the positive/redirect case those branches were apparently also written to cover can't actually occur here, since process_intervention() can only return a positive status when header_sent is false, which never holds in the body filter.

Fix: return NGX_ERROR instead. nginx's own ngx_http_finalize_request() treats NGX_ERROR as "abort the connection, don't try to build a response" (ngx_http_terminate_request()) — the correct behavior once headers can no longer be rewritten. For a response that fits in one buffer this drops the connection before anything goes out; for a larger, multi-buffer response the client sees however much had already been forwarded, then the connection closes — never anything resembling a complete, successfully-served response. Added tests/modsecurity-response-body-deny.t, confirmed to fail on the pre-fix code ([alert] header already sent) and pass with this fix.

Test plan

• Built libmodsecurity v3 + nginx locally (macOS) against both the pre-fix and fixed code; confirmed with curl -v that the pre-fix build corrupts the response (200 headers already sent, then "header already sent" alert, then truncated/duplicated body) and the fixed build cleanly aborts the connection instead, for both a single-buffer (small) and multi-buffer (large) response.
• tests/modsecurity-response-body-deny.t added; ran it directly against both binaries via nginx-tests — fails on the pre-fix code ([alert] header already sent), passes with the fix.
• CI: run the full existing tests/*.t suite (prove modsecurity*.t) on Linux to confirm no platform-specific regression (local verification was on macOS/kqueue, CI runs Linux/epoll).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[fzipi]

Found and fixed a real regression from CI: the fail-closed check on msc_append_request_body/msc_append_response_body/msc_request_body_from_file treated SecRequestBodyLimitAction/SecResponseBodyLimitAction ProcessPartial's by-design truncation-at-limit signal as a hard failure, breaking modsecurity-config-merge.t and modsecurity-request-body(-h2).t's "process partial" cases on every run. Reverted that specific check (kept the process_request_body/process_response_body checks, which don't have this ambiguity). See the updated PR description for the full root-cause writeup. Reproduced locally, fixed, and confirmed the previously-failing tests now pass.

[airween]

Found and fixed a real regression from CI: the fail-closed check on msc_append_request_body/msc_append_response_body/msc_request_body_from_file treated SecRequestBodyLimitAction/SecResponseBodyLimitAction ProcessPartial's by-design truncation-at-limit signal as a hard failure, breaking modsecurity-config-merge.t and modsecurity-request-body(-h2).t's "process partial" cases on every run. Reverted that specific check (kept the process_request_body/process_response_body checks, which don't have this ambiguity). See the updated PR description for the full root-cause writeup. Reproduced locally, fixed, and confirmed the previously-failing tests now pass.

There is a pending PR: owasp-modsecurity/ModSecurity#3476, which is related to this one. I'd wait with this until that will be merged.