Reject some messages a conformant sender would not have sent #728
Conversation
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
The header validation required by HTTP/2 and HTTP/3 is identical, so use a common function for both. This will make it easier to add additional validation in the future. Move the function to ngx_http_parse.c so that it can share code with the HTTP/1.x parser in the future. No functional change intended.
Per RFC9110, HTTP field values never contain leading or trailing whitespace. Strip all such whitespace from HTTP and HTTP field values. The HTTP/1.x parser already stripped spaces but didn't strip tabs, so change the parser to strip tabs as well. In HTTP/2+, the stripping is done during validation. This requires modifying the value. There are three ways to modify the value: 1. Modify the data in-place with memmove(). 2. Move the data pointer to point to after the leading whitespace. 3. Allocate a new buffer and replace the data pointer. Both HPACK and QPACK decompression make a copy of the data, but some code might assume that the data pointer of a field value can safely be passed to ngx_pfree(). Therefore, the first option is chosen. Existing code ensures that header values are NUL-terminated, so the stripping code NUL-pads header values to ensure that the stripped strings have at least as many terminating NUL bytes as they did before being stripped. The stripping code has been tested in a standalone program to make sure that it works correctly, and it correctly strips leading and trailing whitespace from a variety of strings. This code has also been tested with real HTTP/3 requests from Cloudflare's h3i tool. Fixes: nginx#598
RFC9114 requires invalid pseudo-header fields to be rejected, and this is consistent with HTTP/2.
This makes the behavior of HTTP/2 and HTTP/3 much more similar. In particular, the HTTP/3 :authority pseudoheader is used to set the Host header, instead of the virtual server. This is arguably less correct, but it is consistent with the existing HTTP/2 behavior and unbreaks users of PHP-FPM and other FastCGI applications. In the future, NGINX could have a config option that caused :authority and Host to be treated separately in both HTTP/2 and HTTP/3. Fixes: nginx#587 Fixes: nginx#256
RFC9113 and RFC9114 are clear that this header cannot be used in these versions of HTTP, and in other proxies accepting Transfer-Encoding has led to security vulnerabilities. NGINX is safe from the vulnerability because it ignores the header, but this is still wrong. Fixes: nginx#612
HTTP headers must be an RFC9110 token, so only a subset of characters are permitted. RFC9113 and RFC9114 require rejecting invalid header characters in HTTP/2 and HTTP/3 respectively, so reject them in HTTP/1.0 and HTTP/1.1 for consistency. This also requires removing the ignore hack for (presumably ancient) versions of IIS.
RFC9110 is clear that the only CTRL character allowed in header values is HTAB. Conform to the standard, as Varnish, H2O, and (I suspect) Hyper do. This also makes the whitespace-stripping code simpler, as any character that is less than 0x21 is either whitespace or rejected.
RFC9113 and RFC9114 both require requests with connection-specific headers to be treated as malformed, with the exception of "te: trailers". Reject requests containing them.
These are forbidden by the standard, and if they were (invalidly) folded into a header by downstream code, it would allow HTTP response splitting. This is a defense in depth measure.
RFC9112 does not permit them.
This is not permitted by RFC9112.
This is consistent with Node.js.
This forbids chunk extensions that violate RFC9112, and _only_ these chunk extensions. Bad whitespace is permitted, but a bare LF instead of CRLF is not.
HTTP/1.1 trailers must follow the same syntax as HTTP headers.
These are forbidden by the relevant standards.
The state machine never returns with state = sw_chunk_data and a size of zero, nor did it return with state = sw_after_data.
All versions of HTTP forbid field (header and trailer) values from having leading or trailing horizontal whitespace (0x20 and 0x09). In HTTP/1.0 and HTTP/1.1, leading and trailing whitespace must be stripped from the field value before further processing. In HTTP/2 and HTTP/3, leading and trailing whitespace must cause the entire message to be considered malformed. Willy Tarreau (lead developer of HAProxy) has indicated that there are clients that actually do send leading and/or trailing whitespace in HTTP/2 and/or HTTP/3 cookie headers, which is why HAProxy accepts them. Therefore, the fix is disabled by default and must be enabled with the reject_leading_trailing_whitespace directive.
This is consistent with Node.js.
This is consistent with llhttp.
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.
Proposed changes
This is meant for testing, not for merging.