Bug 88507 - Strings length not checked in Xkb
Summary: Strings length not checked in Xkb
Alias: None
Product: xorg
Classification: Unclassified
Component: Security (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: X.Org Security
QA Contact: X.Org Security
URL: http://patchwork.freedesktop.org/patc...
Keywords: security
Depends on:
Reported: 2015-01-16 19:20 UTC by Olivier Fourdan
Modified: 2015-02-10 22:48 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

Proposed patch (4.59 KB, patch)
2015-01-16 19:20 UTC, Olivier Fourdan
no flags Details | Splinter Review
Proposed patch (Updated) (4.63 KB, patch)
2015-01-19 09:33 UTC, Olivier Fourdan
no flags Details | Splinter Review

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.