[PATCH] Variables: avoid possible buffer overrun with some "$sent_http_*"

[Maxim Dounin]

Hello!

On Thu, Apr 27, 2023 at 09:53:30PM +0400, Sergey Kandaurov wrote:

> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1682617947 -14400
> #      Thu Apr 27 21:52:27 2023 +0400
> # Node ID c1ec385d885fba38f15d54263944eed2c74b5733
> # Parent  77d5c662f3d9d9b90425128109d3369c30ef5f07
> Variables: avoid possible buffer overrun with some "$sent_http_*".
> 
> The existing logic to evaluate multi header "$sent_http_*" variables,
> such as $sent_http_cache_control, as previously introduced in 1.23.0,
> doesn't take into account that one or more elements can be cleared,
> yet still present in a linked list, pointed to by the next field.
> Such elements don't contribute to the resulting variable length, an
> attempt to append a separator for them ends up in out of bounds write.
> 
> This is not possible with standard modules, though at least one third
> party module is known to override multi header values this way, so it
> makes sense to harden the logic.
> 
> The fix restores a generic boundary check.
> 
> diff --git a/src/http/ngx_http_variables.c b/src/http/ngx_http_variables.c
> --- a/src/http/ngx_http_variables.c
> +++ b/src/http/ngx_http_variables.c
> @@ -828,7 +828,7 @@ ngx_http_variable_headers_internal(ngx_h
>      ngx_http_variable_value_t *v, uintptr_t data, u_char sep)
>  {
>      size_t            len;
> -    u_char           *p;
> +    u_char           *p, *end;
>      ngx_table_elt_t  *h, *th;
>  
>      h = *(ngx_table_elt_t **) ((char *) r + data);
> @@ -870,6 +870,8 @@ ngx_http_variable_headers_internal(ngx_h
>      v->len = len;
>      v->data = p;
>  
> +    end = p + len;
> +
>      for (th = h; th; th = th->next) {
>  
>          if (th->hash == 0) {
> @@ -878,7 +880,7 @@ ngx_http_variable_headers_internal(ngx_h
>  
>          p = ngx_copy(p, th->value.data, th->value.len);
>  
> -        if (th->next == NULL) {
> +        if (p == end) {
>              break;
>          }
>  

Looks good.

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