Add domain and path checks for secure cookies eviction #14491

KiChjang · 2016-12-08T10:06:36Z

Fixes #14477.

This change is

highfive · 2016-12-08T10:06:40Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/cookie_storage.rs, components/net/cookie.rs

highfive · 2016-12-08T10:06:41Z

Warning

These commits modify net code, but no tests are modified. Please consider adding a test!

KiChjang · 2016-12-08T10:07:00Z

r? @jdm

Ms2ger · 2016-12-08T10:20:29Z

components/net/cookie_storage.rs

-        // Step 1
+        // https://www.ietf.org/id/draft-ietf-httpbis-cookie-alone-01.txt Step 2
+        if !cookie.cookie.secure && source == CookieSource::HTTP &&
+           cookies.iter().filter(|c| {


This looks like a more complicated way of writing .any(..)

Totally is, that function completely slipped my mind.

jdm

We will definitely need tests verifying that the attacks described in the spec fail.

jdm · 2016-12-08T21:19:19Z

components/net/cookie_storage.rs

@@ -62,6 +74,12 @@ impl CookieStorage {

    // http://tools.ietf.org/html/rfc6265#section-5.3
    pub fn push(&mut self, mut cookie: Cookie, source: CookieSource) {
+        // https://www.ietf.org/id/draft-ietf-httpbis-cookie-alone-01.txt Step 1
+        if cookie.cookie.secure && source == CookieSource::HTTP {
+            // New cookie is not from a "secure" protocol, ignore.


CookieSource::HTTP only means that it's an HTTP/HTTPS request, rather than document.cookie. Let's add a ServoUrl argument to CookieSource::HTTP so we can check it here.

jdm · 2016-12-08T21:19:19Z

components/net/cookie_storage.rs

+           cookies.iter().any(|c| {
+                c.cookie.name == cookie.cookie.name &&
+                c.cookie.secure &&
+                domain_match(c.cookie.domain, cookie.cookie.domain) &&


This is missing the "vice-versa" part of step 2.1.3.

jdm · 2016-12-08T21:19:19Z

components/net/cookie.rs

+        string == domain_string || (string.ends_with(domain_string) &&
+                                    string.as_bytes()[string.len()-domain_string.len()-1] == b'.' &&
+                                    string.parse::<Ipv4Addr>().is_err() &&
+                                    string.parse::<Ipv6Addr>().is_err())


Could we do everything after the || on a new line instead?

jdm · 2016-12-08T21:19:19Z

components/net/cookie_storage.rs

@@ -37,23 +37,35 @@ impl CookieStorage {
        let domain = reg_host(cookie.cookie.domain.as_ref().unwrap_or(&"".to_string()));
        let cookies = self.cookies_map.entry(domain).or_insert(vec![]);

-        // Step 1
+        // https://www.ietf.org/id/draft-ietf-httpbis-cookie-alone-01.txt Step 2
+        if !cookie.cookie.secure && source == CookieSource::HTTP &&


We'll want to check the actual request URL here.

jdm · 2016-12-09T20:36:50Z

components/net/cookie_storage.rs

+
+                c.cookie.name == cookie.cookie.name &&
+                c.cookie.secure &&
+                c.cookie.domain == cookie.cookie.domain &&


What happened to performing the symmetric domain matches from the spec? That's different than checking if the domains for the two cookies are identical.

I was thinking that bi-directional domain-matching is equivalent to equality comparison. RFC 2965 states the following:

Host A's name domain-matches host B's if * their host name strings string-compare equal; or * A is a HDN string and has the form NB, where N is a non-empty name string, B has the form .B', and B' is a HDN string. (So, x.y.com domain-matches .Y.com but not Y.com.)

It's the second point that doesn't get covered by the equality check here, since if the domains are not strictly equal they can still match.

Yes, but since it's checking bi-directionally, I don't think it's possible where A domain-matches B and B domain-matches A but A is not equal to B.

Basically it boils down to the statement "if two strings are suffixes of each other, then they are equal".

Nevermind, the IETF draft did not say "and vice versa", it said "or vice versa".

jdm · 2016-12-09T20:36:50Z

components/net/cookie_storage.rs

+                    Cookie::path_match(new, existing)
+                } else {
+                    cookie.cookie.path == c.cookie.path
+                };


Our cookies always have a path according to https://github.com/KiChjang/servo/blob/cf08600e5f390b72a6e094e74633326bad204908/components/net/cookie.rs#L69-L74, so let's use expect on both of them instead.

frewsxcv · 2016-12-09T21:49:11Z

components/net/cookie.rs

-        false
+        string == domain_string ||
+        (string.ends_with(domain_string) &&
+         string.as_bytes()[string.len()-domain_string.len()-1] == b'.' &&


Should we add an assert here that string.len() > domain_string.len() ?

If string ends with domain_string, that is already implicitly true.

jdm · 2016-12-13T20:05:01Z

Note to self: be sure that these changes do not prevent a page using document.cookie to set a secure cookie if the page running the JS has a secure origin.

bors-servo · 2016-12-14T16:26:48Z

☔ The latest upstream changes (presumably #14570) made this pull request unmergeable. Please resolve the merge conflicts.

jdm

Good start on the tests. We need more thorough coverage for some of the cases to be sure we're not breaking the existing implementation and to be sure we're properly blocking all the cases that the specification calls out.

jdm · 2016-12-15T19:02:23Z

components/net/cookie_storage.rs

@@ -37,6 +37,21 @@ impl CookieStorage {
        let domain = reg_host(cookie.cookie.domain.as_ref().unwrap_or(&"".to_string()));
        let cookies = self.cookies_map.entry(domain).or_insert(vec![]);

+        // https://www.ietf.org/id/draft-ietf-httpbis-cookie-alone-01.txt Step 2
+        if !cookie.cookie.secure && !source.is_secure_protocol() &&
+           cookies.iter().any(|c|


Let's do:

if !cookie.cookie.secure && !source.is_secure_protocol() { let new_domain = cookie.cookie.domain.as_ref().unwrap(); let new_path = cookie.cookie.path.as_ref().unwrap(); let any_overlapping = cookies.iter().any(|c| { let existing_domain = c.cookie.domain.as_ref().unwrap(); let existing_path = c.cookie.path.as_ref().unwrap(); c.cookie.name == cookie.cookie.name && c.cookie.secure && (Cookie::domain_match(new_domain, existing_domain) || Cookie::domain_match(existing_domain, new_domain)) && Cookie::path_match(new_path, existing_path) }); if any_overlapping { return Err(()); } }

jdm · 2016-12-15T19:02:23Z

components/net_traits/lib.rs

 pub enum CookieSource {
    /// An HTTP API
-    HTTP,
+    HTTP(ServoUrl),
    /// A non-HTTP API
    NonHTTP,


I think we're going to have to add a ServoUrl to this case to, or we'll break allowing pages to set secure cookies via document.cookie when loaded via a secure scheme.

What would a non-secure non-HTTP source look like?

Using document.cookie on a site with an HTTP URL.

jdm · 2016-12-15T19:02:23Z

tests/unit/net/cookie.rs

+#[test]
+fn test_insecure_cookies_cannot_evict_secure_cookie() {
+    let mut storage = CookieStorage::new(5);
+    let url = ServoUrl::parse("https://home.example.org:8888/cookie-parser?0001").unwrap();


Let's call this secure_url and reuse it later.

jdm · 2016-12-15T19:02:23Z

tests/unit/net/cookie.rs

+        storage.push(cookie, source.clone());
+    }
+
+    let url = ServoUrl::parse("http://home.example.org:8888/cookie-parser?0001").unwrap();


Let's call this insecure_url.

jdm · 2016-12-15T19:02:23Z

tests/unit/net/cookie.rs

+    add_cookie_to_storage(&mut storage, &url, "foo=value; Domain=home.example.org");
+    add_cookie_to_storage(&mut storage, &url, "foo2=value; Domain=.example.org");
+    add_cookie_to_storage(&mut storage, &url, "foo3=value; Path=/foo/bar");
+    add_cookie_to_storage(&mut storage, &url, "foo4=value; Path=/foo");


We need another test that tries setting these cookies using a secure scheme, too.

jdm · 2016-12-15T19:02:23Z

tests/unit/net/cookie.rs

+    let source = CookieSource::HTTP(url.clone());
+    assert_eq!(storage.cookies_for_url(&url, source).unwrap(), "foo4=bar; foo3=bar; foo4=value; foo=bar; foo2=bar");
+}
+


We should add a test that it's still possible to set secure cookies using a NonHTTP source and a secure URL.

jdm · 2016-12-19T13:11:51Z

Seems like the rebase went a bit wonky here?


        Conform conditionals regarding cookie removal with spec

KiChjang · 2016-12-20T19:53:55Z

Still only rebased; haven't gotten around to address comments yet.


        Add domain and path checks for secure cookies eviction


        Add unit tests

jdm · 2016-12-23T22:52:05Z

I finally sat down and reasoned my way through the test results. This looks good to me; thanks for working on it!
@bors-servo: r+

bors-servo · 2016-12-23T22:52:06Z

📌 Commit 16f1947 has been approved by jdm

bors-servo · 2016-12-23T23:26:47Z

⌛ Testing commit 16f1947 with merge b13afcc...


        Auto merge of #14491 - KiChjang:cookie-checks, r=jdm

Add domain and path checks for secure cookies eviction Fixes #14477.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14491)

bors-servo · 2016-12-24T00:21:56Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned nox Dec 8, 2016

highfive added the S-awaiting-review label Dec 8, 2016

highfive assigned jdm and unassigned nox Dec 8, 2016

Ms2ger reviewed Dec 8, 2016

View changes

KiChjang force-pushed the KiChjang:cookie-checks branch from b987b9c to e9fa399 Dec 8, 2016

jdm requested changes Dec 8, 2016

View changes

jdm added S-needs-code-changes S-needs-tests and removed S-awaiting-review labels Dec 8, 2016

KiChjang force-pushed the KiChjang:cookie-checks branch from e9fa399 to cf08600 Dec 9, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 9, 2016

jdm requested changes Dec 9, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 9, 2016

frewsxcv reviewed Dec 9, 2016

View changes

KiChjang force-pushed the KiChjang:cookie-checks branch from cf08600 to 5901f2d Dec 9, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 9, 2016

KiChjang removed the S-needs-tests label Dec 10, 2016

KiChjang force-pushed the KiChjang:cookie-checks branch from 67f6009 to 5cc9952 Dec 10, 2016

highfive added the S-needs-rebase label Dec 14, 2016

jdm requested changes Dec 15, 2016

View changes

jdm added the S-needs-code-changes label Dec 15, 2016

KiChjang force-pushed the KiChjang:cookie-checks branch from 5cc9952 to b31d8f6 Dec 18, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 18, 2016

Conform conditionals regarding cookie removal with spec

64b456f

KiChjang force-pushed the KiChjang:cookie-checks branch from b31d8f6 to 8509127 Dec 20, 2016

KiChjang added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Dec 20, 2016

Add domain and path checks for secure cookies eviction

63a7e8e

KiChjang force-pushed the KiChjang:cookie-checks branch from 8509127 to 294adc8 Dec 21, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 21, 2016

jdm added the S-fails-tidy label Dec 21, 2016

KiChjang force-pushed the KiChjang:cookie-checks branch from 294adc8 to 16f1947 Dec 22, 2016

Add unit tests

Loading status checks…

16f1947

KiChjang removed the S-fails-tidy label Dec 22, 2016

jdm mentioned this pull request Dec 23, 2016

Cookie::appropriate_for_url should check for wss schemes in addition to https #14702

Closed

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 23, 2016

bors-servo merged commit 16f1947 into servo:master Dec 24, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 24, 2016

KiChjang deleted the KiChjang:cookie-checks branch Dec 24, 2016

jdm mentioned this pull request Feb 3, 2017

Investigate first party cookies, cookie prefix, and insecure modification of secure cookies #8700

Open

2 of 3 tasks complete

servo / servo

Add domain and path checks for secure cookies eviction #14491

Add domain and path checks for secure cookies eviction #14491

KiChjang commented Dec 8, 2016 •

edited by larsbergstrom

highfive commented Dec 8, 2016

highfive commented Dec 8, 2016

KiChjang commented Dec 8, 2016

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Dec 13, 2016

bors-servo commented Dec 14, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Dec 19, 2016

KiChjang commented Dec 20, 2016

jdm commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 24, 2016

servo / servo

Join GitHub today

Add domain and path checks for secure cookies eviction #14491

Add domain and path checks for secure cookies eviction #14491

Conversation

KiChjang commented Dec 8, 2016 • edited by larsbergstrom

highfive commented Dec 8, 2016

highfive commented Dec 8, 2016

KiChjang commented Dec 8, 2016

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang Dec 10, 2016 • edited

This comment has been minimized.

KiChjang Dec 10, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Dec 13, 2016

bors-servo commented Dec 14, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Dec 19, 2016

KiChjang commented Dec 20, 2016

jdm commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 24, 2016

KiChjang commented Dec 8, 2016 •

edited by larsbergstrom

KiChjang Dec 10, 2016 •

edited

KiChjang Dec 10, 2016 •

edited