frederik
/
xserver
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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 c06e27b2f6

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
This commit is contained in:
Peter Hutterer 2022-07-05 09:50:41 +10:00 committed by Povilas Kanapickas
parent f1070c01d6
commit dd8caf39e9
1 changed files with 25 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
xkb/xkb.c
View File

@ -6550,7 +6550,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;
@ -6558,6 +6559,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);
@ -6585,6 +6591,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);
}
@ -6596,6 +6607,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);
@ -6647,11 +6662,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);
@ -6670,10 +6680,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;
@ -6691,10 +6697,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;
@ -6730,13 +6732,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;
@ -6747,13 +6753,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;
}
@ -6764,8 +6770,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;
@ -6789,8 +6795,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;