From 7bcb50c0610a18bf43bef0062b2d2dc550823b53 Mon Sep 17 00:00:00 2001 From: Sergey Kandaurov Date: Mon, 9 Aug 2021 18:12:12 +0300 Subject: Disabled HTTP/1.0 requests with Transfer-Encoding. The latest HTTP/1.1 draft describes Transfer-Encoding in HTTP/1.0 as having potentially faulty message framing as that could have been forwarded without handling of the chunked encoding, and forbids processing subsequest requests over that connection: https://github.com/httpwg/http-core/issues/879. While handling of such requests is permitted, the most secure approach seems to reject them. --- src/http/ngx_http_request.c | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'src/http') diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c index 2d1845d02..bf931bf35 100644 --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -1983,6 +1983,14 @@ ngx_http_process_request_header(ngx_http_request_t *r) } if (r->headers_in.transfer_encoding) { + 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 " + "\"Transfer-Encoding\" header"); + ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST); + return NGX_ERROR; + } + if (r->headers_in.transfer_encoding->value.len == 7 && ngx_strncasecmp(r->headers_in.transfer_encoding->value.data, (u_char *) "chunked", 7) == 0) -- cgit From ce5996cdd1b2e150f645efbc337e5a681dbe241c Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Mon, 16 Aug 2021 22:40:31 +0300 Subject: SSL: ciphers now set before loading certificates (ticket #2035). To load old/weak server or client certificates it might be needed to adjust the security level, as introduced in OpenSSL 1.1.0. This change ensures that ciphers are set before loading the certificates, so security level changes via the cipher string apply to certificate loading. --- src/http/modules/ngx_http_grpc_module.c | 12 ++++++------ src/http/modules/ngx_http_proxy_module.c | 12 ++++++------ src/http/modules/ngx_http_ssl_module.c | 14 +++++++------- src/http/modules/ngx_http_uwsgi_module.c | 12 ++++++------ 4 files changed, 25 insertions(+), 25 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 65bd1e6c3..6842b7c6e 100644 --- a/src/http/modules/ngx_http_grpc_module.c +++ b/src/http/modules/ngx_http_grpc_module.c @@ -4896,6 +4896,12 @@ ngx_http_grpc_set_ssl(ngx_conf_t *cf, ngx_http_grpc_loc_conf_t *glcf) cln->handler = ngx_ssl_cleanup_ctx; cln->data = glcf->upstream.ssl; + if (ngx_ssl_ciphers(cf, glcf->upstream.ssl, &glcf->ssl_ciphers, 0) + != NGX_OK) + { + return NGX_ERROR; + } + if (glcf->upstream.ssl_certificate) { if (glcf->upstream.ssl_certificate_key == NULL) { @@ -4927,12 +4933,6 @@ ngx_http_grpc_set_ssl(ngx_conf_t *cf, ngx_http_grpc_loc_conf_t *glcf) } } - if (ngx_ssl_ciphers(cf, glcf->upstream.ssl, &glcf->ssl_ciphers, 0) - != NGX_OK) - { - return NGX_ERROR; - } - if (glcf->upstream.ssl_verify) { if (glcf->ssl_trusted_certificate.len == 0) { ngx_log_error(NGX_LOG_EMERG, cf->log, 0, diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c index 368297e77..084462746 100644 --- a/src/http/modules/ngx_http_proxy_module.c +++ b/src/http/modules/ngx_http_proxy_module.c @@ -4944,6 +4944,12 @@ ngx_http_proxy_set_ssl(ngx_conf_t *cf, ngx_http_proxy_loc_conf_t *plcf) cln->handler = ngx_ssl_cleanup_ctx; cln->data = plcf->upstream.ssl; + if (ngx_ssl_ciphers(cf, plcf->upstream.ssl, &plcf->ssl_ciphers, 0) + != NGX_OK) + { + return NGX_ERROR; + } + if (plcf->upstream.ssl_certificate) { if (plcf->upstream.ssl_certificate_key == NULL) { @@ -4975,12 +4981,6 @@ ngx_http_proxy_set_ssl(ngx_conf_t *cf, ngx_http_proxy_loc_conf_t *plcf) } } - if (ngx_ssl_ciphers(cf, plcf->upstream.ssl, &plcf->ssl_ciphers, 0) - != NGX_OK) - { - return NGX_ERROR; - } - if (plcf->upstream.ssl_verify) { if (plcf->ssl_trusted_certificate.len == 0) { ngx_log_error(NGX_LOG_EMERG, cf->log, 0, diff --git a/src/http/modules/ngx_http_ssl_module.c b/src/http/modules/ngx_http_ssl_module.c index a47d6963a..1a744fff1 100644 --- a/src/http/modules/ngx_http_ssl_module.c +++ b/src/http/modules/ngx_http_ssl_module.c @@ -797,6 +797,13 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child) ngx_http_ssl_npn_advertised, NULL); #endif + if (ngx_ssl_ciphers(cf, &conf->ssl, &conf->ciphers, + conf->prefer_server_ciphers) + != NGX_OK) + { + return NGX_CONF_ERROR; + } + if (ngx_http_ssl_compile_certificates(cf, conf) != NGX_OK) { return NGX_CONF_ERROR; } @@ -829,13 +836,6 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child) } } - if (ngx_ssl_ciphers(cf, &conf->ssl, &conf->ciphers, - conf->prefer_server_ciphers) - != NGX_OK) - { - return NGX_CONF_ERROR; - } - conf->ssl.buffer_size = conf->buffer_size; if (conf->verify) { diff --git a/src/http/modules/ngx_http_uwsgi_module.c b/src/http/modules/ngx_http_uwsgi_module.c index 40a06c78e..4f9c349c2 100644 --- a/src/http/modules/ngx_http_uwsgi_module.c +++ b/src/http/modules/ngx_http_uwsgi_module.c @@ -2432,6 +2432,12 @@ ngx_http_uwsgi_set_ssl(ngx_conf_t *cf, ngx_http_uwsgi_loc_conf_t *uwcf) cln->handler = ngx_ssl_cleanup_ctx; cln->data = uwcf->upstream.ssl; + if (ngx_ssl_ciphers(cf, uwcf->upstream.ssl, &uwcf->ssl_ciphers, 0) + != NGX_OK) + { + return NGX_ERROR; + } + if (uwcf->upstream.ssl_certificate) { if (uwcf->upstream.ssl_certificate_key == NULL) { @@ -2463,12 +2469,6 @@ ngx_http_uwsgi_set_ssl(ngx_conf_t *cf, ngx_http_uwsgi_loc_conf_t *uwcf) } } - if (ngx_ssl_ciphers(cf, uwcf->upstream.ssl, &uwcf->ssl_ciphers, 0) - != NGX_OK) - { - return NGX_ERROR; - } - if (uwcf->upstream.ssl_verify) { if (uwcf->ssl_trusted_certificate.len == 0) { ngx_log_error(NGX_LOG_EMERG, cf->log, 0, -- cgit From c231640eba9e26e963460c83f2907ac6f9abf3fc Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Fri, 20 Aug 2021 03:53:56 +0300 Subject: Upstream: fixed timeouts with gRPC, SSL and select (ticket #2229). With SSL it is possible that an established connection is ready for reading after the handshake. Further, events might be already disabled in case of level-triggered event methods. If this happens and ngx_http_upstream_send_request() blocks waiting for some data from the upstream, such as flow control in case of gRPC, the connection will time out due to no read events on the upstream connection. Fix is to explicitly check the c->read->ready flag if sending request blocks and post a read event if it is set. Note that while it is possible to modify ngx_ssl_handshake() to keep read events active, this won't completely resolve the issue, since there can be data already received during the SSL handshake (see 573bd30e46b4). --- src/http/ngx_http_upstream.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src/http') diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c index 61fad5ca3..01c7877e6 100644 --- a/src/http/ngx_http_upstream.c +++ b/src/http/ngx_http_upstream.c @@ -2113,6 +2113,10 @@ ngx_http_upstream_send_request(ngx_http_request_t *r, ngx_http_upstream_t *u, c->tcp_nopush = NGX_TCP_NOPUSH_UNSET; } + if (c->read->ready) { + ngx_post_event(c->read, &ngx_posted_events); + } + return; } -- cgit From 301efb8a73b7f14e41e2238cfef50b2b98137eab Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Sun, 29 Aug 2021 22:20:34 +0300 Subject: HTTP/2: improved body reading logging. --- src/http/v2/ngx_http_v2.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'src/http') diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index 3bef002bd..ac10c2ab6 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -4154,6 +4154,9 @@ ngx_http_v2_process_request_body(ngx_http_request_t *r, u_char *pos, rb = r->request_body; buf = rb->buf; + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, fc->log, 0, + "http2 process request body"); + if (size) { if (buf->sync) { buf->pos = buf->start = pos; @@ -4364,6 +4367,9 @@ ngx_http_v2_read_unbuffered_request_body(ngx_http_request_t *r) stream = r->stream; fc = r->connection; + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, fc->log, 0, + "http2 read unbuffered request body"); + if (fc->read->timedout) { if (stream->recv_window) { stream->skip_data = 1; -- cgit From 78d9a3af91a31bb77ad74aca7286f1ef2dad89d6 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Sun, 29 Aug 2021 22:20:36 +0300 Subject: HTTP/2: reworked body reading to better match HTTP/1.x code. In particular, now the code always uses a buffer limited by client_body_buffer_size. At the cost of an additional copy it ensures that small DATA frames are not directly mapped to small write() syscalls, but rather buffered in memory before writing. Further, requests without Content-Length are no longer forced to use temporary files. --- src/http/v2/ngx_http_v2.c | 158 ++++++++++++++++++++++++++++------------------ 1 file changed, 96 insertions(+), 62 deletions(-) (limited to 'src/http') diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index ac10c2ab6..461bbff84 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -4032,11 +4032,11 @@ ngx_http_v2_read_request_body(ngx_http_request_t *r) len = r->headers_in.content_length_n; - if (r->request_body_no_buffering && !stream->in_closed) { + if (len < 0 || len > (off_t) clcf->client_body_buffer_size) { + len = clcf->client_body_buffer_size; + } - if (len < 0 || len > (off_t) clcf->client_body_buffer_size) { - len = clcf->client_body_buffer_size; - } + if (r->request_body_no_buffering && !stream->in_closed) { /* * We need a room to store data up to the stream's initial window size, @@ -4050,22 +4050,10 @@ ngx_http_v2_read_request_body(ngx_http_request_t *r) if (len > NGX_HTTP_V2_MAX_WINDOW) { len = NGX_HTTP_V2_MAX_WINDOW; } - - rb->buf = ngx_create_temp_buf(r->pool, (size_t) len); - - } else if (len >= 0 && len <= (off_t) clcf->client_body_buffer_size - && !r->request_body_in_file_only) - { - rb->buf = ngx_create_temp_buf(r->pool, (size_t) len); - - } else { - rb->buf = ngx_calloc_buf(r->pool); - - if (rb->buf != NULL) { - rb->buf->sync = 1; - } } + rb->buf = ngx_create_temp_buf(r->pool, (size_t) len); + if (rb->buf == NULL) { stream->skip_data = 1; return NGX_HTTP_INTERNAL_SERVER_ERROR; @@ -4144,7 +4132,7 @@ static ngx_int_t ngx_http_v2_process_request_body(ngx_http_request_t *r, u_char *pos, size_t size, ngx_uint_t last) { - ngx_buf_t *buf; + size_t n; ngx_int_t rc; ngx_connection_t *fc; ngx_http_request_body_t *rb; @@ -4152,80 +4140,126 @@ ngx_http_v2_process_request_body(ngx_http_request_t *r, u_char *pos, fc = r->connection; rb = r->request_body; - buf = rb->buf; ngx_log_debug0(NGX_LOG_DEBUG_HTTP, fc->log, 0, "http2 process request body"); - if (size) { - if (buf->sync) { - buf->pos = buf->start = pos; - buf->last = buf->end = pos + size; + if (size == 0 && !last) { + return NGX_OK; + } - r->request_body_in_file_only = 1; + for ( ;; ) { + for ( ;; ) { + if (rb->buf->last == rb->buf->end && size) { - } else { - if (size > (size_t) (buf->end - buf->last)) { - ngx_log_error(NGX_LOG_INFO, fc->log, 0, - "client intended to send body data " - "larger than declared"); + if (r->request_body_no_buffering) { - return NGX_HTTP_BAD_REQUEST; + /* should never happen due to flow control */ + + ngx_log_error(NGX_LOG_ALERT, fc->log, 0, + "no space in http2 body buffer"); + + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + + /* update chains */ + + ngx_log_error(NGX_LOG_DEBUG, fc->log, 0, + "http2 body update chains"); + + rc = ngx_http_v2_filter_request_body(r); + + if (rc != NGX_OK) { + return rc; + } + + if (rb->busy != NULL) { + ngx_log_error(NGX_LOG_ALERT, fc->log, 0, + "busy buffers after request body flush"); + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + + rb->buf->pos = rb->buf->start; + rb->buf->last = rb->buf->start; } - buf->last = ngx_cpymem(buf->last, pos, size); - } - } + /* copy body data to the buffer */ - if (last) { - rb->rest = 0; + n = rb->buf->end - rb->buf->last; - if (fc->read->timer_set) { - ngx_del_timer(fc->read); - } + if (n > size) { + n = size; + } - if (r->request_body_no_buffering) { - ngx_post_event(fc->read, &ngx_posted_events); - return NGX_OK; - } + rb->buf->last = ngx_cpymem(rb->buf->last, pos, n); - rc = ngx_http_v2_filter_request_body(r); + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, fc->log, 0, + "http2 request body recv %uz", n); - if (rc != NGX_OK) { - return rc; - } + pos += n; + size -= n; + + if (size == 0 && last) { + rb->rest = 0; + } + + if (r->request_body_no_buffering) { + break; + } + + /* pass buffer to request body filter chain */ + + rc = ngx_http_v2_filter_request_body(r); + + if (rc != NGX_OK) { + return rc; + } + + if (rb->rest == 0) { + break; + } - if (buf->sync) { - /* prevent reusing this buffer in the upstream module */ - rb->buf = NULL; + if (size == 0) { + break; + } } - if (r->headers_in.chunked) { - r->headers_in.content_length_n = rb->received; + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, fc->log, 0, + "http2 request body rest %O", rb->rest); + + if (rb->rest == 0) { + break; } - r->read_event_handler = ngx_http_block_reading; - rb->post_handler(r); + if (size == 0) { + clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module); + ngx_add_timer(fc->read, clcf->client_body_timeout); - return NGX_OK; - } + if (r->request_body_no_buffering) { + ngx_post_event(fc->read, &ngx_posted_events); + return NGX_OK; + } - if (size == 0) { - return NGX_OK; + return NGX_OK; + } } - clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module); - ngx_add_timer(fc->read, clcf->client_body_timeout); + if (fc->read->timer_set) { + ngx_del_timer(fc->read); + } if (r->request_body_no_buffering) { ngx_post_event(fc->read, &ngx_posted_events); return NGX_OK; } - if (buf->sync) { - return ngx_http_v2_filter_request_body(r); + if (r->headers_in.chunked) { + r->headers_in.content_length_n = rb->received; } + r->read_event_handler = ngx_http_block_reading; + rb->post_handler(r); + return NGX_OK; } -- cgit From 9f90d11cf52ccb0a8d086525aa7e4218d31d529a Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Sun, 29 Aug 2021 22:20:38 +0300 Subject: HTTP/2: improved handling of END_STREAM in a separate DATA frame. The save body filter saves the request body to disk once the buffer is full. Yet in HTTP/2 this might happen even if there is no need to save anything to disk, notably when content length is known and the END_STREAM flag is sent in a separate empty DATA frame. Workaround is to provide additional byte in the buffer, so saving the request body won't be triggered. This fixes unexpected request body disk buffering in HTTP/2 observed after the previous change when content length is known and the END_STREAM flag is sent in a separate empty DATA frame. --- src/http/v2/ngx_http_v2.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'src/http') diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index 461bbff84..a037e7a52 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -4034,6 +4034,9 @@ ngx_http_v2_read_request_body(ngx_http_request_t *r) if (len < 0 || len > (off_t) clcf->client_body_buffer_size) { len = clcf->client_body_buffer_size; + + } else { + len++; } if (r->request_body_no_buffering && !stream->in_closed) { -- cgit From 2862eb40e86190bf4d337a6483150f4385fb5358 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Sun, 29 Aug 2021 22:20:44 +0300 Subject: HTTP/2: improved handling of preread unbuffered requests. Previously, fully preread unbuffered requests larger than client body buffer size were saved to disk, despite the fact that "unbuffered" is expected to imply no disk buffering. --- src/http/v2/ngx_http_v2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/http') diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index a037e7a52..9e248758d 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -4039,7 +4039,7 @@ ngx_http_v2_read_request_body(ngx_http_request_t *r) len++; } - if (r->request_body_no_buffering && !stream->in_closed) { + if (r->request_body_no_buffering) { /* * We need a room to store data up to the stream's initial window size, -- cgit From aa02695f5ea70f6628317b56f93e7e8733d4a029 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Sun, 29 Aug 2021 22:20:49 +0300 Subject: Request body: missing comments about initialization. --- src/http/ngx_http_request_body.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/http') diff --git a/src/http/ngx_http_request_body.c b/src/http/ngx_http_request_body.c index 0cae88f77..b72614ff9 100644 --- a/src/http/ngx_http_request_body.c +++ b/src/http/ngx_http_request_body.c @@ -62,11 +62,13 @@ ngx_http_read_client_request_body(ngx_http_request_t *r, /* * set by ngx_pcalloc(): * + * rb->temp_file = NULL; * rb->bufs = NULL; * rb->buf = NULL; * rb->free = NULL; * rb->busy = NULL; * rb->chunked = NULL; + * rb->received = 0; */ rb->rest = -1; -- cgit From fd9d43b087b4247f8cb5f91845be46bd624c175a Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Sun, 29 Aug 2021 22:20:54 +0300 Subject: Request body: added alert to catch duplicate body saving. If due to an error ngx_http_request_body_save_filter() is called more than once with rb->rest == 0, this used to result in a segmentation fault. Added an alert to catch such errors, just in case. --- src/http/ngx_http_request_body.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'src/http') diff --git a/src/http/ngx_http_request_body.c b/src/http/ngx_http_request_body.c index b72614ff9..c5401fd42 100644 --- a/src/http/ngx_http_request_body.c +++ b/src/http/ngx_http_request_body.c @@ -1246,6 +1246,12 @@ ngx_http_request_body_save_filter(ngx_http_request_t *r, ngx_chain_t *in) if (rb->temp_file || r->request_body_in_file_only) { + if (rb->bufs && rb->bufs->buf->in_file) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "body already in file"); + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + if (ngx_http_write_request_body(r) != NGX_OK) { return NGX_HTTP_INTERNAL_SERVER_ERROR; } -- cgit From 2a709213800fd3fd2809881374eb110562b53c08 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Sun, 29 Aug 2021 22:21:03 +0300 Subject: Request body: introduced rb->last_saved flag. It indicates that the last buffer was received by the save filter, and can be used to check this at higher levels. To be used in the following changes. --- src/http/ngx_http_request.h | 1 + src/http/ngx_http_request_body.c | 68 +++++++++++++++++++++++++++++++--------- 2 files changed, 54 insertions(+), 15 deletions(-) (limited to 'src/http') diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h index 63576274e..896657890 100644 --- a/src/http/ngx_http_request.h +++ b/src/http/ngx_http_request.h @@ -302,6 +302,7 @@ typedef struct { ngx_chain_t *busy; ngx_http_chunked_t *chunked; ngx_http_client_body_handler_pt post_handler; + unsigned last_saved:1; } ngx_http_request_body_t; diff --git a/src/http/ngx_http_request_body.c b/src/http/ngx_http_request_body.c index c5401fd42..4a31db0bc 100644 --- a/src/http/ngx_http_request_body.c +++ b/src/http/ngx_http_request_body.c @@ -69,6 +69,7 @@ ngx_http_read_client_request_body(ngx_http_request_t *r, * rb->busy = NULL; * rb->chunked = NULL; * rb->received = 0; + * rb->last_saved = 0; */ rb->rest = -1; @@ -941,15 +942,32 @@ ngx_http_request_body_length_filter(ngx_http_request_t *r, ngx_chain_t *in) rb = r->request_body; + out = NULL; + ll = &out; + if (rb->rest == -1) { ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "http request body content length filter"); rb->rest = r->headers_in.content_length_n; - } - out = NULL; - ll = &out; + if (rb->rest == 0) { + + tl = ngx_chain_get_free_buf(r->pool, &rb->free); + if (tl == NULL) { + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + + b = tl->buf; + + ngx_memzero(b, sizeof(ngx_buf_t)); + + b->last_buf = 1; + + *ll = tl; + ll = &tl->next; + } + } for (cl = in; cl; cl = cl->next) { @@ -1013,6 +1031,9 @@ ngx_http_request_body_chunked_filter(ngx_http_request_t *r, ngx_chain_t *in) rb = r->request_body; + out = NULL; + ll = &out; + if (rb->rest == -1) { ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, @@ -1029,9 +1050,6 @@ ngx_http_request_body_chunked_filter(ngx_http_request_t *r, ngx_chain_t *in) rb->rest = cscf->large_client_header_buffers.size; } - out = NULL; - ll = &out; - for (cl = in; cl; cl = cl->next) { b = NULL; @@ -1188,15 +1206,16 @@ ngx_int_t ngx_http_request_body_save_filter(ngx_http_request_t *r, ngx_chain_t *in) { ngx_buf_t *b; - ngx_chain_t *cl; + ngx_chain_t *cl, *tl, **ll; ngx_http_request_body_t *rb; rb = r->request_body; -#if (NGX_DEBUG) + ll = &rb->bufs; -#if 0 for (cl = rb->bufs; cl; cl = cl->next) { + +#if 0 ngx_log_debug7(NGX_LOG_DEBUG_EVENT, r->connection->log, 0, "http body old buf t:%d f:%d %p, pos %p, size: %z " "file: %O, size: %O", @@ -1205,10 +1224,13 @@ ngx_http_request_body_save_filter(ngx_http_request_t *r, ngx_chain_t *in) cl->buf->last - cl->buf->pos, cl->buf->file_pos, cl->buf->file_last - cl->buf->file_pos); - } #endif + ll = &cl->next; + } + for (cl = in; cl; cl = cl->next) { + ngx_log_debug7(NGX_LOG_DEBUG_EVENT, r->connection->log, 0, "http body new buf t:%d f:%d %p, pos %p, size: %z " "file: %O, size: %O", @@ -1217,16 +1239,32 @@ ngx_http_request_body_save_filter(ngx_http_request_t *r, ngx_chain_t *in) cl->buf->last - cl->buf->pos, cl->buf->file_pos, cl->buf->file_last - cl->buf->file_pos); - } -#endif + if (cl->buf->last_buf) { - /* TODO: coalesce neighbouring buffers */ + if (rb->last_saved) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "duplicate last buf in save filter"); + *ll = NULL; + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } - if (ngx_chain_add_copy(r->pool, &rb->bufs, in) != NGX_OK) { - return NGX_HTTP_INTERNAL_SERVER_ERROR; + rb->last_saved = 1; + } + + tl = ngx_alloc_chain_link(r->pool); + if (tl == NULL) { + *ll = NULL; + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + + tl->buf = cl->buf; + *ll = tl; + ll = &tl->next; } + *ll = NULL; + if (r->request_body_no_buffering) { return NGX_OK; } -- cgit From 67d160bf25e02ba6679bb6c3b9cbdfeb29b759de Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Sun, 29 Aug 2021 22:22:02 +0300 Subject: Request body: reading body buffering in filters. If a filter wants to buffer the request body during reading (for example, to check an external scanner), it can now do so. To make it possible, the code now checks rb->last_saved (introduced in the previous change) along with rb->rest == 0. Since in HTTP/2 this requires flow control to avoid overflowing the request body buffer, so filters which need buffering have to set the rb->filter_need_buffering flag on the first filter call. (Note that each filter is expected to call the next filter, so all filters will be able set the flag if needed.) --- src/http/ngx_http_request.h | 2 + src/http/ngx_http_request_body.c | 48 +++++++++++- src/http/v2/ngx_http_v2.c | 158 +++++++++++++++++++++++++++++++++------ 3 files changed, 182 insertions(+), 26 deletions(-) (limited to 'src/http') diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h index 896657890..b1269d22d 100644 --- a/src/http/ngx_http_request.h +++ b/src/http/ngx_http_request.h @@ -302,6 +302,8 @@ typedef struct { ngx_chain_t *busy; ngx_http_chunked_t *chunked; ngx_http_client_body_handler_pt post_handler; + unsigned filter_need_buffering:1; + unsigned last_sent:1; unsigned last_saved:1; } ngx_http_request_body_t; diff --git a/src/http/ngx_http_request_body.c b/src/http/ngx_http_request_body.c index 4a31db0bc..89a4c7492 100644 --- a/src/http/ngx_http_request_body.c +++ b/src/http/ngx_http_request_body.c @@ -69,6 +69,8 @@ ngx_http_read_client_request_body(ngx_http_request_t *r, * rb->busy = NULL; * rb->chunked = NULL; * rb->received = 0; + * rb->filter_need_buffering = 0; + * rb->last_sent = 0; * rb->last_saved = 0; */ @@ -147,7 +149,7 @@ ngx_http_read_client_request_body(ngx_http_request_t *r, } } - if (rb->rest == 0) { + if (rb->rest == 0 && rb->last_saved) { /* the whole request body was pre-read */ r->request_body_no_buffering = 0; post_handler(r); @@ -175,6 +177,10 @@ ngx_http_read_client_request_body(ngx_http_request_t *r, size += preread; } + if (size == 0) { + size++; + } + } else { size = clcf->client_body_buffer_size; } @@ -273,6 +279,7 @@ ngx_http_do_read_client_request_body(ngx_http_request_t *r) size_t size; ssize_t n; ngx_int_t rc; + ngx_uint_t flush; ngx_chain_t out; ngx_connection_t *c; ngx_http_request_body_t *rb; @@ -280,12 +287,17 @@ ngx_http_do_read_client_request_body(ngx_http_request_t *r) c = r->connection; rb = r->request_body; + flush = 1; ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http read client request body"); for ( ;; ) { for ( ;; ) { + if (rb->rest == 0) { + break; + } + if (rb->buf->last == rb->buf->end) { /* update chains */ @@ -309,12 +321,25 @@ ngx_http_do_read_client_request_body(ngx_http_request_t *r) return NGX_AGAIN; } + if (rb->filter_need_buffering) { + clcf = ngx_http_get_module_loc_conf(r, + ngx_http_core_module); + ngx_add_timer(c->read, clcf->client_body_timeout); + + if (ngx_handle_read_event(c->read, 0) != NGX_OK) { + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + + return NGX_AGAIN; + } + ngx_log_error(NGX_LOG_ALERT, c->log, 0, "busy buffers after request body flush"); return NGX_HTTP_INTERNAL_SERVER_ERROR; } + flush = 0; rb->buf->pos = rb->buf->start; rb->buf->last = rb->buf->start; } @@ -326,6 +351,10 @@ ngx_http_do_read_client_request_body(ngx_http_request_t *r) size = (size_t) rest; } + if (size == 0) { + break; + } + n = c->recv(c, rb->buf->last, size); ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, 0, @@ -350,6 +379,7 @@ ngx_http_do_read_client_request_body(ngx_http_request_t *r) /* pass buffer to request body filter chain */ + flush = 0; out.buf = rb->buf; out.next = NULL; @@ -371,11 +401,19 @@ ngx_http_do_read_client_request_body(ngx_http_request_t *r) ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, 0, "http client request body rest %O", rb->rest); - if (rb->rest == 0) { + if (flush) { + rc = ngx_http_request_body_filter(r, NULL); + + if (rc != NGX_OK) { + return rc; + } + } + + if (rb->rest == 0 && rb->last_saved) { break; } - if (!c->read->ready) { + if (!c->read->ready || rb->rest == 0) { clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module); ngx_add_timer(c->read, clcf->client_body_timeout); @@ -1280,7 +1318,9 @@ ngx_http_request_body_save_filter(ngx_http_request_t *r, ngx_chain_t *in) return NGX_OK; } - /* rb->rest == 0 */ + if (!rb->last_saved) { + return NGX_OK; + } if (rb->temp_file || r->request_body_in_file_only) { diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index 9e248758d..5ccb36360 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -173,7 +173,7 @@ static ngx_int_t ngx_http_v2_construct_cookie_header(ngx_http_request_t *r); static void ngx_http_v2_run_request(ngx_http_request_t *r); static void ngx_http_v2_run_request_handler(ngx_event_t *ev); static ngx_int_t ngx_http_v2_process_request_body(ngx_http_request_t *r, - u_char *pos, size_t size, ngx_uint_t last); + u_char *pos, size_t size, ngx_uint_t last, ngx_uint_t flush); static ngx_int_t ngx_http_v2_filter_request_body(ngx_http_request_t *r); static void ngx_http_v2_read_client_request_body_handler(ngx_http_request_t *r); @@ -1092,7 +1092,7 @@ static u_char * ngx_http_v2_state_read_data(ngx_http_v2_connection_t *h2c, u_char *pos, u_char *end) { - size_t size; + size_t size, window; ngx_buf_t *buf; ngx_int_t rc; ngx_connection_t *fc; @@ -1140,13 +1140,40 @@ ngx_http_v2_state_read_data(ngx_http_v2_connection_t *h2c, u_char *pos, h2c->payload_bytes += size; if (r->request_body) { - rc = ngx_http_v2_process_request_body(r, pos, size, stream->in_closed); + rc = ngx_http_v2_process_request_body(r, pos, size, + stream->in_closed, 0); - if (rc != NGX_OK) { + if (rc != NGX_OK && rc != NGX_AGAIN) { stream->skip_data = 1; ngx_http_finalize_request(r, rc); } + if (rc == NGX_AGAIN && !stream->no_flow_control) { + buf = r->request_body->buf; + window = buf->end - buf->last; + + window -= h2c->state.length - size; + + if (window < stream->recv_window) { + ngx_log_error(NGX_LOG_ALERT, h2c->connection->log, 0, + "http2 negative window update"); + return ngx_http_v2_connection_error(h2c, + NGX_HTTP_V2_INTERNAL_ERROR); + } + + if (window > stream->recv_window) { + if (ngx_http_v2_send_window_update(h2c, stream->node->id, + window - stream->recv_window) + == NGX_ERROR) + { + return ngx_http_v2_connection_error(h2c, + NGX_HTTP_V2_INTERNAL_ERROR); + } + + stream->recv_window = window; + } + } + ngx_http_run_posted_requests(fc); } else if (size) { @@ -4027,6 +4054,17 @@ ngx_http_v2_read_request_body(ngx_http_request_t *r) return NGX_OK; } + rb->rest = 1; + + /* set rb->filter_need_buffering */ + + rc = ngx_http_top_request_body_filter(r, NULL); + + if (rc != NGX_OK) { + stream->skip_data = 1; + return rc; + } + h2scf = ngx_http_get_module_srv_conf(r, ngx_http_v2_module); clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module); @@ -4039,7 +4077,7 @@ ngx_http_v2_read_request_body(ngx_http_request_t *r) len++; } - if (r->request_body_no_buffering) { + if (r->request_body_no_buffering || rb->filter_need_buffering) { /* * We need a room to store data up to the stream's initial window size, @@ -4062,36 +4100,45 @@ ngx_http_v2_read_request_body(ngx_http_request_t *r) return NGX_HTTP_INTERNAL_SERVER_ERROR; } - rb->rest = 1; - buf = stream->preread; if (stream->in_closed) { - r->request_body_no_buffering = 0; + if (!rb->filter_need_buffering) { + r->request_body_no_buffering = 0; + } if (buf) { rc = ngx_http_v2_process_request_body(r, buf->pos, - buf->last - buf->pos, 1); + buf->last - buf->pos, 1, 0); ngx_pfree(r->pool, buf->start); + + } else { + rc = ngx_http_v2_process_request_body(r, NULL, 0, 1, 0); + } + + if (rc != NGX_AGAIN) { return rc; } - return ngx_http_v2_process_request_body(r, NULL, 0, 1); + r->read_event_handler = ngx_http_v2_read_client_request_body_handler; + r->write_event_handler = ngx_http_request_empty_handler; + + return NGX_AGAIN; } if (buf) { rc = ngx_http_v2_process_request_body(r, buf->pos, - buf->last - buf->pos, 0); + buf->last - buf->pos, 0, 0); ngx_pfree(r->pool, buf->start); - if (rc != NGX_OK) { + if (rc != NGX_OK && rc != NGX_AGAIN) { stream->skip_data = 1; return rc; } } - if (r->request_body_no_buffering) { + if (r->request_body_no_buffering || rb->filter_need_buffering) { size = (size_t) len - h2scf->preread_size; } else { @@ -4133,7 +4180,7 @@ ngx_http_v2_read_request_body(ngx_http_request_t *r) static ngx_int_t ngx_http_v2_process_request_body(ngx_http_request_t *r, u_char *pos, - size_t size, ngx_uint_t last) + size_t size, ngx_uint_t last, ngx_uint_t flush) { size_t n; ngx_int_t rc; @@ -4147,8 +4194,8 @@ ngx_http_v2_process_request_body(ngx_http_request_t *r, u_char *pos, ngx_log_debug0(NGX_LOG_DEBUG_HTTP, fc->log, 0, "http2 process request body"); - if (size == 0 && !last) { - return NGX_OK; + if (size == 0 && !last && !flush) { + return NGX_AGAIN; } for ( ;; ) { @@ -4230,7 +4277,7 @@ ngx_http_v2_process_request_body(ngx_http_request_t *r, u_char *pos, ngx_log_debug1(NGX_LOG_DEBUG_HTTP, fc->log, 0, "http2 request body rest %O", rb->rest); - if (rb->rest == 0) { + if (rb->rest == 0 && rb->last_saved) { break; } @@ -4240,10 +4287,10 @@ ngx_http_v2_process_request_body(ngx_http_request_t *r, u_char *pos, if (r->request_body_no_buffering) { ngx_post_event(fc->read, &ngx_posted_events); - return NGX_OK; + return NGX_AGAIN; } - return NGX_OK; + return NGX_AGAIN; } } @@ -4279,7 +4326,7 @@ ngx_http_v2_filter_request_body(ngx_http_request_t *r) rb = r->request_body; buf = rb->buf; - if (buf->pos == buf->last && rb->rest) { + if (buf->pos == buf->last && (rb->rest || rb->last_sent)) { cl = NULL; goto update; } @@ -4342,6 +4389,7 @@ ngx_http_v2_filter_request_body(ngx_http_request_t *r) } b->last_buf = 1; + rb->last_sent = 1; } b->tag = (ngx_buf_tag_t) &ngx_http_v2_filter_request_body; @@ -4361,7 +4409,12 @@ update: static void ngx_http_v2_read_client_request_body_handler(ngx_http_request_t *r) { - ngx_connection_t *fc; + size_t window; + ngx_buf_t *buf; + ngx_int_t rc; + ngx_connection_t *fc; + ngx_http_v2_stream_t *stream; + ngx_http_v2_connection_t *h2c; fc = r->connection; @@ -4387,6 +4440,63 @@ ngx_http_v2_read_client_request_body_handler(ngx_http_request_t *r) ngx_http_finalize_request(r, NGX_HTTP_CLIENT_CLOSED_REQUEST); return; } + + rc = ngx_http_v2_process_request_body(r, NULL, 0, r->stream->in_closed, 1); + + if (rc != NGX_OK && rc != NGX_AGAIN) { + r->stream->skip_data = 1; + ngx_http_finalize_request(r, rc); + return; + } + + if (rc == NGX_OK) { + return; + } + + if (r->request_body->rest == 0) { + return; + } + + stream = r->stream; + h2c = stream->connection; + + buf = r->request_body->buf; + window = buf->end - buf->start; + + if (h2c->state.stream == stream) { + window -= h2c->state.length; + } + + if (window <= stream->recv_window) { + if (window < stream->recv_window) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "http2 negative window update"); + + stream->skip_data = 1; + + ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR); + return; + } + + return; + } + + if (ngx_http_v2_send_window_update(h2c, stream->node->id, + window - stream->recv_window) + == NGX_ERROR) + { + stream->skip_data = 1; + ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR); + return; + } + + stream->recv_window = window; + + if (ngx_http_v2_send_output_queue(h2c) == NGX_ERROR) { + stream->skip_data = 1; + ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR); + return; + } } @@ -4430,10 +4540,14 @@ ngx_http_v2_read_unbuffered_request_body(ngx_http_request_t *r) return rc; } - if (!r->request_body->rest) { + if (r->request_body->rest == 0 && r->request_body->last_saved) { return NGX_OK; } + if (r->request_body->rest == 0) { + return NGX_AGAIN; + } + if (r->request_body->busy != NULL) { return NGX_AGAIN; } -- cgit From 15bf6d8cc950d3aec2d1c3152b39f62be4939025 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Tue, 31 Aug 2021 16:44:13 +0300 Subject: HTTP/2: avoid memcpy() with NULL source and zero length. Prodded by Clang Static Analyzer. --- src/http/v2/ngx_http_v2.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src/http') diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c index 5ccb36360..79c4f17c2 100644 --- a/src/http/v2/ngx_http_v2.c +++ b/src/http/v2/ngx_http_v2.c @@ -4241,7 +4241,9 @@ ngx_http_v2_process_request_body(ngx_http_request_t *r, u_char *pos, n = size; } - rb->buf->last = ngx_cpymem(rb->buf->last, pos, n); + if (n > 0) { + rb->buf->last = ngx_cpymem(rb->buf->last, pos, n); + } ngx_log_debug1(NGX_LOG_DEBUG_HTTP, fc->log, 0, "http2 request body recv %uz", n); -- cgit