gmane.comp.freedesktop.xorg.devel

Xhiv - testing Handling Invalid Values in X libraries

Alan Coopersmith — 2013-05-25T17:15:11

So recently I was stuck trying to figure out how you would test what an
X client library would do if it received a response no X server should
ever send.   Most of our existing tests, like XTS, modify the client side
to send requests to unmodified X servers to see how they respond, but skip
testing how they handle invalid values returned from the X server.

I started considering making an X server extension to allow overriding the
normal responses, or modifying an X server to allow scripting responses to
send back, but those both seemed like larger and more complex solutions than
I wanted to tackle.  While I started feeling a bit stabby, I figured out it
wasn't that hard to just fork a process that fed X protocol responses back
to a client, improvising with the materials at hand to shove harmful contraband
into the protocol steam.   And thus was born "Xhiv", a quick and dirty test
suite for Handling Invalid Values:

http://cgit.freedesktop.org/~alanc/xhiv/
git://people.freedesktop.org/~alanc/xhiv

(I tend to

Re: X protocol specs & X.Org Security Advisory: Protocol handlingissues in X Window System client libraries

Gaetan Nadon — 2013-05-25T16:48:45

A tribute to your leadership. Not only you got the job done, but you go
it done by volunteers!
_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w< at >public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

X protocol specs & X.Org Security Advisory: Protocol handling issuesin X Window System client libraries

Alan Coopersmith — 2013-05-25T03:24:25

     [...] when reviewing these, a lot of them were non obvious unless you had
     the source in one window, the protocol headers in another and the protocol
     spec in a third. This was tedious work to cross-reference and confirm or
     refute each report [...]

which reminded me that huge thanks are also due to Matt & Gaetan and everyone
else who helped getting our documentation modernized.

As tedious as this work was, it was far better than if we had to page through
paper manuals or the old postscript dumps from the old nroff specs - having
html specs online with links to jump directly to the right call encoding or
description made it much faster to see which values could be large enough to
cause overflows, or what values we should be checking to limit the values to.

So once again, thanks for making our specs more usable!

Re: [ANNOUNCE] X.Org Security Advisory: Protocol handling issues in XWindow System client libraries

Daniel Stone — 2013-05-24T08:10:55

Hi,

On 23 May 2013 23:36, Alan Coopersmith <alan.coopersmith-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org> wrote:

Well, yes-ish.  To actually properly fix the problem, you'd have to go
back in time and stop SGI from encoding XKB into the Xlib ABI!
_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w< at >public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL unreviewed] touch fixes for #56578

Colin Harrison — 2013-05-24T06:50:04

Hi,

I got some log spam from the "Abstract cursor refcounting" change and so
just commented it out...


--- ./dix/save_cursor.c 2013-05-24 07:01:07.905179181 +0100
+++ ./dix/cursor.c      2013-05-24 07:21:53.055169797 +0100
< at >< at > -134,12 +134,12 < at >< at >
 CursorPtr
 RefCursor(CursorPtr cursor)
 {
-    ErrorF("%s ::::: cursor is %p", __func__, cursor);
+    /* ErrorF("%s ::::: cursor is %p", __func__, cursor); */
     if (cursor) {
         xorg_backtrace();
         cursor->refcnt++;
     }
-    ErrorF("\n");
+    /* ErrorF("\n"); */
     return cursor;
 }

Thanks,
Colin


_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w< at >public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] Multiple unvalidated patches in CVE-2013-1999

Dave Airlie — 2013-05-24T04:50:34

From: Dave Airlie <airlied-H+wXaHxf7aLQT0dZR+AlfA< at >public.gmane.org>

Al Viro pointed out that Debian started segfaulting in Xine for him,

Reported-by: Al Viro
Signed-off-by: Dave Airlie <airlied-H+wXaHxf7aLQT0dZR+AlfA< at >public.gmane.org>
---
 src/XvMC.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/XvMC.c b/src/XvMC.c
