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

[Maxim Dounin]

Hello!

(It looks like you aren't subscribed to the mailing list, hence 
Cc.)

On Tue, May 07, 2024 at 02:33:21PM -0700, Maksim Yevmenkin wrote:

> hello,
> 
> 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.

> --- a/ports/netflix/nginx/files/nginx/src/http/ngx_http_parse.c
> +++ b/ports/netflix/nginx/files/nginx/src/http/ngx_http_parse.c
> @@ -941,14 +941,14 @@ ngx_http_parse_header_line(ngx_http_request_t
> *r, ngx_buf_t *b,
>                  r->header_start = p;
>                  r->header_end = p;
>                  state = sw_almost_done;
> -                break;
> +                return NGX_HTTP_PARSE_INVALID_HEADER;
>              }
> 
>              if (ch == LF) {
>                  r->header_name_end = p;
>                  r->header_start = p;
>                  r->header_end = p;
> -                goto done;
> +                return NGX_HTTP_PARSE_INVALID_HEADER;
>              }
> 
>              /* IIS may send the duplicate "HTTP/1.1 ..." lines */

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

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