Repository navigation
Conversation
|
✅ All required contributors have signed the F5 CLA for this PR. Thank you! |
|
Companion nginx-tests draft: nginx/nginx-tests#58 |
|
You’ll need to sign the F5 CLA before this can be looked at. |
|
Also, what about field names? Those also need sanitization. Would it be possible to simply skip the whole field if the name is bad? That matches the behavior of typical applications, which ignore unknown headers. |
|
I have hereby read the F5 CLA and agree to its terms |
louzt
changed the title
|
Fair point.
Current shape on the branch:
I also updated the companion test so it now covers both cases:
I agree that skipping the whole field is the better shape for bad names here. |
github-project-automation
Bot
moved this from New
to In Progress
in NGINX OSS Unified Workspace
#590 (comment) suggests that config-time validation for these directives would be a good idea: HTTP field name/value pairs:
CGI-style name/value pairs:
The first six are different than the last three because (to the best of my knowledge) *CGI accept only a subset of what HTTP allows as headers. I belive the regex is This should be a separate PR, though. |
|
Validating the names can be dropped from this PR. Validating the values is much more important, as the names are static and so cannot be attacker-controlled. |
f91fde6 to
1792031
Compare
|
Rebased onto current master and dropped the 3 stale name-validation commits so the C code matches the value-only scope Demi's 2026-06-21 direction requested. Body refreshed to reflect the new patch shape (44 insertions) and the 6/6 test pass on the patched branch. Lab notes and branch matrix: https://gist.github.com/louzt/7bdf370a28126718e7e7b69d53b0ae86 |
|
Takeover notice (same root cause, disclosure): #1933 fixes this root cause at the serialization layer and supersedes this PR as the complete fix. This PR has been quiet since 2026-08-07 (last activity is yours) and ships no test against this repository. Disclosing the relationship openly rather than racing it:
If you return and would rather land this PR first, say so here and #1933 can be rebased on top of it — the serialization sites still need closing either way. If the maintainers prefer your narrower layer instead, #1933 can be closed; no objection from me. |
Summary
This splits the late response-filter part of the CRLF hardening discussion from #590 into a standalone change for add_header and add_trailer field values.
It covers value sanitization only. Field-name validation is intentionally not in scope here, per the maintainer direction from 2026-06-21 that names are static and cannot be attacker-controlled, so value sanitization is the runtime concern worth shipping first.
Why
The proxy request construction path and the response filter path have different failure timing.
For proxy request construction, rejection can happen before unsafe bytes are emitted upstream.
For add_header and especially add_trailer, the invalid field can be discovered late enough that a clean rejection path is not a good fit for the response lifecycle.
This patch keeps the response alive while preventing invalid value bytes from producing malformed serialized fields.
What changed
Patch shape
Validation
Risks / Follow-ups
This is a policy choice for the late response-filter path, not a claim that sanitization is the only acceptable behavior.
If upstream prefers rejection instead, add_header likely needs earlier validation while add_trailer probably needs a dedicated late-body/trailer failure strategy rather than the same return path used by earlier serialization boundaries.
Field-name validation is intentionally out of scope here and remains a separate concern: static config-time validation can be discussed independently if maintainers prefer that boundary.