[PATCH libXi] Properly validate server responses.

[Julien Cristau]

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