[nginx] Request body: body is now cleared on errors.

[Maxim Dounin]

Hello!

On Mon, Aug 26, 2024 at 12:14:48PM +0200, Jiří Setnička via nginx-devel wrote:

> Hello,
> 
> I believe that I encountered segfault caused by this change:
> > details:   http://freenginx.org/hg/nginx/rev/81082b5521dd
> > branches:
> > changeset: 9259:81082b5521dd
> > user:      Maxim Dounin <mdounin at mdounin.ru>
> > date:      Sat Apr 27 18:21:38 2024 +0300
> > description:
> > Request body: body is now cleared on errors.
> > 
> > Previously, after errors the request body was left in a potentially
> > inconsistent state, with r->headers_in.content_length_n which might be
> > larger than buffers actually stored in r->request_body->bufs (or not
> > set at all, in case of HTTP/2 and HTTP/3).  This can cause issues if
> > the request body is subsequently used during error_page handling, such
> > as when proxying.
> > 
> > Fix is to clear r->request_body->bufs if this happens, and set
> > r->headers_in.content_length_n to 0, much like it happens when
> > ngx_http_discard_request_body() is called when returning 413 from
> > ngx_http_core_find_config_phase() for requests with Content-Length.
> > 
> > ...
> > 
> > diff --git a/src/http/ngx_http_request_body.c b/src/http/ngx_http_request_body.c
> > --- a/src/http/ngx_http_request_body.c
> > +++ b/src/http/ngx_http_request_body.c
> > @@ -228,6 +228,11 @@ done:
> >       }
> >       if (rc >= NGX_HTTP_SPECIAL_RESPONSE) {
> > +
> > +        r->lingering_close = 1;
> > +        r->headers_in.content_length_n = 0;
> > +        r->request_body->bufs = NULL;
> > +
> >           r->main->count--;
> >           r->read_event_handler = ngx_http_block_reading;
> >       }
> 
> The r->request_body may not be set here. It could happen at the top of the
> ngx_http_read_client_request_body function, when either ngx_http_test_expect
> or ngx_pcalloc fails and the goto done is executed before setting
> r->request_body.
> 
> I encountered this with clients that sends Expect: 100-continue but
> immediately closes the connection.
> 
> The fix should be straightforward, just to check for existence of the
> r->request_body.

Thanks for reporting this.

Indeed, clearly there is a bug (and I'm able to reproduce it with 
"Expect: 100-continue").

Here is a fix:

# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1724686663 -10800
#      Mon Aug 26 18:37:43 2024 +0300
# Node ID a507fb4679a4578077d301c723855b5a6e54d4eb
# Parent  d6f75dd66761c10d4bfb257ae70a212411b6a69b
Request body: fixed segfault on early errors.

The r->request_body might not be initialized on error handling in
ngx_http_read_client_request_body(), notably if ngx_http_test_expect()
or ngx_pcalloc() fail.  After introduction of request body clearing
in 9259:81082b5521dd (1.27.0), this caused segmentation fault due to
NULL pointer dereference when clearing r->request_body->bufs.

Fix is to explicitly check if r->request_body is available before
clearing r->request_body->bufs.

Reported by Jiří Setnička,
http://freenginx.org/pipermail/nginx-devel/2024-August/000484.html

diff --git a/src/http/ngx_http_request_body.c b/src/http/ngx_http_request_body.c
--- a/src/http/ngx_http_request_body.c
+++ b/src/http/ngx_http_request_body.c
@@ -245,7 +245,10 @@ done:
 
         r->lingering_close = 1;
         r->discard_body = 1;
-        r->request_body->bufs = NULL;
+
+        if (r->request_body) {
+            r->request_body->bufs = NULL;
+        }
 
         r->main->count--;
         r->read_event_handler = ngx_http_block_reading;

-- 
Maxim Dounin
http://mdounin.ru/