Draft: sanitize response header CRLF values

[louzt]

Summary

This is an experimental stacked follow-up for the add_header / add_trailer residual behavior discussed in nginx#590.

It sanitizes runtime-expanded response header/trailer values by copying only when NUL/CR/LF is present, replacing those bytes with spaces before serialization.

This intentionally treats response filters differently from the proxy request path: proxy request construction can reject before upstream bytes are emitted, while trailers may be discovered after the response status/body has already been committed.

Stacking

This draft is stacked on top of #1.

Until that PR lands, the diff includes the log-level commit as well as this sanitization commit. The new policy code in this draft is the second commit only:

HTTP: sanitize response header CRLF values

Why draft

This is not intended as a final upstream policy decision by itself. It is a small reviewable sandbox for one possible response-filter policy:

• reject malformed proxy output before bytes leave NGINX;
• sanitize add_header / add_trailer runtime values to avoid emitting new field syntax when a clean 500 is no longer reliable.

The alternative policy would be deeper response-filter state-machine work to produce deterministic rejection semantics, especially for trailers.

Validation

Normal debug build:

proxy_crlf_injection_sanitize_runtime.t
1..12
ok 1-6  - proxy rejection behavior remains intact
ok 7-10 - add_header/add_trailer keep 200 and emit no injected field/trailer
ok 11   - no alerts
ok 12   - no sanitizer errors

ASAN/UBSAN debug build:

proxy_crlf_injection_sanitize_runtime.t
1..12
ok 1-12

Supplementary lab notes: https://gist.github.com/louzt/7bdf370a28126718e7e7b69d53b0ae86

[DemiMarie]

I see! What happens with the current behavior?

I’m not sure if it is better to mangle data or just close the connection, but arguably this is less likely to cause problems in production. In particular, breaking TLS session resumption is less than great, especially under high load.

[louzt]

With the current reject-on-invalid response-filter behavior, the proxy construction cases are fine because they fail before bytes are sent upstream. The awkward part is add_header / add_trailer.

In my local experiment, trying to convert those late filter errors into a clean 500 did not work reliably: the test still failed for the response-filter cases, and the error log included header already sent plus a worker signal. That matches the concern that the trailer path in particular can be too late to turn the response into a normal special response.

The sanitization branch changes only that late response-filter policy: it keeps the response alive, maps NUL/CR/LF to SP in a request-pool copy, and prevents those bytes from creating new field syntax. The focused test passes for both add_header and add_trailer, with no alerts and no sanitizer errors, including under ASAN/UBSAN.

So I agree with your production-safety concern. Closing the connection may be the purer fail-closed behavior, but at this point in the filter chain it is not a clean failure mode and can harm reuse/resumption. Sanitizing is a smaller operational blast radius, with the caveat that it intentionally mangles the field value: text like X-Injected: yes can remain as literal text inside the same value, but it is no longer serialized as a new field.

If upstream prefers rejection as the policy, I think add_header likely needs earlier validation, while add_trailer needs a separate late-body/trailer strategy rather than the same return path.

[DemiMarie]

Would you mind sending an upstream PR for the add_header and add_trailer changes, along with tests?

Unfortunately, I don’t have much time, so it would be better to send PRs directly to upstream if possible.

[louzt]

Done.

I split this into direct upstream drafts, as requested:

• HTTP: sanitize invalid add_header and add_trailer fields nginx/nginx#1414
• Tests: added add_header/add_trailer field sanitization test. nginx/nginx-tests#58

I kept the scope response-filter-only, separate from the proxy request-boundary work in nginx#590.

The nginx-tests companion is standalone now rather than stacked on the proxy behavior:

• current nginx/nginx master: header_filter_sanitize.t fails 2/8;
• HTTP: sanitize invalid add_header and add_trailer fields nginx/nginx#1414: header_filter_sanitize.t passes 8/8;
• ASAN/UBSAN build of that branch: header_filter_sanitize.t passes 8/8.

I will keep this fork PR around as a local/reference sandbox, but the maintainer-facing discussion can move to the upstream drafts from here.