xserver: Branch 'master' - 4 commits

[Peter Hutterer]

 Xi/sendexev.c |   24 +++++++++++++++++-------
 dix/events.c  |    6 ++++++
 dix/swapreq.c |    7 +++++++
 3 files changed, 30 insertions(+), 7 deletions(-)

New commits:
commit ba336b24052122b136486961c82deac76bbde455
Author: Michal Srb <msrb at suse.com>
Date:   Wed May 24 15:54:42 2017 +0300

    Xi: Do not try to swap GenericEvent.
    
    The SProcXSendExtensionEvent must not attempt to swap GenericEvent because
    it is assuming that the event has fixed size and gives the swapping function
    xEvent-sized buffer.
    
    A GenericEvent would be later rejected by ProcXSendExtensionEvent anyway.
    
    Signed-off-by: Michal Srb <msrb at suse.com>
    Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
    Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>

diff --git a/Xi/sendexev.c b/Xi/sendexev.c
index 5e63bfcca..5c2e0fc56 100644
--- a/Xi/sendexev.c
+++ b/Xi/sendexev.c
@@ -95,9 +95,17 @@ SProcXSendExtensionEvent(ClientPtr client)
 
     eventP = (xEvent *) &stuff[1];
     for (i = 0; i < stuff->num_events; i++, eventP++) {
+        if (eventP->u.u.type == GenericEvent) {
+            client->errorValue = eventP->u.u.type;
+            return BadValue;
+        }
+
         proc = EventSwapVector[eventP->u.u.type & 0177];
-        if (proc == NotImplemented)     /* no swapping proc; invalid event type? */
+        /* no swapping proc; invalid event type? */
+        if (proc == NotImplemented) {
+            client->errorValue = eventP->u.u.type;
             return BadValue;
+        }
         (*proc) (eventP, &eventT);
         *eventP = eventT;
     }
commit 8caed4df36b1f802b4992edcfd282cbeeec35d9d
Author: Michal Srb <msrb at suse.com>
Date:   Wed May 24 15:54:41 2017 +0300

    Xi: Verify all events in ProcXSendExtensionEvent.
    
    The requirement is that events have type in range
    EXTENSION_EVENT_BASE..lastEvent, but it was tested
    only for first event of all.
    
    Signed-off-by: Michal Srb <msrb at suse.com>
    Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
    Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>

