Bug 88507

Summary: Strings length not checked in Xkb
Product: xorg Reporter: Olivier Fourdan <fourdan>
Component: SecurityAssignee: X.Org Security <xorg_security>
Status: RESOLVED FIXED QA Contact: X.Org Security <xorg_security>
Severity: normal    
Priority: medium CC: fourdan, peter.hutterer
Version: gitKeywords: security
Hardware: Other   
OS: All   
URL: http://patchwork.freedesktop.org/patch/40575/
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
Proposed patch
none
Proposed patch (Updated) none

Description Olivier Fourdan 2015-01-16 19:20:59 UTC 
Created attachment 112359 [details] [review]
Proposed patch

This is a follow up on http://patchwork.freedesktop.org/patch/40575/

In xkb/xkb.c, the code that reads the strings in _GetCountedString() blindly takes whatever length is given in the request without checking if the string length actually fits into the request length.

A malicious hand-crafted request could cause the xserver to copy strings from adjacent memory.

Strings position are computed from one to another, given the the strings length are encoded on CARD16, a request could read as far as n strings by 64k far in memory.

Not sure if that should be regarded as a security issue or not (I doubt there is any escalation nor data disclosure), but at best it will crash the X server. 

The proposed patch applies on top of http://patchwork.freedesktop.org/patch/40575/
Comment 1 Peter Hutterer 2015-01-19 01:35:42 UTC 
Comment on attachment 112359 [details] [review]
Proposed patch

Review of attachment 112359 [details] [review]:
-----------------------------------------------------------------

Not happy about the leaks but we can fix this as a follow-up (note it in the commit message though pls). Reviewed-by: me with the error code fixed.

::: xkb/xkb.c
@@ +4971,5 @@
> +    next = wire + XkbPaddedSize(len + 2);
> +    /* Check we're still within the size of the request */
> +    if (client->req_len <
> +        bytes_to_int32(next - (char *) client->requestBuffer))
> +        return BadRequest;

BadRequest would be "unknown request", I think this needs to be BadValue.

@@ +5045,5 @@
> +        if (status != Success)
> +            return status;
> +        status = _GetCountedString(&wire, client, &doodad->text.font);
> +        if (status != Success)
> +            return status;

afaict all these leak anything previously allocated in case of failure. which looking at the rest was the previous behaviour anyway but stil...
Comment 2 Olivier Fourdan 2015-01-19 09:33:21 UTC 
Created attachment 112450 [details] [review]
Proposed patch (Updated)

Update patch to return "BadValue" instead of "BadRequest" and free the previously allocated string.

Note, there is a fundamental leak here, the "doodad" itself.

It gets allocated with XkbAddGeomDoodad() (which is public API, so cannot be changed) which either returns an existing doodad if the name matches a doodad already present, or allocates a new one otherwise.

So there is no way for the caller to know if the doodad was newly allocated or not, so the caller can't reasonably free it. I don't see any reference counter either that would help. I think fixing this leak goes beyond the scope of this patch, so ignoring it for now.
Comment 3 Peter Hutterer 2015-01-19 09:36:21 UTC 
Comment on attachment 112450 [details] [review]
Proposed patch (Updated)

Review of attachment 112450 [details] [review]:
-----------------------------------------------------------------

Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Comment 4 Olivier Fourdan 2015-01-20 08:48:45 UTC 
Please note the patch goes on top of http://patchwork.freedesktop.org/patch/40575/
Comment 5 Olivier Fourdan 2015-01-29 10:51:37 UTC 
Should we take the CVE path or merge the patch?

Not sure where we stand with this now.
Comment 6 Peter Hutterer 2015-02-10 22:19:39 UTC 
This issue has been assigned CVE-2015-0255

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.