From dd8caf39e9e15d8f302e54045dd08d8ebf1025dc 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>

---

 xkb/xkb.c | 46 +++++++++++++++++++++++++---------------------

 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/xkb/xkb.c b/xkb/xkb.c

index 64e52611e..34b2c290b 100644

--- a/xkb/xkb.c

+++ b/xkb/xkb.c

@@ -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;

-- 

2.36.1