Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#1384 closed defect (fixed)

request body may be corrupted when content-length is not set in headers using http2

Reported by: hsiven@… Owned by:
Priority: minor Milestone:
Component: nginx-module Version: 1.13.x
Keywords: http2 request body corrupted content-length Cc:
uname -a: Linux 3.10.106-1-0044 #1 SMP Mon Jul 17 15:20:12 CST 2017 x86_64 x86_64 x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.13.5

Description

Hi,
Recently I use nginx as proxy server with a problem. If a client send a post request without Content-Length set in the request header using http2(proxy_request_buffering is on), the request body may be corrupted when nginx send request body to upstream in ngx_http_upstream_send_request_body. If content-length is set in the header, the request body will be ok. In addition, it will be ok if proxy_request_buffering is off.
I want to find the answer in the code. In ngx_http_v2_read_request_body, if Content-Length is not set in the headers, r->headers_in.content_length_n == -1, will enter below branch.

} else {

if (stream->preread) {

/* enforce writing preread buffer to file */
r->request_body_in_file_only = 1;

}

rb->buf = ngx_calloc_buf(r->pool);

if (rb->buf != NULL) {

rb->buf->sync = 1;

}

}

As rb->buf->sync is set with 1, in ngx_http_v2_process_request_body, will not copy request body from recv_buffer of http2 module to buf->last.

if (size) {

if (buf->sync) {

buf->pos = buf->start = pos;
buf->last = buf->end = pos + 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");

return NGX_HTTP_BAD_REQUEST;

}

buf->last = ngx_cpymem(buf->last, pos, size);

}

}

But recv_buffer is shared memory for all http2 request, the request body may be corrputed when call ngx_http_upstream_send_request_body.
I also try to find whether it is necessary in the rfc7540. My understanding is that, Content-Length in the header is optional. If Conntent-Length is not set, nginx should read request body from DATA frame. I saw a description like this in the rfc7540:

An HTTP POST request that includes request header fields and payload data is transmitted as one HEADERS frame, followed by zero or more CONTINUATION frames containing the request header fields, followed by one or more DATA frames, with the last CONTINUATION (or HEADERS) frame having the END_HEADERS flag set and the final DATA frame having the END_STREAM flag set.

So, in my opinion nginx should handle the request body corretly even if the Content-Length is not set. Am I right?

Change History (6)

comment:1 Changed 2 years ago by pluknet

  • Status changed from new to accepted

comment:2 Changed 2 years ago by vbart

Fixed by b6dc472299da (will be part of nginx 1.13.6).

comment:3 Changed 2 years ago by vbart

  • Resolution set to fixed
  • Status changed from accepted to closed

comment:4 Changed 2 years ago by Valentin Bartenev <vbart@…>

In 7118:b6dc472299da/nginx:

HTTP/2: enforce writing the sync request body buffer to file.

The sync flag of HTTP/2 request body buffer is used when the size of request
body is unknown or bigger than configured "client_body_buffer_size". In this
case the buffer points to body data inside the global receive buffer that is
used for reading all HTTP/2 connections in the worker process. Thus, when the
sync flag is set, the buffer must be flushed to a temporary file, otherwise
the request body data can be overwritten.

Previously, the sync buffer wasn't flushed to a temporary file if the whole
body was received in one DATA frame with the END_STREAM flag and wasn't
copied into the HTTP/2 body preread buffer. As a result, the request body
might be corrupted (ticket #1384).

Now, setting r->request_body_in_file_only enforces writing the sync buffer
to a temporary file in all cases.

comment:5 Changed 2 years ago by Sergey Kandaurov <pluknet@…>

In 1226:124322e9accd/nginx-tests:

Tests: HTTP/2 request body sync buffer test added (ticket #1384).

comment:6 Changed 2 years ago by Valentin Bartenev <vbart@…>

In 7143:e532d397ca5e/nginx:

HTTP/2: enforce writing the sync request body buffer to file.

The sync flag of HTTP/2 request body buffer is used when the size of request
body is unknown or bigger than configured "client_body_buffer_size". In this
case the buffer points to body data inside the global receive buffer that is
used for reading all HTTP/2 connections in the worker process. Thus, when the
sync flag is set, the buffer must be flushed to a temporary file, otherwise
the request body data can be overwritten.

Previously, the sync buffer wasn't flushed to a temporary file if the whole
body was received in one DATA frame with the END_STREAM flag and wasn't
copied into the HTTP/2 body preread buffer. As a result, the request body
might be corrupted (ticket #1384).

Now, setting r->request_body_in_file_only enforces writing the sync buffer
to a temporary file in all cases.

Note: See TracTickets for help on using tickets.