Skip to content

Commit

Permalink
unix,win: fix read past end of pipe name buffer (libuv#4209)
Browse files Browse the repository at this point in the history 
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().
  • Loading branch information
@bnoordhuis
bnoordhuis authored Nov 16, 2023
1 parent f144429 commit 6be130e
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 25 deletions.
29 changes: 19 additions & 10 deletions src/unix/pipe.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,19 @@
#include <stdlib.h>


/* 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;
Expand Down Expand Up @@ -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))
Expand All @@ -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);
Expand All @@ -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;

Expand Down Expand Up @@ -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))
Expand Down
66 changes: 55 additions & 11 deletions src/win/pipe.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}

Expand All @@ -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];
Expand All @@ -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.
Expand All @@ -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;
}
Expand All @@ -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;
}


Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
26 changes: 22 additions & 4 deletions test/test-pipe-getsockname.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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));
Expand Down

0 comments on commit 6be130e

Please sign in to comment.