xserver: Branch 'master' - 4 commits

[GitLab Mirror]

 xkb/xkb.c |   54 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 12 deletions(-)

New commits:
commit 11beef0b7f1ed290348e45618e5fa0d2bffcb72e
Author: Peter Hutterer <peter.hutterer at who-t.net>
Date:   Tue Jul 5 12:06:20 2022 +1000

    xkb: proof GetCountedString against request length attacks
    
    GetCountedString did a check for the whole string to be within the
    request buffer but not for the initial 2 bytes that contain the length
    field. A swapped client could send a malformed request to trigger a
    swaps() on those bytes, writing into random memory.
    
    Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>

diff --git a/xkb/xkb.c b/xkb/xkb.c
index f42f59ef3..1841cff26 100644
--- a/xkb/xkb.c
+++ b/xkb/xkb.c
@@ -5137,6 +5137,11 @@ _GetCountedString(char **wire_inout, ClientPtr client, char **str)
     CARD16 len;
 
     wire = *wire_inout;
+
+    if (client->req_len <
+        bytes_to_int32(wire + 2 - (char *) client->requestBuffer))
+        return BadValue;
+
     len = *(CARD16 *) wire;
     if (client->swapped) {
         swaps(&len);
commit 1bb7767f19969ee6b109f7424ff97738752d18c9
Author: Peter Hutterer <peter.hutterer at who-t.net>
Date:   Wed Jul 13 11:38:16 2022 +1000

    xkb: length-check XkbListComponents before accessing the fields
    
    Each string length field was accessed before checking whether that byte
    was actually part of the client request. No real harm here since it
    would immediately fail with BadLength anyway, but let's be correct here.
    
    Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>

diff --git a/xkb/xkb.c b/xkb/xkb.c
index 0c920393d..f42f59ef3 100644
--- a/xkb/xkb.c
+++ b/xkb/xkb.c
@@ -5870,6 +5870,8 @@ ProcXkbListComponents(ClientPtr client)
      * length wrong. */
     str = (unsigned char *) &stuff[1];
     for (i = 0; i < 6; i++) {
+        if (!_XkbCheckRequestBounds(client, stuff, str, str + 1))
+            return BadLength;
         size = *((uint8_t *)str);
         len = (str + size + 1) - ((unsigned char *) stuff);
         if ((XkbPaddedSize(len) / 4) > stuff->length)
commit 44ae6f44192f5edeaf5ac082870bbdf3262071ef
Author: Peter Hutterer <peter.hutterer at who-t.net>
Date:   Wed Jul 13 11:33:00 2022 +1000

    xkb: length-check XkbGetKbdByName before accessing the fields
    
    This request accessed &stuff[1] before length-checking everything. The
    check was performed afterwards so invalid requests would return
    BadLength anyway, but let's do this before we actually access the
    memory.
    
    Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>

diff --git a/xkb/xkb.c b/xkb/xkb.c
index b79a269e3..0c920393d 100644
--- a/xkb/xkb.c
+++ b/xkb/xkb.c
@@ -5794,7 +5794,8 @@ static unsigned char componentExprLegal[] = {
 };
 
 static char *
-GetComponentSpec(unsigned char **pWire, Bool allowExpr, int *errRtrn)
+GetComponentSpec(ClientPtr client, xkbGetKbdByNameReq *stuff,
+                 unsigned char **pWire, Bool allowExpr, int *errRtrn)
 {
     int len;
     register int i;
@@ -5806,7 +5807,15 @@ GetComponentSpec(unsigned char **pWire, Bool allowExpr, int *errRtrn)
         legal = &componentSpecLegal[0];
 
     wire = *pWire;
+    if (!_XkbCheckRequestBounds(client, stuff, wire, wire + 1)) {
+        *errRtrn = BadLength;
+        return NULL;
+    }
     len = (*(unsigned char *) wire++);
+    if (!_XkbCheckRequestBounds(client, stuff, wire, wire + len)) {
+        *errRtrn = BadLength;
+        return NULL;
+    }
     if (len > 0) {
         str = calloc(1, len + 1);
         if (str) {
@@ -5936,17 +5945,17 @@ ProcXkbGetKbdByName(ClientPtr client)
     status = Success;
     str = (unsigned char *) &stuff[1];
     {
-        char *keymap = GetComponentSpec(&str, TRUE, &status);  /* keymap, unsupported */
+        char *keymap = GetComponentSpec(client, stuff, &str, TRUE, &status);  /* keymap, unsupported */
         if (keymap) {
             free(keymap);
             return BadMatch;
         }
     }
-    names.keycodes = GetComponentSpec(&str, TRUE, &status);
-    names.types = GetComponentSpec(&str, TRUE, &status);
-    names.compat = GetComponentSpec(&str, TRUE, &status);
-    names.symbols = GetComponentSpec(&str, TRUE, &status);
-    names.geometry = GetComponentSpec(&str, TRUE, &status);
+    names.keycodes = GetComponentSpec(client, stuff, &str, TRUE, &status);
+    names.types = GetComponentSpec(client, stuff, &str, TRUE, &status);
+    names.compat = GetComponentSpec(client, stuff, &str, TRUE, &status);
+    names.symbols = GetComponentSpec(client, stuff, &str, TRUE, &status);
+    names.geometry = GetComponentSpec(client, stuff, &str, TRUE, &status);
     if (status == Success) {
         len = str - ((unsigned char *) stuff);
         if ((XkbPaddedSize(len) / 4) != stuff->length)
commit 18f91b950e22c2a342a4fbc55e9ddf7534a707d2
Author: Peter Hutterer <peter.hutterer at who-t.net>
Date:   Wed Jul 13 11:23:09 2022 +1000

    xkb: fix some possible memleaks in XkbGetKbdByName
    
    GetComponentByName returns an allocated string, so let's free that if we
    fail somewhere.
    
    Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>

diff --git a/xkb/xkb.c b/xkb/xkb.c
index 4692895db..b79a269e3 100644
--- a/xkb/xkb.c
+++ b/xkb/xkb.c
@@ -5935,18 +5935,32 @@ ProcXkbGetKbdByName(ClientPtr client)
     xkb = dev->key->xkbInfo->desc;
     status = Success;
     str = (unsigned char *) &stuff[1];
-    if (GetComponentSpec(&str, TRUE, &status))  /* keymap, unsupported */
-        return BadMatch;
+    {
+        char *keymap = GetComponentSpec(&str, TRUE, &status);  /* keymap, unsupported */
+        if (keymap) {
+            free(keymap);
+            return BadMatch;
+        }
+    }
     names.keycodes = GetComponentSpec(&str, TRUE, &status);
     names.types = GetComponentSpec(&str, TRUE, &status);
     names.compat = GetComponentSpec(&str, TRUE, &status);
     names.symbols = GetComponentSpec(&str, TRUE, &status);
     names.geometry = GetComponentSpec(&str, TRUE, &status);
-    if (status != Success)
+    if (status == Success) {
+        len = str - ((unsigned char *) stuff);
+        if ((XkbPaddedSize(len) / 4) != stuff->length)
+            status = BadLength;
+    }
+
+    if (status != Success) {
+        free(names.keycodes);
+        free(names.types);
+        free(names.compat);
+        free(names.symbols);
+        free(names.geometry);
         return status;
-    len = str - ((unsigned char *) stuff);
-    if ((XkbPaddedSize(len) / 4) != stuff->length)
-        return BadLength;
+    }
 
     CHK_MASK_LEGAL(0x01, stuff->want, XkbGBN_AllComponentsMask);
     CHK_MASK_LEGAL(0x02, stuff->need, XkbGBN_AllComponentsMask);