summaryrefslogtreecommitdiffhomepage
path: root/src
diff options
context:
space:
mode:
authorAndrew Clayton <a.clayton@nginx.com>2023-10-05 00:36:00 +0100
committerAndrew Clayton <a.clayton@nginx.com>2023-10-05 13:38:15 +0100
commit30142d2a3c862eef0a32805ee3907535ada32c71 (patch)
treec614d40ba5ab121b23dbb10d2650998c5a81b076 /src
parent1617f2c04514309a8d19d73103c468af564ef3fd (diff)
downloadunit-30142d2a3c862eef0a32805ee3907535ada32c71.tar.gz
unit-30142d2a3c862eef0a32805ee3907535ada32c71.tar.bz2
HTTP: Fix URL with query string rewrite.
On Github, @rlandgrebe reported an issue when trying to rewrite URLs that contained query strings. With the PHP language module we were in fact segfaulting (SIGSEGV) in libphp [93960.462952] unitd[20940]: segfault at 7f307cef6476 ip 00007f2f81a94577 sp 00007fff28a777d0 error 4 in libphp-8.2.so[7f2f818df000+2fd000] likely on CPU 0 (core 0, socket 0) #0 0x00007f2abd494577 in php_default_treat_data (arg=1, str=0x0, destArray=<optimized out>) at /usr/src/debug/php-8.2.10-1.fc38.x86_64/main/php_variables.c:488 488 if (c_var && *c_var) { (gdb) p c_var $1 = 0x7f2bb8880676 <error: Cannot access memory at address 0x7f2bb8880676> This was when trying to get the query string which somehow is pointing off into the woods. This gdb debug session when doing rewrite basically shows the core of the issue (gdb) x /64bs req->fields ... 0x7f7eaaaa8090: "GET" 0x7f7eaaaa8094: "HTTP/1.1" 0x7f7eaaaa809d: "::1" 0x7f7eaaaa80a1: "::1" 0x7f7eaaaa80a5: "8080" 0x7f7eaaaa80aa: "localhost" 0x7f7eaaaa80b4: "/test?q=a" 0x7f7eaaaa80be: "/test" ... (gdb) p target_pos $4 = (void *) 0x7f7eaaaa80b4 (gdb) p query_pos $6 = (void *) 0x7f7eaaaa6af6 (gdb) p r->args->start $8 = (u_char *) 0x7f7ea4002b02 "q=a HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: curl/8.0.1\r\nAccept: */*\r\n\r\n" (gdb) p r->target.start $9 = (u_char *) 0x7f7ea40040c0 "/test?q=a" That last address, 0x7f7ea40040c0, looks out of wack, it should be smaller than r->args->start. That results in a calculation in nxt_router_prepare_msg() if (r->args->start != NULL) { query_pos = nxt_pointer_to(target_pos, r->args->start - r->target.start); nxt_unit_sptr_set(&req->query, query_pos); } else { that goes negative that then is stored in req->query.offset which is a uint32_t and so wraps backwards from UINT_MAX to give us an offset of a little under 4GiB, hence the above invalid memory access. All this happens due to in nxt_http_rewrite() if we have a URL with a query string, we create a new memory allocation to store the transformed URL and query string. We set r->target to point to this new allocation, but we also need to point r->args->start to the start of the query string in this new allocation. Reported-by: René Landgrebe <https://github.com/rlandgrebe> Tested-by: René Landgrebe <https://github.com/rlandgrebe> Tested-by: Liam Crilly <liam.crilly@nginx.com> Fixes: 14d6d97b ("HTTP: added basic URI rewrite.") Closes: <https://github.com/nginx/unit/issues/964> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
Diffstat (limited to 'src')
-rw-r--r--src/nxt_http_rewrite.c1
1 files changed, 1 insertions, 0 deletions
diff --git a/src/nxt_http_rewrite.c b/src/nxt_http_rewrite.c
index ae5c865a..fb216eeb 100644
--- a/src/nxt_http_rewrite.c
+++ b/src/nxt_http_rewrite.c
@@ -93,6 +93,7 @@ nxt_http_rewrite(nxt_task_t *task, nxt_http_request_t *r)
nxt_memcpy(p, r->args->start, r->args->length);
r->target = target;
+ r->args->start = p;
}
r->path = nxt_mp_alloc(r->mem_pool, sizeof(nxt_str_t));