Prevent buffer overrun on NGX_HTTP_REQUEST_HEADER_TOO_LARGE

[Maxim Dounin]

Hello!

On Tue, Oct 07, 2014 at 05:23:57PM +0000, Bondarev, Daniil wrote:

> Hi,
> 
> There is an issue in nginx, when it's returning NGX_HTTP_REQUEST_HEADER_TOO_LARGE
> in ngx_http_process_request_headers:
> When large header buffer is full and the last header size is 1 or 2 bytes more
> than NGX_MAX_ERROR_STR - 300, nginx will write 1 or 2 '.' symbols out of large
> header buffer, causing unpredictable behavior.
> 
> To reproduce it you can send request with total headers size of 'large client buffers size' and
> the last header size of 1749. Valgrind will catch this issue:
> 
> ==10776== Invalid write of size 1                                                                                                                                                                                                                                              
> ==10776==    at 0x426E84: ngx_http_process_request_headers (ngx_http_request.c:1230)                                                                                                                                                                                           
> ...
> 
> The following patch fixes this issue:
> 
> # HG changeset patch
> # User Daniil Bondarev <bondarev at amazon.com>
> # Date 1412401143 25200
> # Node ID 2bbb5284ca7ff658ad50254fe0c5bec14247ba75
> # Parent  6bbad2e732458bf53771e80c63a654b3d7f61963
> Prevent buffer overrun on NGX_HTTP_REQUEST_HEADER_TOO_LARGE
> 
> When large header buffer is full and the last header size is 1 or 2 bytes more
> than NGX_MAX_ERROR_STR - 300, nginx will write 1 or 2 '.' symbols out of large
> header buffer, causing unpredictable behavior.
> 
> The fix is, instead of modifying a buffer, just cut the header and print '...'
> in log line if header is too large.
> 
> diff -r 6bbad2e73245 -r 2bbb5284ca7f src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c       Wed Aug 27 20:51:01 2014 +0400
> +++ b/src/http/ngx_http_request.c       Fri Oct 03 22:39:03 2014 -0700
> @@ -1171,6 +1171,7 @@
>  {
>      u_char                     *p;
>      size_t                      len;
> +    size_t                      print_len;
>      ssize_t                     n;
>      ngx_int_t                   rc, rv;
>      ngx_table_elt_t            *h;
> @@ -1225,14 +1226,13 @@
>  
>                      len = r->header_in->end - p;
>  
> -                    if (len > NGX_MAX_ERROR_STR - 300) {
> -                        len = NGX_MAX_ERROR_STR - 300;
> -                        p[len++] = '.'; p[len++] = '.'; p[len++] = '.';
> -                    }
> +                    /* Since log line size is limited to NGX_MAX_ERROR_STR,
> +                     * nginx has to limit header size it will print into log. */
> +                    print_len = ngx_min(len, NGX_MAX_ERROR_STR - 300);
>  
>                      ngx_log_error(NGX_LOG_INFO, c->log, 0,
> -                                  "client sent too long header line: \"%*s\"",
> -                                  len, r->header_name_start);
> +                                  "client sent too long header line: \"%*s%s\"",
> +                                  print_len, p, len != print_len ? "..." : "");

Thanks for the report.
What do you think about something as simple as:

--- a/src/http/ngx_http_request.c
+++ b/src/http/ngx_http_request.c
@@ -1226,7 +1226,7 @@ ngx_http_process_request_headers(ngx_eve
                     len = r->header_in->end - p;
 
                     if (len > NGX_MAX_ERROR_STR - 300) {
-                        len = NGX_MAX_ERROR_STR - 300;
+                        len = NGX_MAX_ERROR_STR - 300 - 3;
                         p[len++] = '.'; p[len++] = '.'; p[len++] = '.';
                     }
 

?

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