Xtest: disallow GenericEvents in XTestSwapFakeInput
XTestSwapFakeInput assumes all events in this request are sizeof(xEvent) and iterates through these in 32-byte increments. However, a GenericEvent may be of arbitrary length longer than 32 bytes, so any GenericEvent in this list would result in subsequent events to be misparsed. Additional, the swapped event is written into a stack-allocated struct xEvent (size 32 bytes). For any GenericEvent longer than 32 bytes, swapping the event may thus smash the stack like an avocado on toast. Catch this case early and return BadValue for any GenericEvent. Which is what would happen in unswapped setups anyway since XTest doesn't support GenericEvent. CVE-2022-46340, ZDI-CAN 19265 This vulnerability was discovered by: Jan-Niklas Sohn working with Trend Micro Zero Day Initiative Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> Acked-by: Olivier Fourdan <ofourdan@redhat.com>
-
mentioned in merge request !1023 (merged)
-
mentioned in merge request !1024 (merged)
-
mentioned in merge request !1025 (merged)
-
mentioned in merge request !1026 (merged)
-
Frankly i don't see what needed to be fixed here by excluding GenericEvent. We are in the extension Xtest. https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/Xext/xtest.c The code is not relevant for any other extension.
For GenericEvent in this line: https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/Xext/xtest.c#L517 proc = EventSwapVector[evtype]; if evtype is GenericEvent:
GenericEvent is 35, see include/X11/X.h: https://gitlab.freedesktop.org/xorg/proto/xorgproto/-/blob/master/include/X11/X.h#L214 #define GenericEvent 35 Added by: Peter Hutterer 15 years ago.
The event swap procedure is registered as (caught in gdb): (gdb) print EventSwapVector[35] $8 = (EventSwapPtr) 0x563c1c05be21 (gdb) print proc $9 = (EventSwapPtr) 0x563c1c05be21 here: https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/Xext/geext.c#L201 Written by: Peter Hutterer 15 years ago.
The function can be found here: https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/Xext/geext.c#L167 Written by: Peter Hutterer, 16 years ago.
It only does anything on the event data, if there is a function ev_swap registered, written in the GEExtensions array by the initialization. The general initialization of the GEExtensions is done here: https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/Xext/geext.c#L187 It zeroes GEExtension right before setting the event swap procedure, here: memset(GEExtensions, 0, sizeof(GEExtensions)); https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/Xext/geext.c#L199 Written by: Peter Hutterer 16 years ago.
So for the Xtest extension, where we are here, nothing happens to the event data in the function SGEGenericEvent as evswap is NULL.
I don't see, what is outlined in the comment of the modification:
Additional, the swapped event is written into a stack-allocated struct xEvent (size 32 bytes). For any GenericEvent longer than 32 bytes, swapping the event may thus smash the stack like an avocado on toast.
Like shown, SGEGenericEvent does not do anything, so where should this happen ?
What indeed happens is, if the swapping procedure does not do anything, is this line: https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/Xext/xtest.c#L524 *ev = sev; If the (*proc) does not do anything with the local sev, the event in the list is overwritten with random data (sev is not initialized). This might cause weird effects, but as the size of sev is sizeof(xEvent), not any stack corruption.
-
Further examinations down and up the stack still show the same: This last modification is a non-fix for a non-issue, expanded to the bogus CVE 2022-46340. Also the explanations regarding different sizes of the xEvent structures and the looping over them are not correct. All event structures defined in https://gitlab.freedesktop.org/xorg/proto/xorgproto/-/blob/master/include/X11/Xproto.h#L991 have a size of 32 bytes including xGenericEvent https://gitlab.freedesktop.org/xorg/proto/xorgproto/-/blob/master/include/X11/Xproto.h#L1216 added by: Peter Hutterer 15 years ago. However a client can send a value in xReq -> length, that is not a multiple of 32 + sizeof(xReq), e.g. 36. Whatever is passed here, ReadRequestFromClient (see here:) https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/os/io.c#L227 and called here https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/dix/dispatch.c#L515 takes care, that enough data is read from the client and sufficient memory is allocated. So there is no problem. If the client sends a different number, there will be a protocol error. If there was something wrong here, there'd be a lot of other issues. In the XTest extension, where we are here, before the function in question can be called here: https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/Xext/xtest.c#L372 in ProcXTestFakeInput it is tested here: https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/Xext/xtest.c#L198 whether what is following xReq has a size of a multiple of 32 bytes, otherwise BadLength is returned (since at least 19 years). If it was not like that, particularly if too few data was sent not filling the 32 bytes, there whould be a generic access issue, not only regarding GenericEvent. So there can be only multiples of 32 bytes. The loop goes anyway only over complete 32-byte entities and remainders are left untouched. To have more than 1 generic event in a request seems to be an option that is not implemented on the client side of XTest, but still a client might send such stuff to the server. In any case ev->u.u.type value might be set to anything by a client. Whatever it is, all functions in this list https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/dix/tables.c#L585 defined here https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/dix/swaprep.c#L677 act on xEvent, what is 32 bytes. And as already outlined in an earlier post the function SGEGenericEvent https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/Xext/geext.c#L168 does nothing as i verified also by stepping through it in gdb. Theoretically another extension could change EventSwapVector[GenericEvent], but this is not the case and generic extensions obviously should use evswap, and this is NULL here in XTest.
I really wonder, what the guys at zerodayexploit have verified here. Maybe the have seen "proc" assigned some value, that is assumed to come from "some" extension probably not yet added or not examined in detail, whatever. I have the impression this was not really investigated. But the function called here is clearly in XTest and not used anywhere else. If we assume security problems in any function that is called and we do not examime what it actually does, we should not use any software anymore at all.