From e634abad1a51637e126661bf0882871e8d432f42 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 13 Mar 2025 01:32:35 -0400 Subject: [PATCH] Block CRLF injection attacks if NGINX is misconfigured This protects servers running with the following vulnerable configurations: location /b { # $uri can contain a newline proxy_pass http://127.0.0.1:8080/$uri; } location /b { # $uri can contain a newline add_header X-Original-Path $uri; proxy_pass http://127.0.0.1:8080; } location /b { # $uri can contain a newline add_trailer X-Original-Path $uri; proxy_pass http://127.0.0.1:8080; } location /b { # $uri can contain a newline proxy_set_header X-Normalized-URI $uri; proxy_pass http://127.0.0.1:8080; } Previously, these would be vulnerable to a CRLF injection attack, allowing request smuggling. With this patch, the attack is foiled, and NGINX will return a 500 Internal Server Error response to the client. These configurations are still incorrect, which is why NGINX returns a 500 response instead of a 4xx response. Also, there will still be control characters injected into the error log, which is a bug. This change also causes NGINX to fail to start if a add_header, add_trailer, or proxy_set_header directive specifies an invalid field name, rather than sending an invalid field name at runtime. Multi-line header values in configuration files are explicitly detected and allowed, provided that the multi-line value uses the HTTP/1.x obs-fold syntax (with leading whitespace in the second and subsequent lines) and that the newlines are in the literal header value (as opposed to a variable). The obs-fold syntax is replaced by a single space during configuration parsing, so clients and servers receive well-formed data. Signed-off-by: Demi Marie Obenour --- src/core/ngx_inet.c | 9 ++- .../modules/ngx_http_headers_filter_module.c | 21 ++++++ src/http/modules/ngx_http_proxy_module.c | 58 ++++++++++++++- src/http/modules/ngx_http_proxy_v2_module.c | 31 +++++++- src/http/ngx_http.c | 74 +++++++++++++++++++ src/http/ngx_http.h | 9 +++ src/http/ngx_http_header_filter_module.c | 55 ++++++++++++++ src/http/ngx_http_parse.c | 56 ++++++++++++++ 8 files changed, 309 insertions(+), 4 deletions(-) diff --git a/src/core/ngx_inet.c b/src/core/ngx_inet.c index 2233e617b..7d01560d5 100644 --- a/src/core/ngx_inet.c +++ b/src/core/ngx_inet.c @@ -688,11 +688,18 @@ ngx_int_t ngx_parse_url(ngx_pool_t *pool, ngx_url_t *u) { u_char *p; - size_t len; + size_t len, i; p = u->url.data; len = u->url.len; + for (i = 0; i < len; ++i) { + if (p[i] <= 0x20 || p[i] == 0x7F) { + u->err = "Control character in URL (possible CRLF injection attack?)"; + return NGX_ERROR; + } + } + if (len >= 5 && ngx_strncasecmp(p, (u_char *) "unix:", 5) == 0) { return ngx_parse_unix_domain_url(pool, u); } diff --git a/src/http/modules/ngx_http_headers_filter_module.c b/src/http/modules/ngx_http_headers_filter_module.c index f72044097..dd81657f1 100644 --- a/src/http/modules/ngx_http_headers_filter_module.c +++ b/src/http/modules/ngx_http_headers_filter_module.c @@ -252,6 +252,14 @@ ngx_http_headers_filter(ngx_http_request_t *r) return NGX_ERROR; } + if (ngx_http_valid_header_value(value) != NGX_OK) { + ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, + "Header value contains forbidden characters " + "(do you use variables in header values that " + "can contain CR or LF?)"); + return NGX_ERROR; + } + if (h[i].handler(r, &h[i], &value) != NGX_OK) { return NGX_ERROR; } @@ -337,6 +345,14 @@ ngx_http_trailers_filter(ngx_http_request_t *r, ngx_chain_t *in) } if (value.len) { + if (ngx_http_valid_header_value(value) != NGX_OK) { + ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, + "Header value contains forbidden characters " + "(do you use variables in header values that " + "can contain CR or LF?)"); + return NGX_ERROR; + } + t = ngx_list_push(&r->headers_out.trailers); if (t == NULL) { return NGX_ERROR; @@ -851,6 +867,11 @@ ngx_http_headers_add(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) value = cf->args->elts; + if (ngx_conf_check_field_name_and_strip_value(cf, value, value + 1, + value + 2) != NGX_OK) { + return NGX_CONF_ERROR; + } + headers = (ngx_array_t **) ((char *) hcf + cmd->offset); if (*headers == NULL) { diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c index 0b388b30f..d47b32af0 100644 --- a/src/http/modules/ngx_http_proxy_module.c +++ b/src/http/modules/ngx_http_proxy_module.c @@ -108,6 +108,8 @@ static char *ngx_http_proxy_merge_loc_conf(ngx_conf_t *cf, static ngx_int_t ngx_http_proxy_init_headers(ngx_conf_t *cf, ngx_http_proxy_loc_conf_t *conf, ngx_http_proxy_headers_t *headers, ngx_keyval_t *default_headers); +static char *ngx_http_proxy_set_header(ngx_conf_t *cf, + ngx_command_t *cmd, void *conf); static char *ngx_http_proxy_pass(ngx_conf_t *cf, ngx_command_t *cmd, void *conf); @@ -321,7 +323,7 @@ static ngx_command_t ngx_http_proxy_commands[] = { { ngx_string("proxy_set_header"), NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2, - ngx_conf_set_keyval_slot, + ngx_http_proxy_set_header, NGX_HTTP_LOC_CONF_OFFSET, offsetof(ngx_http_proxy_loc_conf_t, headers_source), NULL }, @@ -1177,7 +1179,7 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) key_len, val_len; uintptr_t escape; ngx_buf_t *b; - ngx_str_t method; + ngx_str_t method, tmp; ngx_uint_t i, unparsed_uri; ngx_chain_t *cl, *body; ngx_list_part_t *part; @@ -1327,6 +1329,15 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) continue; } + if (ngx_http_valid_header_name(header[i].key) != NGX_OK + || ngx_http_valid_header_value(header[i].value) != NGX_OK) { + /* oops, request smuggling */ + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Internal malformed header name or value " + "detected (this is a bug!!!!"); + return NGX_ERROR; + } + len += header[i].key.len + sizeof(": ") - 1 + header[i].value.len + sizeof(CRLF) - 1; } @@ -1344,6 +1355,7 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) } cl->buf = b; + cl->next = NULL; /* the request line */ @@ -1382,6 +1394,15 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) u->uri.len = b->last - u->uri.data; + for (uri_len = 0; uri_len < u->uri.len; ++uri_len) { + if (u->uri.data[uri_len] <= 0x20 || u->uri.data[uri_len] == 0x7F) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "URI contains control characters " + "(possible CRLF injection?"); + return NGX_ERROR; + } + } + if (plcf->http_version == NGX_HTTP_VERSION_11) { b->last = ngx_cpymem(b->last, ngx_http_proxy_version_11, sizeof(ngx_http_proxy_version_11) - 1); @@ -1402,6 +1423,7 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) while (*(uintptr_t *) le.ip) { + tmp.data = e.pos; lcode = *(ngx_http_script_len_code_pt *) le.ip; (void) lcode(&le); @@ -1426,14 +1448,30 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) code = *(ngx_http_script_code_pt *) e.ip; code((ngx_http_script_engine_t *) &e); + tmp.len = (size_t)(e.pos - tmp.data); + if (ngx_http_valid_header_name(tmp) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Header name contains forbidden characters " + "(do you use header names with variables?)"); + return NGX_ERROR; + } *e.pos++ = ':'; *e.pos++ = ' '; + tmp.data = e.pos; while (*(uintptr_t *) e.ip) { code = *(ngx_http_script_code_pt *) e.ip; code((ngx_http_script_engine_t *) &e); } e.ip += sizeof(uintptr_t); + tmp.len = (size_t)(e.pos - tmp.data); + if (ngx_http_valid_header_value(tmp) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Header value contains forbidden characters " + "(do you use variables in header values that " + "can contain CR or LF?)"); + return NGX_ERROR; + } *e.pos++ = CR; *e.pos++ = LF; } @@ -4115,6 +4153,22 @@ ngx_http_proxy_merge_loc_conf(ngx_conf_t *cf, void *parent, void *child) return NGX_CONF_OK; } +static char * +ngx_http_proxy_set_header(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) +{ + char *rc; + ngx_str_t *value; + + value = cf->args->elts; + + if (ngx_conf_check_field_name_and_strip_value(cf, value, value + 1, + value + 2) != NGX_OK) { + return NGX_CONF_ERROR; + } + + rc = ngx_conf_set_keyval_slot(cf, cmd, conf); + return rc; +} static ngx_int_t ngx_http_proxy_init_headers(ngx_conf_t *cf, ngx_http_proxy_loc_conf_t *conf, diff --git a/src/http/modules/ngx_http_proxy_v2_module.c b/src/http/modules/ngx_http_proxy_v2_module.c index 4c282a864..8a2fe7742 100644 --- a/src/http/modules/ngx_http_proxy_v2_module.c +++ b/src/http/modules/ngx_http_proxy_v2_module.c @@ -322,7 +322,7 @@ ngx_http_proxy_v2_create_request(ngx_http_request_t *r) loc_len, body_len; uintptr_t escape; ngx_buf_t *b; - ngx_str_t method, *host; + ngx_str_t method, *host, tmp_str; ngx_uint_t i, next, unparsed_uri; ngx_chain_t *cl, *body; ngx_list_part_t *part; @@ -520,6 +520,16 @@ ngx_http_proxy_v2_create_request(ngx_http_request_t *r) continue; } + if (ngx_http_valid_header_name(header[i].key) != NGX_OK + || ngx_http_valid_header_value(header[i].value) != NGX_OK) { + /* oops, request smuggling */ + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Internal malformed header name or value " + "detected (this is a bug!!!!"); + return NGX_ERROR; + } + + len += 1 + NGX_HTTP_V2_INT_OCTETS + header[i].key.len + NGX_HTTP_V2_INT_OCTETS + header[i].value.len; @@ -717,6 +727,15 @@ ngx_http_proxy_v2_create_request(ngx_http_request_t *r) code = *(ngx_http_script_code_pt *) e.ip; code((ngx_http_script_engine_t *) &e); + tmp_str.data = key_tmp; + tmp_str.len = key_len; + if (ngx_http_valid_header_name(tmp_str) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Header name contains forbidden characters " + "(do you use header names with variables?)"); + return NGX_ERROR; + } + b->last = ngx_http_v2_write_name(b->last, key_tmp, key_len, tmp); e.pos = val_tmp; @@ -727,6 +746,16 @@ ngx_http_proxy_v2_create_request(ngx_http_request_t *r) } e.ip += sizeof(uintptr_t); + tmp_str.data = val_tmp; + tmp_str.len = val_len; + if (ngx_http_valid_header_value(tmp_str) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Header value contains forbidden characters " + "(do you use variables in header values that " + "can contain CR or LF?)"); + return NGX_ERROR; + } + b->last = ngx_http_v2_write_value(b->last, val_tmp, val_len, tmp); #if (NGX_DEBUG) diff --git a/src/http/ngx_http.c b/src/http/ngx_http.c index a97cc35f1..d82ce55bb 100644 --- a/src/http/ngx_http.c +++ b/src/http/ngx_http.c @@ -2198,3 +2198,77 @@ ngx_http_set_default_types(ngx_conf_t *cf, ngx_array_t **types, return NGX_OK; } + +ngx_int_t +ngx_conf_check_field_name_and_strip_value(ngx_conf_t *cf, + const ngx_str_t *cmd_name, const ngx_str_t *name, ngx_str_t *value) +{ + size_t out_cursor = 0, cursor = 0, end, original_end; + u_char *ptr, ch; + + if (ngx_http_valid_header_name(*name) != NGX_OK) { + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, + "%V: Invalid HTTP field name", + (ngx_str_t *)cmd_name); + return NGX_ERROR; + } + +#define ISSPACE(c) ((c) == ' ' || (c) == '\t') + end = original_end = value->len; + ptr = value->data; + + while (cursor < end && ISSPACE(ptr[cursor])) { + cursor++; + } + + for ( /* void */; cursor < end; ++cursor) { + ch = ptr[cursor]; + + if (ch == 0) { + goto fail; + } + + if (ch != CR && ch != LF) { + ptr[out_cursor++] = ch; + continue; + } + + if (ch == CR) { + if (end - cursor < 2 || ptr[cursor + 1] != LF) { + goto fail; + } + cursor++; + } + cursor++; + + if (cursor >= end || !ISSPACE(ptr[cursor])) { + goto fail; + } + + do { + cursor++; + } while (cursor < end && ISSPACE(ptr[cursor])); + + while (out_cursor > 0 && ISSPACE(ptr[out_cursor - 1])) { + out_cursor--; + } + + if (out_cursor > 0) { + ptr[out_cursor++] = ' '; + } + } + + while (out_cursor > 0 && ISSPACE(ptr[out_cursor - 1])) { + out_cursor--; + } + + if (out_cursor < original_end) { + memset(ptr + out_cursor, '\0', (size_t)(original_end - out_cursor)); + } + value->len = out_cursor; + return NGX_OK; +fail: + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, + "%V: Invalid HTTP field value", (ngx_str_t *)cmd_name); + return NGX_ERROR; +} diff --git a/src/http/ngx_http.h b/src/http/ngx_http.h index 4e4511cc5..7b2891b7d 100644 --- a/src/http/ngx_http.h +++ b/src/http/ngx_http.h @@ -121,6 +121,15 @@ void ngx_http_split_args(ngx_http_request_t *r, ngx_str_t *uri, ngx_int_t ngx_http_parse_chunked(ngx_http_request_t *r, ngx_buf_t *b, ngx_http_chunked_t *ctx, ngx_uint_t keep_trailers); +/** Check if value is a valid HTTP field name. */ +ngx_int_t ngx_http_valid_header_name(ngx_str_t value); +/** Check if value is a valid HTTP field value. */ +ngx_int_t ngx_http_valid_header_value(ngx_str_t value); +/** Check if the field name and value are valid for an HTTP field name-value + * pair. Leading and trailing ' ' and '\t' are stripped from the field value + * before the value is checked. The value is updated in-place. */ +ngx_int_t ngx_conf_check_field_name_and_strip_value(ngx_conf_t *cf, + const ngx_str_t *cmd_name, const ngx_str_t *name, ngx_str_t *value); ngx_http_request_t *ngx_http_create_request(ngx_connection_t *c); ngx_int_t ngx_http_process_request_uri(ngx_http_request_t *r); diff --git a/src/http/ngx_http_header_filter_module.c b/src/http/ngx_http_header_filter_module.c index 15215c531..a4efdb49f 100644 --- a/src/http/ngx_http_header_filter_module.c +++ b/src/http/ngx_http_header_filter_module.c @@ -435,6 +435,21 @@ ngx_http_header_filter(ngx_http_request_t *r) continue; } + if (ngx_http_valid_header_name(header[i].key) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Header name contains forbidden characters " + "(do you use header names with variables?)"); + return NGX_ERROR; + } + + if (ngx_http_valid_header_value(header[i].value) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Header name contains forbidden characters " + "(do you use variables in header values that " + "can contain CR or LF?)"); + return NGX_ERROR; + } + len += header[i].key.len + sizeof(": ") - 1 + header[i].value.len + sizeof(CRLF) - 1; } @@ -603,6 +618,19 @@ ngx_http_header_filter(ngx_http_request_t *r) continue; } + if (ngx_http_valid_header_name(header[i].key) != NGX_OK) { + ngx_log_error(NGX_LOG_EMERG, r->connection->log, 0, + "BUG: internal bad header name not caught earlier"); + return NGX_ERROR; + } + + if (ngx_http_valid_header_value(header[i].value) != NGX_OK) { + ngx_log_error(NGX_LOG_EMERG, r->connection->log, 0, + "BUG: internal bad header value not caught earlier"); + return NGX_ERROR; + } + + b->last = ngx_copy(b->last, header[i].key.data, header[i].key.len); *b->last++ = ':'; *b->last++ = ' '; @@ -668,6 +696,21 @@ ngx_http_early_hints_filter(ngx_http_request_t *r) continue; } + if (ngx_http_valid_header_name(header[i].key) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Header name contains forbidden characters " + "(do you use header names with variables?)"); + return NGX_ERROR; + } + + if (ngx_http_valid_header_value(header[i].value) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Header name contains forbidden characters " + "(do you use variables in header values that " + "can contain CR or LF?)"); + return NGX_ERROR; + } + len += header[i].key.len + sizeof(": ") - 1 + header[i].value.len + sizeof(CRLF) - 1; } @@ -707,6 +750,18 @@ ngx_http_early_hints_filter(ngx_http_request_t *r) continue; } + if (ngx_http_valid_header_name(header[i].key) != NGX_OK) { + ngx_log_error(NGX_LOG_EMERG, r->connection->log, 0, + "BUG: internal bad header name not caught earlier"); + return NGX_ERROR; + } + + if (ngx_http_valid_header_value(header[i].value) != NGX_OK) { + ngx_log_error(NGX_LOG_EMERG, r->connection->log, 0, + "BUG: internal bad header value not caught earlier"); + return NGX_ERROR; + } + b->last = ngx_copy(b->last, header[i].key.data, header[i].key.len); *b->last++ = ':'; *b->last++ = ' '; diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 81f689e5b..e41a0c9b9 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -2467,3 +2467,59 @@ invalid: return NGX_ERROR; } + + +ngx_int_t +ngx_http_valid_header_value(ngx_str_t value) +{ + size_t j; + + /* TODO: this might break configurations that are invalid, + * but happen to work right now because the leading and/or + * trailing whitespace gets stripped. It should be turned on + * if NGINX ever has a major version bump, or if a mechanism + * is added for the user to opt-in. Sending a message with + * leading or trailing space is much less serious than CRLF + * injection. */ + if (0) { + if (value.len < 1) { + return NGX_OK; + } + + if (value.data[0] == ' ' || value.data[0] == '\t' + || value.data[value.len - 1] == ' ' + || value.data[value.len - 1] == '\t') { + return NGX_ERROR; + } + } + + for (j = 0; j < value.len; ++j) { + if (value.data[j] == '\0' || value.data[j] == CR + || value.data[j] == LF) { + return NGX_ERROR; + } + } + + return NGX_OK; +} + + +ngx_int_t +ngx_http_valid_header_name(ngx_str_t value) +{ + size_t j; + + if (value.len < 1) { + return NGX_ERROR; + } + + for (j = 0; j < value.len; ++j) { + /* todo: check that this is an HTTP TOKEN */ + if (value.data[j] <= 0x20 || value.data[j] == 0x7F + || value.data[j] == ':') { + return NGX_ERROR; + } + } + + return NGX_OK; +}