uninitialized read / out-of-bounds read in wl_connection_demarshal()
When a Wayland server parses an incoming message, wl_client_connection_data() extracts the opcode and size:
wl_connection_copy(connection, p, sizeof p);
opcode = p[1] & 0xffff;
size = p[1] >> 16;
if (len < size)
break;
size
is checked against len
to ensure that enough data is available to handle the request; however, there is no lower bound for size
. size
is then passed to wl_connection_demarshal():
closure = wl_connection_demarshal(client->connection, size,
&client->objects, message);
wl_connection_demarshal() then runs the following code:
struct wl_closure *
wl_connection_demarshal(struct wl_connection *connection,
uint32_t size,
struct wl_map *objects,
const struct wl_message *message)
{
uint32_t *p, *next, *end, length, id;
[...]
int i, count, num_arrays;
[...]
struct wl_closure *closure;
[...]
closure = wl_closure_init(message, size, &num_arrays, NULL);
if (closure == NULL) {
wl_connection_consume(connection, size);
return NULL;
}
count = closure->count;
[...]
p = (uint32_t *)(closure->extra + num_arrays);
end = p + size / sizeof *p;
// audit note: reads `size` bytes into `p`
wl_connection_copy(connection, p, size);
closure->sender_id = *p++;
closure->opcode = *p++ & 0x0000ffff;
wl_closure_init() starts as follows:
static struct wl_closure *
wl_closure_init(const struct wl_message *message, uint32_t size,
int *num_arrays, union wl_argument *args)
{
struct wl_closure *closure;
[...]
if (size) {
*num_arrays = wl_message_count_arrays(message);
closure = malloc(sizeof *closure + size +
*num_arrays * sizeof(struct wl_array));
} else {
closure = malloc(sizeof *closure);
}
The case where size == 0
is special-cased to have different semantics; in particular, it does not initialize *num_arrays
. This means that when wl_connection_demarshal()
is invoked with size == 0
, the calculation p = (uint32_t *)(closure->extra + num_arrays)
will be performed on an uninitialized value, potentially creating a garbage pointer that is then accessed in the line closure->sender_id = *p++;
.
I patched the client to send a size of 0 in serialize_closure(); this resulted in the following server crash:
Thread 1 "weston" received signal SIGSEGV, Segmentation fault.
0xb797c1cc in wl_connection_demarshal (connection=0xa3c5a200, size=0, objects=0xa7f71a18,
message=0xb798dcac <wl_display_requests+12>) at src/connection.c:703
703 closure->sender_id = *p++;
(gdb) print closure
$1 = (struct wl_closure *) 0xa7f71950
(gdb) print num_arrays
$2 = 1102416563
(gdb) print p
$3 = (uint32_t *) 0xbc799a24
When size
isn't zero, but a small value like 4, the code still performs an out-of-bounds read in the following lines:
closure->sender_id = *p++;
closure->opcode = *p++ & 0x0000ffff;
In this case, ASAN reports an out-of-bounds access in the line closure->opcode = *p++ & 0x0000ffff;
:
=================================================================
==20568==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xa7775e80 at pc 0xb712625f bp 0xbfc0e178 sp 0xbfc0e16c
READ of size 4 at 0xa7775e80 thread T0
#0 0xb712625e in wl_connection_demarshal src/connection.c:704
#1 0xb711868a in wl_client_connection_data src/wayland-server.c:398
#2 0xb7120427 in wl_event_source_fd_dispatch src/event-loop.c:95
#3 0xb71222d2 in wl_event_loop_dispatch src/event-loop.c:641
#4 0xb711b7c4 in wl_display_run src/wayland-server.c:1260
#5 0x447559 in main compositor/main.c:2582
#6 0xb6dcb285 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x18285)
#7 0x43c4c0 (/home/user/install/bin/weston+0x54c0)
0xa7775e80 is located 0 bytes to the right of 112-byte region [0xa7775e10,0xa7775e80)
allocated by thread T0 here:
#0 0xb7298194 in malloc (/usr/lib/i386-linux-gnu/libasan.so.3+0xbe194)
#1 0xb7125571 in wl_closure_init src/connection.c:563
#2 0xb71260b0 in wl_connection_demarshal src/connection.c:690
#3 0xb711868a in wl_client_connection_data src/wayland-server.c:398
#4 0xb7120427 in wl_event_source_fd_dispatch src/event-loop.c:95
#5 0xb71222d2 in wl_event_loop_dispatch src/event-loop.c:641
#6 0xb711b7c4 in wl_display_run src/wayland-server.c:1260
#7 0x447559 in main compositor/main.c:2582
#8 0xb6dcb285 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x18285)
SUMMARY: AddressSanitizer: heap-buffer-overflow src/connection.c:704 in wl_connection_demarshal
Shadow bytes around the buggy address:
0x34eeeb80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x34eeeb90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x34eeeba0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x34eeebb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x34eeebc0: fa fa 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x34eeebd0:[fa]fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x34eeebe0: fd fd fd fd fd fd fa fa fa fa fa fa fa fa fd fd
0x34eeebf0: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
0x34eeec00: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
0x34eeec10: fd fd fa fa fa fa fa fa fa fa 00 00 00 00 00 00
0x34eeec20: 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Heap right redzone: fb
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack partial redzone: f4
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==20568==ABORTING
As far as I can tell, these issues lead to server crashes at worst (depending on how the compiler decided to place things on the stack) and can't cause information disclosure: closure->sender_id
and closure->opcode
don't seem to be used, at least on the server; no other data is read from the input buffer, and the missing array space at the end of the closure is never accessed.
Apart from the straightforward fix, it might be a good idea to also avoid overloading length parameters with magic values that change the semantics of the code.