From 5eadaf69e394c030056e4190d86dae0262f8617c Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Tue, 1 Jun 2021 17:37:51 +0300 Subject: Fixed SSL logging with lingering close. Recent fixes to SSL shutdown with lingering close (554c6ae25ffc, 1.19.5) broke logging of SSL variables. To make sure logging of SSL variables works properly, avoid freeing c->ssl when doing an SSL shutdown before lingering close. Reported by Reinis Rozitis (http://mailman.nginx.org/pipermail/nginx/2021-May/060670.html). --- src/http/ngx_http_request.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/http') diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c index 81b27a386..0bb122ce0 100644 --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -3400,6 +3400,8 @@ ngx_http_set_lingering_close(ngx_connection_t *c) if (c->ssl) { ngx_int_t rc; + c->ssl->shutdown_without_free = 1; + rc = ngx_ssl_shutdown(c); if (rc == NGX_ERROR) { -- cgit From dcdf7ec096f0998e689b7f0b0f7541e197eeff6a Mon Sep 17 00:00:00 2001 From: Sergey Kandaurov Date: Thu, 17 Jun 2021 11:43:55 +0300 Subject: gRPC: handling GOAWAY with a higher last stream identifier. Previously, once received from upstream, it couldn't limit opening additional streams in a cached keepalive connection. --- src/http/modules/ngx_http_grpc_module.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'src/http') diff --git a/src/http/modules/ngx_http_grpc_module.c b/src/http/modules/ngx_http_grpc_module.c index 2e20e5ffd..42a540f19 100644 --- a/src/http/modules/ngx_http_grpc_module.c +++ b/src/http/modules/ngx_http_grpc_module.c @@ -121,6 +121,7 @@ typedef struct { unsigned done:1; unsigned status:1; unsigned rst:1; + unsigned goaway:1; ngx_http_request_t *request; @@ -1210,6 +1211,7 @@ ngx_http_grpc_reinit_request(ngx_http_request_t *r) ctx->done = 0; ctx->status = 0; ctx->rst = 0; + ctx->goaway = 0; ctx->connection = NULL; return NGX_OK; @@ -1565,6 +1567,7 @@ ngx_http_grpc_body_output_filter(void *data, ngx_chain_t *in) && ctx->out == NULL && ctx->output_closed && !ctx->output_blocked + && !ctx->goaway && ctx->state == ngx_http_grpc_st_start) { u->keepalive = 1; @@ -1714,6 +1717,8 @@ ngx_http_grpc_process_header(ngx_http_request_t *r) return NGX_HTTP_UPSTREAM_INVALID_HEADER; } + ctx->goaway = 1; + continue; } @@ -1907,6 +1912,7 @@ ngx_http_grpc_process_header(ngx_http_request_t *r) && ctx->out == NULL && ctx->output_closed && !ctx->output_blocked + && !ctx->goaway && b->last == b->pos) { u->keepalive = 1; @@ -2035,6 +2041,7 @@ ngx_http_grpc_filter(void *data, ssize_t bytes) if (ctx->in == NULL && ctx->output_closed && !ctx->output_blocked + && !ctx->goaway && ctx->state == ngx_http_grpc_st_start) { u->keepalive = 1; @@ -2204,6 +2211,8 @@ ngx_http_grpc_filter(void *data, ssize_t bytes) return NGX_ERROR; } + ctx->goaway = 1; + continue; } -- cgit From 693e4134a51b4fd30689ad1e31e6fdffe5ee1429 Mon Sep 17 00:00:00 2001 From: Sergey Kandaurov Date: Thu, 17 Jun 2021 11:44:06 +0300 Subject: gRPC: RST_STREAM(NO_ERROR) handling micro-optimization. After 2096b21fcd10, a single RST_STREAM(NO_ERROR) may not result in an error. This change removes several unnecessary ctx->type checks for such a case. --- src/http/modules/ngx_http_grpc_module.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src/http') diff --git a/src/http/modules/ngx_http_grpc_module.c b/src/http/modules/ngx_http_grpc_module.c index 42a540f19..654bd17b9 100644 --- a/src/http/modules/ngx_http_grpc_module.c +++ b/src/http/modules/ngx_http_grpc_module.c @@ -2177,6 +2177,8 @@ ngx_http_grpc_filter(void *data, ssize_t bytes) } ctx->rst = 1; + + continue; } if (ctx->type == NGX_HTTP_V2_GOAWAY_FRAME) { @@ -3484,6 +3486,8 @@ ngx_http_grpc_parse_rst_stream(ngx_http_request_t *r, ngx_http_grpc_ctx_t *ctx, return NGX_AGAIN; } + ctx->state = ngx_http_grpc_st_start; + return NGX_OK; } -- cgit From d9c1d1bae7ae2c83fb65ca00a47ad6c1199a691e Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 28 Jun 2021 18:01:00 +0300 Subject: Moved TRACE method rejection to a better place. Previously, TRACE requests were rejected before parsing Transfer-Encoding. This is not important since keepalive is not enabled at this point anyway, though rejecting such requests after properly parsing other headers is less likely to cause issues in case of further code changes. --- src/http/ngx_http_request.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'src/http') diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c index 0bb122ce0..b908e2941 100644 --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -1980,13 +1980,6 @@ ngx_http_process_request_header(ngx_http_request_t *r) } } - if (r->method == NGX_HTTP_TRACE) { - ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, - "client sent TRACE method"); - ngx_http_finalize_request(r, NGX_HTTP_NOT_ALLOWED); - return NGX_ERROR; - } - if (r->headers_in.transfer_encoding) { if (r->headers_in.transfer_encoding->value.len == 7 && ngx_strncasecmp(r->headers_in.transfer_encoding->value.data, @@ -2013,6 +2006,13 @@ ngx_http_process_request_header(ngx_http_request_t *r) } } + if (r->method == NGX_HTTP_TRACE) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent TRACE method"); + ngx_http_finalize_request(r, NGX_HTTP_NOT_ALLOWED); + return NGX_ERROR; + } + return NGX_OK; } -- cgit From 5f85bb3714a81d158f4d849ad5c61aec2737a9f0 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 28 Jun 2021 18:01:04 +0300 Subject: Added CONNECT method rejection. No valid CONNECT requests are expected to appear within nginx, since it is not a forward proxy. Further, request line parsing will reject proper CONNECT requests anyway, since we don't allow authority-form of request-target. On the other hand, RFC 7230 specifies separate message length rules for CONNECT which we don't support, so make sure to always reject CONNECTs to avoid potential abuse. --- src/http/ngx_http_parse.c | 5 +++++ src/http/ngx_http_request.c | 7 +++++++ src/http/ngx_http_request.h | 33 +++++++++++++++++---------------- src/http/v2/ngx_http_v2.c | 3 ++- 4 files changed, 31 insertions(+), 17 deletions(-) (limited to 'src/http') diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 20ad89a77..71fa3c7a5 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -246,6 +246,11 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) r->method = NGX_HTTP_OPTIONS; } + if (ngx_str7_cmp(m, 'C', 'O', 'N', 'N', 'E', 'C', 'T', ' ')) + { + r->method = NGX_HTTP_CONNECT; + } + break; case 8: diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c index b908e2941..5b2613870 100644 --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -2006,6 +2006,13 @@ ngx_http_process_request_header(ngx_http_request_t *r) } } + if (r->method == NGX_HTTP_CONNECT) { + ngx_log_error(NGX_LOG_INFO, r->connection->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, r->connection->log, 0, "client sent TRACE method"); diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h index 6dfb4a42f..fa4d5f99f 100644 --- a/src/http/ngx_http_request.h +++ b/src/http/ngx_http_request.h @@ -25,22 +25,23 @@ #define NGX_HTTP_VERSION_11 1001 #define NGX_HTTP_VERSION_20 2000 -#define NGX_HTTP_UNKNOWN 0x0001 -#define NGX_HTTP_GET 0x0002 -#define NGX_HTTP_HEAD 0x0004 -#define NGX_HTTP_POST 0x0008 -#define NGX_HTTP_PUT 0x0010 -#define NGX_HTTP_DELETE 0x0020 -#define NGX_HTTP_MKCOL 0x0040 -#define NGX_HTTP_COPY 0x0080 -#define NGX_HTTP_MOVE 0x0100 -#define NGX_HTTP_OPTIONS 0x0200 -#define NGX_HTTP_PROPFIND 0x0400 -#define NGX_HTTP_PROPPATCH 0x0800 -#define NGX_HTTP_LOCK 0x1000 -#define NGX_HTTP_UNLOCK 0x2000 -#define NGX_HTTP_PATCH 0x4000 -#define NGX_HTTP_TRACE 0x8000 +#define NGX_HTTP_UNKNOWN 0x00000001 +#define NGX_HTTP_GET 0x00000002 +#define NGX_HTTP_HEAD 0x00000004 +#define NGX_HTTP_POST 0x00000008 +#define NGX_HTTP_PUT 0x00000010 +#define NGX_HTTP_DELETE 0x00000020 +#define NGX_HTTP_MKCOL 0x00000040 +#define NGX_HTTP_COPY 0x00000080 +#define NGX_HTTP_MOVE 0x00000100 +#define NGX_HTTP_OPTIONS 0x00000200 +#define NGX_HTTP_PROPFIND 0x00000400 +#define NGX_HTTP_PROPPATCH 0x00000800 +#define NGX_HTTP_LOCK 0x00001000 +#define NGX_HTTP_UNLOCK 0x00002000 +#define NGX_HTTP_PATCH 0x00004000 +#define NGX_HTTP_TRACE 0x00008000 +#define NGX_HTTP_CONNECT 0x00010000 #define NGX_HTTP_CONNECTION_CLOSE 1 #define NGX_HTTP_CONNECTION_KEEP_ALIVE 2 diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index 3611a2e50..423667d47 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -3606,7 +3606,8 @@ ngx_http_v2_parse_method(ngx_http_request_t *r, ngx_str_t *value) { 4, "LOCK", NGX_HTTP_LOCK }, { 6, "UNLOCK", NGX_HTTP_UNLOCK }, { 5, "PATCH", NGX_HTTP_PATCH }, - { 5, "TRACE", NGX_HTTP_TRACE } + { 5, "TRACE", NGX_HTTP_TRACE }, + { 7, "CONNECT", NGX_HTTP_CONNECT } }, *test; if (r->method_name.len) { -- cgit From a6c109fea5c13b8aa13ed95ca00a64d62601042b Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 28 Jun 2021 18:01:06 +0300 Subject: Disabled requests with both Content-Length and Transfer-Encoding. HTTP clients are not allowed to generate such requests since Transfer-Encoding introduction in RFC 2068, and they are not expected to appear in practice except in attempts to perform a request smuggling attack. While handling of such requests is strictly defined, the most secure approach seems to reject them. --- src/http/ngx_http_request.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'src/http') diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c index 5b2613870..2614b998c 100644 --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -1985,8 +1985,15 @@ ngx_http_process_request_header(ngx_http_request_t *r) && ngx_strncasecmp(r->headers_in.transfer_encoding->value.data, (u_char *) "chunked", 7) == 0) { - r->headers_in.content_length = NULL; - r->headers_in.content_length_n = -1; + if (r->headers_in.content_length) { + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "client sent \"Content-Length\" and " + "\"Transfer-Encoding\" headers " + "at the same time"); + ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); + return NGX_ERROR; + } + r->headers_in.chunked = 1; } else { -- cgit From 05395f4889cf0b66e8d049921ad19f1a08319150 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 28 Jun 2021 18:01:13 +0300 Subject: Disabled spaces in URIs (ticket #196). From now on, requests with spaces in URIs are immediately rejected rather than allowed. Spaces were allowed in 31e9677b15a1 (0.8.41) to handle bad clients. It is believed that now this behaviour causes more harm than good. --- src/http/modules/ngx_http_proxy_module.c | 4 +- src/http/ngx_http_parse.c | 72 ++++---------------------------- src/http/ngx_http_request.c | 2 +- src/http/ngx_http_request.h | 3 -- 4 files changed, 11 insertions(+), 70 deletions(-) (limited to 'src/http') diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c index 64190f1a0..d82f5ea21 100644 --- a/src/http/modules/ngx_http_proxy_module.c +++ b/src/http/modules/ngx_http_proxy_module.c @@ -1186,7 +1186,7 @@ ngx_http_proxy_create_key(ngx_http_request_t *r) loc_len = (r->valid_location && ctx->vars.uri.len) ? plcf->location.len : 0; - if (r->quoted_uri || r->space_in_uri || r->internal) { + if (r->quoted_uri || r->internal) { escape = 2 * ngx_escape_uri(NULL, r->uri.data + loc_len, r->uri.len - loc_len, NGX_ESCAPE_URI); } else { @@ -1299,7 +1299,7 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) loc_len = (r->valid_location && ctx->vars.uri.len) ? plcf->location.len : 0; - if (r->quoted_uri || r->space_in_uri || r->internal) { + if (r->quoted_uri || r->internal) { escape = 2 * ngx_escape_uri(NULL, r->uri.data + loc_len, r->uri.len - loc_len, NGX_ESCAPE_URI); } diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 71fa3c7a5..8297a132b 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -116,10 +116,8 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) sw_host_end, sw_host_ip_literal, sw_port, - sw_host_http_09, sw_after_slash_in_uri, sw_check_uri, - sw_check_uri_http_09, sw_uri, sw_http_09, sw_http_H, @@ -398,7 +396,7 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) */ r->uri_start = r->schema_end + 1; r->uri_end = r->schema_end + 2; - state = sw_host_http_09; + state = sw_http_09; break; default: return NGX_HTTP_PARSE_INVALID_REQUEST; @@ -472,35 +470,13 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) */ r->uri_start = r->schema_end + 1; r->uri_end = r->schema_end + 2; - state = sw_host_http_09; - break; - default: - return NGX_HTTP_PARSE_INVALID_REQUEST; - } - break; - - /* space+ after "http://host[:port] " */ - case sw_host_http_09: - switch (ch) { - case ' ': - break; - case CR: - 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; + state = sw_http_09; break; default: return NGX_HTTP_PARSE_INVALID_REQUEST; } break; - /* check "/.", "//", "%", and "\" (Win32) in URI */ case sw_after_slash_in_uri: @@ -512,7 +488,7 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) switch (ch) { case ' ': r->uri_end = p; - state = sw_check_uri_http_09; + state = sw_http_09; break; case CR: r->uri_end = p; @@ -584,7 +560,7 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) break; case ' ': r->uri_end = p; - state = sw_check_uri_http_09; + state = sw_http_09; break; case CR: r->uri_end = p; @@ -621,31 +597,6 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) } break; - /* space+ after URI */ - case sw_check_uri_http_09: - switch (ch) { - case ' ': - break; - case CR: - 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; - break; - default: - r->space_in_uri = 1; - state = sw_check_uri; - p--; - break; - } - break; - - /* URI */ case sw_uri: @@ -692,10 +643,7 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) state = sw_http_H; break; default: - r->space_in_uri = 1; - state = sw_uri; - p--; - break; + return NGX_HTTP_PARSE_INVALID_REQUEST; } break; @@ -1171,9 +1119,7 @@ ngx_http_parse_uri(ngx_http_request_t *r) switch (ch) { case ' ': - r->space_in_uri = 1; - state = sw_check_uri; - break; + return NGX_ERROR; case '.': r->complex_uri = 1; state = sw_uri; @@ -1232,8 +1178,7 @@ ngx_http_parse_uri(ngx_http_request_t *r) r->uri_ext = p + 1; break; case ' ': - r->space_in_uri = 1; - break; + return NGX_ERROR; #if (NGX_WIN32) case '\\': r->complex_uri = 1; @@ -1267,8 +1212,7 @@ ngx_http_parse_uri(ngx_http_request_t *r) switch (ch) { case ' ': - r->space_in_uri = 1; - break; + return NGX_ERROR; case '#': r->complex_uri = 1; break; diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c index 2614b998c..7956610c4 100644 --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -1264,7 +1264,7 @@ ngx_http_process_request_uri(ngx_http_request_t *r) r->unparsed_uri.len = r->uri_end - r->uri_start; r->unparsed_uri.data = r->uri_start; - r->valid_unparsed_uri = (r->space_in_uri || r->empty_path_in_uri) ? 0 : 1; + r->valid_unparsed_uri = r->empty_path_in_uri ? 0 : 1; if (r->uri_ext) { if (r->args_start) { diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h index fa4d5f99f..63576274e 100644 --- a/src/http/ngx_http_request.h +++ b/src/http/ngx_http_request.h @@ -468,9 +468,6 @@ struct ngx_http_request_s { /* URI with "+" */ unsigned plus_in_uri:1; - /* URI with " " */ - unsigned space_in_uri:1; - /* URI with empty path */ unsigned empty_path_in_uri:1; -- cgit From 0b66bd4be777a5b79c5ae0e7dff89fc6429da0fe Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 28 Jun 2021 18:01:15 +0300 Subject: Disabled control characters in URIs. Control characters (0x00-0x1f, 0x7f) were never allowed in URIs, and must be percent-encoded by clients. Further, these are not believed to appear in practice. On the other hand, passing such characters might make various attacks possible or easier, despite the fact that currently allowed control characters are not significant for HTTP request parsing. --- src/http/ngx_http_parse.c | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) (limited to 'src/http') diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 8297a132b..0d36a44e5 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -11,7 +11,7 @@ static uint32_t usual[] = { - 0xffffdbfe, /* 1111 1111 1111 1111 1101 1011 1111 1110 */ + 0x00000000, /* 0000 0000 0000 0000 0000 0000 0000 0000 */ /* ?>=< ;:98 7654 3210 /.-, +*)( '&%$ #"! */ 0x7fff37d6, /* 0111 1111 1111 1111 0011 0111 1101 0110 */ @@ -24,7 +24,7 @@ static uint32_t usual[] = { #endif /* ~}| {zyx wvut srqp onml kjih gfed cba` */ - 0xffffffff, /* 1111 1111 1111 1111 1111 1111 1111 1111 */ + 0x7fffffff, /* 0111 1111 1111 1111 1111 1111 1111 1111 */ 0xffffffff, /* 1111 1111 1111 1111 1111 1111 1111 1111 */ 0xffffffff, /* 1111 1111 1111 1111 1111 1111 1111 1111 */ @@ -528,9 +528,10 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) case '+': r->plus_in_uri = 1; break; - case '\0': - return NGX_HTTP_PARSE_INVALID_REQUEST; default: + if (ch < 0x20 || ch == 0x7f) { + return NGX_HTTP_PARSE_INVALID_REQUEST; + } state = sw_check_uri; break; } @@ -592,8 +593,11 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) case '+': r->plus_in_uri = 1; break; - case '\0': - return NGX_HTTP_PARSE_INVALID_REQUEST; + default: + if (ch < 0x20 || ch == 0x7f) { + return NGX_HTTP_PARSE_INVALID_REQUEST; + } + break; } break; @@ -621,8 +625,11 @@ ngx_http_parse_request_line(ngx_http_request_t *r, ngx_buf_t *b) case '#': r->complex_uri = 1; break; - case '\0': - return NGX_HTTP_PARSE_INVALID_REQUEST; + default: + if (ch < 0x20 || ch == 0x7f) { + return NGX_HTTP_PARSE_INVALID_REQUEST; + } + break; } break; @@ -1118,8 +1125,6 @@ ngx_http_parse_uri(ngx_http_request_t *r) } switch (ch) { - case ' ': - return NGX_ERROR; case '.': r->complex_uri = 1; state = sw_uri; @@ -1150,6 +1155,9 @@ ngx_http_parse_uri(ngx_http_request_t *r) r->plus_in_uri = 1; break; default: + if (ch <= 0x20 || ch == 0x7f) { + return NGX_ERROR; + } state = sw_check_uri; break; } @@ -1177,8 +1185,6 @@ ngx_http_parse_uri(ngx_http_request_t *r) case '.': r->uri_ext = p + 1; break; - case ' ': - return NGX_ERROR; #if (NGX_WIN32) case '\\': r->complex_uri = 1; @@ -1200,6 +1206,11 @@ ngx_http_parse_uri(ngx_http_request_t *r) case '+': r->plus_in_uri = 1; break; + default: + if (ch <= 0x20 || ch == 0x7f) { + return NGX_ERROR; + } + break; } break; @@ -1211,11 +1222,14 @@ ngx_http_parse_uri(ngx_http_request_t *r) } switch (ch) { - case ' ': - return NGX_ERROR; case '#': r->complex_uri = 1; break; + default: + if (ch <= 0x20 || ch == 0x7f) { + return NGX_ERROR; + } + break; } break; } -- cgit From 9ab4d368af63e9c4a0bebc0eda82d668adaa560a Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 28 Jun 2021 18:01:18 +0300 Subject: Disabled control characters and space in header names. Control characters (0x00-0x1f, 0x7f), space, and colon were never allowed in header names. The only somewhat valid use is header continuation which nginx never supported and which is explicitly obsolete by RFC 7230. Previously, such headers were considered invalid and were ignored by default (as per ignore_invalid_headers directive). With this change, such headers are unconditionally rejected. It is expected to make nginx more resilient to various attacks, in particular, with ignore_invalid_headers switched off (which is inherently unsecure, though nevertheless sometimes used in the wild). --- src/http/modules/ngx_http_grpc_module.c | 2 +- src/http/ngx_http_parse.c | 4 ++-- src/http/v2/ngx_http_v2.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) (limited to 'src/http') diff --git a/src/http/modules/ngx_http_grpc_module.c b/src/http/modules/ngx_http_grpc_module.c index 654bd17b9..65bd1e6c3 100644 --- a/src/http/modules/ngx_http_grpc_module.c +++ b/src/http/modules/ngx_http_grpc_module.c @@ -3384,7 +3384,7 @@ ngx_http_grpc_validate_header_name(ngx_http_request_t *r, ngx_str_t *s) return NGX_ERROR; } - if (ch == '\0' || ch == CR || ch == LF) { + if (ch <= 0x20 || ch == 0x7f) { return NGX_ERROR; } } diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 0d36a44e5..6af326dee 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -893,7 +893,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, break; } - if (ch == '\0') { + if (ch <= 0x20 || ch == 0x7f || ch == ':') { return NGX_HTTP_PARSE_INVALID_HEADER; } @@ -961,7 +961,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, break; } - if (ch == '\0') { + if (ch <= 0x20 || ch == 0x7f) { return NGX_HTTP_PARSE_INVALID_HEADER; } diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index 423667d47..3bef002bd 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -3457,7 +3457,7 @@ ngx_http_v2_validate_header(ngx_http_request_t *r, ngx_http_v2_header_t *header) continue; } - if (ch == '\0' || ch == LF || ch == CR || ch == ':' + if (ch <= 0x20 || ch == 0x7f || ch == ':' || (ch >= 'A' && ch <= 'Z')) { ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, -- cgit From 7587778a33bea0ce6f203a8c4de18e33f38b9582 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 28 Jun 2021 18:01:20 +0300 Subject: Improved logging of invalid headers. In 71edd9192f24 logging of invalid headers which were rejected with the NGX_HTTP_PARSE_INVALID_HEADER error was restricted to just the "client sent invalid header line" message, without any attempts to log the header itself. This patch returns logging of the header up to the invalid character and the character itself. The r->header_end pointer is now properly set in all cases to make logging possible. The same logging is also introduced when parsing headers from upstream servers. --- src/http/modules/ngx_http_fastcgi_module.c | 8 +++++--- src/http/modules/ngx_http_proxy_module.c | 8 +++++--- src/http/modules/ngx_http_scgi_module.c | 8 +++++--- src/http/modules/ngx_http_uwsgi_module.c | 8 +++++--- src/http/ngx_http_parse.c | 5 +++++ src/http/ngx_http_request.c | 4 +++- 6 files changed, 28 insertions(+), 13 deletions(-) (limited to 'src/http') diff --git a/src/http/modules/ngx_http_fastcgi_module.c b/src/http/modules/ngx_http_fastcgi_module.c index 5191880e3..69ac0f72c 100644 --- a/src/http/modules/ngx_http_fastcgi_module.c +++ b/src/http/modules/ngx_http_fastcgi_module.c @@ -2019,10 +2019,12 @@ ngx_http_fastcgi_process_header(ngx_http_request_t *r) break; } - /* there was error while a header line parsing */ + /* rc == NGX_HTTP_PARSE_INVALID_HEADER */ - ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, - "upstream sent invalid header"); + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "upstream sent invalid header: \"%*s\\x%02xd...\"", + r->header_end - r->header_name_start, + r->header_name_start, *r->header_end); return NGX_HTTP_UPSTREAM_INVALID_HEADER; } diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c index d82f5ea21..368297e77 100644 --- a/src/http/modules/ngx_http_proxy_module.c +++ b/src/http/modules/ngx_http_proxy_module.c @@ -2019,10 +2019,12 @@ ngx_http_proxy_process_header(ngx_http_request_t *r) return NGX_AGAIN; } - /* there was error while a header line parsing */ + /* rc == NGX_HTTP_PARSE_INVALID_HEADER */ - ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, - "upstream sent invalid header"); + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "upstream sent invalid header: \"%*s\\x%02xd...\"", + r->header_end - r->header_name_start, + r->header_name_start, *r->header_end); return NGX_HTTP_UPSTREAM_INVALID_HEADER; } diff --git a/src/http/modules/ngx_http_scgi_module.c b/src/http/modules/ngx_http_scgi_module.c index 600999c88..570713df9 100644 --- a/src/http/modules/ngx_http_scgi_module.c +++ b/src/http/modules/ngx_http_scgi_module.c @@ -1140,10 +1140,12 @@ ngx_http_scgi_process_header(ngx_http_request_t *r) return NGX_AGAIN; } - /* there was error while a header line parsing */ + /* rc == NGX_HTTP_PARSE_INVALID_HEADER */ - ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, - "upstream sent invalid header"); + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "upstream sent invalid header: \"%*s\\x%02xd...\"", + r->header_end - r->header_name_start, + r->header_name_start, *r->header_end); return NGX_HTTP_UPSTREAM_INVALID_HEADER; } diff --git a/src/http/modules/ngx_http_uwsgi_module.c b/src/http/modules/ngx_http_uwsgi_module.c index 655be98c7..40a06c78e 100644 --- a/src/http/modules/ngx_http_uwsgi_module.c +++ b/src/http/modules/ngx_http_uwsgi_module.c @@ -1361,10 +1361,12 @@ ngx_http_uwsgi_process_header(ngx_http_request_t *r) return NGX_AGAIN; } - /* there was error while a header line parsing */ + /* rc == NGX_HTTP_PARSE_INVALID_HEADER */ - ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, - "upstream sent invalid header"); + ngx_log_error(NGX_LOG_INFO, r->connection->log, 0, + "upstream sent invalid header: \"%*s\\x%02xd...\"", + r->header_end - r->header_name_start, + r->header_name_start, *r->header_end); return NGX_HTTP_UPSTREAM_INVALID_HEADER; } diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index 6af326dee..6460da293 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -894,6 +894,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, } if (ch <= 0x20 || ch == 0x7f || ch == ':') { + r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; } @@ -962,6 +963,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, } if (ch <= 0x20 || ch == 0x7f) { + r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; } @@ -984,6 +986,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, r->header_end = p; goto done; case '\0': + r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; default: r->header_start = p; @@ -1007,6 +1010,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, r->header_end = p; goto done; case '\0': + r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; } break; @@ -1022,6 +1026,7 @@ ngx_http_parse_header_line(ngx_http_request_t *r, ngx_buf_t *b, case LF: goto done; case '\0': + r->header_end = p; return NGX_HTTP_PARSE_INVALID_HEADER; default: state = sw_value; diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c index 7956610c4..2e7c30fb6 100644 --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -1522,7 +1522,9 @@ ngx_http_process_request_headers(ngx_event_t *rev) /* rc == NGX_HTTP_PARSE_INVALID_HEADER */ ngx_log_error(NGX_LOG_INFO, c->log, 0, - "client sent invalid header line"); + "client sent invalid header line: \"%*s\\x%02xd...\"", + r->header_end - r->header_name_start, + r->header_name_start, *r->header_end); ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); break; -- cgit From 07c63a42640e59bf5e3399cfdafd498b61671780 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 28 Jun 2021 18:01:24 +0300 Subject: Disabled control characters in the Host header. Control characters (0x00-0x1f, 0x7f) and space are not expected to appear in the Host header. Requests with such characters in the Host header are now unconditionally rejected. --- src/http/ngx_http_request.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'src/http') diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c index 2e7c30fb6..2d1845d02 100644 --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -2176,15 +2176,16 @@ ngx_http_validate_host(ngx_str_t *host, ngx_pool_t *pool, ngx_uint_t alloc) } break; - case '\0': - return NGX_DECLINED; - default: if (ngx_path_separator(ch)) { return NGX_DECLINED; } + if (ch <= 0x20 || ch == 0x7f) { + return NGX_DECLINED; + } + if (ch >= 'A' && ch <= 'Z') { alloc = 1; } -- cgit