ngx_http_proxy_rewrite data not null terminated but ngx_strcasestrn expects it to be

[Markus Linnala]

From src/http/modules/ngx_http_proxy_module.c

2323 data does not have space for null termination.

  2312  static ngx_int_t
  2313  ngx_http_proxy_rewrite(ngx_http_request_t *r, ngx_table_elt_t *h, size_t prefix,
  2314      size_t len, ngx_str_t *replacement)
  2315  {
  2316      u_char  *p, *data;
  2317      size_t   new_len;
  2318  
  2319      new_len = replacement->len + h->value.len - len;
  2320  
  2321      if (replacement->len > len) {
  2322  
  2323          data = ngx_pnalloc(r->pool, new_len);
  2324          if (data == NULL) {
  2325              return NGX_ERROR;
  2326          }

But ngx_strcasestrn expects it to have null termination, used for example here:

  2176      if (plcf->cookie_paths) {
  2177          p = ngx_strcasestrn(h->value.data + prefix, "path=", 5 - 1);
  2178  

This is visible if I use ​git://github.com/shrimp/no-pool-nginx.git and gcc with -fsanitize=address.

% ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer ASAN_OPTIONS=symbolize=1 TEST_NGINX_CATLOG=1 TEST_NGINX_BINARY=./nginx-1.5.13-asan/objs/nginx prove -v proxy_cookie.t
proxy_cookie.t .. 
1..8
ok 1 - domain rewrite
not ok 2 - domain rewrite with vars
ok 3 - domain regex rewrite
ok 4 - path rewrite
ok 5 - path rewrite with vars
ok 6 - path regex rewrite
ok 7 - path caseless regex rewrite
ok 8 - domain and path rewrite
2014/04/22 19:45:53 [notice] 15510#0: using the "epoll" event method
2014/04/22 19:45:53 [notice] 15510#0: nginx/1.5.13 (no pool)
2014/04/22 19:45:53 [notice] 15510#0: built by gcc 4.8.2 20131212 (Red Hat 4.8.2-7) (GCC) 
2014/04/22 19:45:53 [notice] 15510#0: OS: Linux 3.13.10-200.fc20.x86_64
2014/04/22 19:45:53 [notice] 15510#0: getrlimit(RLIMIT_NOFILE): 1024:4096
2014/04/22 19:45:53 [notice] 15510#0: start worker processes
2014/04/22 19:45:53 [notice] 15510#0: start worker process 15512
2014/04/22 19:45:53 [warn] 15512#0: *3 using uninitialized "sc_path" variable, client: 127.0.0.1, server: localhost, request: "GET /?domain=www.Example.org HTTP/1.0", host: "127.0.0.1:8081"
2014/04/22 19:45:53 [warn] 15512#0: *6 using uninitialized "sc_path" variable, client: 127.0.0.1, server: localhost, request: "GET /?domain=.LocalHost.com HTTP/1.0", host: "127.0.0.1:8081"
=================================================================
==15512== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60080000a238 at pc 0x41427c bp 0x7fff2f394430 sp 0x7fff2f394420
READ of size 1 at 0x60080000a238 thread T0
    #0 0x41427b in ngx_strcasestrn /home/user/nginx-tests/nginx-1.5.13-asan/src/core/ngx_string.c:694
    #1 0x5090ce in ngx_http_proxy_rewrite_cookie /home/user/nginx-tests/nginx-1.5.13-asan/src/http/modules/ngx_http_proxy_module.c:2177
    #2 0x4a73a4 in ngx_http_upstream_rewrite_set_cookie /home/user/nginx-tests/nginx-1.5.13-asan/src/http/ngx_http_upstream.c:4155
    #3 0x4ac67c in ngx_http_upstream_process_headers /home/user/nginx-tests/nginx-1.5.13-asan/src/http/ngx_http_upstream.c:2081
    #4 0x4b12d8 in ngx_http_upstream_process_header /home/user/nginx-tests/nginx-1.5.13-asan/src/http/ngx_http_upstream.c:1730
    #5 0x4ab805 in ngx_http_upstream_handler /home/user/nginx-tests/nginx-1.5.13-asan/src/http/ngx_http_upstream.c:977
    #6 0x459128 in ngx_epoll_process_events /home/user/nginx-tests/nginx-1.5.13-asan/src/event/modules/ngx_epoll_module.c:691
    #7 0x444201 in ngx_process_events_and_timers /home/user/nginx-tests/nginx-1.5.13-asan/src/event/ngx_event.c:248
    #8 0x455f77 in ngx_worker_process_cycle /home/user/nginx-tests/nginx-1.5.13-asan/src/os/unix/ngx_process_cycle.c:816
    #9 0x451b9d in ngx_spawn_process /home/user/nginx-tests/nginx-1.5.13-asan/src/os/unix/ngx_process.c:198
    #10 0x4537ce in ngx_start_worker_processes /home/user/nginx-tests/nginx-1.5.13-asan/src/os/unix/ngx_process_cycle.c:364
    #11 0x4570c2 in ngx_master_process_cycle /home/user/nginx-tests/nginx-1.5.13-asan/src/os/unix/ngx_process_cycle.c:136
    #12 0x406639 in main /home/user/nginx-tests/nginx-1.5.13-asan/src/core/nginx.c:407
    #13 0x39fde21d64 in __libc_start_main (/lib64/libc.so.6+0x39fde21d64)
    #14 0x4033d8 in _start (/home/user/nginx-tests/nginx-1.5.13-asan/objs/nginx+0x4033d8)
0x60080000a238 is located 0 bytes to the right of 40-byte region [0x60080000a210,0x60080000a238)
allocated by thread T0 here:
    #0 0x7ffe8c24d219 in __interceptor_malloc /usr/src/debug/gcc-4.8.2-20131212/obj-x86_64-redhat-linux/x86_64-redhat-linux/libsanitizer/asan/../../../../libsanitizer/asan/asan_malloc_linux.cc:71
    #1 0x44bd37 in ngx_alloc /home/user/nginx-tests/nginx-1.5.13-asan/src/os/unix/ngx_alloc.c:22
    #2 0x409678 in ngx_malloc /home/user/nginx-tests/nginx-1.5.13-asan/src/core/ngx_palloc.c:121
    #3 0x409738 in ngx_pnalloc /home/user/nginx-tests/nginx-1.5.13-asan/src/core/ngx_palloc.c:149
    #4 0x504408 in ngx_http_proxy_rewrite /home/user/nginx-tests/nginx-1.5.13-asan/src/http/modules/ngx_http_proxy_module.c:2323
    #5 0x506775 in ngx_http_proxy_rewrite_domain_handler /home/user/nginx-tests/nginx-1.5.13-asan/src/http/modules/ngx_http_proxy_module.c:2308
    #6 0x508ee0 in ngx_http_proxy_rewrite_cookie_value /home/user/nginx-tests/nginx-1.5.13-asan/src/http/modules/ngx_http_proxy_module.c:2215
    #7 0x509054 in ngx_http_proxy_rewrite_cookie /home/user/nginx-tests/nginx-1.5.13-asan/src/http/modules/ngx_http_proxy_module.c:2164
    #8 0x4a73a4 in ngx_http_upstream_rewrite_set_cookie /home/user/nginx-tests/nginx-1.5.13-asan/src/http/ngx_http_upstream.c:4155
    #9 0x4ac67c in ngx_http_upstream_process_headers /home/user/nginx-tests/nginx-1.5.13-asan/src/http/ngx_http_upstream.c:2081
    #10 0x4b12d8 in ngx_http_upstream_process_header /home/user/nginx-tests/nginx-1.5.13-asan/src/http/ngx_http_upstream.c:1730
    #11 0x4ab805 in ngx_http_upstream_handler /home/user/nginx-tests/nginx-1.5.13-asan/src/http/ngx_http_upstream.c:977
    #12 0x459128 in ngx_epoll_process_events /home/user/nginx-tests/nginx-1.5.13-asan/src/event/modules/ngx_epoll_module.c:691
    #13 0x444201 in ngx_process_events_and_timers /home/user/nginx-tests/nginx-1.5.13-asan/src/event/ngx_event.c:248
    #14 0x455f77 in ngx_worker_process_cycle /home/user/nginx-tests/nginx-1.5.13-asan/src/os/unix/ngx_process_cycle.c:816
    #15 0x451b9d in ngx_spawn_process /home/user/nginx-tests/nginx-1.5.13-asan/src/os/unix/ngx_process.c:198
    #16 0x4537ce in ngx_start_worker_processes /home/user/nginx-tests/nginx-1.5.13-asan/src/os/unix/ngx_process_cycle.c:364
    #17 0x4570c2 in ngx_master_process_cycle /home/user/nginx-tests/nginx-1.5.13-asan/src/os/unix/ngx_process_cycle.c:136
    #18 0x406639 in main /home/user/nginx-tests/nginx-1.5.13-asan/src/core/nginx.c:407
    #19 0x39fde21d64 in __libc_start_main (/lib64/libc.so.6+0x39fde21d64)
SUMMARY: AddressSanitizer: heap-buffer-overflow /home/user/nginx-tests/nginx-1.5.13-asan/src/core/ngx_string.c:694 ngx_strcasestrn
Shadow bytes around the buggy address:
  0x0c017fff93f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c017fff9400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c017fff9410: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c017fff9420: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c017fff9430: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c017fff9440: fa fa 00 00 00 00 00[fa]fa fa 00 00 00 00 04 fa
  0x0c017fff9450: fa fa 00 00 00 00 07 fa fa fa 00 00 00 00 04 fa
  0x0c017fff9460: fa fa 00 00 00 00 05 fa fa fa 00 00 00 00 00 00
  0x0c017fff9470: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
  0x0c017fff9480: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
  0x0c017fff9490: fa fa 00 00 00 00 00 00 fa fa fd fd fd fd fd fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==15512== ABORTING
2014/04/22 19:45:53 [notice] 15510#0: signal 17 (SIGCHLD) received
2014/04/22 19:45:53 [notice] 15510#0: worker process 15512 exited with code 1
2014/04/22 19:45:53 [notice] 15510#0: start worker process 15513
2014/04/22 19:45:53 [warn] 15513#0: *9 using uninitialized "sc_path" variable, client: 127.0.0.1, server: localhost, request: "GET /?domain=www.example.COM HTTP/1.0", host: "127.0.0.1:8081"
2014/04/22 19:45:53 [warn] 15513#0: *12 using uninitialized "sc_domain" variable, client: 127.0.0.1, server: localhost, request: "GET /?path=/path/test.html HTTP/1.0", host: "127.0.0.1:8081"
2014/04/22 19:45:53 [warn] 15513#0: *15 using uninitialized "sc_domain" variable, client: 127.0.0.1, server: localhost, request: "GET /?path=/localhost/test.html HTTP/1.0", host: "127.0.0.1:8081"
2014/04/22 19:45:53 [warn] 15513#0: *18 using uninitialized "sc_domain" variable, client: 127.0.0.1, server: localhost, request: "GET /?path=/regex/test.html HTTP/1.0", host: "127.0.0.1:8081"
2014/04/22 19:45:53 [warn] 15513#0: *21 using uninitialized "sc_domain" variable, client: 127.0.0.1, server: localhost, request: "GET /?path=/CASEless/test.html HTTP/1.0", host: "127.0.0.1:8081"
2014/04/22 19:45:53 [notice] 15510#0: signal 3 (SIGQUIT) received, shutting down
2014/04/22 19:45:53 [notice] 15513#0: gracefully shutting down
2014/04/22 19:45:53 [notice] 15513#0: exiting
2014/04/22 19:45:53 [notice] 15513#0: exit
2014/04/22 19:45:53 [notice] 15510#0: signal 17 (SIGCHLD) received
2014/04/22 19:45:53 [notice] 15510#0: worker process 15513 exited with code 0
2014/04/22 19:45:53 [notice] 15510#0: exit
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/8 subtests 

Test Summary Report
-------------------
proxy_cookie.t (Wstat: 256 Tests: 8 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=8,  0 wallclock secs ( 0.03 usr  0.01 sys +  0.07 cusr  0.10 csys =  0.21 CPU)
Result: FAIL

These kind of errors are not possible to diagnose without no-pool patch.

With this patch testsuite passes.

diff -r 16405e02e612 -r 4661b04922ae src/http/modules/ngx_http_proxy_module.c
--- a/src/http/modules/ngx_http_proxy_module.c  Tue Apr 22 18:56:49 2014 +0400
+++ b/src/http/modules/ngx_http_proxy_module.c  Tue Apr 22 19:57:41 2014 +0300
@@ -2363,9 +2363,9 @@
 
     new_len = replacement->len + h->value.len - len;
 
-    if (replacement->len > len) {
+    if (replacement->len >= len) {
 
-        data = ngx_pnalloc(r->pool, new_len);
+        data = ngx_pnalloc(r->pool, new_len + 1);
         if (data == NULL) {
             return NGX_ERROR;
         }
@@ -2376,6 +2376,8 @@
         ngx_memcpy(p, h->value.data + prefix + len,
                    h->value.len - len - prefix);
 
+        data[new_len] = '\0';
+
         h->value.data = data;
 
     } else {

As far as I can see no other use of ngx_strcasestrn or ngx_strstrn does have this problem. But you might want to check that. It is really hard to see if ngx_strstrn/ngx_strcasestrn first argument is null terminated as most of a time argument is actually ngx_str_t data and that is not null terminated by default. Maybe first argument should be of type ngx_str_t *.

Also ngx_strstrn/ngx_strcasestrn does not seem to work like strstr/strcasestr and return general substring match. ngx_strcasestrn only returns match from a start.

[Maxim Dounin]

Yes, thanks, that's a known problem. Valentin has slightly better patch for this, but he failed to provide proper commit log for it.

[Valentin V. Bartenev]

Fixed by fbfdf8017748.