[PATCH xserver/hw/xfree86/parser/scan.c] potential buffer overrun

[Peter Hutterer]

On Tue, Oct 11, 2011 at 04:11:28AM +0200, vdb at picaros.org wrote:
> > 
> > On Sat, Sep 17, 2011 at 06:58:18PM +0200, vdb at picaros.org wrote:
> > > This patch fixes a potential buffer overrun in xf86addComment().  The 
> > > overrun occurs if the comment string to add doesn't start with a 
> > > leading '#'.  
> > 
> > out of interest - how do you trigger this? Presumably xf86addComment should
> > only be called for comments, which would require the leading '#'
> 
> The code which calculates the new buffer size is incorrect.  In the 
> normal case, a leading '#', the reallocated buffer is one byte too 
> long.  In the other case the buffer will be one byte short and an 
> overrun occurs.
> 
> The code has explicit support for adding comments without a leading 
> hash.  It is this support which causes the bug.  
>
> It seems not good to have support for a feature which causes an 
> overrun when used.  But indeed, no bug is seen since all calls to 
> xf86addComment() include the leading '#' as read from xorg.conf.  

sorry, nearly dropped this one. I've merged it in, thanks. Was about to say
that a simple if (!iscomment) len++; may be enough but this code is awful
enough and your patch cleans it up to be more sane. Much appreciated.

I've also squashed in a very basic testcase for test/xfree86.c to reproduce
the overflow.

Cheers,
  Peter
> 
> Just my view, Servaas Vandenberghe.
> 
> > > The bug can be seen from the code assuming 
> > >  curlen > 0 && eol_seen == 0 && iscomment == 0 :
> > > 
> > > char *
> > > xf86addComment(char *cur, char *add)
> > > 
> > > <...>
> > > 
> > > 	str = add;
> > > ->	iscomment = 0;
> > > 	while (*str) {
> > > 	    if (*str != ' ' && *str != '\t')
> > > 		break;
> > > 	    ++str;
> > > 	}
> > > ->	iscomment = (*str == '#');
> > > 
> > >         len = strlen(add);
> > >         endnewline = add[len - 1] == '\n';
> > >         len +=  1 + iscomment + (!hasnewline) + (!endnewline) + eol_seen;
> > >                     ^^^^^^^^^
> > >         if ((str = realloc(cur, len + curlen)) == NULL)
> > >                 return cur;
> > > 
> > >         cur = str;
> > > 
> > >         if (eol_seen || (curlen && !hasnewline))
> > >                 cur[curlen++] = '\n';
> > > ->      if (!iscomment)
> > > ->              cur[curlen++] = '#';
> > >         strcpy(cur + curlen, add);
> > >         if (!endnewline)
> > >                 strcat(cur, "\n");
> > > 
> > > The patch below is just a bugfix and tries to stay as close as 
> > > possible to the original code.  See also: 
> > >  http://lists.x.org/archives/xorg-devel/2011-August/024775.html .
> > > 
> > > Signed-off-by: Servaas Vandenberghe <vdb at picaros.org>
> > > diff --git a/hw/xfree86/parser/scan.c b/hw/xfree86/parser/scan.c
> > > index 1cff3bc..b188503 100644
> > > --- a/hw/xfree86/parser/scan.c
> > > +++ b/hw/xfree86/parser/scan.c
> > > @@ -1093,7 +1093,7 @@ char *
> > >  xf86addComment(char *cur, char *add)
> > >  {
> > >  	char *str;
> > > -	int len, curlen, iscomment, hasnewline = 0, endnewline;
> > > +	int len, curlen, iscomment, hasnewline = 0, insnewline, endnewline;
> > >  
> > >  	if (add == NULL || add[0] == '\0')
> > >  		return cur;
> > > @@ -1108,7 +1108,6 @@ xf86addComment(char *cur, char *add)
> > >  		curlen = 0;
> > >  
> > >  	str = add;
> > > -	iscomment = 0;
> > >  	while (*str) {
> > >  	    if (*str != ' ' && *str != '\t')
> > >  		break;
> > > @@ -1118,14 +1117,23 @@ xf86addComment(char *cur, char *add)
> > >  
> > >  	len = strlen(add);
> > >  	endnewline = add[len - 1] == '\n';
> > > -	len +=  1 + iscomment + (!hasnewline) + (!endnewline) + eol_seen;
> > >  
> > > -	if ((str = realloc(cur, len + curlen)) == NULL)
> > > +	insnewline = eol_seen || (curlen && !hasnewline);
> > > +	if (insnewline)
> > > +		len++;
> > > +	if (!iscomment)
> > > +		len++;
> > > +	if (!endnewline)
> > > +		len++;
> > > +
> > > +	/* Allocate + 1 char for '\0' terminator. */
> > > +	str = realloc(cur, curlen + len + 1);
> > > +	if (!str)
> > >  		return cur;
> > >  
> > >  	cur = str;
> > >  
> > > -	if (eol_seen || (curlen && !hasnewline))
> > > +	if (insnewline)
> > >  		cur[curlen++] = '\n';
> > >  	if (!iscomment)
> > >  		cur[curlen++] = '#';