Re: [Bug-wget] [PATCH] Fix stack overflow with way too many cookies.

[Tim Rühsen]

Hi Tobias,

likely or not, it is a good catch !

I rated your patch as 'trivial' (it is below the threshold where we have to 
ask for a FSF copyright assignment), made a few amendments and pushed it.

Thanks for your contribution.

Regards, Tim


On Mittwoch, 10. August 2016 19:09:34 CEST Tobias Stoeckmann wrote:
> If wget supports cookies, which is the default, it will eventually sort
> them based on their domain, path, and name attributes. In order to
> perform this sorting, quick sort is used. And for this, an array
> containing pointers to all relevant cookies is constructed on the stack
> with alloca().
> 
> If wget has to handle an insanely large amount of cookies (~700,000 on
> 32 bit systems or ~530,000 on 64 bit systems), the stack is not large
> enough to hold these pointers, leading to undefined behaviour according
> to POSIX; expect a segmentation fault in real life. ;)
> 
> This is a very unlikely thing to happen. Either you have to create a
> cookie file that contains so many cookies, in which case it's your fault!
> Or you are running "wget -m http://some_host";. In that case, an evil
> server could exploit wget's default infinite recursion, adding cookies
> in every single HTTP response to wget. This way, the cookie storage will
> slowly fill up and wget could crash.
> 
> I write "could crash" because the runtime performance of cookie handling
> is clearly not designed for such a large amount of cookies. It's like
> approaching the event horizon of a black hole due to O(n^2). ;)
> 
> This command would create a crashing cookie-file and calls wget with it:
> 
> $ for i in $(seq 1 700000)
> do
>   echo "localhost:8080        FALSE   /       FALSE   0       $i      1"
> done > cookies
> $ wget --load-cookies cookies http://localhost:8080
> 
> If you want to see that something happens, I recommend to disable the
> qsort() calls in src/cookies.c as well as the find_matching_cookie call.
> This cookie file guarantees that the cookies are unique.
> 
> My patch moves the memory handling to the heap. But I think it would
> also be totally sufficient to add a constant like COOKIE_MAX to avoid
> overflows on the stack.
> 
> Signed-off-by: Tobias Stoeckmann <address@hidden>
> ---
>  src/cookies.c | 10 ++++++++--
>  src/http.c    |  7 +++++--
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/src/cookies.c b/src/cookies.c
> index 81ecfa5..624c9ff 100644
> --- a/src/cookies.c
> +++ b/src/cookies.c
> @@ -45,6 +45,7 @@ as that of the covered work.  */
> 
>  #include "wget.h"
> 
> +#include <limits.h>
>  #include <stdio.h>
>  #include <string.h>
>  #include <stdlib.h>
> @@ -1018,7 +1019,7 @@ cookie_header (struct cookie_jar *jar, const char
> *host,
> 
>    struct cookie *cookie;
>    struct weighed_cookie *outgoing;
> -  int count, i, ocnt;
> +  size_t count, i, ocnt;
>    char *result;
>    int result_size, pos;
>    PREPEND_SLASH (path);         /* see cookie_handle_set_cookie */
> @@ -1051,7 +1052,11 @@ cookie_header (struct cookie_jar *jar, const char
> *host, return NULL;                /* no cookies matched */
> 
>    /* Allocate the array. */
> -  outgoing = alloca_array (struct weighed_cookie, count);
> +  if (count > SIZE_MAX / sizeof (struct weighed_cookie))
> +    return NULL;                /* unable to process so many cookies */
> +  outgoing = malloc(count * sizeof (struct weighed_cookie));
> +  if (outgoing == NULL)
> +    return NULL;             /* out of memory */
> 
>    /* Fill the array with all the matching cookies from the chains that
>       match HOST. */
> @@ -1111,6 +1116,7 @@ cookie_header (struct cookie_jar *jar, const char
> *host, }
>      }
>    result[pos++] = '\0';
> +  free(outgoing);
>    assert (pos == result_size);
>    return result;
>  }
> diff --git a/src/http.c b/src/http.c
> index 1091121..cf6d7a9 100644
> --- a/src/http.c
> +++ b/src/http.c
> @@ -344,7 +344,9 @@ request_send (const struct request *req, int fd, FILE
> *warc_tmp) /* "\r\n\0" */
>    size += 3;
> 
> -  p = request_string = alloca_array (char, size);
> +  p = request_string = malloc (size);
> +  if (request_string == NULL)
> +    return -1;
> 
>    /* Generate the request. */
> 
> @@ -379,8 +381,9 @@ request_send (const struct request *req, int fd, FILE
> *warc_tmp) /* Write a copy of the data to the WARC record. */
>        int warc_tmp_written = fwrite (request_string, 1, size - 1,
> warc_tmp); if (warc_tmp_written != size - 1)
> -        return -2;
> +        write_error = -2;
>      }
> +  free(request_string);
>    return write_error;
>  }

signature.asc
Description: This is a digitally signed message part.