Re: [PATCH libXrandr] Avoid out of boundary accesses on illegal responses

[Julien Cristau]

On Sat, Jan  7, 2017 at 19:15:42 +0100, Tobias Stoeckmann wrote:

> Hi Julien,
> 
> On Sat, Jan 07, 2017 at 07:03:17PM +0100, Julien Cristau wrote:
> > It looks like we're leaking 'attr' on these error paths?
> 
> confirmed. That is what I get for copying the error handling of the
> attr == NULL case...
> 
Pushed as
https://cgit.freedesktop.org/xorg/lib/libXrandr/commit/?id=87227e5fc79750d3eccc3c3482a3c5b3f2af2e90

I kind of wonder if those error paths shouldn't just IOError, if we're
talking to a broken and/or malicious X server what's the point trying to
recover...

Cheers,
Julien
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libXrandr] Avoid out of boundary accesses on illegal responses

[Tobias Stoeckmann]

Hi Julien,

On Sat, Jan 07, 2017 at 07:03:17PM +0100, Julien Cristau wrote:
> It looks like we're leaking 'attr' on these error paths?

confirmed. That is what I get for copying the error handling of the
attr == NULL case...


diff --git a/src/XrrCrtc.c b/src/XrrCrtc.c
index 6665092..8316b78 100644
--- a/src/XrrCrtc.c
+++ b/src/XrrCrtc.c
@@ -459,6 +459,7 @@ XRRGetCrtcTransform (Display*dpy,
 e = extra;
 
 if (e + rep.pendingNbytesFilter > end) {
+   XFree (attr);
XFree (extra);
return False;
 }
@@ -468,6 +469,7 @@ XRRGetCrtcTransform (Display*dpy,
 for (p = 0; p < rep.pendingNparamsFilter; p++) {
INT32   f;
if (e + 4 > end) {
+   XFree (attr);
XFree (extra);
return False;
}
@@ -478,6 +480,7 @@ XRRGetCrtcTransform (Display*dpy,
 attr->pendingNparams = rep.pendingNparamsFilter;
 
 if (e + rep.currentNbytesFilter > end) {
+   XFree (attr);
XFree (extra);
return False;
 }
@@ -487,6 +490,7 @@ XRRGetCrtcTransform (Display*dpy,
 for (p = 0; p < rep.currentNparamsFilter; p++) {
INT32   f;
if (e + 4 > end) {
+   XFree (attr);
XFree (extra);
return False;
}
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH libXrandr] Avoid out of boundary accesses on illegal responses

[Julien Cristau]

[resending with xorg-devel cc added which I forgot the first time around]

On Sun, Sep 25, 2016 at 22:50:44 +0200, Matthieu Herrb wrote:

> diff --git a/src/XrrCrtc.c b/src/XrrCrtc.c
> index 5ae35c5..6665092 100644
> --- a/src/XrrCrtc.c
> +++ b/src/XrrCrtc.c
[...]
> @@ -357,7 +378,7 @@ XRRGetCrtcTransform (Display  *dpy,
>  xRRGetCrtcTransformReq   *req;
>  int  major_version, minor_version;
>  XRRCrtcTransformAttributes   *attr;
> -char *extra = NULL, *e;
> +char *extra = NULL, *end = NULL, *e;
>  int  p;
>  
>  *attributes = NULL;
> @@ -395,9 +416,17 @@ XRRGetCrtcTransform (Display *dpy,
>   else
>   {
>   int extraBytes = rep.length * 4 - CrtcTransformExtra;
> - extra = Xmalloc (extraBytes);
> + if (rep.length < INT_MAX / 4 &&
> + rep.length * 4 >= CrtcTransformExtra) {
> + extra = Xmalloc (extraBytes);
> + end = extra + extraBytes;
> + } else
> + extra = NULL;
>   if (!extra) {
> - _XEatDataWords (dpy, rep.length - (CrtcTransformExtra >> 2));
> + if (rep.length > (CrtcTransformExtra >> 2))
> + _XEatDataWords (dpy, rep.length - (CrtcTransformExtra >> 
> 2));
> + else
> + _XEatDataWords (dpy, rep.length);
>   UnlockDisplay (dpy);
>   SyncHandle ();
>   return False;
> @@ -429,22 +458,38 @@ XRRGetCrtcTransform (Display*dpy,
>  
>  e = extra;
>  
> +if (e + rep.pendingNbytesFilter > end) {
> + XFree (extra);
> + return False;
> +}
>  memcpy (attr->pendingFilter, e, rep.pendingNbytesFilter);
>  attr->pendingFilter[rep.pendingNbytesFilter] = '\0';
>  e += (rep.pendingNbytesFilter + 3) & ~3;
>  for (p = 0; p < rep.pendingNparamsFilter; p++) {
>   INT32   f;
> + if (e + 4 > end) {
> + XFree (extra);
> + return False;
> + }
>   memcpy (, e, 4);
>   e += 4;
>   attr->pendingParams[p] = (XFixed) f;
>  }
>  attr->pendingNparams = rep.pendingNparamsFilter;
>  
> +if (e + rep.currentNbytesFilter > end) {
> + XFree (extra);
> + return False;
> +}
>  memcpy (attr->currentFilter, e, rep.currentNbytesFilter);
>  attr->currentFilter[rep.currentNbytesFilter] = '\0';
>  e += (rep.currentNbytesFilter + 3) & ~3;
>  for (p = 0; p < rep.currentNparamsFilter; p++) {
>   INT32   f;
> + if (e + 4 > end) {
> + XFree (extra);
> + return False;
> + }
>   memcpy (, e, 4);
>   e += 4;
>   attr->currentParams[p] = (XFixed) f;

It looks like we're leaking 'attr' on these error paths?

Cheers,
Julien
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel