[PATCH libXi] Properly validate server responses.
Julien Cristau
jcristau at debian.org
Wed Oct 5 07:49:41 UTC 2016
On Sun, Sep 25, 2016 at 22:50:43 +0200, Matthieu Herrb wrote:
> From: Tobias Stoeckmann <tobias at stoeckmann.org>
>
> By validating length fields from server responses, out of boundary
> accesses and endless loops can be mitigated.
>
> Signed-off-by: Tobias Stoeckmann <tobias at stoeckmann.org>
> Reviewed-by: Matthieu Herrb <matthieu at herrb.eu>
> ---
> src/XGMotion.c | 3 ++-
> src/XGetBMap.c | 3 ++-
> src/XGetDCtl.c | 6 ++++--
> src/XGetFCtl.c | 7 ++++++-
> src/XGetKMap.c | 14 +++++++++++---
> src/XGetMMap.c | 11 +++++++++--
> src/XIQueryDevice.c | 36 ++++++++++++++++++++++++++++++++++--
> src/XListDev.c | 21 +++++++++++++++------
> src/XOpenDev.c | 13 ++++++++++---
> src/XQueryDv.c | 8 ++++++--
> 10 files changed, 99 insertions(+), 23 deletions(-)
>
[...]
> diff --git a/src/XIQueryDevice.c b/src/XIQueryDevice.c
> index fb8504f..a457cd6 100644
> --- a/src/XIQueryDevice.c
> +++ b/src/XIQueryDevice.c
> @@ -26,6 +26,7 @@
> #include <config.h>
> #endif
>
> +#include <limits.h>
> #include <stdint.h>
> #include <X11/Xlibint.h>
> #include <X11/extensions/XI2proto.h>
> @@ -43,6 +44,7 @@ XIQueryDevice(Display *dpy, int deviceid, int *ndevices_return)
> xXIQueryDeviceReq *req;
> xXIQueryDeviceReply reply;
> char *ptr;
> + char *end;
> int i;
> char *buf;
>
> @@ -60,14 +62,24 @@ XIQueryDevice(Display *dpy, int deviceid, int *ndevices_return)
> if (!_XReply(dpy, (xReply*) &reply, 0, xFalse))
> goto error;
>
> - *ndevices_return = reply.num_devices;
> - info = Xmalloc((reply.num_devices + 1) * sizeof(XIDeviceInfo));
> + if (reply.length < INT_MAX / 4)
> + {
> + *ndevices_return = reply.num_devices;
> + info = Xmalloc((reply.num_devices + 1) * sizeof(XIDeviceInfo));
> + }
> + else
> + {
> + *ndevices_return = 0;
> + info = NULL;
> + }
> +
> if (!info)
> goto error;
>
> buf = Xmalloc(reply.length * 4);
Should we check for allocation failure here?
> _XRead(dpy, buf, reply.length * 4);
> ptr = buf;
> + end = buf + reply.length * 4;
>
> /* info is a null-terminated array */
> info[reply.num_devices].name = NULL;
> @@ -79,6 +91,9 @@ XIQueryDevice(Display *dpy, int deviceid, int *ndevices_return)
> XIDeviceInfo *lib = &info[i];
> xXIDeviceInfo *wire = (xXIDeviceInfo*)ptr;
>
> + if (ptr + sizeof(xXIDeviceInfo) > end)
> + goto error_loop;
> +
> lib->deviceid = wire->deviceid;
> lib->use = wire->use;
> lib->attachment = wire->attachment;
> @@ -87,12 +102,23 @@ XIQueryDevice(Display *dpy, int deviceid, int *ndevices_return)
>
> ptr += sizeof(xXIDeviceInfo);
>
> + if (ptr + wire->name_len > end)
> + goto error_loop;
> +
> lib->name = Xcalloc(wire->name_len + 1, 1);
> + if (lib->name == NULL)
> + goto error_loop;
> strncpy(lib->name, ptr, wire->name_len);
> + lib->name[wire->name_len] = '\0';
> ptr += ((wire->name_len + 3)/4) * 4;
>
> sz = size_classes((xXIAnyInfo*)ptr, nclasses);
> lib->classes = Xmalloc(sz);
> + if (lib->classes == NULL)
> + {
> + Xfree(lib->name);
> + goto error_loop;
> + }
> ptr += copy_classes(lib, (xXIAnyInfo*)ptr, &nclasses);
> /* We skip over unused classes */
> lib->num_classes = nclasses;
> @@ -103,6 +129,12 @@ XIQueryDevice(Display *dpy, int deviceid, int *ndevices_return)
> SyncHandle();
> return info;
>
> +error_loop:
> + while (--i >= 0)
> + {
> + Xfree(info[i].name);
> + Xfree(info[i].classes);
> + }
> error:
> UnlockDisplay(dpy);
> error_unlocked:
AFAICT this is missing
Xfree(info);
Xfree(buf);
Cheers,
Julien
More information about the xorg-devel
mailing list