[PATCH 15 of 20] Upstream: header handlers can now return parsing errors

Maxim Dounin mdounin at mdounin.ru
Fri May 13 00:24:25 UTC 2022


Hello!

On Thu, May 12, 2022 at 12:26:37AM +0400, Sergey Kandaurov wrote:

> On Thu, Apr 21, 2022 at 01:18:55AM +0300, Maxim Dounin wrote:
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1650492336 -10800
> > #      Thu Apr 21 01:05:36 2022 +0300
> > # Node ID ab424b5e32405aeec54ccdfe38e9408209209e0a
> > # Parent  b110c54778e8f6af3ea402c0838a4f289dcd813e
> > Upstream: header handlers can now return parsing errors.
> > 
> > With this change, duplicate Content-Length and Transfer-Encoding headers
> > are now rejected.  Further, responses with invalid Content-Length or
> > Transfer-Encoding headers are now rejected, as well as responses with both
> > Content-Length and Transfer-Encoding.
> 
> jftr, various 3rd party modules that call header handlers:
> mogilefs, passenger, ajp_module, nginx-clojure, srcache

This should degrade nicely to a generic NGX_ERROR.  Slightly less 
user-friendly, though shouldn't be a big issue even if duplicate 
headers are returned.

> 
> > 
> > diff --git a/src/http/modules/ngx_http_fastcgi_module.c b/src/http/modules/ngx_http_fastcgi_module.c
> > --- a/src/http/modules/ngx_http_fastcgi_module.c
> > +++ b/src/http/modules/ngx_http_fastcgi_module.c
> > @@ -2007,8 +2007,12 @@ ngx_http_fastcgi_process_header(ngx_http
> >                  hh = ngx_hash_find(&umcf->headers_in_hash, h->hash,
> >                                     h->lowcase_key, h->key.len);
> >  
> > -                if (hh && hh->handler(r, h, hh->offset) != NGX_OK) {
> > -                    return NGX_ERROR;
> > +                if (hh) {
> > +                    rc = hh->handler(r, h, hh->offset);
> > +
> > +                    if (rc != NGX_OK) {
> > +                        return rc;
> > +                    }
> >                  }
> >  
> >                  ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> > diff --git a/src/http/modules/ngx_http_grpc_module.c b/src/http/modules/ngx_http_grpc_module.c
> > --- a/src/http/modules/ngx_http_grpc_module.c
> > +++ b/src/http/modules/ngx_http_grpc_module.c
> > @@ -1891,8 +1891,12 @@ ngx_http_grpc_process_header(ngx_http_re
> >                  hh = ngx_hash_find(&umcf->headers_in_hash, h->hash,
> >                                     h->lowcase_key, h->key.len);
> >  
> > -                if (hh && hh->handler(r, h, hh->offset) != NGX_OK) {
> > -                    return NGX_ERROR;
> > +                if (hh) {
> > +                    rc = hh->handler(r, h, hh->offset);
> > +
> > +                    if (rc != NGX_OK) {
> > +                        return rc;
> > +                    }
> >                  }
> >  
> >                  continue;
> > diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c
> > --- a/src/http/modules/ngx_http_proxy_module.c
> > +++ b/src/http/modules/ngx_http_proxy_module.c
> > @@ -1930,8 +1930,12 @@ ngx_http_proxy_process_header(ngx_http_r
> >              hh = ngx_hash_find(&umcf->headers_in_hash, h->hash,
> >                                 h->lowcase_key, h->key.len);
> >  
> > -            if (hh && hh->handler(r, h, hh->offset) != NGX_OK) {
> > -                return NGX_ERROR;
> > +            if (hh) {
> > +                rc = hh->handler(r, h, hh->offset);
> > +
> > +                if (rc != NGX_OK) {
> > +                    return rc;
> > +                }
> >              }
> >  
> >              ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> > diff --git a/src/http/modules/ngx_http_scgi_module.c b/src/http/modules/ngx_http_scgi_module.c
> > --- a/src/http/modules/ngx_http_scgi_module.c
> > +++ b/src/http/modules/ngx_http_scgi_module.c
> > @@ -1114,8 +1114,12 @@ ngx_http_scgi_process_header(ngx_http_re
> >              hh = ngx_hash_find(&umcf->headers_in_hash, h->hash,
> >                                 h->lowcase_key, h->key.len);
> >  
> > -            if (hh && hh->handler(r, h, hh->offset) != NGX_OK) {
> > -                return NGX_ERROR;
> > +            if (hh) {
> > +                rc = hh->handler(r, h, hh->offset);
> > +
> > +                if (rc != NGX_OK) {
> > +                    return rc;
> > +                }
> >              }
> >  
> >              ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> > diff --git a/src/http/modules/ngx_http_uwsgi_module.c b/src/http/modules/ngx_http_uwsgi_module.c
> > --- a/src/http/modules/ngx_http_uwsgi_module.c
> > +++ b/src/http/modules/ngx_http_uwsgi_module.c
> > @@ -1340,8 +1340,12 @@ ngx_http_uwsgi_process_header(ngx_http_r
> >              hh = ngx_hash_find(&umcf->headers_in_hash, h->hash,
> >                                 h->lowcase_key, h->key.len);
> >  
> > -            if (hh && hh->handler(r, h, hh->offset) != NGX_OK) {
> > -                return NGX_ERROR;
> > +            if (hh) {
> > +                rc = hh->handler(r, h, hh->offset);
> > +
> > +                if (rc != NGX_OK) {
> > +                    return rc;
> > +                }
> >              }
> >  
> >              ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> > diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> > --- a/src/http/ngx_http_upstream.c
> > +++ b/src/http/ngx_http_upstream.c
> > @@ -4633,10 +4633,34 @@ ngx_http_upstream_process_content_length
> >  
> >      u = r->upstream;
> >  
> > +    if (u->headers_in.content_length) {
> > +        ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> > +                      "upstream sent duplicate header line: \"%V: %V\", "
> > +                      "previous value: \"%V: %V\"",
> > +                      &h->key, &h->value,
> > +                      &u->headers_in.content_length->key,
> > +                      &u->headers_in.content_length->value);
> > +        return NGX_HTTP_UPSTREAM_INVALID_HEADER;
> > +    }
> > +
> > +    if (u->headers_in.transfer_encoding) {
> > +        ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> > +                      "upstream sent \"Content-Length\" and "
> > +                      "\"Transfer-Encoding\" headers at the same time");
> > +        return NGX_HTTP_UPSTREAM_INVALID_HEADER;
> > +    }
> > +
> >      h->next = NULL;
> >      u->headers_in.content_length = h;
> >      u->headers_in.content_length_n = ngx_atoof(h->value.data, h->value.len);
> >  
> > +    if (u->headers_in.content_length_n == NGX_ERROR) {
> > +        ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> > +                      "upstream sent invalid \"Content-Length\" header: "
> > +                      "\"%V: %V\"", &h->key, &h->value);
> > +        return NGX_HTTP_UPSTREAM_INVALID_HEADER;
> > +    }
> > +
> >      return NGX_OK;
> >  }
> >  
> > @@ -5021,14 +5045,37 @@ ngx_http_upstream_process_transfer_encod
> >      ngx_http_upstream_t  *u;
> >  
> >      u = r->upstream;
> > +
> > +    if (u->headers_in.transfer_encoding) {
> > +        ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> > +                      "upstream sent duplicate header line: \"%V: %V\", "
> > +                      "previous value: \"%V: %V\"",
> > +                      &h->key, &h->value,
> > +                      &u->headers_in.transfer_encoding->key,
> > +                      &u->headers_in.transfer_encoding->value);
> > +        return NGX_HTTP_UPSTREAM_INVALID_HEADER;
> > +    }
> > +
> > +    if (u->headers_in.content_length) {
> > +        ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> > +                      "upstream sent \"Content-Length\" and "
> > +                      "\"Transfer-Encoding\" headers at the same time");
> > +        return NGX_HTTP_UPSTREAM_INVALID_HEADER;
> > +    }
> > +
> >      u->headers_in.transfer_encoding = h;
> >      h->next = NULL;
> >  
> > -    if (ngx_strlcasestrn(h->value.data, h->value.data + h->value.len,
> > -                         (u_char *) "chunked", 7 - 1)
> > -        != NULL)
> > +    if (h->value.len == 7
> > +        && ngx_strncasecmp(h->value.data, (u_char *) "chunked", 7) == 0)
> >      {
> >          u->headers_in.chunked = 1;
> > +
> > +    } else {
> > +        ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> > +                      "upstream sent unknown \"Transfer-Encoding\": \"%V\"",
> > +                      &h->value);
> > +        return NGX_HTTP_UPSTREAM_INVALID_HEADER;
> >      }
> >  
> >      return NGX_OK;
> 
> This is a subtle(?) change, which makes "chunked" the only valid header value.
> OTOH, it looks fine since other values require explicit support as well.

Note "responses with invalid ... Transfer-Encoding headers are 
now rejected" in the commit log.

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



More information about the nginx-devel mailing list