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

Peter Hutterer peter.hutterer at who-t.net
Wed May 29 21:48:10 PDT 2013


On Tue, May 28, 2013 at 09:22:11AM +0200, walter harms wrote:
> 
> 
> 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

true if mask_len is unsigned, but it is a signed int. so if the user
supplies a mask_len of INT_MAX, mask_len + 3 is undefined and (mask_len +
3)/4 is undefined to.

if it wraps around, (mask->mask_len + 3)/4 < 0xffff is true and we continue
on our merry way. doesn't have any other effect as long as calloc is happy 
but still.

Cheers,
   Peter
 
> > +    /* 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();
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
> 


More information about the xorg-devel mailing list