[PATCH 3/8] Xephyr: integer overflow in ephyrHostGLXGetStringFromServer()

[walter harms]

Am 06.07.2013 08:47, schrieb Alan Coopersmith:
> reply.length & reply.size are CARD32s and need to be bounds checked before
> multiplying or adding to come up with the total size to allocate, to avoid
> integer overflow leading to underallocation and writing data from the
> network past the end of the allocated buffer.
> 
> Reported-by: Ilja Van Sprundel <ivansprundel at ioactive.com>
> Signed-off-by: Alan Coopersmith <alan.coopersmith at oracle.com>
> ---
>  hw/kdrive/ephyr/ephyrhostglx.c |   47 +++++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/kdrive/ephyr/ephyrhostglx.c b/hw/kdrive/ephyr/ephyrhostglx.c
> index 5c6c40f..90f450c 100644
> --- a/hw/kdrive/ephyr/ephyrhostglx.c
> +++ b/hw/kdrive/ephyr/ephyrhostglx.c
> @@ -137,7 +137,7 @@ ephyrHostGLXQueryVersion(int *a_major, int *a_minor)
>  }
>  
>  /**
> - * GLX protocol structure for the ficticious "GXLGenericGetString" request.
> + * GLX protocol structure for the ficticious "GLXGenericGetString" request.
>   * 
>   * This is a non-existant protocol packet.  It just so happens that all of
>   * the real protocol packets used to request a string from the server have
> @@ -169,7 +169,8 @@ ephyrHostGLXGetStringFromServer(int a_screen_number,
>      int default_screen = DefaultScreen(dpy);
>      xGLXGenericGetStringReq *req = NULL;
>      xGLXSingleReply reply;
> -    int length = 0, numbytes = 0, major_opcode = 0, get_string_op = 0;
> +    unsigned long length = 0, numbytes = 0;
> +    int major_opcode = 0, get_string_op = 0;
>  
>      EPHYR_RETURN_VAL_IF_FAIL(dpy && a_string, FALSE);
>  
> @@ -209,36 +210,46 @@ ephyrHostGLXGetStringFromServer(int a_screen_number,
>  
>      _XReply(dpy, (xReply *) &reply, 0, False);
>  
> -    length = reply.length * 4;
> -    if (!length) {
> -        numbytes = 0;
> -    }
> -    else {
> +#if UINT32_MAX >= (ULONG_MAX / 4)
> +    if (reply.length >= (ULONG_MAX / 4))
> +        goto eat_out;
> +#endif

I am not sure what is going on here, i am missing the else branch,
For all systems where UINT == ULONG this will be a noop. Is that intended ?

stdint.h:# define UINT32_MAX            (4294967295U)
limits.h:#   define ULONG_MAX   4294967295UL


just my 2 cents

re,
 wh

> +    if (reply.length > 0) {
> +        length = (unsigned long) reply.length * 4;
>          numbytes = reply.size;
> +        if (numbytes > length) {
> +            EPHYR_LOG_ERROR("string length %d longer than reply length %d\n",
> +                            numbytes, length);
> +            goto eat_out;
> +        }
>      }
>      EPHYR_LOG("going to get a string of size:%d\n", numbytes);
>  
> -    *a_string = (char *) Xmalloc(numbytes + 1);
> -    if (!a_string) {
> +    if (numbytes < INT_MAX)
> +        *a_string = Xcalloc(numbytes + 1, 1);
> +    else
> +        *a_string = NULL;
> +    if (*a_string == NULL) {
>          EPHYR_LOG_ERROR("allocation failed\n");
> -        goto out;
> +        goto eat_out;
>      }
>  
> -    memset(*a_string, 0, numbytes + 1);
>      if (_XRead(dpy, *a_string, numbytes)) {
> -        UnlockDisplay(dpy);
> -        SyncHandle();
>          EPHYR_LOG_ERROR("read failed\n");
> -        goto out;
> +        length = 0; /* if read failed, no idea how much to eat */
> +    }
> +    else {
> +        length -= numbytes;
> +        EPHYR_LOG("strname:%#x, strvalue:'%s', strlen:%d\n",
> +                  a_string_name, *a_string, numbytes);
> +        is_ok = TRUE;
>      }
> -    length -= numbytes;
> +
> + eat_out:
>      _XEatData(dpy, length);
>      UnlockDisplay(dpy);
>      SyncHandle();
> -    EPHYR_LOG("strname:%#x, strvalue:'%s', strlen:%d\n",
> -              a_string_name, *a_string, numbytes);
>  
> -    is_ok = TRUE;
>   out:
>      EPHYR_LOG("leave\n");
>      return is_ok;