[PATCH 1/2] libX11: check size of GetReqExtra after XFlush

[Alan Coopersmith]

Sorry, was distracted long enough that all memory of these patches had been
flushed from my cache, so I had to dig into them again to reunderstand them.

I think they look good enough now, so I've added
Reviewed-by: Alan Coopersmith <alan.coopersmith at oracle.com>
and pushed both to git master:
To ssh://git.freedesktop.org/git/xorg/lib/libX11
    24d3ee0..feb131b  master -> master

	-alan-


On 07/18/13 03:52 PM, Kees Cook wrote:
> Re-re-ping. :) Can anyone commit these two patches please?
>
> Thanks!
>
> -Kees
>
> On Sun, Jun 09, 2013 at 11:13:42AM -0700, Kees Cook wrote:
>> Two users of GetReqExtra pass arbitrarily sized allocations from the
>> caller (ModMap and Host). Adjust _XGetRequest() (called by the GetReqExtra
>> macro) to double-check the requested length and invalidate "req" when
>> this happens. Users of GetReqExtra passing lengths greater than the Xlib
>> buffer size (normally 16K) must check "req" and fail gracefully instead
>> of crashing.
>>
>> Any callers of GetReqExtra that do not check "req" for NULL
>> will experience this change, in the pathological case, as a NULL
>> dereference instead of a buffer overflow. This is an improvement, but
>> the documentation for GetReqExtra has been updated to reflect the need
>> to check the value of "req" after the call.
>>
>> Bug that manifested the problem:
>> https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628
>>
>> Signed-off-by: Kees Cook <kees at outflux.net>
>> ---
>>   specs/libX11/AppC.xml | 4 +++-
>>   src/XlibInt.c         | 8 ++++++++
>>   2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/specs/libX11/AppC.xml b/specs/libX11/AppC.xml
>> index df25027..0b37048 100644
>> --- a/specs/libX11/AppC.xml
>> +++ b/specs/libX11/AppC.xml
>> @@ -2468,7 +2468,9 @@ which is the same as
>>   <function>GetReq</function>
>>   except that it takes an additional argument (the number of
>>   extra bytes to allocate in the output buffer after the request structure).
>> -This number should always be a multiple of four.
>> +This number should always be a multiple of four. Note that it is possible
>> +for req to be set to NULL as a defensive measure if the requested length
>> +exceeds the Xlib's buffer size (normally 16K).
>>   </para>
>>   </sect2>
>>   <sect2 id="Variable_Length_Arguments">
>> diff --git a/src/XlibInt.c b/src/XlibInt.c
>> index b06e57b..c3273a8 100644
>> --- a/src/XlibInt.c
>> +++ b/src/XlibInt.c
>> @@ -1733,6 +1733,14 @@ void *_XGetRequest(Display *dpy, CARD8 type, size_t len)
>>
>>       if (dpy->bufptr + len > dpy->bufmax)
>>   	_XFlush(dpy);
>> +    /* Request still too large, so do not allow it to overflow. */
>> +    if (dpy->bufptr + len > dpy->bufmax) {
>> +	fprintf(stderr,
>> +		"Xlib: request %d length %zd would exceed buffer size.\n",
>> +		type, len);
>> +	/* Changes failure condition from overflow to NULL dereference. */
>> +	return NULL;
>> +    }
>>
>>       if (len % 4)
>>   	fprintf(stderr,
>> --
>> 1.8.1.2
>


-- 
	-Alan Coopersmith-              alan.coopersmith at oracle.com
	 Oracle Solaris Engineering - http://blogs.oracle.com/alanc