I have added the first two out the three recommendations in the draft. The third one is relevant when cookies have to be removed in case the total number of cookies hit a predefined upper bound, I'm not sure whether we do that in wget?
As you mentioned, I had to change some method prototypes to get the uri scheme. I made sure that I replaced all instances of those function calls with the right call. The tests run fine, so hopefully I haven't broken anything. I am attaching the patch files, please review them. Thanks, Kush On Sun, Jan 24, 2016 at 4:39 AM, Darshit Shah <[email protected]> wrote: > On 23 January 2016 at 23:36, Kushagra Singh > <[email protected]> wrote: > > Thanks a lot for the help! > > > > I've made some progress, but have a couple of more questions > > > > - I can't manage to find the http-only-flag in the cookie struct, do we > not > > store this? > Since Wget supports only HTTP, this is not required. The HttpOnly > attribute prevents access to script code, but since Wget never > executes them it is not necessary at all. Although, it may be a good > idea to explicitly store the flag for Wget saves the cookies to a > file. Maybe, we should add this. > > > - The draft asks to check whether the "scheme" component of the > > "request-uri" denotes a secure protocol or not. Currently I am checking > > using "#ifdef HAVE_SSL". I am not sure whether this is the right way to > do > > so, since having SSL with wget does not necessarily mean that the current > > connection is secure. > > Ideally, a code base should have as few #ifdef statements as possible. > They make reading the code very difficult for a human. That said, in > this scenario it is the absolute wrong technique. You will want to > access the scheme from the request URI. Find a way to access this > information, you may need to change some method prototypes to make > this happen. > > > - To check whether there exists a cookie whose domain, domain-matches the > > domain of a new cookie, we should iterate through the chains returned by > > find_chains_of_host right? > > That ought to work, I think. > > > > > Regards, > > Kush > > > > -- > Thanking You, > Darshit Shah >
From dfffa874c9d56c36f56ddf7d7b44e728ba4dc671 Mon Sep 17 00:00:00 2001 From: kush789 <[email protected]> Date: Thu, 21 Jan 2016 13:34:16 +0530 Subject: [PATCH 1/4] Added recomendation 1 of draft-west-leave-secure-cookies-alone-04 --- src/cookies.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/cookies.c b/src/cookies.c index 81ecfa5..2537790 100644 --- a/src/cookies.c +++ b/src/cookies.c @@ -439,8 +439,22 @@ parse_set_cookie (const char *set_cookie, bool silent) } else if (TOKEN_IS (name, "secure")) { - /* ignore value completely */ +#ifdef HAVE_SSL + /* Ignore value completely since secure is a value-less + attribute */ cookie->secure = 1; +#else + /* Deleting cookie since secure only flag is set but no OpenSSL + or GNUTLS */ + if (!silent) + logprintf (LOG_NOTQUIET, + _("Trying to create secure only cookie, but connection + is not secure (OpenSSl or GNUTLS not configured)\n + Set-Cookie : %s\n"), + quotearg_style (escape_quoting_style, set_cookie)); + delete_cookie (cookie); + return NULL; +#endif } /* else: Ignore unrecognized attribute. */ } -- 1.9.1
From d420f619c252dd7793fb067ebdce340ed3d1430f Mon Sep 17 00:00:00 2001 From: kush789 <[email protected]> Date: Sun, 24 Jan 2016 13:49:07 +0530 Subject: [PATCH 2/4] Redid reccomendation I, passing url->scheme to cookie_handle_set_cookie in gethttp() --- src/cookies.c | 58 ++++++++++++++++++++++++++++++++++++++++------------------ src/cookies.h | 5 ++++- src/http.c | 2 +- 3 files changed, 45 insertions(+), 20 deletions(-) diff --git a/src/cookies.c b/src/cookies.c index 2537790..ea1a8df 100644 --- a/src/cookies.c +++ b/src/cookies.c @@ -350,7 +350,8 @@ discard_matching_cookie (struct cookie_jar *jar, struct cookie *cookie) filled. */ static struct cookie * -parse_set_cookie (const char *set_cookie, bool silent) +parse_set_cookie (const char *set_cookie, enum url_scheme scheme, + bool silent) { const char *ptr = set_cookie; struct cookie *cookie = cookie_new (); @@ -439,22 +440,21 @@ parse_set_cookie (const char *set_cookie, bool silent) } else if (TOKEN_IS (name, "secure")) { -#ifdef HAVE_SSL + if (scheme == SCHEME_HTTPS) /* Ignore value completely since secure is a value-less - attribute */ - cookie->secure = 1; -#else - /* Deleting cookie since secure only flag is set but no OpenSSL - or GNUTLS */ - if (!silent) - logprintf (LOG_NOTQUIET, - _("Trying to create secure only cookie, but connection - is not secure (OpenSSl or GNUTLS not configured)\n - Set-Cookie : %s\n"), - quotearg_style (escape_quoting_style, set_cookie)); - delete_cookie (cookie); - return NULL; -#endif + attribute*/ + cookie->secure = 1; + else + { + /* Deleting cookie since secure only flag is set but connection + is not secure. */ + if (!silent) + logprintf (LOG_NOTQUIET, + _("Trying to create secure only cookie, but connection is not secure.\nSet-Cookie : %s\n"), + quotearg_style (escape_quoting_style, set_cookie)); + delete_cookie (cookie); + return NULL; + } } /* else: Ignore unrecognized attribute. */ } @@ -713,6 +713,27 @@ check_path_match (const char *cookie_path, const char *path) s = PS_newstr; \ } while (0) +static int +check_cookie_matching_name_domain (struct cookie_jar *jar, + struct cookie *cookie) +{ + struct cookie *chain, *prev; + + chain = hash_table_get (jar->chains, cookie->domain); + + if (!chain) + return 0; + + prev = NULL; + for (; chain; prev = chain, chain = chain->next) + if (0 == strcmp (cookie->attr, chain->attr) + && 1 == chain->secure) + { + return 1; + } + + return 0; +} /* Process the HTTP `Set-Cookie' header. This results in storing the cookie or discarding a matching one, or ignoring it completely, all @@ -721,7 +742,8 @@ check_path_match (const char *cookie_path, const char *path) void cookie_handle_set_cookie (struct cookie_jar *jar, const char *host, int port, - const char *path, const char *set_cookie) + const char *path, enum url_scheme scheme, + const char *set_cookie) { struct cookie *cookie; cookies_now = time (NULL); @@ -731,7 +753,7 @@ cookie_handle_set_cookie (struct cookie_jar *jar, simply prepend slash to PATH. */ PREPEND_SLASH (path); - cookie = parse_set_cookie (set_cookie, false); + cookie = parse_set_cookie (set_cookie, scheme, false); if (!cookie) goto out; diff --git a/src/cookies.h b/src/cookies.h index b4281e1..d565b09 100644 --- a/src/cookies.h +++ b/src/cookies.h @@ -31,13 +31,16 @@ as that of the covered work. */ #ifndef COOKIES_H #define COOKIES_H +#include "url.h" + struct cookie_jar; struct cookie_jar *cookie_jar_new (void); void cookie_jar_delete (struct cookie_jar *); void cookie_handle_set_cookie (struct cookie_jar *, const char *, int, - const char *, const char *); + const char *, enum url_scheme, + const char *); char *cookie_header (struct cookie_jar *, const char *, int, const char *, bool); diff --git a/src/http.c b/src/http.c index 8916d2b..f7d42b2 100644 --- a/src/http.c +++ b/src/http.c @@ -3276,7 +3276,7 @@ gethttp (struct url *u, struct http_stat *hs, int *dt, struct url *proxy, { char *set_cookie; BOUNDED_TO_ALLOCA (scbeg, scend, set_cookie); cookie_handle_set_cookie (wget_cookie_jar, u->host, u->port, - u->path, set_cookie); + u->path, u->scheme, set_cookie); } } -- 1.9.1
From 493ad523500fa700f1c3bc8b912b9342f8ce0356 Mon Sep 17 00:00:00 2001 From: kush789 <[email protected]> Date: Sun, 24 Jan 2016 15:01:02 +0530 Subject: [PATCH 3/4] Added recomendation 2 --- src/cookies.c | 56 +++++++++++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/src/cookies.c b/src/cookies.c index ea1a8df..190236e 100644 --- a/src/cookies.c +++ b/src/cookies.c @@ -713,39 +713,26 @@ check_path_match (const char *cookie_path, const char *path) s = PS_newstr; \ } while (0) -static int -check_cookie_matching_name_domain (struct cookie_jar *jar, - struct cookie *cookie) -{ - struct cookie *chain, *prev; - - chain = hash_table_get (jar->chains, cookie->domain); - - if (!chain) - return 0; - - prev = NULL; - for (; chain; prev = chain, chain = chain->next) - if (0 == strcmp (cookie->attr, chain->attr) - && 1 == chain->secure) - { - return 1; - } - - return 0; -} - /* Process the HTTP `Set-Cookie' header. This results in storing the cookie or discarding a matching one, or ignoring it completely, all depending on the contents. */ +static int +find_chains_of_host (struct cookie_jar *jar, const char *host, + struct cookie *dest[]); + +static int +count_char (const char *string, char chr); + void cookie_handle_set_cookie (struct cookie_jar *jar, const char *host, int port, const char *path, enum url_scheme scheme, const char *set_cookie) { - struct cookie *cookie; + struct cookie *cookie, *old_cookie; + struct cookie **chains; + int chain_count, i; cookies_now = time (NULL); /* Wget's paths don't begin with '/' (blame rfc1808), but cookie @@ -803,6 +790,29 @@ cookie_handle_set_cookie (struct cookie_jar *jar, } } + if ((cookie->secure == 0) && (scheme != SCHEME_HTTPS)) + { + /* If an old cookie exists such that the all of the following are + true, then discard the new cookie. + + - The "domain" domain-matches the domain of the new cookie + - The "name" matches the "name" of the new cookie + - Secure-only flag of old cookie is set */ + + chains = alloca_array (struct cookie *, 1 + count_char (host, '.')); + chain_count = find_chains_of_host (jar, host, chains); + + for (i = 0; i < chain_count; i++) + for (old_cookie = chains[i]; old_cookie; old_cookie = old_cookie->next) + { + if (!cookie_expired_p(old_cookie) + && !(old_cookie->domain_exact + && 0 != strcasecmp (host, old_cookie->domain)) + && 0 == strcasecmp(old_cookie->attr, cookie->attr) + && 1 == old_cookie->secure) + goto out; + } + } /* Now store the cookie, or discard an existing cookie, if discarding was requested. */ -- 1.9.1
From 48941100df4f870ef6dc44e8f638d85092ff0de3 Mon Sep 17 00:00:00 2001 From: kush789 <[email protected]> Date: Sun, 24 Jan 2016 15:04:49 +0530 Subject: [PATCH 4/4] Moved a couple of methods up to remove unnecessary prototypes --- src/cookies.c | 134 ++++++++++++++++++++++++++++------------------------------ 1 file changed, 64 insertions(+), 70 deletions(-) diff --git a/src/cookies.c b/src/cookies.c index 190236e..1797a73 100644 --- a/src/cookies.c +++ b/src/cookies.c @@ -713,16 +713,74 @@ check_path_match (const char *cookie_path, const char *path) s = PS_newstr; \ } while (0) -/* Process the HTTP `Set-Cookie' header. This results in storing the - cookie or discarding a matching one, or ignoring it completely, all - depending on the contents. */ +/* Return a count of how many times CHR occurs in STRING. */ static int -find_chains_of_host (struct cookie_jar *jar, const char *host, - struct cookie *dest[]); +count_char (const char *string, char chr) +{ + const char *p; + int count = 0; + for (p = string; *p; p++) + if (*p == chr) + ++count; + return count; +} + +/* Find the cookie chains whose domains match HOST and store them to + DEST. + + A cookie chain is the head of a list of cookies that belong to a + host/domain. Given HOST "img.search.xemacs.org", this function + will return the chains for "img.search.xemacs.org", + "search.xemacs.org", and "xemacs.org" -- those of them that exist + (if any), that is. + + DEST should be large enough to accept (in the worst case) as many + elements as there are domain components of HOST. */ static int -count_char (const char *string, char chr); +find_chains_of_host (struct cookie_jar *jar, const char *host, + struct cookie *dest[]) +{ + int dest_count = 0; + int passes, passcnt; + + /* Bail out quickly if there are no cookies in the jar. */ + if (!hash_table_count (jar->chains)) + return 0; + + if (numeric_address_p (host)) + /* If host is an IP address, only check for the exact match. */ + passes = 1; + else + /* Otherwise, check all the subdomains except the top-level (last) + one. As a domain with N components has N-1 dots, the number of + passes equals the number of dots. */ + passes = count_char (host, '.'); + + passcnt = 0; + + /* Find chains that match HOST, starting with exact match and + progressing to less specific domains. For instance, given HOST + fly.srk.fer.hr, first look for fly.srk.fer.hr's chain, then + srk.fer.hr's, then fer.hr's. */ + while (1) + { + struct cookie *chain = hash_table_get (jar->chains, host); + if (chain) + dest[dest_count++] = chain; + if (++passcnt >= passes) + break; + host = strchr (host, '.') + 1; + } + + return dest_count; +} + +/* Process the HTTP `Set-Cookie' header. This results in storing the + cookie or discarding a matching one, or ignoring it completely, all + depending on the contents. */ + void cookie_handle_set_cookie (struct cookie_jar *jar, @@ -834,70 +892,6 @@ cookie_handle_set_cookie (struct cookie_jar *jar, previously stored cookies. Entry point is `build_cookies_request'. */ -/* Return a count of how many times CHR occurs in STRING. */ - -static int -count_char (const char *string, char chr) -{ - const char *p; - int count = 0; - for (p = string; *p; p++) - if (*p == chr) - ++count; - return count; -} - -/* Find the cookie chains whose domains match HOST and store them to - DEST. - - A cookie chain is the head of a list of cookies that belong to a - host/domain. Given HOST "img.search.xemacs.org", this function - will return the chains for "img.search.xemacs.org", - "search.xemacs.org", and "xemacs.org" -- those of them that exist - (if any), that is. - - DEST should be large enough to accept (in the worst case) as many - elements as there are domain components of HOST. */ - -static int -find_chains_of_host (struct cookie_jar *jar, const char *host, - struct cookie *dest[]) -{ - int dest_count = 0; - int passes, passcnt; - - /* Bail out quickly if there are no cookies in the jar. */ - if (!hash_table_count (jar->chains)) - return 0; - - if (numeric_address_p (host)) - /* If host is an IP address, only check for the exact match. */ - passes = 1; - else - /* Otherwise, check all the subdomains except the top-level (last) - one. As a domain with N components has N-1 dots, the number of - passes equals the number of dots. */ - passes = count_char (host, '.'); - - passcnt = 0; - - /* Find chains that match HOST, starting with exact match and - progressing to less specific domains. For instance, given HOST - fly.srk.fer.hr, first look for fly.srk.fer.hr's chain, then - srk.fer.hr's, then fer.hr's. */ - while (1) - { - struct cookie *chain = hash_table_get (jar->chains, host); - if (chain) - dest[dest_count++] = chain; - if (++passcnt >= passes) - break; - host = strchr (host, '.') + 1; - } - - return dest_count; -} - /* If FULL_PATH begins with PREFIX, return the length of PREFIX, zero otherwise. */ -- 1.9.1
From 0c8959ac8cee320b2c6fe52f914b5c3540d125a0 Mon Sep 17 00:00:00 2001 From: kush789 <[email protected]> Date: Sun, 24 Jan 2016 16:57:42 +0530 Subject: [PATCH] Fixed call to parse_set_cookie in cookie tests --- src/cookies.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cookies.c b/src/cookies.c index 1797a73..f39998b 100644 --- a/src/cookies.c +++ b/src/cookies.c @@ -1457,7 +1457,7 @@ test_cookies (void) const char **expected = tests_succ[i].results; struct cookie *c; - c = parse_set_cookie (data, true); + c = parse_set_cookie (data, SCHEME_HTTPS, true); if (!c) { printf ("NULL cookie returned for valid data: %s\n", data); @@ -1497,7 +1497,7 @@ test_cookies (void) { struct cookie *c; char *data = tests_fail[i]; - c = parse_set_cookie (data, true); + c = parse_set_cookie (data, SCHEME_HTTPS, true); if (c) printf ("Failed to report error on invalid data: %s\n", data); } -- 1.9.1
