From 78e1122a3c94a150219b4b6e1e594ae5bfdd8d68 Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Sat, 4 Mar 2023 23:52:51 +0000 Subject: Router: Fix allocation of request buffer sent to application. This fixes an issue reported by @Peter2121 on GitHub. In nxt_router_prepare_msg() we create a buffer (nxt_unit_request_t *req) that gets sent to an application process that contains details about a client request. The req structure comprises various members with the final member being an array (specified as a flexible array member, with its actual length denoted by the req->fields_count member) of nxt_unit_field_t's. These structures specify the length and offset for the various request headers name/value pairs which are stored after some request metadata that is stored immediately after this array of structs as individual nul terminated strings. After this we have the body content data (if any). So it looks a little like (gdb) x /64bs 0x7f38c976e060 0x7f38c976e060: "\353\346\244\t\006" <-- First nxt_unit_field_t 0x7f38c976e066: "" 0x7f38c976e067: "" 0x7f38c976e068: "T\001" 0x7f38c976e06b: "" 0x7f38c976e06c: "Z\001" 0x7f38c976e06f: "" ... 0x7f38c976e170: "\362#\244\v$" <-- Last nxt_unit_field_t 0x7f38c976e176: "" 0x7f38c976e177: "" 0x7f38c976e178: "\342\002" 0x7f38c976e17b: "" 0x7f38c976e17c: "\352\002" 0x7f38c976e17f: "" 0x7f38c976e180: "POST" <-- Start of request metadata 0x7f38c976e185: "HTTP/1.1" 0x7f38c976e18e: "unix:" 0x7f38c976e194: "unix:/dev/shm/842.sock" 0x7f38c976e1ab: "" 0x7f38c976e1ac: "fedora" 0x7f38c976e1b3: "/842.php" 0x7f38c976e1bc: "HTTP_HOST" <-- Start of header fields 0x7f38c976e1c6: "fedora" 0x7f38c976e1cd: "HTTP_X_FORWARDED_PROTO" 0x7f38c976e1e4: "https" ... 0x7f38c976e45a: "HTTP_COOKIE" 0x7f38c976e466: "PHPSESSID=8apkg25r9s9vju3pi085i21eh4" 0x7f38c976e48b: "public_form=sended" <-- Body content Well that's how things are supposed to look! When using Unix domain sockets what we actually got looked like ... 0x7f6141f3445a: "HTTP_COOKIE" 0x7f6141f34466: "PHPSESSID=uo5b2nu9buijkc89jotbgmd60vpublic_form=sended" Here, the body content (from a POST for example) has been appended straight onto the end of the last header field value. In this case corrupting the PHP session cookie. The body content would still be found by the application as its offset into this buffer is correct. This problem was actually caused by a0327445 ("PHP: allowed to specify URLs without a trailing '/'.") which added an extra item into this request buffer specifying the port number that unit is listening on that handled this request. Unfortunately when I wrote that patch I didn't increase the size of this request buffer to accommodate it. When using normal TCP sockets we actually end up allocating more space than required for this buffer, we track the end of this buffer up to where the body content would go and so we have a few spare bytes between the nul byte of the last field header value and the start of the body content. When using Unix domain sockets, they have no associated port number and thus the port number has a length of 0 bytes, but we still write a '\0' in there using up a byte that we didn't account for, this causes us to loose the nul byte of the last header fields value causing the body data to be appended to the last header field value. The fix is simple, account for the local port length, we also add 1 to it, this covers the nul byte, even if there is no port as with Unix domain sockets. Closes: Fixes: a0327445 ("PHP: allowed to specify URLs without a trailing '/'.") Reviewed-by: Alejandro Colomar Signed-off-by: Andrew Clayton --- src/nxt_router.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nxt_router.c b/src/nxt_router.c index 17f6c572..4637cc68 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -5208,6 +5208,7 @@ nxt_router_prepare_msg(nxt_task_t *task, nxt_http_request_t *r, + r->version.length + 1 + r->remote->length + 1 + r->local->length + 1 + + nxt_sockaddr_port_length(r->local) + 1 + r->server_name.length + 1 + r->target.length + 1 + (r->path->start != r->target.start ? r->path->length + 1 : 0); -- cgit