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

[Julien Cristau]

Hi all,

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