errata for X server infoleak
4 March, 2015 by tedu@tedunangst.com | openbsd
Patches are now available to fix an information leak in the XkbSetGeometry
request of X servers. For more information, see the X.org advisory.
We experienced a slight delay getting patches out, as you can see from the
date in the patch. This is a comparatively minor issue so we didn't rush
things until correctly signed patches were available.
http://www.x.org/wiki/Development/Security/Advisory-2015-02-10/
http://ftp.openbsd.org/pub/OpenBSD/patches/5.5/common/021_xserver.patch.sig
http://ftp.openbsd.org/pub/OpenBSD/patches/5.6/common/016_xserver.patch.sig
untrusted comment: signature from openbsd 5.6 base private key
RWR0EANmo9nqholgu2GQCCaaJuP9HvfU/V5+SgCtPaxbMZfHJRNbbCXzdsIWAL0Dfr9kMeNbiOs21lUgA4Ej3AFsptAdQsB9JQk
OpenBSD 5.6 errata 16, February 20, 2015:
Information leak in the XkbSetGeometry request of X servers
Olivier Fourdan from Red Hat has discovered a protocol handling issue
in the way the X server code base handles the XkbSetGeometry request.
Apply patch using:
signify -Vep /etc/signify/openbsd-56-base.pub -x 016_xserver.patch.sig \
-m - | (cd /usr/xenocara && patch -p0)
Then build and install a new xserver:
cd /usr/xenocara/xserver
make -f Makefile.bsd-wrapper obj
make -f Makefile.bsd-wrapper build
Index: xserver/xkb/xkb.c
diff -u xserver/xkb/xkb.c:1.10 xenocara/xserver/xkb/xkb.c:1.10.2.1
--- xserver/xkb/xkb.c:1.10 Fri May 2 21:27:51 2014
+++ xserver/xkb/xkb.c Thu Feb 12 08:29:20 2015
@@ -4957,26 +4957,29 @@
/***====================================================================***/
-static char *
-_GetCountedString(char **wire_inout, Bool swap)
+static Status
+_GetCountedString(char **wire_inout, ClientPtr client, char **str)
{
- char *wire, *str;
- CARD16 len, *plen;
+ char *wire, *next;
+ CARD16 len;
wire = *wire_inout;
- plen = (CARD16 *) wire;
- if (swap) {
- swaps(plen);
- }
- len = *plen;
- str = malloc(len + 1);
- if (str) {
- memcpy(str, &wire[2], len);
- str[len] = '\0';
+ len = *(CARD16 *) wire;
+ if (client->swapped) {
+ swaps(&len);
}
- 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 BadValue;
+ *str = malloc(len + 1);
+ if (!*str)
+ return BadAlloc;
+ memcpy(*str, &wire[2], len);
+ *(*str + len) = '\0';
+ *wire_inout = next;
+ return Success;
}
static Status
@@ -4985,25 +4988,29 @@
{
char *wire;
xkbDoodadWireDesc *dWire;
+ xkbAnyDoodadWireDesc any;
+ xkbTextDoodadWireDesc text;
XkbDoodadPtr doodad;
+ Status status;
dWire = (xkbDoodadWireDesc *) (*wire_inout);
+ any = dWire->any;
wire = (char *) &dWire[1];
if (client->swapped) {
- swapl(&dWire->any.name);
- swaps(&dWire->any.top);
- swaps(&dWire->any.left);
- swaps(&dWire->any.angle);
+ swapl(&any.name);
+ swaps(&any.top);
+ swaps(&any.left);
+ swaps(&any.angle);
}
CHK_ATOM_ONLY(dWire->any.name);
- doodad = XkbAddGeomDoodad(geom, section, dWire->any.name);
+ doodad = XkbAddGeomDoodad(geom, section, any.name);
if (!doodad)
return BadAlloc;
doodad->any.type = dWire->any.type;
doodad->any.priority = dWire->any.priority;
- doodad->any.top = dWire->any.top;
- doodad->any.left = dWire->any.left;
- doodad->any.angle = dWire->any.angle;
+ doodad->any.top = any.top;
+ doodad->any.left = any.left;
+ doodad->any.angle = any.angle;
switch (doodad->any.type) {
case XkbOutlineDoodad:
case XkbSolidDoodad:
@@ -5026,15 +5033,22 @@
dWire->text.colorNdx);
return BadMatch;
}
+ text = dWire->text;
if (client->swapped) {
- swaps(&dWire->text.width);
- swaps(&dWire->text.height);
+ swaps(&text.width);
+ swaps(&text.height);
}
- doodad->text.width = dWire->text.width;
- doodad->text.height = dWire->text.height;
+ 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) {
+ free (doodad->text.text);
+ return status;
+ }
break;
case XkbIndicatorDoodad:
if (dWire->indicator.onColorNdx >= geom->num_colors) {
@@ -5069,7 +5083,9 @@
}
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);
@@ -5301,18 +5317,20 @@
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);
@@ -5346,9 +5364,9 @@
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;