Attachment 112359 Details for Bug 88507 – Proposed patch

[patch] Proposed patch

0002-xkb-Check-strings-length-against-request-size.patch (text/plain), 4.59 KB, created by Olivier Fourdan on 2015-01-16 19:20:59 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Olivier Fourdan

Created: 2015-01-16 19:20:59 UTC

Size: 4.59 KB

patch

obsolete

>From 68c9d812702c4f852928f16f30fc44ceffa6b281 Mon Sep 17 00:00:00 2001
>From: Olivier Fourdan <ofourdan@redhat.com>
>Date: Fri, 16 Jan 2015 08:44:45 +0100
>Subject: [PATCH 2/2] xkb: Check strings length against request size
>
>Ensure that the given strings length in an XkbSetGeometry request remain
>within the limits of the size of the request.
>
>Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
>---
> xkb/xkb.c | 63 ++++++++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 38 insertions(+), 25 deletions(-)
>
>diff --git a/xkb/xkb.c b/xkb/xkb.c
>index b9a3ac4..e0c5b63 100644
>--- a/xkb/xkb.c
>+++ b/xkb/xkb.c
>@@ -4957,25 +4957,29 @@ ProcXkbGetGeometry(ClientPtr client)
> 
> /***====================================================================***/
> 
>-static char *
>-_GetCountedString(char **wire_inout, Bool swap)
>+static Status
>+_GetCountedString(char **wire_inout, ClientPtr client, char **str)
> {
>-    char *wire, *str;
>+    char *wire, *next;
>     CARD16 len;
> 
>     wire = *wire_inout;
>     len = *(CARD16 *) wire;
>-    if (swap) {
>+    if (client->swapped) {
>         swaps(&len);
>     }
>-    str = malloc(len + 1);
>-    if (str) {
>-        memcpy(str, &wire[2], len);
>-        str[len] = '\0';
>-    }
>-    wire += XkbPaddedSize(len + 2);
>-    *wire_inout = wire;
>-    return str;
>+    next = wire + XkbPaddedSize(len + 2);
>+    /* Check we're still within the size of the request */
>+    if (client->req_len <
>+        bytes_to_int32(next - (char *) client->requestBuffer))
>+        return BadRequest;
>+    *str = malloc(len + 1);
>+    if (!*str)
>+        return BadAlloc;
>+    memcpy(*str, &wire[2], len);
>+    *(*str + (char *) len) = '\0';
>+    *wire_inout = next;
>+    return Success;
> }
> 
> static Status
>@@ -4987,6 +4991,7 @@ _CheckSetDoodad(char **wire_inout,
>     xkbAnyDoodadWireDesc any;
>     xkbTextDoodadWireDesc text;
>     XkbDoodadPtr doodad;
>+    Status status;
> 
>     dWire = (xkbDoodadWireDesc *) (*wire_inout);
>     any = dWire->any;
>@@ -5036,8 +5041,12 @@ _CheckSetDoodad(char **wire_inout,
>         doodad->text.width = text.width;
>         doodad->text.height = text.height;
>         doodad->text.color_ndx = dWire->text.colorNdx;
>-        doodad->text.text = _GetCountedString(&wire, client->swapped);
>-        doodad->text.font = _GetCountedString(&wire, client->swapped);
>+        status = _GetCountedString(&wire, client, &doodad->text.text);
>+        if (status != Success)
>+            return status;
>+        status = _GetCountedString(&wire, client, &doodad->text.font);
>+        if (status != Success)
>+            return status;
>         break;
>     case XkbIndicatorDoodad:
>         if (dWire->indicator.onColorNdx >= geom->num_colors) {
>@@ -5072,7 +5081,9 @@ _CheckSetDoodad(char **wire_inout,
>         }
>         doodad->logo.color_ndx = dWire->logo.colorNdx;
>         doodad->logo.shape_ndx = dWire->logo.shapeNdx;
>-        doodad->logo.logo_name = _GetCountedString(&wire, client->swapped);
>+        status = _GetCountedString(&wire, client, &doodad->logo.logo_name);
>+        if (status != Success)
>+            return status;
>         break;
>     default:
>         client->errorValue = _XkbErrCode2(0x4F, dWire->any.type);
>@@ -5304,18 +5315,20 @@ _CheckSetGeom(XkbGeometryPtr geom, xkbSetGeometryReq * req, ClientPtr client)
>     char *wire;
> 
>     wire = (char *) &req[1];
>-    geom->label_font = _GetCountedString(&wire, client->swapped);
>+    status = _GetCountedString(&wire, client, &geom->label_font);
>+    if (status != Success)
>+        return status;
> 
>     for (i = 0; i < req->nProperties; i++) {
>         char *name, *val;
> 
>-        name = _GetCountedString(&wire, client->swapped);
>-        if (!name)
>-            return BadAlloc;
>-        val = _GetCountedString(&wire, client->swapped);
>-        if (!val) {
>+        status = _GetCountedString(&wire, client, &name);
>+        if (status != Success)
>+            return status;
>+        status = _GetCountedString(&wire, client, &val);
>+        if (status != Success) {
>             free(name);
>-            return BadAlloc;
>+            return status;
>         }
>         if (XkbAddGeomProperty(geom, name, val) == NULL) {
>             free(name);
>@@ -5349,9 +5362,9 @@ _CheckSetGeom(XkbGeometryPtr geom, xkbSetGeometryReq * req, ClientPtr client)
>     for (i = 0; i < req->nColors; i++) {
>         char *name;
> 
>-        name = _GetCountedString(&wire, client->swapped);
>-        if (!name)
>-            return BadAlloc;
>+        status = _GetCountedString(&wire, client, &name);
>+        if (status != Success)
>+            return status;
>         if (!XkbAddGeomColor(geom, name, geom->num_colors)) {
>             free(name);
>             return BadAlloc;
>-- 
>2.1.0
>

Actions: View | Splinter Review

Attachments on bug 88507: 112359 | 112450

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.