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

[Eric Anholt]

Peter Hutterer <peter.hutterer at who-t.net> writes:

> On Mon, Sep 25, 2017 at 12:55:47PM -0700, Eric Anholt wrote:
>> Michal Srb <msrb at suse.com> writes:
>> 
>> > On neděle 24. září 2017 0:20:07 CEST Eric Anholt wrote:
>> >> Michal Srb <msrb at suse.com> 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 <peter.hutterer at who-t.net> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20171009/b5e5f307/attachment.sig>