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 <[email protected]> > --- > 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.
