[PATCH libXi 1/3] Fix potential corruption in mask_len handling

[walter harms]

Am 28.05.2013 07:52, schrieb Peter Hutterer:
> First: check for allocation failure on the mask.
> XI2 requires that the mask is zeroed, so we can't just Data() the mask
> provided by the client (it will pad) - we need a tmp buffer. Make sure that
> doesn't fail.
> 
> Second:
> req->mask_len is a uint16_t, so check against malicious mask_lens that would
> cause us to corrupt memory on copy, as the code always allocates
> req->mask_len * 4, but copies mask->mask_len bytes.
> 
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>  src/XIGrabDevice.c  | 18 ++++++++++++------
>  src/XIPassiveGrab.c |  9 ++++++++-
>  src/XISelEv.c       | 30 +++++++++++++++++++++++++-----
>  3 files changed, 45 insertions(+), 12 deletions(-)
> 
> diff --git a/src/XIGrabDevice.c b/src/XIGrabDevice.c
> index dd1bd10..2bff3d8 100644
> --- a/src/XIGrabDevice.c
> +++ b/src/XIGrabDevice.c
> @@ -50,6 +50,17 @@ XIGrabDevice(Display* dpy, int deviceid, Window grab_window, Time time,
>      if (_XiCheckExtInit(dpy, XInput_2_0, extinfo) == -1)
>  	return (NoSuchExtension);
>  
> +    if (mask->mask_len > INT_MAX - 3 ||
> +        (mask->mask_len + 3)/4 >= 0xffff)
> +        return BadValue;
> +

Is the INT_MAX needed here ? running X on 16bit machines seems very odd
(is that possible ?)
That makes the following possible
 (mask->mask_len + 3)/4 >= 0xffff
 mask->mask_len + 3 >= 0xffff * 4
 mask->mask_len >=  0xffff00 -3



just my two cents,
re
 wh

> +    /* mask->mask_len is in bytes, but we need 4-byte units on the wire,
> +     * and they need to be padded with 0 */
> +    len = (mask->mask_len + 3)/4;
> +    buff = calloc(4, len);
> +    if (!buff)
> +        return BadAlloc;
> +
>      GetReq(XIGrabDevice, req);
>      req->reqType  = extinfo->codes->major_opcode;
>      req->ReqType  = X_XIGrabDevice;
> @@ -59,14 +70,9 @@ XIGrabDevice(Display* dpy, int deviceid, Window grab_window, Time time,
>      req->grab_mode = grab_mode;
>      req->paired_device_mode = paired_device_mode;
>      req->owner_events = owner_events;
> -    req->mask_len = (mask->mask_len + 3)/4;
> +    req->mask_len = len;
>      req->cursor = cursor;
>  
> -
> -    /* mask->mask_len is in bytes, but we need 4-byte units on the wire,
> -     * and they need to be padded with 0 */
> -    len = req->mask_len;
> -    buff = calloc(1, len * 4);
>      memcpy(buff, mask->mask, mask->mask_len);
>  
>      SetReqLen(req, len, len);
> diff --git a/src/XIPassiveGrab.c b/src/XIPassiveGrab.c
> index 53b4084..4ed2f09 100644
> --- a/src/XIPassiveGrab.c
> +++ b/src/XIPassiveGrab.c
> @@ -51,6 +51,14 @@ _XIPassiveGrabDevice(Display* dpy, int deviceid, int grabtype, int detail,
>      if (_XiCheckExtInit(dpy, XInput_2_0, extinfo) == -1)
>  	return -1;
>  
> +    if (mask->mask_len > INT_MAX - 3 ||
> +        (mask->mask_len + 3)/4 >= 0xffff)
> +        return -1;
> +
> +    buff = calloc(4, (mask->mask_len + 3)/4);
> +    if (!buff)
> +        return -1;
> +
>      GetReq(XIPassiveGrabDevice, req);
>      req->reqType = extinfo->codes->major_opcode;
>      req->ReqType = X_XIPassiveGrabDevice;
> @@ -68,7 +76,6 @@ _XIPassiveGrabDevice(Display* dpy, int deviceid, int grabtype, int detail,
>      len = req->mask_len + num_modifiers;
>      SetReqLen(req, len, len);
>  
> -    buff = calloc(4, req->mask_len);
>      memcpy(buff, mask->mask, mask->mask_len);
>      Data(dpy, buff, req->mask_len * 4);
>      for (i = 0; i < num_modifiers; i++)
> diff --git a/src/XISelEv.c b/src/XISelEv.c
> index 0471bef..55c0a6a 100644
> --- a/src/XISelEv.c
> +++ b/src/XISelEv.c
> @@ -53,6 +53,8 @@ XISelectEvents(Display* dpy, Window win, XIEventMask* masks, int num_masks)
>      int i;
>      int len = 0;
>      int r = Success;
> +    int max_mask_len = 0;
> +    char *buff;
>  
>      XExtDisplayInfo *info = XInput_find_display(dpy);
>      LockDisplay(dpy);
> @@ -60,6 +62,26 @@ XISelectEvents(Display* dpy, Window win, XIEventMask* masks, int num_masks)
>          r = NoSuchExtension;
>          goto out;
>      }
> +
> +    for (i = 0; i < num_masks; i++) {
> +        current = &masks[i];
> +        if (current->mask_len > INT_MAX - 3 ||
> +            (current->mask_len + 3)/4 >= 0xffff) {
> +            r = -1;
> +            goto out;
> +        }
> +        if (current->mask_len > max_mask_len)
> +            max_mask_len = current->mask_len;
> +    }
> +
> +    /* max_mask_len is in bytes, but we need 4-byte units on the wire,
> +     * and they need to be padded with 0 */
> +    buff = calloc(4, ((max_mask_len + 3)/4));
> +    if (!buff) {
> +        r = -1;
> +        goto out;
> +    }
> +
>      GetReq(XISelectEvents, req);
>  
>      req->reqType = info->codes->major_opcode;
> @@ -79,19 +101,17 @@ XISelectEvents(Display* dpy, Window win, XIEventMask* masks, int num_masks)
>  
>      for (i = 0; i < num_masks; i++)
>      {
> -        char *buff;
>          current = &masks[i];
>          mask.deviceid = current->deviceid;
>          mask.mask_len = (current->mask_len + 3)/4;
> -        /* masks.mask_len is in bytes, but we need 4-byte units on the wire,
> -         * and they need to be padded with 0 */
> -        buff = calloc(1, mask.mask_len * 4);
> +
> +        memset(buff, 0, max_mask_len);
>          memcpy(buff, current->mask, current->mask_len);
>          Data(dpy, (char*)&mask, sizeof(xXIEventMask));
>          Data(dpy, buff, mask.mask_len * 4);
> -        free(buff);
>      }
>  
> +    free(buff);
>  out:
>      UnlockDisplay(dpy);
>      SyncHandle();