[
https://issues.apache.org/jira/browse/THRIFT-5930?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Pengpeng Hou updated THRIFT-5930:
---------------------------------
Description:
Hi，
I rechecked current upstream head of the C GLib transport and this still looks
like a real stack overflow bug.
In `lib/c_glib/src/thrift/c_glib/transport/thrift_server_socket.c`, the
Unix-socket bind 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 same transport family has the matching client-side open path in
`thrift_socket.c`, but this issue is specifically about the server-side
bind/open path. The source path is stored via a GObject property and is not
length-validated before the copy. `sun_path` is a fixed-size field in `struct
sockaddr_un`, so an oversized Unix socket path can overflow the stack object
before bind/listen runs.
I rechecked the current code and did not find any local guard that limits the
copy to `sizeof(pin.sun_path) - 1`.
*How to reproduce*
1. Build current Apache Thrift with the C GLib transport.
2. Create or configure a `ThriftServerSocket` with a Unix domain socket path
longer than `sizeof(struct sockaddr_un.sun_path) - 1`.
3. Call the server-side open/bind path so it reaches the `memcpy()` into
`pin.sun_path`.
4. The copy overflows the local stack `sockaddr_un` object before the socket is
bound.
was:
Hi，
I rechecked current upstream head of the C GLib transport and this still looks
like a real stack overflow bug.
In `lib/c_glib/src/thrift/c_glib/transport/thrift_server_socket.c`, the
Unix-socket bind 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 same transport family has the matching client-side open path in
`thrift_socket.c`, but this issue is specifically about the server-side
bind/open path. The source path is stored via a GObject property and is not
length-validated before the copy. `sun_path` is a fixed-size field in `struct
sockaddr_un`, so an oversized Unix socket path can overflow the stack object
before bind/listen runs.
I rechecked the current code and did not find any local guard that limits the
copy to `sizeof(pin.sun_path) - 1`.
*How to reproduce*
1. Build current Apache Thrift with the C GLib transport.
2. Create or configure a `ThriftServerSocket` with a Unix domain socket path
longer than `sizeof(struct sockaddr_un.sun_path) - 1`.
3. Call the server-side open/bind path so it reaches the `memcpy()` into
`pin.sun_path`.
4. The copy overflows the local stack `sockaddr_un` object before the socket is
bound.
This is easiest to observe with ASan, but the bug is present even without it
because the destination is a fixed-size stack field and the copy length is
taken directly from `strlen(tsocket->path) + 1`.
> thrift_server_socket() copies Unix socket paths into sockaddr_un.sun_path
> without bounds checking
> -------------------------------------------------------------------------------------------------
>
> Key: THRIFT-5930
> URL: https://issues.apache.org/jira/browse/THRIFT-5930
> Project: Thrift
> Issue Type: Bug
> Reporter: Pengpeng Hou
> Priority: Major
>
> Hi，
> I rechecked current upstream head of the C GLib transport and this still
> looks like a real stack overflow bug.
> In `lib/c_glib/src/thrift/c_glib/transport/thrift_server_socket.c`, the
> Unix-socket bind 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 same transport family has the matching client-side open path in
> `thrift_socket.c`, but this issue is specifically about the server-side
> bind/open path. The source path is stored via a GObject property and is not
> length-validated before the copy. `sun_path` is a fixed-size field in `struct
> sockaddr_un`, so an oversized Unix socket path can overflow the stack object
> before bind/listen runs.
> I rechecked the current code and did not find any local guard that limits the
> copy to `sizeof(pin.sun_path) - 1`.
>
> *How to reproduce*
> 1. Build current Apache Thrift with the C GLib transport.
> 2. Create or configure a `ThriftServerSocket` with a Unix domain socket path
> longer than `sizeof(struct sockaddr_un.sun_path) - 1`.
> 3. Call the server-side open/bind path so it reaches the `memcpy()` into
> `pin.sun_path`.
> 4. The copy overflows the local stack `sockaddr_un` object before the socket
> is bound.
>
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
