#546 closed defect (fixed)
ngx_http_proxy_rewrite data not null terminated but ngx_strcasestrn expects it to be
Reported by: | Markus Linnala | Owned by: | Valentin V. Bartenev |
---|---|---|---|
Priority: | minor | Milestone: | |
Component: | nginx-core | Version: | 1.5.x |
Keywords: | Cc: | ||
uname -a: | Linux tester 3.13.10-200.fc20.x86_64 #1 SMP Mon Apr 14 20:34:16 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux | ||
nginx -V: |
nginx version: nginx/1.5.13 (no pool)
built by gcc 4.8.2 20131212 (Red Hat 4.8.2-7) (GCC) configure arguments: --with-cc-opt=' -fsanitize=address -fno-omit-frame-pointer' --with-ld-opt=' -fsanitize=address -fno-omit-frame-pointer' |
Description
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.
Change History (3)
comment:1 by , 10 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:3 by , 7 years ago
sensitive: | 1 → 0 |
---|
Yes, thanks, that's a known problem. Valentin has slightly better patch for this, but he failed to provide proper commit log for it.