[PATCH 4 of 4] HTTP/2: reject HTTP/2 requests with connection-specific headers

[Maxim Dounin]

Hello!

On Tue, Jun 13, 2017 at 05:19:48AM -0700, Piotr Sikora via nginx-devel wrote:

> # HG changeset patch
> # User Piotr Sikora <piotrsikora at google.com>
> # Date 1490516709 25200
> #      Sun Mar 26 01:25:09 2017 -0700
> # Node ID e2abc3bc3fc12b788d2631d3c47215acdc4ebbe6
> # Parent  6263d68cb96042d8f8974a4a3945226227ce13b9
> HTTP/2: reject HTTP/2 requests with connection-specific headers.
> 
> Signed-off-by: Piotr Sikora <piotrsikora at google.com>
> 
> diff -r 6263d68cb960 -r e2abc3bc3fc1 src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -19,6 +19,8 @@ static ngx_int_t ngx_http_alloc_large_he
>  
>  static ngx_int_t ngx_http_process_header_line(ngx_http_request_t *r,
>      ngx_table_elt_t *h, ngx_uint_t offset);
> +static ngx_int_t ngx_http_process_http1_header_line(ngx_http_request_t *r,
> +    ngx_table_elt_t *h, ngx_uint_t offset);
>  static ngx_int_t ngx_http_process_unique_header_line(ngx_http_request_t *r,
>      ngx_table_elt_t *h, ngx_uint_t offset);
>  static ngx_int_t ngx_http_process_multi_header_lines(ngx_http_request_t *r,
> @@ -146,7 +148,7 @@ ngx_http_header_t  ngx_http_headers_in[]
>  
>      { ngx_string("Upgrade"),
>                   offsetof(ngx_http_headers_in_t, upgrade),
> -                 ngx_http_process_header_line },
> +                 ngx_http_process_http1_header_line },
>  
>  #if (NGX_HTTP_GZIP)
>      { ngx_string("Accept-Encoding"),
> @@ -161,8 +163,13 @@ ngx_http_header_t  ngx_http_headers_in[]
>                   offsetof(ngx_http_headers_in_t, authorization),
>                   ngx_http_process_unique_header_line },
>  
> -    { ngx_string("Keep-Alive"), offsetof(ngx_http_headers_in_t, keep_alive),
> -                 ngx_http_process_header_line },
> +    { ngx_string("Keep-Alive"),
> +                 offsetof(ngx_http_headers_in_t, keep_alive),
> +                 ngx_http_process_http1_header_line },
> +
> +    { ngx_string("Proxy-Connection"),
> +                 offsetof(ngx_http_headers_in_t, proxy_connection),
> +                 ngx_http_process_http1_header_line },

I'm highly sceptical about the whole series in general, and this 
patch specifically.

In particular, the "Proxy-Connection" header is not something even 
defined by any standard, and even in its non-standard [broken] 
meaning never expected to be used in connections to nginx.  Not to 
mention that Proxy-Authorization, a standard-defined hop-by-hop 
(connection-specific in terms of HTTP/2) header, is not checked 
anywhere.

Additionally, I really think that disabling upgrades is one of the 
big mistakes of HTTP/2.  It would be much more logical to 
interpret a HTTP/2 stream as a connection to upgrade, and allow to 
multiplex arbitrary protocols via a single HTTP/2 connection.

Unless there are practical reasons for these changes, I would 
rather reject the series.

-- 
Maxim Dounin
http://nginx.org/