From af3602b48c5454d4528b43f1ebdfb08c79f0030f Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Tue, 10 Aug 2021 09:33:33 +0100 Subject: [PATCH] Fix out-of-bounds copy in xcb_send_event() calls xcb_send_event always copies 32 bytes, so we have to pad all xcb_*_event_t to 32 bytes to avoid leaking uninitialized stack memory. I found this problem while running kwin_x11 on a CHERI-RISC-V system (which has bounded pointers). The xcb_send_event() implementation has a memcpy() that was copying 32 bytes but the event passed was a bounded to 28 bytes, so this resulted in a run-time exception in X11Client::sendClientMessage(). The same problem exists in Selection::sendSelectionNotify(), but this time we could end up copying up to 8 bytes since xcb_selection_notify_event_t is only 24 bytes. This disclosure of uninitialized data could in theory have a security impact if it leaks a pointer value (e.g. a return address) as part of an exploit chain that needs to bypass ASLR. However, the selection notify events go directly to the XServer and you most likely already have a serious problem if an attacker has full control over the XServer. It is possible that the configure notify events go directly to an untrusted client, but even if they do this leak is not directly exploitable. See also https://gitlab.freedesktop.org/xorg/lib/libxcb/-/issues/18 --- src/x11client.cpp | 37 ++++++++++++++++++++++++------------- src/xwl/drag.cpp | 1 + src/xwl/selection.cpp | 29 ++++++++++++++++------------- 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/src/x11client.cpp b/src/x11client.cpp index 59e150cbbd..456b83fcfe 100644 --- a/src/x11client.cpp +++ b/src/x11client.cpp @@ -1745,6 +1745,10 @@ void X11Client::updateHiddenPreview() void X11Client::sendClientMessage(xcb_window_t w, xcb_atom_t a, xcb_atom_t protocol, uint32_t data1, uint32_t data2, uint32_t data3) { xcb_client_message_event_t ev; + // Every X11 event is 32 bytes (see man xcb_send_event), so XCB will copy + // 32 unconditionally. Add a static_assert to ensure we don't disclose + // stack memory. + static_assert(sizeof(ev) == 32, "Would leak stack data otherwise"); memset(&ev, 0, sizeof(ev)); ev.response_type = XCB_CLIENT_MESSAGE; ev.window = w; @@ -3650,19 +3654,26 @@ QSize X11Client::basicUnit() const */ void X11Client::sendSyntheticConfigureNotify() { - xcb_configure_notify_event_t c; - memset(&c, 0, sizeof(c)); - c.response_type = XCB_CONFIGURE_NOTIFY; - c.event = window(); - c.window = window(); - c.x = m_clientGeometry.x(); - c.y = m_clientGeometry.y(); - c.width = m_clientGeometry.width(); - c.height = m_clientGeometry.height(); - c.border_width = 0; - c.above_sibling = XCB_WINDOW_NONE; - c.override_redirect = 0; - xcb_send_event(connection(), true, c.event, XCB_EVENT_MASK_STRUCTURE_NOTIFY, reinterpret_cast(&c)); + // Every X11 event is 32 bytes (see man xcb_send_event), so XCB will copy + // 32 unconditionally. Use a union to ensure we don't disclose stack memory. + union { + xcb_configure_notify_event_t event; + char buffer[32]; + } u; + static_assert(sizeof(u.event) < 32, "wouldn't need the union otherwise"); + memset(&u, 0, sizeof(u)); + xcb_configure_notify_event_t &c = u.event; + u.event.response_type = XCB_CONFIGURE_NOTIFY; + u.event.event = window(); + u.event.window = window(); + u.event.x = m_clientGeometry.x(); + u.event.y = m_clientGeometry.y(); + u.event.width = m_clientGeometry.width(); + u.event.height = m_clientGeometry.height(); + u.event.border_width = 0; + u.event.above_sibling = XCB_WINDOW_NONE; + u.event.override_redirect = 0; + xcb_send_event(connection(), true, c.event, XCB_EVENT_MASK_STRUCTURE_NOTIFY, reinterpret_cast(&u)); xcb_flush(connection()); } diff --git a/src/xwl/drag.cpp b/src/xwl/drag.cpp index 74c6091acf..b74f5ad57c 100644 --- a/src/xwl/drag.cpp +++ b/src/xwl/drag.cpp @@ -35,6 +35,7 @@ void Drag::sendClientMessage(xcb_window_t target, xcb_atom_t type, xcb_client_me type, // type *data, // data }; + static_assert(sizeof(ev) == 32, "Would leak stack data otherwise"); xcb_connection_t *xcbConn = kwinApp()->x11Connection(); xcb_send_event(xcbConn, diff --git a/src/xwl/selection.cpp b/src/xwl/selection.cpp index c9eff6c3e0..64a5f3c0a4 100644 --- a/src/xwl/selection.cpp +++ b/src/xwl/selection.cpp @@ -134,21 +134,24 @@ bool Selection::filterEvent(xcb_generic_event_t *event) void Selection::sendSelectionNotify(xcb_selection_request_event_t *event, bool success) { - xcb_selection_notify_event_t notify; - notify.response_type = XCB_SELECTION_NOTIFY; - notify.sequence = 0; - notify.time = event->time; - notify.requestor = event->requestor; - notify.selection = event->selection; - notify.target = event->target; - notify.property = success ? event->property : xcb_atom_t(XCB_ATOM_NONE); + // Every X11 event is 32 bytes (see man xcb_send_event), so XCB will copy + // 32 unconditionally. Use a union to ensure we don't disclose stack memory. + union { + xcb_selection_notify_event_t notify; + char buffer[32]; + } u; + memset(&u, 0, sizeof(u)); + static_assert(sizeof(u.notify) < 32, "wouldn't need the union otherwise"); + u.notify.response_type = XCB_SELECTION_NOTIFY; + u.notify.sequence = 0; + u.notify.time = event->time; + u.notify.requestor = event->requestor; + u.notify.selection = event->selection; + u.notify.target = event->target; + u.notify.property = success ? event->property : xcb_atom_t(XCB_ATOM_NONE); xcb_connection_t *xcbConn = kwinApp()->x11Connection(); - xcb_send_event(xcbConn, - 0, - event->requestor, - XCB_EVENT_MASK_NO_EVENT, - (const char *)¬ify); + xcb_send_event(xcbConn, 0, event->requestor, XCB_EVENT_MASK_NO_EVENT, (const char *)&u); xcb_flush(xcbConn); }