[
https://issues.apache.org/jira/browse/THRIFT-5931?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Pengpeng Hou updated THRIFT-5931:
---------------------------------
Description:
Hi ,
I rechecked current upstream head and this SSL error formatting path still
looks unsafe.
In `lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c`,
`thrift_ssl_socket_get_ssl_error()` builds a formatted message in a fixed `char
buffer[1024]` and tracks the remaining space with an `int buffer_size = 1024`.
The code subtracts the return value of `snprintf()` from `buffer_size` and then
reuses that counter and the derived pointer for later appends:
buffer_size -= snprintf(buffer, buffer_size, "SSL %s: ", error_msg);
...
buffer_size -= snprintf(buffer + (1024 - buffer_size), buffer_size, "\n\t");
buffer_size -= snprintf(buffer + (1024 - buffer_size), buffer_size,
"%lX(%s) -> %s", ...);
The problem is that `snprintf()` returns the length it tried to produce, not a
clamped remaining size. Once the formatted text becomes longer than the
remaining space, `buffer_size` can go negative. After that, the next
subtraction and pointer calculation are no longer safe: the code can end up
passing a negative value as the size argument and computing a write pointer
beyond the end of the 1024-byte stack buffer.
I rechecked current head and did not find any guard that stops the append loop
before the remaining-space counter underflows.
*How to reproduce*
1. Build current Apache Thrift with the C GLib SSL transport.
2. Trigger a TLS failure path that produces enough OpenSSL error strings to
keep appending to the local 1024-byte buffer.
3. Once the formatted text exceeds the remaining space, `buffer_size`
underflows and later appends reuse the bad counter.
4. With sanitizers enabled, this shows up as an out-of-bounds write or invalid
pointer arithmetic in `thrift_ssl_socket_get_ssl_error()`.
was:
Hi ,
I rechecked current upstream head and this SSL error formatting path still
looks unsafe.
In `lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c`,
`thrift_ssl_socket_get_ssl_error()` builds a formatted message in a fixed `char
buffer[1024]` and tracks the remaining space with an `int buffer_size = 1024`.
The code subtracts the return value of `snprintf()` from `buffer_size` and then
reuses that counter and the derived pointer for later appends:
buffer_size -= snprintf(buffer, buffer_size, "SSL %s: ", error_msg);
...
buffer_size -= snprintf(buffer + (1024 - buffer_size), buffer_size, "\n\t");
buffer_size -= snprintf(buffer + (1024 - buffer_size), buffer_size,
"%lX(%s) -> %s", ...);
The problem is that `snprintf()` returns the length it tried to produce, not a
clamped remaining size. Once the formatted text becomes longer than the
remaining space, `buffer_size` can go negative. After that, the next
subtraction and pointer calculation are no longer safe: the code can end up
passing a negative value as the size argument and computing a write pointer
beyond the end of the 1024-byte stack buffer.
I rechecked current head and did not find any guard that stops the append loop
before the remaining-space counter underflows.
*How to reproduce*
1. Build current Apache Thrift with the C GLib SSL transport.
2. Trigger a TLS failure path that produces enough OpenSSL error strings to
keep appending to the local 1024-byte buffer.
3. Once the formatted text exceeds the remaining space, `buffer_size`
underflows and later appends reuse the bad counter.
4. With sanitizers enabled, this shows up as an out-of-bounds write or invalid
pointer arithmetic in `thrift_ssl_socket_get_ssl_error()`.
In practice, any long enough error stack or long enough formatted error content
can drive the same path.
> thrift_ssl_socket_get_ssl_error() can underflow its remaining-buffer counter
> and write past the stack buffer
> ------------------------------------------------------------------------------------------------------------
>
> Key: THRIFT-5931
> URL: https://issues.apache.org/jira/browse/THRIFT-5931
> Project: Thrift
> Issue Type: Bug
> Reporter: Pengpeng Hou
> Priority: Major
>
> Hi ,
> I rechecked current upstream head and this SSL error formatting path still
> looks unsafe.
> In `lib/c_glib/src/thrift/c_glib/transport/thrift_ssl_socket.c`,
> `thrift_ssl_socket_get_ssl_error()` builds a formatted message in a fixed
> `char buffer[1024]` and tracks the remaining space with an `int buffer_size =
> 1024`. The code subtracts the return value of `snprintf()` from `buffer_size`
> and then reuses that counter and the derived pointer for later appends:
> buffer_size -= snprintf(buffer, buffer_size, "SSL %s: ", error_msg);
> ...
> buffer_size -= snprintf(buffer + (1024 - buffer_size), buffer_size,
> "\n\t");
> buffer_size -= snprintf(buffer + (1024 - buffer_size), buffer_size,
> "%lX(%s) -> %s", ...);
> The problem is that `snprintf()` returns the length it tried to produce, not
> a clamped remaining size. Once the formatted text becomes longer than the
> remaining space, `buffer_size` can go negative. After that, the next
> subtraction and pointer calculation are no longer safe: the code can end up
> passing a negative value as the size argument and computing a write pointer
> beyond the end of the 1024-byte stack buffer.
> I rechecked current head and did not find any guard that stops the append
> loop before the remaining-space counter underflows.
>
> *How to reproduce*
> 1. Build current Apache Thrift with the C GLib SSL transport.
> 2. Trigger a TLS failure path that produces enough OpenSSL error strings to
> keep appending to the local 1024-byte buffer.
> 3. Once the formatted text exceeds the remaining space, `buffer_size`
> underflows and later appends reuse the bad counter.
> 4. With sanitizers enabled, this shows up as an out-of-bounds write or
> invalid pointer arithmetic in `thrift_ssl_socket_get_ssl_error()`.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
