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

[Iven Xu]

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?

[Valentin V. Bartenev]

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

[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.

[Sergey Kandaurov <pluknet@…>]

In 1226:124322e9accd/nginx-tests:

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

[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.

[Maxim Dounin]

See also #1973.