From 7a09b52c98ac8d840a8a9907b1a1d9a9e684bcf5 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Thu, 13 Dec 2018 09:57:58 +0100
Subject: [PATCH] cookies: leave secure cookies alone
Only allow secure origins to be able to write cookies with the
'secure' flag set. This reduces the risk of non-secure origins
to influence the state of secure origins. This implements IETF
Internet-Draft draft-ietf-httpbis-cookie-alone-01 which updates
RFC6265.
Closes #2956
Reviewed-by: Daniel Stenberg <daniel@haxx.se>
---
docs/HTTP-COOKIES.md | 4 +-
docs/TODO | 8 ----
lib/cookie.c | 55 ++++++++++++++++++++++----
lib/cookie.h | 5 ++-
lib/http.c | 4 +-
lib/setopt.c | 4 +-
tests/data/Makefile.inc | 2 +-
tests/data/test1155 | 4 +-
tests/data/test1561 | 86 +++++++++++++++++++++++++++++++++++++++++
tests/data/test31 | 18 ---------
tests/data/test61 | 1 -
11 files changed, 148 insertions(+), 43 deletions(-)
create mode 100644 tests/data/test1561
diff --git a/docs/HTTP-COOKIES.md b/docs/HTTP-COOKIES.md
index a1b2834543..66e39d2325 100644
--- a/docs/HTTP-COOKIES.md
+++ b/docs/HTTP-COOKIES.md
@@ -18,7 +18,9 @@
original [Netscape spec from 1994](https://curl.haxx.se/rfc/cookie_spec.html).
In 2011, [RFC6265](https://www.ietf.org/rfc/rfc6265.txt) was finally
- published and details how cookies work within HTTP.
+ published and details how cookies work within HTTP. In 2017, an update was
+ [drafted](https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone-01)
+ to deprecate modification of 'secure' cookies from non-secure origins.
## Cookies saved to disk
diff --git a/docs/TODO b/docs/TODO
index f7fd722a86..e0d8ed68ff 100644
--- a/docs/TODO
+++ b/docs/TODO
@@ -73,7 +73,6 @@
5.5 auth= in URLs
5.6 Refuse "downgrade" redirects
5.7 QUIC
- 5.8 Leave secure cookies alone
6. TELNET
6.1 ditch stdin
@@ -605,13 +604,6 @@
implemented. This, to allow other projects to benefit from the work and to
thus broaden the interest and chance of others to participate.
-5.8 Leave secure cookies alone
-
- Non-secure origins (HTTP sites) should not be allowed to set or modify
- cookies with the 'secure' property:
-
- https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone-01
-
6. TELNET
diff --git a/lib/cookie.c b/lib/cookie.c
index 3dc85ee5ca..bc0ab0dfe3 100644
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -433,9 +433,10 @@ Curl_cookie_add(struct Curl_easy *data,
bool noexpire, /* if TRUE, skip remove_expired() */
char *lineptr, /* first character of the line */
const char *domain, /* default domain */
- const char *path) /* full path used when this cookie is set,
+ const char *path, /* full path used when this cookie is set,
used to get default path for the cookie
unless set */
+ bool secure) /* TRUE if connection is over secure origin */
{
struct Cookie *clist;
struct Cookie *co;
@@ -546,8 +547,20 @@ Curl_cookie_add(struct Curl_easy *data,
/* this was a "<name>=" with no content, and we must allow
'secure' and 'httponly' specified this weirdly */
done = TRUE;
- if(strcasecompare("secure", name))
- co->secure = TRUE;
+ /*
+ * secure cookies are only allowed to be set when the connection is
+ * using a secure protocol, or when the cookie is being set by
+ * reading from file
+ */
+ if(strcasecompare("secure", name)) {
+ if(secure || !c->running) {
+ co->secure = TRUE;
+ }
+ else {
+ badcookie = TRUE;
+ break;
+ }
+ }
else if(strcasecompare("httponly", name))
co->httponly = TRUE;
else if(sep)
@@ -831,7 +844,13 @@ Curl_cookie_add(struct Curl_easy *data,
fields++; /* add a field and fall down to secure */
/* FALLTHROUGH */
case 3:
- co->secure = strcasecompare(ptr, "TRUE")?TRUE:FALSE;
+ co->secure = FALSE;
+ if(strcasecompare(ptr, "TRUE")) {
+ if(secure || c->running)
+ co->secure = TRUE;
+ else
+ badcookie = TRUE;
+ }
break;
case 4:
if(curlx_strtoofft(ptr, NULL, 10, &co->expires))
@@ -929,9 +948,31 @@ Curl_cookie_add(struct Curl_easy *data,
/* the domains were identical */
if(clist->spath && co->spath) {
- if(strcasecompare(clist->spath, co->spath)) {
- replace_old = TRUE;
+ if(clist->secure && !co->secure) {
+ size_t cllen;
+ const char *sep;
+
+ /*
+ * A non-secure cookie may not overlay an existing secure cookie.
+ * For an existing cookie "a" with path "/login", refuse a new
+ * cookie "a" with for example path "/login/en", while the path
+ * "/loginhelper" is ok.
+ */
+
+ sep = strchr(clist->spath + 1, '/');
+
+ if(sep)
+ cllen = sep - clist->spath;
+ else
+ cllen = strlen(clist->spath);
+
+ if(strncasecompare(clist->spath, co->spath, cllen)) {
+ freecookie(co);
+ return NULL;
+ }
}
+ else if(strcasecompare(clist->spath, co->spath))
+ replace_old = TRUE;
else
replace_old = FALSE;
}
@@ -1103,7 +1144,7 @@ struct CookieInfo *Curl_cookie_init(struct Curl_easy *data,
while(*lineptr && ISBLANK(*lineptr))
lineptr++;
- Curl_cookie_add(data, c, headerline, TRUE, lineptr, NULL, NULL);
+ Curl_cookie_add(data, c, headerline, TRUE, lineptr, NULL, NULL, TRUE);
}
free(line); /* free the line buffer */
remove_expired(c); /* run this once, not on every cookie */
diff --git a/lib/cookie.h b/lib/cookie.h
index a9f90ca715..3ee457c622 100644
--- a/lib/cookie.h
+++ b/lib/cookie.h
@@ -7,7 +7,7 @@
* | (__| |_| | _ <| |___
* \___|\___/|_| \_\_____|
*
- * Copyright (C) 1998 - 2017, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.
*
* This software is licensed as described in the file COPYING, which
* you should have received as part of this distribution. The terms
@@ -85,7 +85,8 @@ struct Curl_easy;
struct Cookie *Curl_cookie_add(struct Curl_easy *data,
struct CookieInfo *, bool header, bool noexpiry,
char *lineptr,
- const char *domain, const char *path);
+ const char *domain, const char *path,
+ bool secure);
struct Cookie *Curl_cookie_getlist(struct CookieInfo *, const char *,
const char *, bool);
diff --git a/lib/http.c b/lib/http.c
index 345100f6c8..0a3e462435 100644
--- a/lib/http.c
+++ b/lib/http.c
@@ -3873,7 +3873,9 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data,
here, or else use real peer host name. */
conn->allocptr.cookiehost?
conn->allocptr.cookiehost:conn->host.name,
- data->state.up.path);
+ data->state.up.path,
+ (conn->handler->protocol&CURLPROTO_HTTPS)?
+ TRUE:FALSE);
Curl_share_unlock(data, CURL_LOCK_DATA_COOKIE);
}
#endif
diff --git a/lib/setopt.c b/lib/setopt.c
index 1627aba6df..01a890b396 100644
--- a/lib/setopt.c
+++ b/lib/setopt.c
@@ -803,12 +803,12 @@ CURLcode Curl_vsetopt(struct Curl_easy *data, CURLoption option,
if(checkprefix("Set-Cookie:", argptr))
/* HTTP Header format line */
Curl_cookie_add(data, data->cookies, TRUE, FALSE, argptr + 11, NULL,
- NULL);
+ NULL, TRUE);
else
/* Netscape format line */
Curl_cookie_add(data, data->cookies, FALSE, FALSE, argptr, NULL,
- NULL);
+ NULL, TRUE);
Curl_share_unlock(data, CURL_LOCK_DATA_COOKIE);
free(argptr);
diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc
index dd38f89647..250aa2004b 100644
--- a/tests/data/Makefile.inc
+++ b/tests/data/Makefile.inc
@@ -176,7 +176,7 @@ test1533 test1534 test1535 test1536 test1537 test1538 \
test1540 \
test1550 test1551 test1552 test1553 test1554 test1555 test1556 test1557 \
\
-test1560 \
+test1560 test1561 \
\
test1590 \
test1600 test1601 test1602 test1603 test1604 test1605 test1606 test1607 \
diff --git a/tests/data/test1155 b/tests/data/test1155
index 9bf3254604..3db824d58a 100644
--- a/tests/data/test1155
+++ b/tests/data/test1155
@@ -14,7 +14,7 @@ cookies
HTTP/1.1 200 OK
Date: Thu, 09 Nov 2010 14:49:00 GMT
Content-Length: 0
-Set-Cookie: domain=value;secure;path=/
+Set-Cookie: domain=value;path=/
</data>
</reply>
@@ -48,7 +48,7 @@ Accept: */*
# https://curl.haxx.se/docs/http-cookies.html
# This file was generated by libcurl! Edit at your own risk.
-127.0.0.1 FALSE / TRUE 0 domain value
+127.0.0.1 FALSE / FALSE 0 domain value
</file>
</verify>
</testcase>
diff --git a/tests/data/test1561 b/tests/data/test1561
new file mode 100644
index 0000000000..356dc94e4c
--- /dev/null
+++ b/tests/data/test1561
@@ -0,0 +1,86 @@
+<testcase>
+<info>
+<keywords>
+HTTPS
+HTTP
+HTTP GET
+cookies
+cookiejar
+HTTP replaced headers
+</keywords>
+</info>
+
+# Server-side
+<reply>
+<data1>
+HTTP/1.1 200 OK
+Date: Thu, 09 Nov 2010 14:49:00 GMT
+Server: test-server/fake
+Set-Cookie: super=secret; domain=example.com; path=/1561; secure;
+Set-Cookie: supersuper=secret; domain=example.com; path=/1561/login/; secure;
+Content-Length: 7
+
+nomnom
+</data1>
+<data2>
+HTTP/1.1 200 OK
+Date: Thu, 09 Nov 2010 14:49:00 GMT
+Server: test-server/fake
+Set-Cookie: super=secret; domain=example.com; path=/1561; httponly;
+Set-Cookie: super=secret; domain=example.com; path=/1561/; httponly;
+Set-Cookie: super=secret; domain=example.com; path=/15; httponly;
+Set-Cookie: public=yes; domain=example.com; path=/foo;
+Set-Cookie: supersuper=secret; domain=example.com; path=/1561/login/en;
+Set-Cookie: supersuper=secret; domain=example.com; path=/1561/login;
+Set-Cookie: secureoverhttp=yes; domain=example.com; path=/1561; secure;
+Content-Length: 7
+
+nomnom
+</data2>
+</reply>
+
+# Client-side
+<client>
+<features>
+SSL
+</features>
+<server>
+http
+https
+</server>
+<name>
+HTTP
+</name>
+<command>
+-k https://%HOSTIP:%HTTPSPORT/15610001 -L -c log/jar1561.txt -H "Host: www.example.com" http://%HOSTIP:%HTTPPORT/15610002 -L -c log/jar1561.txt -H "Host: www.example.com"
+</command>
+</client>
+<verify>
+<strip>
+^User-Agent:.*
+</strip>
+<protocol>
+GET /15610001 HTTP/1.1
+Host: www.example.com
+User-Agent: curl/7.62.0-DEV
+Accept: */*
+
+GET /15610002 HTTP/1.1
+Host: www.example.com
+User-Agent: curl/7.62.0-DEV
+Accept: */*
+
+</protocol>
+<file name="log/jar1561.txt" mode="text">
+# Netscape HTTP Cookie File
+# https://curl.haxx.se/docs/http-cookies.html
+# This file was generated by libcurl! Edit at your own risk.
+
+.example.com TRUE /foo FALSE 0 public yes
+.example.com TRUE /1561/login/ TRUE 0 supersuper secret
+#HttpOnly_.example.com TRUE /15 FALSE 0 super secret
+</file>
+
+</verify>
+
+</testcase>
diff --git a/tests/data/test31 b/tests/data/test31
index 78f3766e9b..58398c55d9 100644
--- a/tests/data/test31
+++ b/tests/data/test31
@@ -100,7 +100,6 @@ Accept: */*
# https://curl.haxx.se/docs/http-cookies.html
# This file was generated by libcurl! Edit at your own risk.
-127.0.0.1 FALSE /we/want/ TRUE 0 securewithspace after
127.0.0.1 FALSE /we/want/ FALSE 0 prespace yes before
127.0.0.1 FALSE /we/want/ FALSE 0 withspaces2 before equals
127.0.0.1 FALSE /we/want/ FALSE 0 withspaces yes within and around
@@ -108,28 +107,11 @@ Accept: */*
#HttpOnly_127.0.0.1 FALSE /silly/ FALSE 0 magic yessir
127.0.0.1 FALSE /we/want/ FALSE 2054030187 nodomain value
127.0.0.1 FALSE / FALSE 0 partmatch present
-#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec8 myvalue9
-#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec7 myvalue8
-#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec6 myvalue7
-#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec5 myvalue6
-#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec4 myvalue5
-#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec3 myvalue4
-#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec2 myvalue3
-#HttpOnly_127.0.0.1 FALSE /p4/ TRUE 0 httpandsec myvalue2
#HttpOnly_127.0.0.1 FALSE /p4/ FALSE 0 httponly myvalue1
#HttpOnly_127.0.0.1 FALSE /p4/ FALSE 0 httpo4 value4
#HttpOnly_127.0.0.1 FALSE /p3/ FALSE 0 httpo3 value3
#HttpOnly_127.0.0.1 FALSE /p2/ FALSE 0 httpo2 value2
#HttpOnly_127.0.0.1 FALSE /p1/ FALSE 0 httpo1 value1
-127.0.0.1 FALSE /secure9/ TRUE 0 secure very1
-127.0.0.1 FALSE /secure8/ TRUE 0 sec8value secure8
-127.0.0.1 FALSE /secure7/ TRUE 0 sec7value secure7
-127.0.0.1 FALSE /secure6/ TRUE 0 sec6value secure6
-127.0.0.1 FALSE /secure5/ TRUE 0 sec5value secure5
-127.0.0.1 FALSE /secure4/ TRUE 0 sec4value secure4
-127.0.0.1 FALSE /secure3/ TRUE 0 sec3value secure3
-127.0.0.1 FALSE /secure2/ TRUE 0 sec2value secure2
-127.0.0.1 FALSE /secure1/ TRUE 0 sec1value secure1
127.0.0.1 FALSE /overwrite FALSE 0 overwrite this2
127.0.0.1 FALSE /silly/ FALSE 0 ismatch this
</file>
diff --git a/tests/data/test61 b/tests/data/test61
index 784163fa96..2709f5112e 100644
--- a/tests/data/test61
+++ b/tests/data/test61
@@ -65,7 +65,6 @@ Accept: */*
# https://curl.haxx.se/docs/http-cookies.html
# This file was generated by libcurl! Edit at your own risk.
-.foo.com TRUE /moo TRUE 0 test3 maybe
.host.foo.com TRUE /we/want/ FALSE 2054030187 test2 yes
#HttpOnly_.foo.com TRUE /we/want/ FALSE 2054030187 test yes
</file>
--
GitLab