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 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...
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 on attachment 112450 [details] [review] Proposed patch (Updated) Review of attachment 112450 [details] [review]: ----------------------------------------------------------------- Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Please note the patch goes on top of http://patchwork.freedesktop.org/patch/40575/
Should we take the CVE path or merge the patch? Not sure where we stand with this now.
This issue has been assigned CVE-2015-0255
http://cgit.freedesktop.org/xorg/xserver/commit/?id=20079c36cf7d377938ca5478447d8b9045cb7d43
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.