From df2f71d720e3158077ea9774793608fd6dbe79be Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 7 Apr 2025 17:30:13 -0400 Subject: [PATCH] HTTP: Strip or reject leading or trailing HWS in field values All versions of HTTP forbid field (header and trailer) values from having leading or trailing horizontal whitespace (HWS, 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 default is to always strip leading and trailing whitespace, regardless of the HTTP version. Standards-conforming behavior can be obtained with the new reject_leading_trailing_whitespace directive. If one only wants to effect how client requests are processed, one can use reject_leading_trailing_whitespace_client. If one only wants to effect processing of upstream responses, one can use reject_leading_trailing_whitespace_upstream. Fixes: #187 Fixes: #598 --- src/http/ngx_http_core_module.c | 42 ++++++++++++++++++++++ src/http/ngx_http_core_module.h | 3 ++ src/http/ngx_http_parse.c | 3 ++ src/http/ngx_http_v23_common.c | 62 +++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+) diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c index 4e1c67282..30db88e57 100644 --- a/src/http/ngx_http_core_module.c +++ b/src/http/ngx_http_core_module.c @@ -266,6 +266,29 @@ 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("reject_leading_trailing_whitespace_client"), + 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_client), + NULL }, + + { ngx_string("reject_leading_trailing_whitespace_upstream"), + 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_upstream), + NULL }, + { ngx_string("merge_slashes"), NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_FLAG, ngx_conf_set_flag_slot, @@ -3547,6 +3570,9 @@ 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->reject_leading_trailing_whitespace_client = NGX_CONF_UNSET; + cscf->reject_leading_trailing_whitespace_upstream = NGX_CONF_UNSET; cscf->file_name = cf->conf_file->file.name.data; cscf->line = cf->conf_file->line; @@ -3595,6 +3621,22 @@ 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); + if (conf->reject_leading_trailing_whitespace_client == NGX_CONF_UNSET) { + conf->reject_leading_trailing_whitespace_client = ( + conf->reject_leading_trailing_whitespace != NGX_CONF_UNSET ? + conf->reject_leading_trailing_whitespace : + (prev->reject_leading_trailing_whitespace_client != NGX_CONF_UNSET ? + prev->reject_leading_trailing_whitespace_client : 0)); + } + + if (conf->reject_leading_trailing_whitespace_upstream == NGX_CONF_UNSET) { + conf->reject_leading_trailing_whitespace_upstream = ( + conf->reject_leading_trailing_whitespace != NGX_CONF_UNSET ? + conf->reject_leading_trailing_whitespace : + (prev->reject_leading_trailing_whitespace_upstream != NGX_CONF_UNSET ? + prev->reject_leading_trailing_whitespace_upstream : 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 a13d7ade5..905a80150 100644 --- a/src/http/ngx_http_core_module.h +++ b/src/http/ngx_http_core_module.h @@ -212,6 +212,9 @@ typedef struct { unsigned allow_connect:1; ngx_http_core_loc_conf_t **named_locations; + ngx_flag_t reject_leading_trailing_whitespace; + ngx_flag_t reject_leading_trailing_whitespace_client; + ngx_flag_t reject_leading_trailing_whitespace_upstream; } ngx_http_core_srv_conf_t; diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index e1328da04..87798ebbd 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -1027,6 +1027,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; @@ -1051,6 +1052,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; @@ -1071,6 +1073,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; diff --git a/src/http/ngx_http_v23_common.c b/src/http/ngx_http_v23_common.c index fe009919a..81d167949 100644 --- a/src/http/ngx_http_v23_common.c +++ b/src/http/ngx_http_v23_common.c @@ -17,6 +17,7 @@ ngx_http_v23_validate_header(ngx_http_request_t *r, ngx_str_t *name, ngx_str_t *value, ngx_int_t is_client) { u_char ch; + ngx_str_t tmp; ngx_uint_t i; ngx_http_core_srv_conf_t *cscf; @@ -61,6 +62,11 @@ ngx_http_v23_validate_header(ngx_http_request_t *r, } } + /* 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]; @@ -75,6 +81,62 @@ ngx_http_v23_validate_header(ngx_http_request_t *r, } } + if (!ngx_isspace(value->data[0]) + && !ngx_isspace(value->data[value->len - 1])) { + /* Fast path: nothing to strip. */ + return NGX_OK; + } + + if (is_client ? cscf->reject_leading_trailing_whitespace_client + : cscf->reject_leading_trailing_whitespace_upstream) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "%s sent header \"%V\" with " + "leading or trailing space", + is_client ? "client" : "upstream", name); + + return NGX_ERROR; + } + + tmp = *value; + + /* + * 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(). After the loop, the string is either + * empty or ends with a non-whitespace character. + */ + 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, and + * this check guarantees it is not empty and starts with whitespace. + * Therefore, 'tmp' must end with a non-whitespace character, and must + * be of length at least 2. This means that it is 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; }