[security-team] [PATCH libX11 1/2] The validation of server responses avoids out of boundary accesses.

[Johannes Segitz]

Hi,

I didn't see any reply to your comment on this and other patches. Was that
handled somewhere else or are those patches considered final?

Thanks,
Johannes

On Wed, Oct 05, 2016 at 08:56:52AM +0200, Julien Cristau wrote:
> Sorry I didn't get a chance to look at these before they went public...
> 
> On Sun, Sep 25, 2016 at 22:50:40 +0200, Matthieu Herrb wrote:
> 
> > From: Tobias Stoeckmann <tobias at stoeckmann.org>
> > 
> > v2: FontNames.c  return a NULL list whenever a single
> > length field from the server is incohent.
> > 
> > Signed-off-by: Tobias Stoeckmann <tobias at stoeckmann.org>
> > Reviewed-by: Matthieu Herrb <matthieu at herrb.eu>
> > ---
> >  src/FontNames.c | 23 +++++++++++++++++------
> >  src/ListExt.c   | 12 ++++++++----
> >  src/ModMap.c    |  3 ++-
> >  3 files changed, 27 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/FontNames.c b/src/FontNames.c
> > index 21dcafe..e55f338 100644
> > --- a/src/FontNames.c
> > +++ b/src/FontNames.c
> > @@ -66,7 +66,7 @@ int *actualCount)	/* RETURN */
> >  
> >      if (rep.nFonts) {
> >  	flist = Xmalloc (rep.nFonts * sizeof(char *));
> > -	if (rep.length < (INT_MAX >> 2)) {
> > +	if (rep.length > 0 && rep.length < (INT_MAX >> 2)) {
> >  	    rlen = rep.length << 2;
> >  	    ch = Xmalloc(rlen + 1);
> >  	    /* +1 to leave room for last null-terminator */
> > @@ -93,11 +93,22 @@ int *actualCount)	/* RETURN */
> >  	    if (ch + length < chend) {
> >  		flist[i] = ch + 1;  /* skip over length */
> >  		ch += length + 1;  /* find next length ... */
> > -		length = *(unsigned char *)ch;
> > -		*ch = '\0';  /* and replace with null-termination */
> > -		count++;
> > -	    } else
> > -		flist[i] = NULL;
> > +		if (ch <= chend) {
> > +		    length = *(unsigned char *)ch;
> > +		    *ch = '\0';  /* and replace with null-termination */
> > +		    count++;
> > +		} else {
> > +                    Xfree(flist);
> > +                    flist = NULL;
> > +                    count = 0;
> > +                    break;
> > +		}
> > +	    } else {
> > +                Xfree(flist);
> > +                flist = NULL;
> > +                count = 0;
> > +                break;
> > +            }
> >  	}
> >      }
> >      *actualCount = count;
> 
> I believe these new error paths are missing Xfree(ch).
> 
> > diff --git a/src/ListExt.c b/src/ListExt.c
> > index be6b989..0516e45 100644
> > --- a/src/ListExt.c
> > +++ b/src/ListExt.c
> > @@ -55,7 +55,7 @@ char **XListExtensions(
> >  
> >  	if (rep.nExtensions) {
> >  	    list = Xmalloc (rep.nExtensions * sizeof (char *));
> > -	    if (rep.length < (INT_MAX >> 2)) {
> > +	    if (rep.length > 0 && rep.length < (INT_MAX >> 2)) {
> >  		rlen = rep.length << 2;
> >  		ch = Xmalloc (rlen + 1);
> >                  /* +1 to leave room for last null-terminator */
> > @@ -80,9 +80,13 @@ char **XListExtensions(
> >  		if (ch + length < chend) {
> >  		    list[i] = ch+1;  /* skip over length */
> >  		    ch += length + 1; /* find next length ... */
> > -		    length = *ch;
> > -		    *ch = '\0'; /* and replace with null-termination */
> > -		    count++;
> > +		    if (ch <= chend) {
> > +			length = *ch;
> > +			*ch = '\0'; /* and replace with null-termination */
> > +			count++;
> > +		    } else {
> > +			list[i] = NULL;
> > +		    }
> >  		} else
> >  		    list[i] = NULL;
> >  	    }
> 
> This might have been more readable by just replacing the previous
> if (ch + length < chend)
> with
> if (ch + length + 1 < chend)
> ?
> 
> Cheers,
> Julien
> _______________________________________________
> xorg-security mailing list
> xorg-security at lists.x.org
> https://lists.x.org/mailman/listinfo/xorg-security

Best regards/Gruesse,
Johannes
-- 
GPG Key E7C81FA0       EE16 6BCE AD56 E034 BFB3  3ADD 7BF7 29D5 E7C8 1FA0
Subkey fingerprint:    250F 43F5 F7CE 6F1E 9C59  4F95 BC27 DD9D 2CC4 FD66
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20161109/2703db41/attachment.sig>