diff --git a/Xi/sendexev.c b/Xi/sendexev.c
index 1cf118ab6..5e63bfcca 100644
--- a/Xi/sendexev.c
+++ b/Xi/sendexev.c
@@ -117,7 +117,7 @@ SProcXSendExtensionEvent(ClientPtr client)
 int
 ProcXSendExtensionEvent(ClientPtr client)
 {
-    int ret;
+    int ret, i;
     DeviceIntPtr dev;
     xEvent *first;
     XEventClass *list;
@@ -141,10 +141,12 @@ ProcXSendExtensionEvent(ClientPtr client)
     /* The client's event type must be one defined by an extension. */
 
     first = ((xEvent *) &stuff[1]);
-    if (!((EXTENSION_EVENT_BASE <= first->u.u.type) &&
-          (first->u.u.type < lastEvent))) {
-        client->errorValue = first->u.u.type;
-        return BadValue;
+    for (i = 0; i < stuff->num_events; i++) {
+        if (!((EXTENSION_EVENT_BASE <= first[i].u.u.type) &&
+            (first[i].u.u.type < lastEvent))) {
+            client->errorValue = first[i].u.u.type;
+            return BadValue;
+        }
     }
 
     list = (XEventClass *) (first + stuff->num_events);
commit 215f894965df5fb0bb45b107d84524e700d2073c
Author: Michal Srb <msrb at suse.com>
Date:   Wed May 24 15:54:40 2017 +0300

    dix: Disallow GenericEvent in SendEvent request.
    
    The SendEvent request holds xEvent which is exactly 32 bytes long, no more,
    no less. Both ProcSendEvent and SProcSendEvent verify that the received data
    exactly match the request size. However nothing stops the client from passing
    in event with xEvent::type = GenericEvent and any value of
    xGenericEvent::length.
    
    In the case of ProcSendEvent, the event will be eventually passed to
    WriteEventsToClient which will see that it is Generic event and copy the
    arbitrary length from the receive buffer (and possibly past it) and send it to
    the other client. This allows clients to copy unitialized heap memory out of X
    server or to crash it.
    
    In case of SProcSendEvent, it will attempt to swap the incoming event by
    calling a swapping function from the EventSwapVector array. The swapped event
    is written to target buffer, which in this case is local xEvent variable. The
    xEvent variable is 32 bytes long, but the swapping functions for GenericEvents
    expect that the target buffer has size matching the size of the source
    GenericEvent. This allows clients to cause stack buffer overflows.
    
    Signed-off-by: Michal Srb <msrb at suse.com>
    Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
    Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>

diff --git a/dix/events.c b/dix/events.c
index 3e3a01ef9..d3a33ea3f 100644
--- a/dix/events.c
+++ b/dix/events.c
@@ -5366,6 +5366,12 @@ ProcSendEvent(ClientPtr client)
         client->errorValue = stuff->event.u.u.type;
         return BadValue;
     }
+    /* Generic events can have variable size, but SendEvent request holds
+       exactly 32B of event data. */
+    if (stuff->event.u.u.type == GenericEvent) {
+        client->errorValue = stuff->event.u.u.type;
+        return BadValue;
+    }
     if (stuff->event.u.u.type == ClientMessage &&
         stuff->event.u.u.detail != 8 &&
         stuff->event.u.u.detail != 16 && stuff->event.u.u.detail != 32) {
diff --git a/dix/swapreq.c b/dix/swapreq.c
index 719e9b81c..67850593b 100644
--- a/dix/swapreq.c
+++ b/dix/swapreq.c
@@ -292,6 +292,13 @@ SProcSendEvent(ClientPtr client)
     swapl(&stuff->destination);
     swapl(&stuff->eventMask);
 
+    /* Generic events can have variable size, but SendEvent request holds
+       exactly 32B of event data. */
+    if (stuff->event.u.u.type == GenericEvent) {
+        client->errorValue = stuff->event.u.u.type;
+        return BadValue;
+    }
+
     /* Swap event */
     proc = EventSwapVector[stuff->event.u.u.type & 0177];
     if (!proc || proc == NotImplemented)        /* no swapping proc; invalid event type? */
commit 05442de962d3dc624f79fc1a00eca3ffc5489ced
Author: Michal Srb <msrb at suse.com>
Date:   Wed May 24 15:54:39 2017 +0300

    Xi: Zero target buffer in SProcXSendExtensionEvent.
    
    Make sure that the xEvent eventT is initialized with zeros, the same way as
    in SProcSendEvent.
    
    Some event swapping functions do not overwrite all 32 bytes of xEvent
    structure, for example XSecurityAuthorizationRevoked. Two cooperating
    clients, one swapped and the other not, can send
    XSecurityAuthorizationRevoked event to each other to retrieve old stack data
    from X server. This can be potentialy misused to go around ASLR or
    stack-protector.
    
    Signed-off-by: Michal Srb <msrb at suse.com>
    Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
    Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>

diff --git a/Xi/sendexev.c b/Xi/sendexev.c
index 11d82029f..1cf118ab6 100644
--- a/Xi/sendexev.c
+++ b/Xi/sendexev.c
@@ -78,7 +78,7 @@ SProcXSendExtensionEvent(ClientPtr client)
 {
     CARD32 *p;
     int i;
-    xEvent eventT;
+    xEvent eventT = { .u.u.type = 0 };
     xEvent *eventP;
     EventSwapPtr proc;