From c68c7adcdbe01440c6e82e8051f776838cc0f641 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 25 Mar 2025 14:58:32 -0400 Subject: [PATCH] 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. Stripping leading and/or trailing whitespace would require either allocating a new buffer or changing the pointers in the existing buffer, and I am not familiar enough with NGINX to know if subsequent code expects a buffer that was allocated in a particualar way. If header values were ever passed to ngx_pfree(), munging them to skip leading whitespace would mean that a request with leading whitespace would cause ngx_pfree() to be called with an invalid pointer, which would be a security vulnerability. Rejecting the request doesn't introduce any new error paths that clients cannot already trigger, and it doesn't risk violating any invariants that existing code might assume. Also, Varnish Cache rejects HTTP/2 requests with leading and/or trailing whitespace in field values, so there is precedent for doing so. --- src/http/ngx_http.h | 3 ++- src/http/ngx_http_core_module.c | 11 +++++++++++ src/http/ngx_http_core_module.h | 1 + src/http/ngx_http_parse.c | 24 +++++++++++++++++++----- 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/http/ngx_http.h b/src/http/ngx_http.h index cb4a1e68a..2d891be77 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, @@ -178,6 +177,8 @@ ngx_uint_t ngx_http_degraded(ngx_http_request_t *); #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); 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, 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 0e253d99f..2b083660c 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -1136,16 +1136,30 @@ ngx_http_v23_validate_header(ngx_http_request_t *r, ngx_str_t *name, r->invalid_header = 1; } - for (i = 0; i != value->len; i++) { - ch = value->data[i]; - - if (ch == '\0' || ch == LF || ch == CR) { + if (value->len > 0) { + if (cscf->reject_leading_trailing_whitespace + && (value->data[0] == ' ' || value->data[0] == '\t' + || value->data[value->len - 1] == ' ' + || value->data[value->len - 1] == '\t')) { ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, "client sent header \"%V\" with " - "invalid value", name, value); + " leading or trailing space", + name); return NGX_ERROR; } + + 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", name, value); + + return NGX_ERROR; + } + } } return NGX_OK;