index cb42487..74c8b85 100644
--- a/src/XvMC.c
+++ b/src/XvMC.c
< at >< at > -585,15 +585,15 < at >< at > Status XvMCGetDRInfo(Display *dpy, XvPortID port,
 if (*name && *busID && tmpBuf) {
     _XRead(dpy, tmpBuf, realSize);
     strncpy(*name,tmpBuf,rep.nameLen);
-    name[rep.nameLen - 1] = '\0';
+    (*name)[rep.nameLen - 1] = '\0';
     strncpy(*busID,tmpBuf+rep.nameLen,rep.busIDLen);
-    busID[rep.busIDLen - 1] = '\0';
+    (*busID)[rep.busIDLen - 1] = '\0';
     XFree(tmpBuf);
 } else {
     XFree(*name);
     *name = NULL;
     XFree(*busID);
-    *name = NULL;
+    *busID = NULL;
     XFree(tmpBuf);
 
     _XEatDataWords(dpy, rep.length);

XvMC regression due to security patch

Dave Airlie — 2013-05-24T04:50:33

Al Viro pinged me about this, any distro that pushed out the libXvMC patches
has broken Xine!

Dave.

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w< at >public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Plans for X library releases

Alan Coopersmith — 2013-05-24T03:14:34

Patches are fine, but releases are easier to package, especially with
as many Xlib patches as we put forth today.

So if everyone could be testing the current git masters and reporting
any issues found (and hopefully the tinderboxes will do so as well),
then we'll be less likely to put out immediate brown-bag re-releases.

I've just pushed a RC of Xlib 1.6 out, and as mentioned in its announcement,
unless any blockers turn up, plan to push out 1.6.0 final in the first week
of June.

For the rest of the libraries, I'm not planning on any RC's, just testing
of git, and then final releases next week (since Monday is a US holiday,
that's Tuesday May 28 - Friday May 31).

Releases I plan to make next week from the current git master branches:

libdmx 1.1.3
libFS 1.0.5
libXcursor 1.1.14
libXext 1.3.2
libXfixes 5.0.1
libXinerama 1.1.3
libXp 1.0.2
libXrandr 1.4.1
libXrender 0.9.8
libXt 1.1.4
libXtst 1.2.1
libXv 1.0.8
libXvMC 1.0.8
libXxf86dga 1.1.4
libXxf86vm 1.1.3

Releases I plan to make next week from non-master

Re: [ANNOUNCE] X.Org Security Advisory: Protocol handling issuesin X Window System client libraries

Alan Coopersmith — 2013-05-23T22:36:35

BTW, I see that Ilja also mentioned these (without giving full details
on the holes) in his recent CanSecWest talk, which is an interesting
read:

http://cansecwest.com/slides/2013/Assessing%20the%20Linux%20Desktop%20Security%20-%20Ilja%20van%20Sprundel.ppt

I still agree with most of my quotes that got captured there, including the one
blaming daniels for not saving us from all manner of XKB woes.   (I know, XKB2
would fix it all, if only the laptop was returned by the thief we all curse.)

Re: [stable pull req] cherry-pick VT switch and setdesired mode fixes

Matt Dew — 2013-05-23T21:19:38

Merged.

a11cf8d..348de79  server-1.14-branch -> server-1.14-branch


Peter, yours should be in there as well.

Matt


On 05/08/2013 11:12 PM, Matt Dew wrote:
_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w< at >public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: followup for libX11 XKB security fixes

Daniel Stone — 2013-05-23T18:51:21

Seems right to me too.

Acked-by: Daniel Stone <daniel-rLtY4a/8tF1rovVCs/uTlw< at >public.gmane.org>

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w< at >public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: followup for libX11 XKB security fixes

Alan Coopersmith — 2013-05-23T18:33:13

I'd originally coded all the checks for xkb->max_key_code, then in testing
found that because Xlib accesses the xkb->max_key_code'th entry of the array,
it needed to be xkb->max_key_code + 1, and I updated the checks - I guess I
missed those two.   Sorry about that.

Reviewed-by: Alan Coopersmith <alan.coopersmith-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org>

On 05/23/13 10:26 AM, Julien Cristau wrote:

followup for libX11 XKB security fixes

Julien Cristau — 2013-05-23T17:26:50

Hi,

I noticed some inconsistencies in the XKB security changes in Xlib.
Resending to the public list now that the embargo is lifted.

Here's another change I think is necessary:

diff --git a/src/xkb/XKBGetMap.c b/src/xkb/XKBGetMap.c
index 0875dfd..c73e655 100644
--- a/src/xkb/XKBGetMap.c
+++ b/src/xkb/XKBGetMap.c
< at >< at > -426,7 +426,7 < at >< at > XkbServerMapPtrsrv;
 
     if ( rep->totalVModMapKeys>0 ) {
 if (((int) rep->firstVModMapKey + rep->nVModMapKeys)
-     > xkb->max_key_code)
+     > xkb->max_key_code + 1)
     return BadLength;
 if (((xkb->server==NULL)||(xkb->server->vmodmap==NULL))&&
     (XkbAllocServerMap(xkb,XkbVirtualModMapMask,0)!=Success)) {
diff --git a/src/xkb/XKBNames.c b/src/xkb/XKBNames.c
index 6faef02..6df7406 100644
--- a/src/xkb/XKBNames.c
+++ b/src/xkb/XKBNames.c
< at >< at > -180,7 +180,7 < at >< at > _XkbReadGetNamesReply(Display *dpy,
     nKeys= xkb->max_key_code+1;
     names->keys= _XkbTypedCalloc(nKeys,XkbKeyNameRec);
 }
-else if ( ((int)rep->firstKey + rep->nKeys) > xkb->max_key_code)
+if

Re: [PULL unreviewed] scaling, valuator initialization

Keith Packard — 2013-05-23T16:43:07



I've gone ahead and merged these; the patches do what they say on the
box, I got stuck trying to figure out if that was actually what we
wanted though...

   2ccb9fb..7205888  master -> master

Re: [PULL unreviewed] touch fixes for #56578

Keith Packard — 2013-05-23T16:41:40


Agreed. I've read through them a couple of times and still don't feel
like I understand the code well enough to review it myself.


Merged.
   891123c..2ccb9fb  master -> master

Re: [PULL] Xephyr man page fixes

Keith Packard — 2013-05-23T16:30:34


Merged.
   7e97166..891123c  master -> master

[PATCH 3/5] Xephyr: integer overflow inephyrHostGLXGetStringFromServer()

Alan Coopersmith — 2013-05-23T16:27:28

reply.length & reply.size are CARD32s and need to be bounds checked before
multiplying or adding to come up with the total size to allocate, to avoid
integer overflow leading to underallocation and writing data from the
network past the end of the allocated buffer.

Reported-by: Ilja Van Sprundel <ivansprundel-zvaqeIZJvw5Wk0Htik3J/w< at >public.gmane.org>
Signed-off-by: Alan Coopersmith <alan.coopersmith-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org>
---
 hw/kdrive/ephyr/ephyrhostglx.c |   40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/hw/kdrive/ephyr/ephyrhostglx.c b/hw/kdrive/ephyr/ephyrhostglx.c
index 5c6c40f..2327fc0 100644
--- a/hw/kdrive/ephyr/ephyrhostglx.c
+++ b/hw/kdrive/ephyr/ephyrhostglx.c
< at >< at > -169,7 +169,8 < at >< at > ephyrHostGLXGetStringFromServer(int a_screen_number,
     int default_screen = DefaultScreen(dpy);
     xGLXGenericGetStringReq *req = NULL;
     xGLXSingleReply reply;
-    int length = 0, numbytes = 0, major_opcode = 0, get_string_op = 0;
+

[PATCH 5/5] Xephyr: integer overflow in XF86DRIGetClientDriverName()

Alan Coopersmith — 2013-05-23T16:27:30

clientDriverNameLength is a CARD32 and needs to be bounds checked before
adding one to it to come up with the total size to allocate, to avoid
integer overflow leading to underallocation and writing data from the
network past the end of the allocated buffer.

Reported-by: Ilja Van Sprundel <ivansprundel-zvaqeIZJvw5Wk0Htik3J/w< at >public.gmane.org>
Signed-off-by: Alan Coopersmith <alan.coopersmith-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org>
---
 hw/kdrive/ephyr/XF86dri.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/kdrive/ephyr/XF86dri.c b/hw/kdrive/ephyr/XF86dri.c
index b1f070e..6a4c6c5 100644
--- a/hw/kdrive/ephyr/XF86dri.c
+++ b/hw/kdrive/ephyr/XF86dri.c
< at >< at > -327,9 +327,11 < at >< at > XF86DRIGetClientDriverName(Display * dpy, int screen,
     *ddxDriverPatchVersion = rep.ddxDriverPatchVersion;
 
     if (rep.length) {
-        if (!
-            (*clientDriverName =
-             (char *) calloc(rep.clientDriverNameLength + 1, 1))) {
+        if (rep.clientDriverNameLength < INT_MAX)
+

[PATCH 4/5] Xephyr: integer overflow in XF86DRIOpenConnection()

Alan Coopersmith — 2013-05-23T16:27:29

busIdStringLength is a CARD32 and needs to be bounds checked before adding
one to it to come up with the total size to allocate, to avoid integer
overflow leading to underallocation and writing data from the network past
the end of the allocated buffer.

Reported-by: Ilja Van Sprundel <ivansprundel-zvaqeIZJvw5Wk0Htik3J/w< at >public.gmane.org>
Signed-off-by: Alan Coopersmith <alan.coopersmith-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org>
---
 hw/kdrive/ephyr/XF86dri.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/kdrive/ephyr/XF86dri.c b/hw/kdrive/ephyr/XF86dri.c
index 9d742f3..b1f070e 100644
--- a/hw/kdrive/ephyr/XF86dri.c
+++ b/hw/kdrive/ephyr/XF86dri.c
< at >< at > -225,7 +225,11 < at >< at > XF86DRIOpenConnection(Display * dpy, int screen,
     }
 
     if (rep.length) {
-        if (!(*busIdString = (char *) calloc(rep.busIdStringLength + 1, 1))) {
+        if (rep.busIdStringLength < INT_MAX)
+            *busIdString = calloc(rep.busIdStringLength + 1, 1);
+        else
+            *busIdString =

[PATCH 1/5] Xdmx: integer overflow in GetGLXVisualConfigs()

Alan Coopersmith — 2013-05-23T16:27:26

numVisuals & numProps are both CARD32 and need to be bounds checked before
multiplying by structure sizes to come up with the total size to allocate,
to avoid integer overflow leading to underallocation and writing data from
the network past the end of the allocated buffer.

Reported-by: Ilja Van Sprundel <ivansprundel-zvaqeIZJvw5Wk0Htik3J/w< at >public.gmane.org>
Signed-off-by: Alan Coopersmith <alan.coopersmith-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org>
---
 hw/dmx/dmx_glxvisuals.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/dmx/dmx_glxvisuals.c b/hw/dmx/dmx_glxvisuals.c
index f903b74..027557a 100644
--- a/hw/dmx/dmx_glxvisuals.c
+++ b/hw/dmx/dmx_glxvisuals.c
< at >< at > -37,6 +37,7 < at >< at >
 #include <GL/glxproto.h>
 #include <X11/extensions/Xext.h>
 #include <X11/extensions/extutil.h>
+#include <limits.h>
 
 #include "dmx_glxvisuals.h"
 
< at >< at > -84,7 +85,10 < at >< at > GetGLXVisualConfigs(Display * dpy, int screen, int *nconfigs)
         SyncHandle();
         return NULL;
     }
-    props = (I

[PATCH 0/5] integer overflows in Xdmx & Xephyr

Alan Coopersmith — 2013-05-23T16:27:25

As part of the report of the security bugs announced this morning, the same
researcher also reported similar issues in the client-side GLX & DRI code in
the Xdmx & Xephyr X servers.   Since these are not normally installed setuid
or otherwise with higher privileges than the underlying X servers that they
connect to, the X.Org security team agreed to treat these as simple bug fixes,
not security issues.

These are candidates for stable releases as they do fix server crashes, but
only in rare cases when malformed protocol is sent by the X server they 
display onto.

Alan Coopersmith (5):
  Xdmx: integer overflow in GetGLXVisualConfigs()
  Xdmx: integer overflow in GetGLXFBConfigs()
  Xephyr: integer overflow in ephyrHostGLXGetStringFromServer()
  Xephyr: integer overflow in XF86DRIOpenConnection()
  Xephyr: integer overflow in XF86DRIGetClientDriverName()

 hw/dmx/dmx_glxvisuals.c        |   25 +++++++++++++++++--------
 hw/kdrive/ephyr/XF86dri.c      |   14 ++++++++++----
 hw/kdrive/ephyr/ephyrhostglx.c |   4