[PATCH] Geo: fix uninitialized memory access

[Roman Arutyunyan]

Hi Piotr,

On Wed, Feb 28, 2024 at 01:21:40AM +0000, Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora <piotr at aviatrix.com>
> # Date 1708977621 0
> #      Mon Feb 26 20:00:21 2024 +0000
> # Branch patch005
> # Node ID fe6f8a72d42970df176ea53f4f0aea16947ba5b8
> # Parent  52936793ac076072c3544aa4e27f973d2f8fecda
> Geo: fix uninitialized memory access.
> 
> Found with MemorySanitizer.
> 
> Signed-off-by: Piotr Sikora <piotr at aviatrix.com>
> 
> diff -r 52936793ac07 -r fe6f8a72d429 src/http/modules/ngx_http_geo_module.c
> --- a/src/http/modules/ngx_http_geo_module.c	Mon Feb 26 20:00:19 2024 +0000
> +++ b/src/http/modules/ngx_http_geo_module.c	Mon Feb 26 20:00:21 2024 +0000
> @@ -1259,7 +1259,7 @@
>          return gvvn->value;
>      }
>  
> -    val = ngx_palloc(ctx->pool, sizeof(ngx_http_variable_value_t));
> +    val = ngx_pcalloc(ctx->pool, sizeof(ngx_http_variable_value_t));
>      if (val == NULL) {
>          return NULL;
>      }
> diff -r 52936793ac07 -r fe6f8a72d429 src/stream/ngx_stream_geo_module.c
> --- a/src/stream/ngx_stream_geo_module.c	Mon Feb 26 20:00:19 2024 +0000
> +++ b/src/stream/ngx_stream_geo_module.c	Mon Feb 26 20:00:21 2024 +0000
> @@ -1209,7 +1209,7 @@
>          return gvvn->value;
>      }
>  
> -    val = ngx_palloc(ctx->pool, sizeof(ngx_stream_variable_value_t));
> +    val = ngx_pcalloc(ctx->pool, sizeof(ngx_stream_variable_value_t));
>      if (val == NULL) {
>          return NULL;
>      }
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

Thanks for the patch, looks valid, except we no longer need to explicitly
initialize fields to zero.  Also, I think we need more details about the
uninitialized memory access.  See updated patch.

--
Roman Arutyunyan
-------------- next part --------------
# HG changeset patch
# User Piotr Sikora <piotr at aviatrix.com>
# Date 1710427040 -14400
#      Thu Mar 14 18:37:20 2024 +0400
# Node ID bd1a4807521bd830ab73c11d6ff3c9b75f5c45f0
# Parent  2ed3f57dca0a664340bca2236c7d614902db4180
Geo: fix uninitialized memory access.

While copying ngx_http_variable_value_t structures to geo binary base in
ngx_http_geo_copy_values(), uninitialized parts of these structures are copied
as well.  These include the "escape" field and possible holes.  Calculating
crc32 of this data triggers uninitialized memory access.

Found with MemorySanitizer.

Signed-off-by: Piotr Sikora <piotr at aviatrix.com>

diff --git a/src/http/modules/ngx_http_geo_module.c b/src/http/modules/ngx_http_geo_module.c
--- a/src/http/modules/ngx_http_geo_module.c
+++ b/src/http/modules/ngx_http_geo_module.c
@@ -1259,7 +1259,7 @@ ngx_http_geo_value(ngx_conf_t *cf, ngx_h
         return gvvn->value;
     }
 
-    val = ngx_palloc(ctx->pool, sizeof(ngx_http_variable_value_t));
+    val = ngx_pcalloc(ctx->pool, sizeof(ngx_http_variable_value_t));
     if (val == NULL) {
         return NULL;
     }
@@ -1271,8 +1271,6 @@ ngx_http_geo_value(ngx_conf_t *cf, ngx_h
     }
 
     val->valid = 1;
-    val->no_cacheable = 0;
-    val->not_found = 0;
 
     gvvn = ngx_palloc(ctx->temp_pool,
                       sizeof(ngx_http_geo_variable_value_node_t));
diff --git a/src/stream/ngx_stream_geo_module.c b/src/stream/ngx_stream_geo_module.c
--- a/src/stream/ngx_stream_geo_module.c
+++ b/src/stream/ngx_stream_geo_module.c
@@ -1209,7 +1209,7 @@ ngx_stream_geo_value(ngx_conf_t *cf, ngx
         return gvvn->value;
     }
 
-    val = ngx_palloc(ctx->pool, sizeof(ngx_stream_variable_value_t));
+    val = ngx_pcalloc(ctx->pool, sizeof(ngx_stream_variable_value_t));
     if (val == NULL) {
         return NULL;
     }
@@ -1221,8 +1221,6 @@ ngx_stream_geo_value(ngx_conf_t *cf, ngx
     }
 
     val->valid = 1;
-    val->no_cacheable = 0;
-    val->not_found = 0;
 
     gvvn = ngx_palloc(ctx->temp_pool,
                       sizeof(ngx_stream_geo_variable_value_node_t));