[
https://issues.apache.org/jira/browse/THRIFT-5933?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Pengpeng Hou updated THRIFT-5933:
---------------------------------
Description:
Hello,
I reviewed current upstream head of the C GLib transport and found a real stack
overflow bug in the Unix-socket open path.
In `lib/c_glib/src/thrift/c_glib/transport/thrift_socket.c`, the Unix-socket
open path creates a stack `struct sockaddr_un pin`, zeroes it, sets
`sun_family`, and then copies `tsocket->path` with:
memcpy(pin.sun_path, tsocket->path, strlen(tsocket->path) + 1);
The path is populated via the GObject property setter and is simply duplicated
with `g_strdup(g_value_get_string(value))`. There is no length validation at
that point, and there is no local guard before the copy into `sun_path`.
Because `sun_path` is a fixed-size field in `struct sockaddr_un`, an oversized
Unix socket path can overflow the stack object before `connect()` is even
called.
*How to reproduce*
1. Build current Apache Thrift with the C GLib transport.
2. Configure a `ThriftSocket` with a Unix domain socket path longer than
`sizeof(struct sockaddr_un.sun_path) - 1`.
3. Call `thrift_socket_open()`.
4. The function copies the entire string into `pin.sun_path` with `memcpy()`
and overflows the local stack `sockaddr_un`.
was:
Hello,
I reviewed current upstream head of the C GLib transport and found a real stack
overflow bug in the Unix-socket open path.
In `lib/c_glib/src/thrift/c_glib/transport/thrift_socket.c`, the Unix-socket
open path creates a stack `struct sockaddr_un pin`, zeroes it, sets
`sun_family`, and then copies `tsocket->path` with:
memcpy(pin.sun_path, tsocket->path, strlen(tsocket->path) + 1);
The path is populated via the GObject property setter and is simply duplicated
with `g_strdup(g_value_get_string(value))`. There is no length validation at
that point, and there is no local guard before the copy into `sun_path`.
Because `sun_path` is a fixed-size field in `struct sockaddr_un`, an oversized
Unix socket path can overflow the stack object before `connect()` is even
called.
This is the client-side twin of the same Unix-socket-path problem in the server
transport. If you prefer to keep the reports separate, this one can stay
focused on `thrift_socket_open()` only.
*How to reproduce*
1. Build current Apache Thrift with the C GLib transport.
2. Configure a `ThriftSocket` with a Unix domain socket path longer than
`sizeof(struct sockaddr_un.sun_path) - 1`.
3. Call `thrift_socket_open()`.
4. The function copies the entire string into `pin.sun_path` with `memcpy()`
and overflows the local stack `sockaddr_un`.
> thrift_socket_open() copies Unix socket paths into sockaddr_un.sun_path
> without length validation
> -------------------------------------------------------------------------------------------------
>
> Key: THRIFT-5933
> URL: https://issues.apache.org/jira/browse/THRIFT-5933
> Project: Thrift
> Issue Type: Bug
> Reporter: Pengpeng Hou
> Priority: Major
>
> Hello,
> I reviewed current upstream head of the C GLib transport and found a real
> stack overflow bug in the Unix-socket open path.
> In `lib/c_glib/src/thrift/c_glib/transport/thrift_socket.c`, the Unix-socket
> open path creates a stack `struct sockaddr_un pin`, zeroes it, sets
> `sun_family`, and then copies `tsocket->path` with:
> memcpy(pin.sun_path, tsocket->path, strlen(tsocket->path) + 1);
> The path is populated via the GObject property setter and is simply
> duplicated with `g_strdup(g_value_get_string(value))`. There is no length
> validation at that point, and there is no local guard before the copy into
> `sun_path`. Because `sun_path` is a fixed-size field in `struct sockaddr_un`,
> an oversized Unix socket path can overflow the stack object before
> `connect()` is even called.
>
> *How to reproduce*
> 1. Build current Apache Thrift with the C GLib transport.
> 2. Configure a `ThriftSocket` with a Unix domain socket path longer than
> `sizeof(struct sockaddr_un.sun_path) - 1`.
> 3. Call `thrift_socket_open()`.
> 4. The function copies the entire string into `pin.sun_path` with `memcpy()`
> and overflows the local stack `sockaddr_un`.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
