From 80b41e100204d827ef54c916a6c8cab1365b97d7 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 25 Mar 2025 14:58:32 -0400 Subject: [PATCH 01/21] HTTP: Use common header validation function for HTTP/2 and HTTP/3 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. --- src/http/ngx_http.h | 2 + src/http/ngx_http_parse.c | 62 +++++++++++++++++++++++++++++++ src/http/v2/ngx_http_v2.c | 57 +--------------------------- src/http/v3/ngx_http_v3_request.c | 55 +-------------------------- 4 files changed, 67 insertions(+), 109 deletions(-) diff --git a/src/http/ngx_http.h b/src/http/ngx_http.h index cb4a1e68a..b31cd4a1d 100644 --- a/src/http/ngx_http.h +++ b/src/http/ngx_http.h @@ -182,6 +182,8 @@ ngx_int_t ngx_http_huff_decode(u_char *state, u_char *src, size_t len, u_char **dst, ngx_uint_t last, ngx_log_t *log); size_t ngx_http_huff_encode(u_char *src, size_t len, u_char *dst, ngx_uint_t lower); +ngx_int_t ngx_http_v23_validate_header(ngx_http_request_t *r, + ngx_str_t *name, ngx_str_t *value); #endif diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index a45c04554..f1d2f9b5e 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -1090,6 +1090,68 @@ header_done: } +#if (NGX_HTTP_V2 || NGX_HTTP_V3) +ngx_int_t +ngx_http_v23_validate_header(ngx_http_request_t *r, ngx_str_t *name, + ngx_str_t *value) +{ + u_char ch; + ngx_uint_t i; + ngx_http_core_srv_conf_t *cscf; + + r->invalid_header = 0; + + cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module); + + if (name->len < 1) { + ngx_log_error(NGX_LOG_EMERG, r->connection->log, 0, + "BUG: internal zero-length header name"); + + return NGX_ERROR; + } + + for (i = (name->data[0] == ':'); i != name->len; i++) { + ch = name->data[i]; + + if ((ch >= 'a' && ch <= 'z') + || (ch == '-') + || (ch >= '0' && ch <= '9') + || (ch == '_' && cscf->underscores_in_headers)) + { + continue; + } + + if (ch <= 0x20 || ch == 0x7f || ch == ':' + || (ch >= 'A' && ch <= 'Z')) + { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent invalid header name: \"%V\"", + name); + + return NGX_ERROR; + } + + r->invalid_header = 1; + } + + for (i = 0; i != value->len; i++) { + ch = value->data[i]; + + if (ch == '\0' || ch == LF || ch == CR) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent header \"%V\" with " + "invalid value: \"%V\"", + name, value); + + return NGX_ERROR; + } + } + + return NGX_OK; +} +#endif + + ngx_int_t ngx_http_parse_uri(ngx_http_request_t *r) { diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index 91a28b228..25b71aa4a 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -142,8 +142,6 @@ static ngx_http_v2_out_frame_t *ngx_http_v2_get_frame( static ngx_int_t ngx_http_v2_frame_handler(ngx_http_v2_connection_t *h2c, ngx_http_v2_out_frame_t *frame); -static ngx_int_t ngx_http_v2_validate_header(ngx_http_request_t *r, - ngx_http_v2_header_t *header); static ngx_int_t ngx_http_v2_pseudo_header(ngx_http_request_t *r, ngx_http_v2_header_t *header); static ngx_int_t ngx_http_v2_parse_path(ngx_http_request_t *r, @@ -1774,7 +1772,8 @@ ngx_http_v2_state_process_header(ngx_http_v2_connection_t *h2c, u_char *pos, fc = r->connection; /* TODO Optimization: validate headers while parsing. */ - if (ngx_http_v2_validate_header(r, header) != NGX_OK) { + if (ngx_http_v23_validate_header(r, &header->name, &header->value) + != NGX_OK) { ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); goto error; } @@ -3232,58 +3231,6 @@ ngx_http_v2_get_closed_node(ngx_http_v2_connection_t *h2c) } -static ngx_int_t -ngx_http_v2_validate_header(ngx_http_request_t *r, ngx_http_v2_header_t *header) -{ - u_char ch; - ngx_uint_t i; - ngx_http_core_srv_conf_t *cscf; - - r->invalid_header = 0; - - cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module); - - for (i = (header->name.data[0] == ':'); i != header->name.len; i++) { - ch = header->name.data[i]; - - if ((ch >= 'a' && ch <= 'z') - || (ch == '-') - || (ch >= '0' && ch <= '9') - || (ch == '_' && cscf->underscores_in_headers)) - { - continue; - } - - if (ch <= 0x20 || ch == 0x7f || ch == ':' - || (ch >= 'A' && ch <= 'Z')) - { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent invalid header name: \"%V\"", - &header->name); - - return NGX_ERROR; - } - - r->invalid_header = 1; - } - - for (i = 0; i != header->value.len; i++) { - ch = header->value.data[i]; - - if (ch == '\0' || ch == LF || ch == CR) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent header \"%V\" with " - "invalid value: \"%V\"", - &header->name, &header->value); - - return NGX_ERROR; - } - } - - return NGX_OK; -} - - static ngx_int_t ngx_http_v2_pseudo_header(ngx_http_request_t *r, ngx_http_v2_header_t *header) { diff --git a/src/http/v3/ngx_http_v3_request.c b/src/http/v3/ngx_http_v3_request.c index e41ad50a8..e2ffc7543 100644 --- a/src/http/v3/ngx_http_v3_request.c +++ b/src/http/v3/ngx_http_v3_request.c @@ -17,8 +17,6 @@ static void ngx_http_v3_cleanup_request(void *data); static void ngx_http_v3_process_request(ngx_event_t *rev); static ngx_int_t ngx_http_v3_process_header(ngx_http_request_t *r, ngx_str_t *name, ngx_str_t *value); -static ngx_int_t ngx_http_v3_validate_header(ngx_http_request_t *r, - ngx_str_t *name, ngx_str_t *value); static ngx_int_t ngx_http_v3_process_pseudo_header(ngx_http_request_t *r, ngx_str_t *name, ngx_str_t *value); static ngx_int_t ngx_http_v3_init_pseudo_headers(ngx_http_request_t *r); @@ -632,7 +630,7 @@ ngx_http_v3_process_header(ngx_http_request_t *r, ngx_str_t *name, r->v3_parse->header_limit -= len; - if (ngx_http_v3_validate_header(r, name, value) != NGX_OK) { + if (ngx_http_v23_validate_header(r, name, value) != NGX_OK) { ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); return NGX_ERROR; } @@ -692,57 +690,6 @@ ngx_http_v3_process_header(ngx_http_request_t *r, ngx_str_t *name, } -static ngx_int_t -ngx_http_v3_validate_header(ngx_http_request_t *r, ngx_str_t *name, - ngx_str_t *value) -{ - u_char ch; - ngx_uint_t i; - ngx_http_core_srv_conf_t *cscf; - - r->invalid_header = 0; - - cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module); - - for (i = (name->data[0] == ':'); i != name->len; i++) { - ch = name->data[i]; - - if ((ch >= 'a' && ch <= 'z') - || (ch == '-') - || (ch >= '0' && ch <= '9') - || (ch == '_' && cscf->underscores_in_headers)) - { - continue; - } - - if (ch <= 0x20 || ch == 0x7f || ch == ':' - || (ch >= 'A' && ch <= 'Z')) - { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent invalid header name: \"%V\"", name); - - return NGX_ERROR; - } - - r->invalid_header = 1; - } - - for (i = 0; i != value->len; i++) { - ch = value->data[i]; - - if (ch == '\0' || ch == LF || ch == CR) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent header \"%V\" with " - "invalid value: \"%V\"", name, value); - - return NGX_ERROR; - } - } - - return NGX_OK; -} - - static ngx_int_t ngx_http_v3_process_pseudo_header(ngx_http_request_t *r, ngx_str_t *name, ngx_str_t *value) From 3897c97cc314e02d75ad92d84ad4c13924151e8e Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 7 Apr 2025 17:30:13 -0400 Subject: [PATCH 02/21] Strip leading and trailing whitespace from HTTP field values 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: #598 --- src/http/ngx_http.h | 6 ++- src/http/ngx_http_parse.c | 67 ++++++++++++++++++++++++++++++- src/http/v2/ngx_http_v2.c | 3 +- src/http/v3/ngx_http_v3_request.c | 2 +- 4 files changed, 73 insertions(+), 5 deletions(-) diff --git a/src/http/ngx_http.h b/src/http/ngx_http.h index b31cd4a1d..3f1b0b7e3 100644 --- a/src/http/ngx_http.h +++ b/src/http/ngx_http.h @@ -182,7 +182,11 @@ ngx_int_t ngx_http_huff_decode(u_char *state, u_char *src, size_t len, u_char **dst, ngx_uint_t last, ngx_log_t *log); size_t ngx_http_huff_encode(u_char *src, size_t len, u_char *dst, ngx_uint_t lower); -ngx_int_t ngx_http_v23_validate_header(ngx_http_request_t *r, +/* + * Check if a header name and/or value is valid. If the value is valid, + * strip leading and trailing space from it. + */ +ngx_int_t ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, ngx_str_t *value); #endif diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index f1d2f9b5e..5665acbc4 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -10,6 +10,11 @@ #include +#if (NGX_HTTP_V2 || NGX_HTTP_V3) +static inline ngx_int_t ngx_isspace(u_char ch); +#endif + + static uint32_t usual[] = { 0x00000000, /* 0000 0000 0000 0000 0000 0000 0000 0000 */ @@ -972,6 +977,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, case sw_space_before_value: switch (ch) { case ' ': + case '\t': break; case CR: r->header_start = p; @@ -996,6 +1002,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, case sw_value: switch (ch) { case ' ': + case '\t': r->header_end = p; state = sw_space_after_value; break; @@ -1016,6 +1023,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, case sw_space_after_value: switch (ch) { case ' ': + case '\t': break; case CR: state = sw_almost_done; @@ -1091,11 +1099,21 @@ header_done: #if (NGX_HTTP_V2 || NGX_HTTP_V3) + + +static inline ngx_int_t +ngx_isspace(u_char ch) +{ + return ch == ' ' || ch == '\t'; +} + + ngx_int_t -ngx_http_v23_validate_header(ngx_http_request_t *r, ngx_str_t *name, +ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, ngx_str_t *value) { u_char ch; + ngx_str_t tmp; ngx_uint_t i; ngx_http_core_srv_conf_t *cscf; @@ -1134,6 +1152,11 @@ ngx_http_v23_validate_header(ngx_http_request_t *r, ngx_str_t *name, r->invalid_header = 1; } + /* Keep subsequent code from having to special-case empty strings. */ + if (value->len == 0) { + return NGX_OK; + } + for (i = 0; i != value->len; i++) { ch = value->data[i]; @@ -1147,6 +1170,48 @@ ngx_http_v23_validate_header(ngx_http_request_t *r, ngx_str_t *name, } } + tmp = *value; + + if (!ngx_isspace(tmp.data[0]) + && !ngx_isspace(tmp.data[tmp.len - 1])) { + /* Fast path: nothing to strip. */ + return NGX_OK; + } + + /* + * Strip trailing whitespace. Do this first so that + * if the string is all whitespace, tmp.data is not a + * past-the-end pointer (which cannot be safely passed + * to memmove()) + */ + while (tmp.len && ngx_isspace(tmp.data[tmp.len - 1])) { + tmp.len--; + } + + /* Strip leading whitespace */ + if (tmp.len && ngx_isspace(tmp.data[0])) { + /* + * Last loop guaranteed that 'tmp' does not end with whitespace, so + * it's safe to keep going until a non-whitespace character is found. + */ + do { + tmp.len--; + tmp.data++; + } while (ngx_isspace(tmp.data[0])); + + /* Move remaining string to start of buffer. */ + memmove(value->data, tmp.data, tmp.len); + } + + /* + * NUL-pad the data, so that if it was NUL-terminated before, it stil is. + * At least one byte will have been stripped, so value->data + tmp.len + * is not a past-the-end pointer. + */ + memset(value->data + tmp.len, '\0', value->len - tmp.len); + + /* Fix up length and return. */ + value->len = tmp.len; return NGX_OK; } #endif diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index 25b71aa4a..11e082ca3 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -1772,8 +1772,7 @@ ngx_http_v2_state_process_header(ngx_http_v2_connection_t *h2c, u_char *pos, fc = r->connection; /* TODO Optimization: validate headers while parsing. */ - if (ngx_http_v23_validate_header(r, &header->name, &header->value) - != NGX_OK) { + if (ngx_http_v23_fixup_header(r, &header->name, &header->value) != NGX_OK) { ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); goto error; } diff --git a/src/http/v3/ngx_http_v3_request.c b/src/http/v3/ngx_http_v3_request.c index e2ffc7543..7aade0400 100644 --- a/src/http/v3/ngx_http_v3_request.c +++ b/src/http/v3/ngx_http_v3_request.c @@ -630,7 +630,7 @@ ngx_http_v3_process_header(ngx_http_request_t *r, ngx_str_t *name, r->v3_parse->header_limit -= len; - if (ngx_http_v23_validate_header(r, name, value) != NGX_OK) { + if (ngx_http_v23_fixup_header(r, name, value) != NGX_OK) { ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); return NGX_ERROR; } From c7d26ccc9b29d40b6b22ba52862e37fb1f933b2b Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 21 Mar 2025 19:48:30 -0400 Subject: [PATCH 03/21] HTTP/3: Do not allow invalid pseudo-header fields RFC9114 requires invalid pseudo-header fields to be rejected, and this is consistent with HTTP/2. --- src/http/v3/ngx_http_v3_request.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/http/v3/ngx_http_v3_request.c b/src/http/v3/ngx_http_v3_request.c index 7aade0400..fff5dd909 100644 --- a/src/http/v3/ngx_http_v3_request.c +++ b/src/http/v3/ngx_http_v3_request.c @@ -635,6 +635,10 @@ ngx_http_v3_process_header(ngx_http_request_t *r, ngx_str_t *name, return NGX_ERROR; } + if (name->len && name->data[0] == ':') { + return ngx_http_v3_process_pseudo_header(r, name, value); + } + if (r->invalid_header) { cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module); @@ -646,10 +650,6 @@ ngx_http_v3_process_header(ngx_http_request_t *r, ngx_str_t *name, } } - if (name->len && name->data[0] == ':') { - return ngx_http_v3_process_pseudo_header(r, name, value); - } - if (ngx_http_v3_init_pseudo_headers(r) != NGX_OK) { return NGX_ERROR; } From 17ce4bcfa3c87b5aa9cc1ee895b9112859f010b2 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 22 Mar 2025 01:03:00 -0400 Subject: [PATCH 04/21] HTTP: Use common header code for v2 and v3 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: #587 Fixes: #256 --- src/http/ngx_http.c | 300 ++++++++++++++++++++++++++++++ src/http/ngx_http.h | 2 + src/http/v2/ngx_http_v2.c | 293 +---------------------------- src/http/v3/ngx_http_v3_request.c | 237 ++--------------------- 4 files changed, 319 insertions(+), 513 deletions(-) diff --git a/src/http/ngx_http.c b/src/http/ngx_http.c index d835f896e..7a02db7ea 100644 --- a/src/http/ngx_http.c +++ b/src/http/ngx_http.c @@ -68,6 +68,17 @@ static ngx_int_t ngx_http_add_addrs6(ngx_conf_t *cf, ngx_http_port_t *hport, ngx_http_conf_addr_t *addr); #endif +#if (NGX_HTTP_V2 || NGX_HTTP_V3) +static ngx_int_t ngx_http_v23_parse_path(ngx_http_request_t *r, + ngx_str_t *value); +static ngx_int_t ngx_http_v23_parse_method(ngx_http_request_t *r, + ngx_str_t *value); +static ngx_int_t ngx_http_v23_parse_scheme(ngx_http_request_t *r, + ngx_str_t *value); +static ngx_int_t ngx_http_v23_parse_authority(ngx_http_request_t *r, + ngx_str_t *value); +#endif + ngx_uint_t ngx_http_max_module; @@ -1227,6 +1238,295 @@ ngx_http_add_listen(ngx_conf_t *cf, ngx_http_core_srv_conf_t *cscf, return ngx_http_add_address(cf, cscf, port, lsopt); } +#if (NGX_HTTP_V2 || NGX_HTTP_V3) + +ngx_int_t +ngx_http_v23_pseudo_header(ngx_http_request_t *r, ngx_str_t *name, + ngx_str_t *value) +{ + ngx_int_t rc; + + name->len--; + name->data++; + + switch (name->len) { + case 4: + if (ngx_memcmp(name->data, "path", sizeof("path") - 1) + == 0) + { + rc = ngx_http_v23_parse_path(r, value); + goto known; + } + + break; + + case 6: + if (ngx_memcmp(name->data, "method", sizeof("method") - 1) + == 0) + { + rc = ngx_http_v23_parse_method(r, value); + goto known; + } + + if (ngx_memcmp(name->data, "scheme", sizeof("scheme") - 1) + == 0) + { + rc = ngx_http_v23_parse_scheme(r, value); + goto known; + } + + break; + + case 9: + if (ngx_memcmp(name->data, "authority", sizeof("authority") - 1) + == 0) + { + rc = ngx_http_v23_parse_authority(r, value); + goto known; + } + + break; + } + + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent unknown pseudo-header \":%V\"", + name); + rc = NGX_DECLINED; +known: + if (rc == NGX_DECLINED) { + ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); + rc = NGX_ABORT; + } + return rc; +} + + +static ngx_int_t +ngx_http_v23_parse_path(ngx_http_request_t *r, ngx_str_t *value) +{ + if (r->unparsed_uri.len) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent duplicate :path header"); + + return NGX_DECLINED; + } + + if (value->len == 0) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent empty :path header"); + + return NGX_DECLINED; + } + + r->uri_start = value->data; + r->uri_end = value->data + value->len; + + if (ngx_http_parse_uri(r) != NGX_OK) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent invalid :path header: \"%V\"", value); + + return NGX_DECLINED; + } + + if (ngx_http_process_request_uri(r) != NGX_OK) { + /* + * request has been finalized already + * in ngx_http_process_request_uri() + */ + return NGX_ABORT; + } + + return NGX_OK; +} + + +static ngx_int_t +ngx_http_v23_parse_method(ngx_http_request_t *r, ngx_str_t *value) +{ + size_t k, len; + ngx_uint_t n; + const u_char *p, *m; + + /* + * This array takes less than 256 sequential bytes, + * and if typical CPU cache line size is 64 bytes, + * it is prefetched for 4 load operations. + */ + static const struct { + u_char len; + const u_char method[11]; + uint32_t value; + } tests[] = { + { 3, "GET", NGX_HTTP_GET }, + { 4, "POST", NGX_HTTP_POST }, + { 4, "HEAD", NGX_HTTP_HEAD }, + { 7, "OPTIONS", NGX_HTTP_OPTIONS }, + { 8, "PROPFIND", NGX_HTTP_PROPFIND }, + { 3, "PUT", NGX_HTTP_PUT }, + { 5, "MKCOL", NGX_HTTP_MKCOL }, + { 6, "DELETE", NGX_HTTP_DELETE }, + { 4, "COPY", NGX_HTTP_COPY }, + { 4, "MOVE", NGX_HTTP_MOVE }, + { 9, "PROPPATCH", NGX_HTTP_PROPPATCH }, + { 4, "LOCK", NGX_HTTP_LOCK }, + { 6, "UNLOCK", NGX_HTTP_UNLOCK }, + { 5, "PATCH", NGX_HTTP_PATCH }, + { 5, "TRACE", NGX_HTTP_TRACE }, + { 7, "CONNECT", NGX_HTTP_CONNECT } + }, *test; + + if (r->method_name.len) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent duplicate :method header"); + + return NGX_DECLINED; + } + + if (value->len == 0) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent empty :method header"); + + return NGX_DECLINED; + } + + r->method_name.len = value->len; + r->method_name.data = value->data; + + len = r->method_name.len; + n = sizeof(tests) / sizeof(tests[0]); + test = tests; + + do { + if (len == test->len) { + p = r->method_name.data; + m = test->method; + k = len; + + do { + if (*p++ != *m++) { + goto next; + } + } while (--k); + + r->method = test->value; + return NGX_OK; + } + + next: + test++; + + } while (--n); + + p = r->method_name.data; + + do { + if ((*p < 'A' || *p > 'Z') && *p != '_' && *p != '-') { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent invalid method: \"%V\"", + &r->method_name); + + return NGX_DECLINED; + } + + p++; + + } while (--len); + + return NGX_OK; +} + + +static ngx_int_t +ngx_http_v23_parse_scheme(ngx_http_request_t *r, ngx_str_t *value) +{ + u_char c, ch; + ngx_uint_t i; + + if (r->schema.len) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent duplicate :scheme header"); + + return NGX_DECLINED; + } + + if (value->len == 0) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent empty :scheme header"); + + return NGX_DECLINED; + } + + for (i = 0; i < value->len; i++) { + ch = value->data[i]; + + c = (u_char) (ch | 0x20); + if (c >= 'a' && c <= 'z') { + continue; + } + + if (((ch >= '0' && ch <= '9') || ch == '+' || ch == '-' || ch == '.') + && i > 0) + { + continue; + } + + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent invalid :scheme header: \"%V\"", value); + + return NGX_DECLINED; + } + + r->schema = *value; + + return NGX_OK; +} + + +static ngx_int_t +ngx_http_v23_parse_authority(ngx_http_request_t *r, ngx_str_t *value) +{ + ngx_table_elt_t *h; + ngx_http_header_t *hh; + ngx_http_core_main_conf_t *cmcf; + + static ngx_str_t host = ngx_string("host"); + + h = ngx_list_push(&r->headers_in.headers); + if (h == NULL) { + return NGX_ERROR; + } + + h->hash = ngx_hash(ngx_hash(ngx_hash('h', 'o'), 's'), 't'); + + h->key.len = host.len; + h->key.data = host.data; + + h->value.len = value->len; + h->value.data = value->data; + + h->lowcase_key = host.data; + + cmcf = ngx_http_get_module_main_conf(r, ngx_http_core_module); + + hh = ngx_hash_find(&cmcf->headers_in_hash, h->hash, + h->lowcase_key, h->key.len); + + if (hh == NULL) { + return NGX_ERROR; + } + + if (hh->handler(r, h, hh->offset) != NGX_OK) { + /* + * request has been finalized already + * in ngx_http_process_host() + */ + return NGX_ABORT; + } + + return NGX_OK; +} + +#endif static ngx_int_t ngx_http_add_addresses(ngx_conf_t *cf, ngx_http_core_srv_conf_t *cscf, diff --git a/src/http/ngx_http.h b/src/http/ngx_http.h index 3f1b0b7e3..1bfb7aa7f 100644 --- a/src/http/ngx_http.h +++ b/src/http/ngx_http.h @@ -188,6 +188,8 @@ size_t ngx_http_huff_encode(u_char *src, size_t len, u_char *dst, */ ngx_int_t ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, ngx_str_t *value); +ngx_int_t ngx_http_v23_pseudo_header(ngx_http_request_t *r, + ngx_str_t *name, ngx_str_t *value); #endif diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index 11e082ca3..daf0cffdc 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -142,16 +142,6 @@ static ngx_http_v2_out_frame_t *ngx_http_v2_get_frame( static ngx_int_t ngx_http_v2_frame_handler(ngx_http_v2_connection_t *h2c, ngx_http_v2_out_frame_t *frame); -static ngx_int_t ngx_http_v2_pseudo_header(ngx_http_request_t *r, - ngx_http_v2_header_t *header); -static ngx_int_t ngx_http_v2_parse_path(ngx_http_request_t *r, - ngx_str_t *value); -static ngx_int_t ngx_http_v2_parse_method(ngx_http_request_t *r, - ngx_str_t *value); -static ngx_int_t ngx_http_v2_parse_scheme(ngx_http_request_t *r, - ngx_str_t *value); -static ngx_int_t ngx_http_v2_parse_authority(ngx_http_request_t *r, - ngx_str_t *value); static ngx_int_t ngx_http_v2_construct_request_line(ngx_http_request_t *r); static ngx_int_t ngx_http_v2_cookie(ngx_http_request_t *r, ngx_http_v2_header_t *header); @@ -1778,7 +1768,7 @@ ngx_http_v2_state_process_header(ngx_http_v2_connection_t *h2c, u_char *pos, } if (header->name.data[0] == ':') { - rc = ngx_http_v2_pseudo_header(r, header); + rc = ngx_http_v23_pseudo_header(r, &header->name, &header->value); if (rc == NGX_OK) { ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, @@ -1792,11 +1782,6 @@ ngx_http_v2_state_process_header(ngx_http_v2_connection_t *h2c, u_char *pos, goto error; } - if (rc == NGX_DECLINED) { - ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); - goto error; - } - return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_INTERNAL_ERROR); } @@ -3229,282 +3214,6 @@ ngx_http_v2_get_closed_node(ngx_http_v2_connection_t *h2c) return node; } - -static ngx_int_t -ngx_http_v2_pseudo_header(ngx_http_request_t *r, ngx_http_v2_header_t *header) -{ - header->name.len--; - header->name.data++; - - switch (header->name.len) { - case 4: - if (ngx_memcmp(header->name.data, "path", sizeof("path") - 1) - == 0) - { - return ngx_http_v2_parse_path(r, &header->value); - } - - break; - - case 6: - if (ngx_memcmp(header->name.data, "method", sizeof("method") - 1) - == 0) - { - return ngx_http_v2_parse_method(r, &header->value); - } - - if (ngx_memcmp(header->name.data, "scheme", sizeof("scheme") - 1) - == 0) - { - return ngx_http_v2_parse_scheme(r, &header->value); - } - - break; - - case 9: - if (ngx_memcmp(header->name.data, "authority", sizeof("authority") - 1) - == 0) - { - return ngx_http_v2_parse_authority(r, &header->value); - } - - break; - } - - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent unknown pseudo-header \":%V\"", - &header->name); - - return NGX_DECLINED; -} - - -static ngx_int_t -ngx_http_v2_parse_path(ngx_http_request_t *r, ngx_str_t *value) -{ - if (r->unparsed_uri.len) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent duplicate :path header"); - - return NGX_DECLINED; - } - - if (value->len == 0) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent empty :path header"); - - return NGX_DECLINED; - } - - r->uri_start = value->data; - r->uri_end = value->data + value->len; - - if (ngx_http_parse_uri(r) != NGX_OK) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent invalid :path header: \"%V\"", value); - - return NGX_DECLINED; - } - - if (ngx_http_process_request_uri(r) != NGX_OK) { - /* - * request has been finalized already - * in ngx_http_process_request_uri() - */ - return NGX_ABORT; - } - - return NGX_OK; -} - - -static ngx_int_t -ngx_http_v2_parse_method(ngx_http_request_t *r, ngx_str_t *value) -{ - size_t k, len; - ngx_uint_t n; - const u_char *p, *m; - - /* - * This array takes less than 256 sequential bytes, - * and if typical CPU cache line size is 64 bytes, - * it is prefetched for 4 load operations. - */ - static const struct { - u_char len; - const u_char method[11]; - uint32_t value; - } tests[] = { - { 3, "GET", NGX_HTTP_GET }, - { 4, "POST", NGX_HTTP_POST }, - { 4, "HEAD", NGX_HTTP_HEAD }, - { 7, "OPTIONS", NGX_HTTP_OPTIONS }, - { 8, "PROPFIND", NGX_HTTP_PROPFIND }, - { 3, "PUT", NGX_HTTP_PUT }, - { 5, "MKCOL", NGX_HTTP_MKCOL }, - { 6, "DELETE", NGX_HTTP_DELETE }, - { 4, "COPY", NGX_HTTP_COPY }, - { 4, "MOVE", NGX_HTTP_MOVE }, - { 9, "PROPPATCH", NGX_HTTP_PROPPATCH }, - { 4, "LOCK", NGX_HTTP_LOCK }, - { 6, "UNLOCK", NGX_HTTP_UNLOCK }, - { 5, "PATCH", NGX_HTTP_PATCH }, - { 5, "TRACE", NGX_HTTP_TRACE }, - { 7, "CONNECT", NGX_HTTP_CONNECT } - }, *test; - - if (r->method_name.len) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent duplicate :method header"); - - return NGX_DECLINED; - } - - if (value->len == 0) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent empty :method header"); - - return NGX_DECLINED; - } - - r->method_name.len = value->len; - r->method_name.data = value->data; - - len = r->method_name.len; - n = sizeof(tests) / sizeof(tests[0]); - test = tests; - - do { - if (len == test->len) { - p = r->method_name.data; - m = test->method; - k = len; - - do { - if (*p++ != *m++) { - goto next; - } - } while (--k); - - r->method = test->value; - return NGX_OK; - } - - next: - test++; - - } while (--n); - - p = r->method_name.data; - - do { - if ((*p < 'A' || *p > 'Z') && *p != '_' && *p != '-') { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent invalid method: \"%V\"", - &r->method_name); - - return NGX_DECLINED; - } - - p++; - - } while (--len); - - return NGX_OK; -} - - -static ngx_int_t -ngx_http_v2_parse_scheme(ngx_http_request_t *r, ngx_str_t *value) -{ - u_char c, ch; - ngx_uint_t i; - - if (r->schema.len) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent duplicate :scheme header"); - - return NGX_DECLINED; - } - - if (value->len == 0) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent empty :scheme header"); - - return NGX_DECLINED; - } - - for (i = 0; i < value->len; i++) { - ch = value->data[i]; - - c = (u_char) (ch | 0x20); - if (c >= 'a' && c <= 'z') { - continue; - } - - if (((ch >= '0' && ch <= '9') || ch == '+' || ch == '-' || ch == '.') - && i > 0) - { - continue; - } - - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent invalid :scheme header: \"%V\"", value); - - return NGX_DECLINED; - } - - r->schema = *value; - - return NGX_OK; -} - - -static ngx_int_t -ngx_http_v2_parse_authority(ngx_http_request_t *r, ngx_str_t *value) -{ - ngx_table_elt_t *h; - ngx_http_header_t *hh; - ngx_http_core_main_conf_t *cmcf; - - static ngx_str_t host = ngx_string("host"); - - h = ngx_list_push(&r->headers_in.headers); - if (h == NULL) { - return NGX_ERROR; - } - - h->hash = ngx_hash(ngx_hash(ngx_hash('h', 'o'), 's'), 't'); - - h->key.len = host.len; - h->key.data = host.data; - - h->value.len = value->len; - h->value.data = value->data; - - h->lowcase_key = host.data; - - cmcf = ngx_http_get_module_main_conf(r, ngx_http_core_module); - - hh = ngx_hash_find(&cmcf->headers_in_hash, h->hash, - h->lowcase_key, h->key.len); - - if (hh == NULL) { - return NGX_ERROR; - } - - if (hh->handler(r, h, hh->offset) != NGX_OK) { - /* - * request has been finalized already - * in ngx_http_process_host() - */ - return NGX_ABORT; - } - - return NGX_OK; -} - - static ngx_int_t ngx_http_v2_construct_request_line(ngx_http_request_t *r) { diff --git a/src/http/v3/ngx_http_v3_request.c b/src/http/v3/ngx_http_v3_request.c index fff5dd909..3e22b4414 100644 --- a/src/http/v3/ngx_http_v3_request.c +++ b/src/http/v3/ngx_http_v3_request.c @@ -28,31 +28,6 @@ static ngx_int_t ngx_http_v3_do_read_client_request_body(ngx_http_request_t *r); static ngx_int_t ngx_http_v3_request_body_filter(ngx_http_request_t *r, ngx_chain_t *in); - -static const struct { - ngx_str_t name; - ngx_uint_t method; -} ngx_http_v3_methods[] = { - - { ngx_string("GET"), NGX_HTTP_GET }, - { ngx_string("POST"), NGX_HTTP_POST }, - { ngx_string("HEAD"), NGX_HTTP_HEAD }, - { ngx_string("OPTIONS"), NGX_HTTP_OPTIONS }, - { ngx_string("PROPFIND"), NGX_HTTP_PROPFIND }, - { ngx_string("PUT"), NGX_HTTP_PUT }, - { ngx_string("MKCOL"), NGX_HTTP_MKCOL }, - { ngx_string("DELETE"), NGX_HTTP_DELETE }, - { ngx_string("COPY"), NGX_HTTP_COPY }, - { ngx_string("MOVE"), NGX_HTTP_MOVE }, - { ngx_string("PROPPATCH"), NGX_HTTP_PROPPATCH }, - { ngx_string("LOCK"), NGX_HTTP_LOCK }, - { ngx_string("UNLOCK"), NGX_HTTP_UNLOCK }, - { ngx_string("PATCH"), NGX_HTTP_PATCH }, - { ngx_string("TRACE"), NGX_HTTP_TRACE }, - { ngx_string("CONNECT"), NGX_HTTP_CONNECT } -}; - - void ngx_http_v3_init_stream(ngx_connection_t *c) { @@ -389,6 +364,14 @@ ngx_http_v3_wait_request_handler(ngx_event_t *rev) c->data = r; c->requests = (c->quic->id >> 2) + 1; + if (ngx_list_init(&r->headers_in.headers, r->pool, 20, + sizeof(ngx_table_elt_t)) + != NGX_OK) + { + ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR); + return; + } + cln = ngx_pool_cleanup_add(r->pool, 0); if (cln == NULL) { ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR); @@ -694,153 +677,17 @@ static ngx_int_t ngx_http_v3_process_pseudo_header(ngx_http_request_t *r, ngx_str_t *name, ngx_str_t *value) { - u_char ch, c; - ngx_uint_t i; - if (r->request_line.len) { ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, "client sent out of order pseudo-headers"); - goto failed; + ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); + return NGX_ERROR; } - if (name->len == 7 && ngx_strncmp(name->data, ":method", 7) == 0) { - - if (r->method_name.len) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent duplicate \":method\" header"); - goto failed; - } - - if (value->len == 0) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent empty \":method\" header"); - goto failed; - } - - r->method_name = *value; - - for (i = 0; i < sizeof(ngx_http_v3_methods) - / sizeof(ngx_http_v3_methods[0]); i++) - { - if (value->len == ngx_http_v3_methods[i].name.len - && ngx_strncmp(value->data, - ngx_http_v3_methods[i].name.data, value->len) - == 0) - { - r->method = ngx_http_v3_methods[i].method; - break; - } - } - - for (i = 0; i < value->len; i++) { - ch = value->data[i]; - - if ((ch < 'A' || ch > 'Z') && ch != '_' && ch != '-') { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent invalid method: \"%V\"", value); - goto failed; - } - } - - ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, - "http3 method \"%V\" %ui", value, r->method); - return NGX_OK; + if (ngx_http_v23_pseudo_header(r, name, value) != NGX_OK) { + return NGX_ERROR; } - - if (name->len == 5 && ngx_strncmp(name->data, ":path", 5) == 0) { - - if (r->uri_start) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent duplicate \":path\" header"); - goto failed; - } - - if (value->len == 0) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent empty \":path\" header"); - goto failed; - } - - r->uri_start = value->data; - r->uri_end = value->data + value->len; - - if (ngx_http_parse_uri(r) != NGX_OK) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent invalid \":path\" header: \"%V\"", - value); - goto failed; - } - - ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, - "http3 path \"%V\"", value); - return NGX_OK; - } - - if (name->len == 7 && ngx_strncmp(name->data, ":scheme", 7) == 0) { - - if (r->schema.len) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent duplicate \":scheme\" header"); - goto failed; - } - - if (value->len == 0) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent empty \":scheme\" header"); - goto failed; - } - - for (i = 0; i < value->len; i++) { - ch = value->data[i]; - - c = (u_char) (ch | 0x20); - if (c >= 'a' && c <= 'z') { - continue; - } - - if (((ch >= '0' && ch <= '9') - || ch == '+' || ch == '-' || ch == '.') - && i > 0) - { - continue; - } - - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent invalid \":scheme\" header: \"%V\"", - value); - goto failed; - } - - r->schema = *value; - - ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, - "http3 schema \"%V\"", value); - return NGX_OK; - } - - if (name->len == 10 && ngx_strncmp(name->data, ":authority", 10) == 0) { - - if (r->host_start) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent duplicate \":authority\" header"); - goto failed; - } - - r->host_start = value->data; - r->host_end = value->data + value->len; - - ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, - "http3 authority \"%V\"", value); - return NGX_OK; - } - - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent unknown pseudo-header \"%V\"", name); - -failed: - - ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); - return NGX_ERROR; + return NGX_OK; } @@ -928,14 +775,6 @@ ngx_http_v3_init_pseudo_headers(ngx_http_request_t *r) r->headers_in.server = host; } - if (ngx_list_init(&r->headers_in.headers, r->pool, 20, - sizeof(ngx_table_elt_t)) - != NGX_OK) - { - ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR); - return NGX_ERROR; - } - return NGX_OK; failed: @@ -975,38 +814,11 @@ ngx_http_v3_process_request_header(ngx_http_request_t *r) return NGX_ERROR; } - if (r->headers_in.server.len == 0) { - ngx_log_error(NGX_LOG_INFO, c->log, 0, - "client sent neither \":authority\" nor \"Host\" header"); - goto failed; + if (ngx_http_process_request_header(r) != NGX_OK) { + return NGX_ERROR; } - if (r->headers_in.host) { - if (r->headers_in.host->value.len != r->headers_in.server.len - || ngx_memcmp(r->headers_in.host->value.data, - r->headers_in.server.data, - r->headers_in.server.len) - != 0) - { - ngx_log_error(NGX_LOG_INFO, c->log, 0, - "client sent \":authority\" and \"Host\" headers " - "with different values"); - goto failed; - } - } - - if (r->headers_in.content_length) { - r->headers_in.content_length_n = - ngx_atoof(r->headers_in.content_length->value.data, - r->headers_in.content_length->value.len); - - if (r->headers_in.content_length_n == NGX_ERROR) { - ngx_log_error(NGX_LOG_INFO, c->log, 0, - "client sent invalid \"Content-Length\" header"); - goto failed; - } - - } else { + if (!r->headers_in.content_length) { b = r->header_in; n = b->last - b->pos; @@ -1029,24 +841,7 @@ ngx_http_v3_process_request_header(ngx_http_request_t *r) } } - if (r->method == NGX_HTTP_CONNECT) { - ngx_log_error(NGX_LOG_INFO, c->log, 0, "client sent CONNECT method"); - ngx_http_finalize_request(r, NGX_HTTP_NOT_ALLOWED); - return NGX_ERROR; - } - - if (r->method == NGX_HTTP_TRACE) { - ngx_log_error(NGX_LOG_INFO, c->log, 0, "client sent TRACE method"); - ngx_http_finalize_request(r, NGX_HTTP_NOT_ALLOWED); - return NGX_ERROR; - } - return NGX_OK; - -failed: - - ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); - return NGX_ERROR; } From 1e3098851650d994d20f7c77171a307bf786010f Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 7 Apr 2025 23:07:55 -0400 Subject: [PATCH 05/21] HTTP: Reject HTTP/2 and HTTP/3 requests with Transfer-Encoding 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: #612 --- src/http/ngx_http_request.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c index ceac8d307..092d25509 100644 --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -2015,9 +2015,9 @@ ngx_http_process_request_header(ngx_http_request_t *r) } if (r->headers_in.transfer_encoding) { - if (r->http_version < NGX_HTTP_VERSION_11) { + if (r->http_version != NGX_HTTP_VERSION_11) { ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent HTTP/1.0 request with " + "client sent non-HTTP/1.1 request with " "\"Transfer-Encoding\" header"); ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); return NGX_ERROR; From b1c4b0757f1c8418b2ef39c9a283010a1a187e71 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 14 Mar 2025 01:00:59 -0400 Subject: [PATCH 06/21] HTTP: Reject invalid header names 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. --- src/http/ngx_http_parse.c | 68 +++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 38 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 5665acbc4..0b6a52bff 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -816,6 +816,29 @@ done: return NGX_OK; } +static ngx_int_t +ngx_http_non_alnum_dash_header_char(u_char ch) +{ + switch (ch) { + case '!': + case '#': + case '$': + case '%': + case '&': + case '\'': + case '*': + case '+': + case '.': + case '^': + case '_': + case '`': + case '|': + case '~': + return 1; + default: + return 0; + } +} ngx_int_t ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, @@ -829,7 +852,6 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, sw_space_before_value, sw_value, sw_space_after_value, - sw_ignore_line, sw_almost_done, sw_header_almost_done } state; @@ -880,22 +902,14 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, break; } - if (ch == '_') { - if (allow_underscores) { - hash = ngx_hash(0, ch); - r->lowcase_header[0] = ch; - i = 1; - - } else { - hash = 0; - i = 0; - r->invalid_header = 1; - } - + if (ch == '_' && allow_underscores) { + hash = ngx_hash(0, ch); + r->lowcase_header[0] = ch; + i = 1; break; } - if (ch <= 0x20 || ch == 0x7f || ch == ':') { + if (!ngx_http_non_alnum_dash_header_char(ch)) { r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; } @@ -954,17 +968,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, goto done; } - /* IIS may send the duplicate "HTTP/1.1 ..." lines */ - if (ch == '/' - && r->upstream - && p - r->header_name_start == 4 - && ngx_strncmp(r->header_name_start, "HTTP", 4) == 0) - { - state = sw_ignore_line; - break; - } - - if (ch <= 0x20 || ch == 0x7f) { + if (!ngx_http_non_alnum_dash_header_char(ch)) { r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; } @@ -1039,17 +1043,6 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, } break; - /* ignore header line */ - case sw_ignore_line: - switch (ch) { - case LF: - state = sw_start; - break; - default: - break; - } - break; - /* end of header line */ case sw_almost_done: switch (ch) { @@ -1139,8 +1132,7 @@ ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, continue; } - if (ch <= 0x20 || ch == 0x7f || ch == ':' - || (ch >= 'A' && ch <= 'Z')) + if (!ngx_http_non_alnum_dash_header_char(ch)) { ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, "client sent invalid header name: \"%V\"", From ae76c643008b7074b6dc9e63d6761bcb0a15a598 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 9 Apr 2025 01:30:30 -0400 Subject: [PATCH 07/21] HTTP: Reject invalid field values 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. --- src/http/ngx_http_parse.c | 47 ++++++++++++++++----------------------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 0b6a52bff..24fea0da4 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -10,11 +10,6 @@ #include -#if (NGX_HTTP_V2 || NGX_HTTP_V3) -static inline ngx_int_t ngx_isspace(u_char ch); -#endif - - static uint32_t usual[] = { 0x00000000, /* 0000 0000 0000 0000 0000 0000 0000 0000 */ @@ -992,13 +987,14 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, r->header_start = p; r->header_end = p; goto done; - case '\0': + default: + if (ch > 0x20 && ch != 0x7f) { + r->header_start = p; + state = sw_value; + break; + } r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; - default: - r->header_start = p; - state = sw_value; - break; } break; @@ -1017,7 +1013,9 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, case LF: r->header_end = p; goto done; - case '\0': + default: + if (ch > 0x20 && ch != 0x7f) + break; r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; } @@ -1034,12 +1032,13 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, break; case LF: goto done; - case '\0': + default: + if (ch > 0x20 && ch != 0x7f) { + state = sw_value; + break; + } r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; - default: - state = sw_value; - break; } break; @@ -1094,13 +1093,6 @@ header_done: #if (NGX_HTTP_V2 || NGX_HTTP_V3) -static inline ngx_int_t -ngx_isspace(u_char ch) -{ - return ch == ' ' || ch == '\t'; -} - - ngx_int_t ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, ngx_str_t *value) @@ -1152,7 +1144,7 @@ ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, for (i = 0; i != value->len; i++) { ch = value->data[i]; - if (ch == '\0' || ch == LF || ch == CR) { + if (ch < 0x20 ? ch != 0x09 : ch == 0x7f) { ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, "client sent header \"%V\" with " "invalid value: \"%V\"", @@ -1164,8 +1156,7 @@ ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, tmp = *value; - if (!ngx_isspace(tmp.data[0]) - && !ngx_isspace(tmp.data[tmp.len - 1])) { + if (tmp.data[0] > 0x20 && tmp.data[tmp.len - 1] > 0x20) { /* Fast path: nothing to strip. */ return NGX_OK; } @@ -1176,12 +1167,12 @@ ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, * past-the-end pointer (which cannot be safely passed * to memmove()) */ - while (tmp.len && ngx_isspace(tmp.data[tmp.len - 1])) { + while (tmp.len && tmp.data[tmp.len - 1] <= 0x20) { tmp.len--; } /* Strip leading whitespace */ - if (tmp.len && ngx_isspace(tmp.data[0])) { + if (tmp.len && tmp.data[0] <= 0x20) { /* * Last loop guaranteed that 'tmp' does not end with whitespace, so * it's safe to keep going until a non-whitespace character is found. @@ -1189,7 +1180,7 @@ ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, do { tmp.len--; tmp.data++; - } while (ngx_isspace(tmp.data[0])); + } while (tmp.data[0] <= 0x20); /* Move remaining string to start of buffer. */ memmove(value->data, tmp.data, tmp.len); From 279ae488a455c2122b9a92e93272b7660159e3ff Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 13 Mar 2025 01:36:49 -0400 Subject: [PATCH 08/21] HTTP: Reject hop-by-hop headers in HTTP/2 and HTTP/3 requests 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. --- src/http/ngx_http_parse.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 24fea0da4..5cbe1257b 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -1097,6 +1097,7 @@ ngx_int_t ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, ngx_str_t *value) { + int bad; u_char ch; ngx_str_t tmp; ngx_uint_t i; @@ -1154,6 +1155,37 @@ ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, } } + bad = 0; + switch (name->len) { +#define X(s) \ + case sizeof("" s) - 1: \ + bad = memcmp(name->data, s, sizeof(s) - 1) == 0; \ + break + X("upgrade"); + X("transfer-encoding"); + X("proxy-connection"); + X("proxy-authorization"); + X("proxy-authenticate"); +#undef X + case 10: + bad = memcmp(name->data, "connection", 10) == 0 + || memcmp(name->data, "keep-alive", 10) == 0; + break; + case 2: + /* te: trailiers is allowed, all other te values forbidden */ + bad = name->data[0] == 't' && name->data[1] == 'e' + && !(value->len == 8 && memcmp(value->data, "trailers", 8) == 0); + break; + } + + if (bad) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent forbidden hop-by-hop header \"%V\" with " + "value: \"%V\"", name, value); + + return NGX_ERROR; + } + tmp = *value; if (tmp.data[0] > 0x20 && tmp.data[tmp.len - 1] > 0x20) { From 98d266924f151cfa50c387ebdf96d9521929186a Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 26 Mar 2025 22:33:54 -0400 Subject: [PATCH 09/21] Proxy: Reject Transfer-Encoding or Content-Length trailers 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. --- src/http/modules/ngx_http_proxy_module.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c index d4c5abf62..29ce6cf36 100644 --- a/src/http/modules/ngx_http_proxy_module.c +++ b/src/http/modules/ngx_http_proxy_module.c @@ -2635,7 +2635,24 @@ ngx_http_proxy_process_trailer(ngx_http_request_t *r, ngx_buf_t *buf) if (rc == NGX_OK) { - /* a header line has been parsed successfully */ + /* A trailer line has been parsed successfully. + * Do not allow trailers that would, if turned into + * headers, interfere with request framing. */ + switch (r->header_name_end - r->header_name_start) { +#define X(x) \ + case sizeof(x "") - 1: \ + /* The size is always less than the number of bytes in \ + * the pre-casefolded area. */ \ + if (memcmp(r->lowcase_header, x, sizeof(x) - 1) == 0) { \ + return NGX_ERROR; \ + } else break + X("transfer-encoding"); + X("content-length"); + X("upgrade"); +#undef X + default: + break; + } h = ngx_list_push(&r->upstream->headers_in.trailers); if (h == NULL) { From dab8c0e762c1315409b5bd0dc2e75fbc70b3bf1e Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 25 Apr 2025 02:17:29 -0400 Subject: [PATCH 10/21] HTTP: Do not allow header lines with no colon RFC9112 does not permit them. --- src/http/ngx_http_parse.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 5cbe1257b..e2c908d32 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -956,13 +956,6 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, break; } - if (ch == LF) { - r->header_name_end = p; - r->header_start = p; - r->header_end = p; - goto done; - } - if (!ngx_http_non_alnum_dash_header_char(ch)) { r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; From e30ddb7a3b437b929d74c7685a495e5a78fa3020 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 14 Mar 2025 02:10:46 -0400 Subject: [PATCH 11/21] HTTP: Do not allow multiple CRs before LF This is not permitted by RFC9112. --- src/http/ngx_http_parse.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index e2c908d32..f7ee2fb14 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -1040,8 +1040,6 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, switch (ch) { case LF: goto done; - case CR: - break; default: return NGX_HTTP_PARSE_INVALID_HEADER; } From 0d2c8754ee24d37a4180ca13bb4480905e3cc036 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 16 Mar 2025 17:03:04 -0400 Subject: [PATCH 12/21] HTTP: Do not allow request lines to end with bare CR This is consistent with Node.js. --- src/http/ngx_http_parse.c | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index f7ee2fb14..9590d9d44 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -492,10 +492,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) r->http_minor = 9; state = sw_almost_done; break; - case LF: - r->uri_end = p; - r->http_minor = 9; - goto done; case '.': r->complex_uri = 1; state = sw_uri; @@ -565,10 +561,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) r->http_minor = 9; state = sw_almost_done; break; - case LF: - r->uri_end = p; - r->http_minor = 9; - goto done; #if (NGX_WIN32) case '\\': r->complex_uri = 1; @@ -615,10 +607,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) r->http_minor = 9; state = sw_almost_done; break; - case LF: - r->uri_end = p; - r->http_minor = 9; - goto done; case '#': r->complex_uri = 1; break; @@ -639,9 +627,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) r->http_minor = 9; state = sw_almost_done; break; - case LF: - r->http_minor = 9; - goto done; case 'H': r->http_protocol.data = p; state = sw_http_H; @@ -769,8 +754,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) case CR: state = sw_almost_done; break; - case LF: - goto done; default: return NGX_HTTP_PARSE_INVALID_REQUEST; } From 1be6976866f98f205bdab4f7f454b7c2a1762d6f Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 23 Mar 2025 16:18:19 -0400 Subject: [PATCH 13/21] Reject malformed chunk extensions 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. --- src/http/ngx_http_parse.c | 243 ++++++++++++++++++++++++-------------- 1 file changed, 154 insertions(+), 89 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 9590d9d44..7067b8f1d 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -97,6 +97,11 @@ static uint32_t usual[] = { #endif +static inline ngx_int_t +ngx_http_field_value_char(u_char ch) +{ + return ch >= 0x20 ? ch != 0x7f : ch == 0x09; +} /* gcc, icc, msvc and others compile these switches as an jump table */ @@ -818,6 +823,17 @@ ngx_http_non_alnum_dash_header_char(u_char ch) } } +static ngx_int_t +ngx_http_token_char(u_char ch) +{ + u_char c = (ch | 0x20); + if (('a' <= c && c <= 'z') || ('0' <= c && c <= '9') || c == '-') { + return 1; + } + + return ngx_http_non_alnum_dash_header_char(ch); +} + ngx_int_t ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, ngx_uint_t allow_underscores) @@ -1117,9 +1133,7 @@ ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, } for (i = 0; i != value->len; i++) { - ch = value->data[i]; - - if (ch < 0x20 ? ch != 0x09 : ch == 0x7f) { + if (!ngx_http_field_value_char(value->data[i])) { ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, "client sent header \"%V\" with " "invalid value: \"%V\"", @@ -1916,6 +1930,11 @@ ngx_http_parse_status_line(ngx_http_request_t *r, ngx_buf_t *b, break; case LF: goto done; + default: + if (ch < 0x20 || ch == 0x7f) { + return NGX_ERROR; + } + break; } break; @@ -2263,13 +2282,18 @@ ngx_http_parse_chunked(ngx_http_request_t *r, ngx_buf_t *b, enum { sw_chunk_start = 0, sw_chunk_size, + sw_chunk_extension_before_semi, sw_chunk_extension, + sw_chunk_extension_bws_before_equal, + sw_chunk_extension_name, + sw_chunk_extension_value_start, + sw_chunk_extension_quoted_value, + sw_chunk_extension_value_quoted_backslash, + sw_chunk_extension_unquoted_value, sw_chunk_extension_almost_done, sw_chunk_data, sw_after_data, sw_after_data_almost_done, - sw_last_chunk_extension, - sw_last_chunk_extension_almost_done, sw_trailer, sw_trailer_almost_done, sw_trailer_header, @@ -2326,62 +2350,120 @@ ngx_http_parse_chunked(ngx_http_request_t *r, ngx_buf_t *b, ctx->size = ctx->size * 16 + (c - 'a' + 10); break; } + /* fall through */ - if (ctx->size == 0) { - - switch (ch) { - case CR: - state = sw_last_chunk_extension_almost_done; - break; - case LF: - if (keep_trailers) { - goto done; - } - state = sw_trailer; - break; - case ';': - case ' ': - case '\t': - state = sw_last_chunk_extension; - break; - default: - goto invalid; - } - - break; - } - + case sw_chunk_extension_before_semi: +before_semi: switch (ch) { case CR: state = sw_chunk_extension_almost_done; break; - case LF: - state = sw_chunk_data; - break; case ';': + state = sw_chunk_extension; + break; case ' ': case '\t': - state = sw_chunk_extension; + /* + * This switch is also used by other states, so set + * the state explicitly here. + */ + state = sw_chunk_extension_before_semi; + break; /* BWS */ + default: + goto invalid; + } + break; + + case sw_chunk_extension: + if (ngx_http_token_char(ch)) { + state = sw_chunk_extension_name; + break; + } + switch (ch) { + case ' ': + case '\t': + break; /* BWS */ + default: + goto invalid; + } + break; + + case sw_chunk_extension_name: + if (ngx_http_token_char(ch)) { + break; + } + + state = sw_chunk_extension_bws_before_equal; + + /* fall through */ + + case sw_chunk_extension_bws_before_equal: + switch (ch) { + case ' ': + case '\t': + break; /* BWS */ + case '=': + state = sw_chunk_extension_value_start; break; default: goto invalid; } - break; - case sw_chunk_extension: - switch (ch) { - case CR: - state = sw_chunk_extension_almost_done; + case sw_chunk_extension_value_start: + if (ngx_http_token_char(ch)) { + state = sw_chunk_extension_unquoted_value; break; - case LF: - state = sw_chunk_data; + } + switch (ch) { + case ' ': + case '\t': + break; /* BWS */ + case '"': + state = sw_chunk_extension_quoted_value; + break; + default: + goto invalid; } break; + case sw_chunk_extension_quoted_value: + if (ch == '"') { + state = sw_chunk_extension_before_semi; + break; + } + if (ch == '\\') { + state = sw_chunk_extension_value_quoted_backslash; + break; + } + if (ngx_http_field_value_char(ch)) { + break; + } + goto invalid; + + case sw_chunk_extension_value_quoted_backslash: + if (ngx_http_field_value_char(ch)) { + state = sw_chunk_extension_quoted_value; + break; + } + goto invalid; + + case sw_chunk_extension_unquoted_value: + if (ngx_http_token_char(ch)) { + break; + } + goto before_semi; + case sw_chunk_extension_almost_done: if (ch == LF) { - state = sw_chunk_data; + if (ctx->size) { + state = sw_chunk_data; + break; + } + if (keep_trailers) { + goto done; + } + state = sw_trailer; break; } goto invalid; @@ -2391,44 +2473,15 @@ ngx_http_parse_chunked(ngx_http_request_t *r, ngx_buf_t *b, goto data; case sw_after_data: - switch (ch) { - case CR: + if (ch == CR) { state = sw_after_data_almost_done; break; - case LF: - state = sw_chunk_start; - break; - default: - goto invalid; - } - break; - - case sw_after_data_almost_done: - if (ch == LF) { - state = sw_chunk_start; - break; } goto invalid; - case sw_last_chunk_extension: - switch (ch) { - case CR: - state = sw_last_chunk_extension_almost_done; - break; - case LF: - if (keep_trailers) { - goto done; - } - state = sw_trailer; - } - break; - - case sw_last_chunk_extension_almost_done: + case sw_after_data_almost_done: if (ch == LF) { - if (keep_trailers) { - goto done; - } - state = sw_trailer; + state = sw_chunk_start; break; } goto invalid; @@ -2476,34 +2529,47 @@ data: ctx->state = state; b->pos = pos; - if (ctx->size > NGX_MAX_OFF_T_VALUE - 5) { + if (ctx->size > NGX_MAX_OFF_T_VALUE - 11) { goto invalid; } - + off_t min_length = (ctx->size ? ctx->size + 6 /* CR LF "0" CR LF LF */ + : 1 /* LF */); switch (state) { - case sw_chunk_start: - ctx->length = 3 /* "0" LF LF */; + ctx->length = 4 /* "0" CR LF LF */; break; case sw_chunk_size: - ctx->length = 1 /* LF */ - + (ctx->size ? ctx->size + 4 /* LF "0" LF LF */ - : 1 /* LF */); + case sw_chunk_extension_before_semi: + case sw_chunk_extension_unquoted_value: + ctx->length = 2 /* CR LF */ + min_length; + break; + case sw_chunk_extension_almost_done: + ctx->length = 1 /* LF */ + min_length; break; case sw_chunk_extension: - case sw_chunk_extension_almost_done: - ctx->length = 1 /* LF */ + ctx->size + 4 /* LF "0" LF LF */; + ctx->length = 5 /* a=b CR LF */ + min_length; + break; + case sw_chunk_extension_bws_before_equal: + case sw_chunk_extension_name: + ctx->length = 4 /* =b CR LF */ + min_length; + break; + case sw_chunk_extension_value_start: + ctx->length = 3 /* b CR LF */ + min_length; + break; + case sw_chunk_extension_quoted_value: + ctx->length = 3 /* " CR LF */ + min_length; + break; + case sw_chunk_extension_value_quoted_backslash: + ctx->length = 4 /* a" CR LF */ + min_length; break; case sw_chunk_data: - ctx->length = ctx->size + 4 /* LF "0" LF LF */; + ctx->length = min_length; break; case sw_after_data: - case sw_after_data_almost_done: - ctx->length = 4 /* LF "0" LF LF */; + ctx->length = 6 /* CR LF "0" CR LF LF */; break; - case sw_last_chunk_extension: - case sw_last_chunk_extension_almost_done: - ctx->length = 2 /* LF LF */; + case sw_after_data_almost_done: + ctx->length = 5 /* LF "0" CR LF LF */; break; case sw_trailer: case sw_trailer_almost_done: @@ -2513,7 +2579,6 @@ data: case sw_trailer_header_almost_done: ctx->length = 2 /* LF LF */; break; - } return rc; From 1c0426c8c0fd9b521bfc90ba45e5b3767cb1deb0 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 26 Mar 2025 23:13:32 -0400 Subject: [PATCH 14/21] Forbid malformed HTTP/1.1 trailers HTTP/1.1 trailers must follow the same syntax as HTTP headers. --- src/http/ngx_http_parse.c | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 7067b8f1d..58995b81b 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -2296,7 +2296,8 @@ ngx_http_parse_chunked(ngx_http_request_t *r, ngx_buf_t *b, sw_after_data_almost_done, sw_trailer, sw_trailer_almost_done, - sw_trailer_header, + sw_trailer_name, + sw_trailer_value, sw_trailer_header_almost_done } state; @@ -2494,7 +2495,11 @@ before_semi: case LF: goto done; default: - state = sw_trailer_header; + if (ngx_http_token_char(ch)) { + state = sw_trailer_name; + break; + } + goto invalid; } break; @@ -2504,15 +2509,26 @@ before_semi: } goto invalid; - case sw_trailer_header: - switch (ch) { - case CR: + case sw_trailer_name: + if (ngx_http_token_char(ch)) { + break; + } + if (ch == ':') { + state = sw_trailer_value; + break; + } + goto invalid; + + case sw_trailer_value: + if (ngx_http_field_value_char(ch)) { + break; + } + if (ch == CR) { state = sw_trailer_header_almost_done; break; - case LF: - state = sw_trailer; } - break; + + /* fall through */ case sw_trailer_header_almost_done: if (ch == LF) { @@ -2553,6 +2569,12 @@ data: case sw_chunk_extension_name: ctx->length = 4 /* =b CR LF */ + min_length; break; + case sw_trailer_name: + ctx->length = 3 /* : LF LF */; + break; + case sw_trailer_value: + ctx->length = 2 /* LF LF */; + break; case sw_chunk_extension_value_start: ctx->length = 3 /* b CR LF */ + min_length; break; @@ -2575,7 +2597,6 @@ data: case sw_trailer_almost_done: ctx->length = 1 /* LF */; break; - case sw_trailer_header: case sw_trailer_header_almost_done: ctx->length = 2 /* LF LF */; break; From 626b1d9bbaf5803a841c2a10153362f5c9ad53bd Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 26 Mar 2025 23:34:29 -0400 Subject: [PATCH 15/21] HTTP: Reject trailers involved in framing These are forbidden by the relevant standards. --- src/http/ngx_http_parse.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 58995b81b..241d83e94 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -2497,6 +2497,8 @@ before_semi: default: if (ngx_http_token_char(ch)) { state = sw_trailer_name; + r->lowcase_index = 1; + r->lowcase_header[0] = (ch | 0x20); break; } goto invalid; @@ -2511,9 +2513,28 @@ before_semi: case sw_trailer_name: if (ngx_http_token_char(ch)) { + if (r->lowcase_index < NGX_HTTP_LC_HEADER_LEN) { + /* ASCII uppercase letters become the lowercase ones. + * '-' is unchanged. */ + r->lowcase_header[r->lowcase_index++] = (ch | 0x20); + } break; } if (ch == ':') { + switch (r->lowcase_index) { +#define X(v) \ + case sizeof(v "") - 1: \ + if (memcmp(r->lowcase_header, v, r->lowcase_index) != 0) { \ + goto invalid; \ + } \ + break + X("transfer-encoding"); + X("content-length"); + X("upgrade"); +#undef X + default: + break; + } state = sw_trailer_value; break; } From 436282ff7449eeb197e4cb95ff980cf6e4df3994 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 7 Apr 2025 01:25:03 -0400 Subject: [PATCH 16/21] HTTP: Remove redundant state from chunk parsing The state machine never returns with state = sw_chunk_data and a size of zero, nor did it return with state = sw_after_data. --- src/http/ngx_http_parse.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 241d83e94..f6d71e2f7 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -2292,7 +2292,6 @@ ngx_http_parse_chunked(ngx_http_request_t *r, ngx_buf_t *b, sw_chunk_extension_unquoted_value, sw_chunk_extension_almost_done, sw_chunk_data, - sw_after_data, sw_after_data_almost_done, sw_trailer, sw_trailer_almost_done, @@ -2303,10 +2302,6 @@ ngx_http_parse_chunked(ngx_http_request_t *r, ngx_buf_t *b, state = ctx->state; - if (state == sw_chunk_data && ctx->size == 0) { - state = sw_after_data; - } - rc = NGX_AGAIN; for (pos = b->pos; pos < b->last; pos++) { @@ -2470,10 +2465,10 @@ before_semi: goto invalid; case sw_chunk_data: - rc = NGX_OK; - goto data; - - case sw_after_data: + if (ctx->size != 0) { + rc = NGX_OK; + goto data; + } if (ch == CR) { state = sw_after_data_almost_done; break; @@ -2608,9 +2603,6 @@ data: case sw_chunk_data: ctx->length = min_length; break; - case sw_after_data: - ctx->length = 6 /* CR LF "0" CR LF LF */; - break; case sw_after_data_almost_done: ctx->length = 5 /* LF "0" CR LF LF */; break; From bb0a2b9de8781fe42dcab755fef551ce6829a306 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 25 Mar 2025 14:58:32 -0400 Subject: [PATCH 17/21] HTTP: Allow rejecting leading and trailing whitespace in HTTP2+ fields 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. --- src/http/ngx_http.h | 1 - src/http/ngx_http_core_module.c | 11 +++++++++++ src/http/ngx_http_core_module.h | 1 + src/http/ngx_http_parse.c | 11 +++++++++-- 4 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/http/ngx_http.h b/src/http/ngx_http.h index 1bfb7aa7f..883d2ec72 100644 --- a/src/http/ngx_http.h +++ b/src/http/ngx_http.h @@ -97,7 +97,6 @@ int ngx_http_ssl_servername(ngx_ssl_conn_t *ssl_conn, int *ad, void *arg); int ngx_http_ssl_certificate(ngx_ssl_conn_t *ssl_conn, void *arg); #endif - ngx_int_t ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b); ngx_int_t ngx_http_parse_uri(ngx_http_request_t *r); ngx_int_t ngx_http_parse_complex_uri(ngx_http_request_t *r, diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c index 92c3eae8a..495d476ac 100644 --- a/src/http/ngx_http_core_module.c +++ b/src/http/ngx_http_core_module.c @@ -259,6 +259,13 @@ static ngx_command_t ngx_http_core_commands[] = { offsetof(ngx_http_core_srv_conf_t, ignore_invalid_headers), NULL }, + { ngx_string("reject_leading_trailing_whitespace"), + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_FLAG, + ngx_conf_set_flag_slot, + NGX_HTTP_SRV_CONF_OFFSET, + offsetof(ngx_http_core_srv_conf_t, reject_leading_trailing_whitespace), + NULL }, + { ngx_string("merge_slashes"), NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_FLAG, ngx_conf_set_flag_slot, @@ -3476,6 +3483,7 @@ ngx_http_core_create_srv_conf(ngx_conf_t *cf) cscf->ignore_invalid_headers = NGX_CONF_UNSET; cscf->merge_slashes = NGX_CONF_UNSET; cscf->underscores_in_headers = NGX_CONF_UNSET; + cscf->reject_leading_trailing_whitespace = NGX_CONF_UNSET; cscf->file_name = cf->conf_file->file.name.data; cscf->line = cf->conf_file->line; @@ -3522,6 +3530,9 @@ ngx_http_core_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child) ngx_conf_merge_value(conf->underscores_in_headers, prev->underscores_in_headers, 0); + ngx_conf_merge_value(conf->reject_leading_trailing_whitespace, + prev->reject_leading_trailing_whitespace, 0); + if (conf->server_names.nelts == 0) { /* the array has 4 empty preallocated elements, so push cannot fail */ sn = ngx_array_push(&conf->server_names); diff --git a/src/http/ngx_http_core_module.h b/src/http/ngx_http_core_module.h index e7e266bf8..8e7e9eb8b 100644 --- a/src/http/ngx_http_core_module.h +++ b/src/http/ngx_http_core_module.h @@ -208,6 +208,7 @@ typedef struct { #endif ngx_http_core_loc_conf_t **named_locations; + ngx_flag_t reject_leading_trailing_whitespace; } ngx_http_core_srv_conf_t; diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index f6d71e2f7..6d2991c38 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -1136,8 +1136,7 @@ ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, if (!ngx_http_field_value_char(value->data[i])) { ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, "client sent header \"%V\" with " - "invalid value: \"%V\"", - name, value); + "invalid value", name); return NGX_ERROR; } @@ -1181,6 +1180,14 @@ ngx_http_v23_fixup_header(ngx_http_request_t *r, ngx_str_t *name, return NGX_OK; } + if (cscf->reject_leading_trailing_whitespace) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent header \"%V\" with " + "leading or trailing space", + name); + return NGX_ERROR; + } + /* * Strip trailing whitespace. Do this first so that * if the string is all whitespace, tmp.data is not a From bd37faff7250ab8f61c81500535e2c94e9bd3800 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 16 Mar 2025 17:06:02 -0400 Subject: [PATCH 18/21] HTTP: do not allow headers to end with a bare LF This is consistent with Node.js. --- src/http/ngx_http_parse.c | 49 ++++++++++----------------------------- 1 file changed, 12 insertions(+), 37 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 6d2991c38..4fefeba71 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -881,9 +881,6 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, r->header_end = p; state = sw_header_almost_done; break; - case LF: - r->header_end = p; - goto header_done; default: state = sw_name; @@ -975,10 +972,6 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, r->header_end = p; state = sw_almost_done; break; - case LF: - r->header_start = p; - r->header_end = p; - goto done; default: if (ch > 0x20 && ch != 0x7f) { r->header_start = p; @@ -1002,9 +995,6 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, r->header_end = p; state = sw_almost_done; break; - case LF: - r->header_end = p; - goto done; default: if (ch > 0x20 && ch != 0x7f) break; @@ -1022,8 +1012,6 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, case CR: state = sw_almost_done; break; - case LF: - goto done; default: if (ch > 0x20 && ch != 0x7f) { state = sw_value; @@ -1036,22 +1024,25 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, /* end of header line */ case sw_almost_done: - switch (ch) { - case LF: - goto done; - default: + if (ch != LF) { return NGX_HTTP_PARSE_INVALID_HEADER; } - break; + + b->pos = p + 1; + r->state = sw_start; + r->header_hash = hash; + r->lowcase_index = i; + return NGX_OK; /* end of header */ case sw_header_almost_done: - switch (ch) { - case LF: - goto header_done; - default: + if (ch != LF) { return NGX_HTTP_PARSE_INVALID_HEADER; } + + b->pos = p + 1; + r->state = sw_start; + return NGX_HTTP_PARSE_HEADER_DONE; } } @@ -1061,22 +1052,6 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, r->lowcase_index = i; return NGX_AGAIN; - -done: - - b->pos = p + 1; - r->state = sw_start; - r->header_hash = hash; - r->lowcase_index = i; - - return NGX_OK; - -header_done: - - b->pos = p + 1; - r->state = sw_start; - - return NGX_HTTP_PARSE_HEADER_DONE; } From 7032a60fa0b45d515e32ab2520eced021fbd71dd Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 16 Mar 2025 17:07:40 -0400 Subject: [PATCH 19/21] HTTP: do not allow status line to end with bare LF This is consistent with llhttp. --- src/http/ngx_http_parse.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 4fefeba71..b08f65245 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -732,10 +732,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) break; } - if (ch == LF) { - goto done; - } - if (ch == ' ') { state = sw_spaces_after_digit; break; @@ -1896,8 +1892,6 @@ ngx_http_parse_status_line(ngx_http_request_t *r, ngx_buf_t *b, case CR: state = sw_almost_done; break; - case LF: - goto done; default: return NGX_ERROR; } @@ -1910,7 +1904,6 @@ ngx_http_parse_status_line(ngx_http_request_t *r, ngx_buf_t *b, state = sw_almost_done; break; - case LF: goto done; default: if (ch < 0x20 || ch == 0x7f) { From 9950dfbc947c57759b09d1b386e60f428d4aed66 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 10 Jun 2025 15:16:58 -0400 Subject: [PATCH 20/21] Do not allow trailers to end with bare LF --- src/http/ngx_http_parse.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index b08f65245..2791375b5 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -2458,22 +2458,17 @@ before_semi: goto invalid; case sw_trailer: - switch (ch) { - case CR: + if (ch == CR) { state = sw_trailer_almost_done; break; - case LF: - goto done; - default: - if (ngx_http_token_char(ch)) { - state = sw_trailer_name; - r->lowcase_index = 1; - r->lowcase_header[0] = (ch | 0x20); - break; - } - goto invalid; } - break; + if (ngx_http_token_char(ch)) { + state = sw_trailer_name; + r->lowcase_index = 1; + r->lowcase_header[0] = (ch | 0x20); + break; + } + goto invalid; case sw_trailer_almost_done: if (ch == LF) { @@ -2518,8 +2513,7 @@ before_semi: state = sw_trailer_header_almost_done; break; } - - /* fall through */ + goto invalid; case sw_trailer_header_almost_done: if (ch == LF) { From 9b8e4a19370ee4894ead6ef60941ef7b9d044d04 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 10 Jun 2025 15:19:20 -0400 Subject: [PATCH 21/21] Do not support bad whitespace in chunk extensions --- src/http/ngx_http_parse.c | 45 +++++---------------------------------- 1 file changed, 5 insertions(+), 40 deletions(-) diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 2791375b5..f7214ca55 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -2259,7 +2259,6 @@ ngx_http_parse_chunked(ngx_http_request_t *r, ngx_buf_t *b, sw_chunk_size, sw_chunk_extension_before_semi, sw_chunk_extension, - sw_chunk_extension_bws_before_equal, sw_chunk_extension_name, sw_chunk_extension_value_start, sw_chunk_extension_quoted_value, @@ -2332,14 +2331,6 @@ before_semi: case ';': state = sw_chunk_extension; break; - case ' ': - case '\t': - /* - * This switch is also used by other states, so set - * the state explicitly here. - */ - state = sw_chunk_extension_before_semi; - break; /* BWS */ default: goto invalid; } @@ -2350,53 +2341,28 @@ before_semi: state = sw_chunk_extension_name; break; } - switch (ch) { - case ' ': - case '\t': - break; /* BWS */ - default: - goto invalid; - } - break; + goto invalid; case sw_chunk_extension_name: if (ngx_http_token_char(ch)) { break; } - - state = sw_chunk_extension_bws_before_equal; - - /* fall through */ - - case sw_chunk_extension_bws_before_equal: - switch (ch) { - case ' ': - case '\t': - break; /* BWS */ - case '=': + if (ch == '=') { state = sw_chunk_extension_value_start; break; - default: - goto invalid; } - break; + goto invalid; case sw_chunk_extension_value_start: if (ngx_http_token_char(ch)) { state = sw_chunk_extension_unquoted_value; break; } - switch (ch) { - case ' ': - case '\t': - break; /* BWS */ - case '"': + if (ch == '"') { state = sw_chunk_extension_quoted_value; break; - default: - goto invalid; } - break; + goto invalid; case sw_chunk_extension_quoted_value: if (ch == '"') { @@ -2550,7 +2516,6 @@ data: case sw_chunk_extension: ctx->length = 5 /* a=b CR LF */ + min_length; break; - case sw_chunk_extension_bws_before_equal: case sw_chunk_extension_name: ctx->length = 4 /* =b CR LF */ + min_length; break;