You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
180 lines
7.3 KiB
180 lines
7.3 KiB
From 45a0af83129eb7dc244c5118360afc1972a686c7 Mon Sep 17 00:00:00 2001 |
|
From: Peter Hutterer <peter.hutterer@who-t.net> |
|
Date: Tue, 5 Jul 2022 09:50:41 +1000 |
|
Subject: [PATCH xserver 2/3] xkb: swap XkbSetDeviceInfo and |
|
XkbSetDeviceInfoCheck |
|
|
|
XKB often uses a FooCheck and Foo function pair, the former is supposed |
|
to check all values in the request and error out on BadLength, |
|
BadValue, etc. The latter is then called once we're confident the values |
|
are good (they may still fail on an individual device, but that's a |
|
different topic). |
|
|
|
In the case of XkbSetDeviceInfo, those functions were incorrectly |
|
named, with XkbSetDeviceInfo ending up as the checker function and |
|
XkbSetDeviceInfoCheck as the setter function. As a result, the setter |
|
function was called before the checker function, accessing request |
|
data and modifying device state before we ensured that the data is |
|
valid. |
|
|
|
In particular, the setter function relied on values being already |
|
byte-swapped. This in turn could lead to potential OOB memory access. |
|
|
|
Fix this by correctly naming the functions and moving the length checks |
|
over to the checker function. These were added in 87c64fc5b0 to the |
|
wrong function, probably due to the incorrect naming. |
|
|
|
Fixes ZDI-CAN 16070, CVE-2022-2320. |
|
|
|
This vulnerability was discovered by: |
|
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative |
|
|
|
Introduced in c06e27b2f6fd9f7b9f827623a48876a225264132 |
|
|
|
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> |
|
(cherry picked from commit dd8caf39e9e15d8f302e54045dd08d8ebf1025dc) |
|
--- |
|
xkb/xkb.c | 46 +++++++++++++++++++++++++--------------------- |
|
1 file changed, 25 insertions(+), 21 deletions(-) |
|
|
|
diff --git a/xkb/xkb.c b/xkb/xkb.c |
|
index 684394d77..36464a770 100644 |
|
--- a/xkb/xkb.c |
|
+++ b/xkb/xkb.c |
|
@@ -6554,7 +6554,8 @@ ProcXkbGetDeviceInfo(ClientPtr client) |
|
static char * |
|
CheckSetDeviceIndicators(char *wire, |
|
DeviceIntPtr dev, |
|
- int num, int *status_rtrn, ClientPtr client) |
|
+ int num, int *status_rtrn, ClientPtr client, |
|
+ xkbSetDeviceInfoReq * stuff) |
|
{ |
|
xkbDeviceLedsWireDesc *ledWire; |
|
int i; |
|
@@ -6562,6 +6563,11 @@ CheckSetDeviceIndicators(char *wire, |
|
|
|
ledWire = (xkbDeviceLedsWireDesc *) wire; |
|
for (i = 0; i < num; i++) { |
|
+ if (!_XkbCheckRequestBounds(client, stuff, ledWire, ledWire + 1)) { |
|
+ *status_rtrn = BadLength; |
|
+ return (char *) ledWire; |
|
+ } |
|
+ |
|
if (client->swapped) { |
|
swaps(&ledWire->ledClass); |
|
swaps(&ledWire->ledID); |
|
@@ -6589,6 +6595,11 @@ CheckSetDeviceIndicators(char *wire, |
|
atomWire = (CARD32 *) &ledWire[1]; |
|
if (nNames > 0) { |
|
for (n = 0; n < nNames; n++) { |
|
+ if (!_XkbCheckRequestBounds(client, stuff, atomWire, atomWire + 1)) { |
|
+ *status_rtrn = BadLength; |
|
+ return (char *) atomWire; |
|
+ } |
|
+ |
|
if (client->swapped) { |
|
swapl(atomWire); |
|
} |
|
@@ -6600,6 +6611,10 @@ CheckSetDeviceIndicators(char *wire, |
|
mapWire = (xkbIndicatorMapWireDesc *) atomWire; |
|
if (nMaps > 0) { |
|
for (n = 0; n < nMaps; n++) { |
|
+ if (!_XkbCheckRequestBounds(client, stuff, mapWire, mapWire + 1)) { |
|
+ *status_rtrn = BadLength; |
|
+ return (char *) mapWire; |
|
+ } |
|
if (client->swapped) { |
|
swaps(&mapWire->virtualMods); |
|
swapl(&mapWire->ctrls); |
|
@@ -6651,11 +6666,6 @@ SetDeviceIndicators(char *wire, |
|
xkbIndicatorMapWireDesc *mapWire; |
|
XkbSrvLedInfoPtr sli; |
|
|
|
- if (!_XkbCheckRequestBounds(client, stuff, ledWire, ledWire + 1)) { |
|
- *status_rtrn = BadLength; |
|
- return (char *) ledWire; |
|
- } |
|
- |
|
namec = mapc = statec = 0; |
|
sli = XkbFindSrvLedInfo(dev, ledWire->ledClass, ledWire->ledID, |
|
XkbXI_IndicatorMapsMask); |
|
@@ -6674,10 +6684,6 @@ SetDeviceIndicators(char *wire, |
|
memset((char *) sli->names, 0, XkbNumIndicators * sizeof(Atom)); |
|
for (n = 0, bit = 1; n < XkbNumIndicators; n++, bit <<= 1) { |
|
if (ledWire->namesPresent & bit) { |
|
- if (!_XkbCheckRequestBounds(client, stuff, atomWire, atomWire + 1)) { |
|
- *status_rtrn = BadLength; |
|
- return (char *) atomWire; |
|
- } |
|
sli->names[n] = (Atom) *atomWire; |
|
if (sli->names[n] == None) |
|
ledWire->namesPresent &= ~bit; |
|
@@ -6695,10 +6701,6 @@ SetDeviceIndicators(char *wire, |
|
if (ledWire->mapsPresent) { |
|
for (n = 0, bit = 1; n < XkbNumIndicators; n++, bit <<= 1) { |
|
if (ledWire->mapsPresent & bit) { |
|
- if (!_XkbCheckRequestBounds(client, stuff, mapWire, mapWire + 1)) { |
|
- *status_rtrn = BadLength; |
|
- return (char *) mapWire; |
|
- } |
|
sli->maps[n].flags = mapWire->flags; |
|
sli->maps[n].which_groups = mapWire->whichGroups; |
|
sli->maps[n].groups = mapWire->groups; |
|
@@ -6734,13 +6736,17 @@ SetDeviceIndicators(char *wire, |
|
} |
|
|
|
static int |
|
-_XkbSetDeviceInfo(ClientPtr client, DeviceIntPtr dev, |
|
+_XkbSetDeviceInfoCheck(ClientPtr client, DeviceIntPtr dev, |
|
xkbSetDeviceInfoReq * stuff) |
|
{ |
|
char *wire; |
|
|
|
wire = (char *) &stuff[1]; |
|
if (stuff->change & XkbXI_ButtonActionsMask) { |
|
+ int sz = stuff->nBtns * SIZEOF(xkbActionWireDesc); |
|
+ if (!_XkbCheckRequestBounds(client, stuff, wire, (char *) wire + sz)) |
|
+ return BadLength; |
|
+ |
|
if (!dev->button) { |
|
client->errorValue = _XkbErrCode2(XkbErr_BadClass, ButtonClass); |
|
return XkbKeyboardErrorCode; |
|
@@ -6751,13 +6757,13 @@ _XkbSetDeviceInfo(ClientPtr client, DeviceIntPtr dev, |
|
dev->button->numButtons); |
|
return BadMatch; |
|
} |
|
- wire += (stuff->nBtns * SIZEOF(xkbActionWireDesc)); |
|
+ wire += sz; |
|
} |
|
if (stuff->change & XkbXI_IndicatorsMask) { |
|
int status = Success; |
|
|
|
wire = CheckSetDeviceIndicators(wire, dev, stuff->nDeviceLedFBs, |
|
- &status, client); |
|
+ &status, client, stuff); |
|
if (status != Success) |
|
return status; |
|
} |
|
@@ -6768,8 +6774,8 @@ _XkbSetDeviceInfo(ClientPtr client, DeviceIntPtr dev, |
|
} |
|
|
|
static int |
|
-_XkbSetDeviceInfoCheck(ClientPtr client, DeviceIntPtr dev, |
|
- xkbSetDeviceInfoReq * stuff) |
|
+_XkbSetDeviceInfo(ClientPtr client, DeviceIntPtr dev, |
|
+ xkbSetDeviceInfoReq * stuff) |
|
{ |
|
char *wire; |
|
xkbExtensionDeviceNotify ed; |
|
@@ -6793,8 +6799,6 @@ _XkbSetDeviceInfoCheck(ClientPtr client, DeviceIntPtr dev, |
|
if (stuff->firstBtn + stuff->nBtns > nBtns) |
|
return BadValue; |
|
sz = stuff->nBtns * SIZEOF(xkbActionWireDesc); |
|
- if (!_XkbCheckRequestBounds(client, stuff, wire, (char *) wire + sz)) |
|
- return BadLength; |
|
memcpy((char *) &acts[stuff->firstBtn], (char *) wire, sz); |
|
wire += sz; |
|
ed.reason |= XkbXI_ButtonActionsMask; |
|
-- |
|
2.36.1 |
|
|
|
|