[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