From 6be130e1b865af62e6509d4ead9da5c911af5e12 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 16 Nov 2023 09:05:51 +0100 Subject: [PATCH] unix,win: fix read past end of pipe name buffer (#4209) Passing a socket name without a trailing nul byte to uv_pipe_bind2() or (on Windows) uv_pipe_connect2() resulted in reading beyond the end of the name buffer when copying or converting it. Fix that by copying the socket name to temporary storage first and add the trailing nul byte explicitly. Add a check for embedded nul bytes in the socket name. Fix a small memory leak in the Windows error path of uv_pipe_bind2(). --- src/unix/pipe.c | 29 ++++++++++------ src/win/pipe.c | 66 ++++++++++++++++++++++++++++++------ test/test-pipe-getsockname.c | 26 +++++++++++--- 3 files changed, 96 insertions(+), 25 deletions(-) diff --git a/src/unix/pipe.c b/src/unix/pipe.c index 117b8ee52fc..bb2806f579a 100644 --- a/src/unix/pipe.c +++ b/src/unix/pipe.c @@ -30,6 +30,19 @@ #include +/* Does the file path contain embedded nul bytes? */ +static int includes_nul(const char *s, size_t n) { + if (n == 0) + return 0; +#ifdef __linux__ + /* Accept abstract socket namespace path ("\0/virtual/path"). */ + s++; + n--; +#endif + return NULL != memchr(s, '\0', n); +} + + int uv_pipe_init(uv_loop_t* loop, uv_pipe_t* handle, int ipc) { uv__stream_init(loop, (uv_stream_t*)handle, UV_NAMED_PIPE); handle->shutdown_req = NULL; @@ -65,11 +78,8 @@ int uv_pipe_bind2(uv_pipe_t* handle, if (namelen == 0) return UV_EINVAL; -#ifndef __linux__ - /* Abstract socket namespace only works on Linux. */ - if (*name == '\0') + if (includes_nul(name, namelen)) return UV_EINVAL; -#endif if (flags & UV_PIPE_NO_TRUNCATE) if (namelen > sizeof(saddr.sun_path)) @@ -91,9 +101,11 @@ int uv_pipe_bind2(uv_pipe_t* handle, * automatically since they're not real file system entities. */ if (*name != '\0') { - pipe_fname = uv__strdup(name); + pipe_fname = uv__malloc(namelen + 1); if (pipe_fname == NULL) return UV_ENOMEM; + memcpy(pipe_fname, name, namelen); + pipe_fname[namelen] = '\0'; } err = uv__socket(AF_UNIX, SOCK_STREAM, 0); @@ -117,7 +129,7 @@ int uv_pipe_bind2(uv_pipe_t* handle, /* Success. */ handle->flags |= UV_HANDLE_BOUND; - handle->pipe_fname = pipe_fname; /* NULL or a strdup'ed copy. */ + handle->pipe_fname = pipe_fname; /* NULL or a copy of |name| */ handle->io_watcher.fd = sockfd; return 0; @@ -249,11 +261,8 @@ int uv_pipe_connect2(uv_connect_t* req, if (namelen == 0) return UV_EINVAL; -#ifndef __linux__ - /* Abstract socket namespace only works on Linux. */ - if (*name == '\0') + if (includes_nul(name, namelen)) return UV_EINVAL; -#endif if (flags & UV_PIPE_NO_TRUNCATE) if (namelen > sizeof(saddr.sun_path)) diff --git a/src/win/pipe.c b/src/win/pipe.c index 17e4a520caf..d5102ed5c44 100644 --- a/src/win/pipe.c +++ b/src/win/pipe.c @@ -98,6 +98,14 @@ static void eof_timer_destroy(uv_pipe_t* pipe); static void eof_timer_close_cb(uv_handle_t* handle); +/* Does the file path contain embedded nul bytes? */ +static int includes_nul(const char *s, size_t n) { + if (n == 0) + return 0; + return NULL != memchr(s, '\0', n); +} + + static void uv__unique_pipe_name(char* ptr, char* name, size_t size) { snprintf(name, size, "\\\\?\\pipe\\uv\\%p-%lu", ptr, GetCurrentProcessId()); } @@ -705,6 +713,7 @@ int uv_pipe_bind2(uv_pipe_t* handle, uv_loop_t* loop = handle->loop; int i, err; uv_pipe_accept_t* req; + char* name_copy; if (flags & ~UV_PIPE_NO_TRUNCATE) { return UV_EINVAL; @@ -718,7 +727,7 @@ int uv_pipe_bind2(uv_pipe_t* handle, return UV_EINVAL; } - if (*name == '\0') { + if (includes_nul(name, namelen)) { return UV_EINVAL; } @@ -730,14 +739,24 @@ int uv_pipe_bind2(uv_pipe_t* handle, return UV_EINVAL; } + name_copy = uv__malloc(namelen + 1); + if (name_copy == NULL) { + return UV_ENOMEM; + } + + memcpy(name_copy, name, namelen); + name_copy[namelen] = '\0'; + if (!(handle->flags & UV_HANDLE_PIPESERVER)) { handle->pipe.serv.pending_instances = default_pending_pipe_instances; } + err = UV_ENOMEM; handle->pipe.serv.accept_reqs = (uv_pipe_accept_t*) uv__malloc(sizeof(uv_pipe_accept_t) * handle->pipe.serv.pending_instances); - if (!handle->pipe.serv.accept_reqs) - return UV_ENOMEM; + if (handle->pipe.serv.accept_reqs == NULL) { + goto error; + } for (i = 0; i < handle->pipe.serv.pending_instances; i++) { req = &handle->pipe.serv.accept_reqs[i]; @@ -747,9 +766,14 @@ int uv_pipe_bind2(uv_pipe_t* handle, req->next_pending = NULL; } - err = uv__convert_utf8_to_utf16(name, &handle->name); - if (err) - return err; + /* TODO(bnoordhuis) Add converters that take a |length| parameter. */ + err = uv__convert_utf8_to_utf16(name_copy, &handle->name); + uv__free(name_copy); + name_copy = NULL; + + if (err) { + goto error; + } /* * Attempt to create the first pipe with FILE_FLAG_FIRST_PIPE_INSTANCE. @@ -761,9 +785,11 @@ int uv_pipe_bind2(uv_pipe_t* handle, TRUE)) { err = GetLastError(); if (err == ERROR_ACCESS_DENIED) { - err = WSAEADDRINUSE; /* Translates to UV_EADDRINUSE. */ + err = UV_EADDRINUSE; } else if (err == ERROR_PATH_NOT_FOUND || err == ERROR_INVALID_NAME) { - err = WSAEACCES; /* Translates to UV_EACCES. */ + err = UV_EACCES; + } else { + err = uv_translate_sys_error(err); } goto error; } @@ -775,10 +801,13 @@ int uv_pipe_bind2(uv_pipe_t* handle, return 0; error: + uv__free(handle->pipe.serv.accept_reqs); uv__free(handle->name); + uv__free(name_copy); + handle->pipe.serv.accept_reqs = NULL; handle->name = NULL; - return uv_translate_sys_error(err); + return err; } @@ -855,6 +884,7 @@ int uv_pipe_connect2(uv_connect_t* req, size_t nameSize; HANDLE pipeHandle = INVALID_HANDLE_VALUE; DWORD duplex_flags; + char* name_copy; loop = handle->loop; UV_REQ_INIT(req, UV_CONNECT); @@ -876,10 +906,18 @@ int uv_pipe_connect2(uv_connect_t* req, return UV_EINVAL; } - if (*name == '\0') { + if (includes_nul(name, namelen)) { return UV_EINVAL; } + name_copy = uv__malloc(namelen + 1); + if (name_copy == NULL) { + return UV_ENOMEM; + } + + memcpy(name_copy, name, namelen); + name_copy[namelen] = '\0'; + if (handle->flags & UV_HANDLE_PIPESERVER) { err = ERROR_INVALID_PARAMETER; goto error; @@ -890,7 +928,11 @@ int uv_pipe_connect2(uv_connect_t* req, } uv__pipe_connection_init(handle); - err = uv__convert_utf8_to_utf16(name, &handle->name); + /* TODO(bnoordhuis) Add converters that take a |length| parameter. */ + err = uv__convert_utf8_to_utf16(name_copy, &handle->name); + uv__free(name_copy); + name_copy = NULL; + if (err) { err = ERROR_NO_UNICODE_TRANSLATION; goto error; @@ -936,6 +978,8 @@ int uv_pipe_connect2(uv_connect_t* req, return 0; error: + uv__free(name_copy); + if (handle->name) { uv__free(handle->name); handle->name = NULL; diff --git a/test/test-pipe-getsockname.c b/test/test-pipe-getsockname.c index eb09d88fd2b..bac723c6c07 100644 --- a/test/test-pipe-getsockname.c +++ b/test/test-pipe-getsockname.c @@ -91,16 +91,24 @@ TEST_IMPL(pipe_getsockname) { RETURN_SKIP(NO_SELF_CONNECT); #endif uv_loop_t* loop; + char namebuf[256]; char buf[1024]; + size_t namelen; size_t len; int r; + snprintf(namebuf, sizeof(namebuf), "%s-oob", TEST_PIPENAME); + namelen = sizeof(TEST_PIPENAME) - 1; + loop = uv_default_loop(); ASSERT_NOT_NULL(loop); r = uv_pipe_init(loop, &pipe_server, 0); ASSERT_OK(r); + r = uv_pipe_bind2(&pipe_server, "bad\0path", 8, 0); + ASSERT_EQ(r, UV_EINVAL); + len = sizeof buf; r = uv_pipe_getsockname(&pipe_server, buf, &len); ASSERT_EQ(r, UV_EBADF); @@ -109,9 +117,13 @@ TEST_IMPL(pipe_getsockname) { r = uv_pipe_getpeername(&pipe_server, buf, &len); ASSERT_EQ(r, UV_EBADF); - r = uv_pipe_bind(&pipe_server, TEST_PIPENAME); + r = uv_pipe_bind2(&pipe_server, namebuf, namelen, 0); ASSERT_OK(r); +#ifndef _WIN32 + ASSERT_STR_EQ(pipe_server.pipe_fname, TEST_PIPENAME); +#endif + len = sizeof buf; r = uv_pipe_getsockname(&pipe_server, buf, &len); ASSERT_OK(r); @@ -138,7 +150,13 @@ TEST_IMPL(pipe_getsockname) { r = uv_pipe_getpeername(&pipe_client, buf, &len); ASSERT_EQ(r, UV_EBADF); - uv_pipe_connect(&connect_req, &pipe_client, TEST_PIPENAME, pipe_client_connect_cb); + r = uv_pipe_connect2(&connect_req, + &pipe_client, + namebuf, + namelen, + 0, + pipe_client_connect_cb); + ASSERT_OK(r); len = sizeof buf; r = uv_pipe_getsockname(&pipe_client, buf, &len); @@ -171,7 +189,7 @@ TEST_IMPL(pipe_getsockname_abstract) { buflen = sizeof(buf); memset(buf, 0, sizeof(buf)); ASSERT_OK(uv_pipe_init(uv_default_loop(), &pipe_server, 0)); - ASSERT_OK(uv_pipe_bind2(&pipe_server, name, sizeof(name), 0)); + ASSERT_OK(uv_pipe_bind2(&pipe_server, name, sizeof(name) - 1, 0)); ASSERT_OK(uv_pipe_getsockname(&pipe_server, buf, &buflen)); ASSERT_MEM_EQ(name, buf, sizeof(name)); ASSERT_OK(uv_listen((uv_stream_t*) &pipe_server, @@ -181,7 +199,7 @@ TEST_IMPL(pipe_getsockname_abstract) { ASSERT_OK(uv_pipe_connect2(&connect_req, &pipe_client, name, - sizeof(name), + sizeof(name) - 1, 0, pipe_client_connect_cb)); ASSERT_OK(uv_run(uv_default_loop(), UV_RUN_DEFAULT));