[patch] reject http header without colon (:) in the header name

[Maxim Dounin]

Hello!

On Wed, May 08, 2024 at 10:59:30AM -0700, Maksim Yevmenkin wrote:

> > > it appears that nginx would happily accept http header without colon
> > > (:) in the header name. the patch below tries to address this.
> >
> > Yes.  I believe it was allowed as a part of the following change
> > in nginx 0.1.29 (509:9b8c906f6e63,
> > https://freenginx.org/hg/nginx/rev/9b8c906f6e63#l85.68):
> >
> >     *) Change: nginx now passes the invalid lines in a client request
> >        headers or a backend response header.
> >
> > With introduction of "ignore_invalid_headers" in nginx 0.1.30
> > (511:c12967aadd87) this wasn't changed though, which is probably
> > an oversight.
> >
> > Such headers are essentially recognized as headers with an
> > empty value, and I don't think that this can be an issue.  Still,
> > I don't object hardening the parser and rejecting such headers.
> >
> > I don't think this change will affect any real clients, though
> > potentially might affect some misbehaving backend code, such as
> > loosely written fastcgi scripts.  If its the case, such code needs
> > to be fixed.
> >
> > Alternatively, we can consider marking such headers as invalid, so
> > they will be ignored from clients (unless "ignore_invalid_headers off;"
> > is specified) and passed from backend servers to clients.  Not
> > sure it worth the effort though.
> 
> [...]
> 
> > Removing relevant if blocks should be enough, these will be
> > handled by the following "if (ch <= 0x20...)" block.  Please take
> > a look at the following patch:
> >
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1715122480 -10800
> > #      Wed May 08 01:54:40 2024 +0300
> > # Node ID e9aa91a1861dcabfd1e9ebb7d5d96cdb42c8dcd4
> > # Parent  93bbb9fbf30dd82709551610f05e22eac17717d4
> > Disabled handling of headers without a colon.
> >
> > Starting with nginx 0.1.29 (509:9b8c906f6e63), header names not followed
> > by a colon and a value were allowed.  Such headers were interpreted as
> > headers with an empty value.  With this change, such headers are
> > unconditionally rejected.
> >
> > Requested by Maksim Yevmenkin.
> >
> > diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c
> > --- a/src/http/ngx_http_parse.c
> > +++ b/src/http/ngx_http_parse.c
> > @@ -961,21 +961,6 @@ ngx_http_parse_header_line(ngx_http_requ
> >                  break;
> >              }
> >
> > -            if (ch == CR) {
> > -                r->header_name_end = p;
> > -                r->header_start = p;
> > -                r->header_end = p;
> > -                state = sw_almost_done;
> > -                break;
> > -            }
> > -
> > -            if (ch == LF) {
> > -                r->header_name_end = p;
> > -                r->header_start = p;
> > -                r->header_end = p;
> > -                goto done;
> > -            }
> > -
> >              /* IIS may send the duplicate "HTTP/1.1 ..." lines */
> >              if (ch == '/'
> >                  && r->upstream
> 
> this patch works.
> 
> !thanks
> max

Committed, thanks for prodding this.

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