servo: Merge #14491 - Add domain and path checks for secure cookies eviction (from KiChjang:cookie-checks); r=jdm
authorKeith Yeung <kungfukeith11@gmail.com>
Fri, 23 Dec 2016 15:26:46 -0800
changeset 340402 e4e77b5d0aeaa9907a500a66df66799b253fd8d5
parent 340401 6f479e92d4fedfbfae0c5f64214225e3b63e6387
child 340403 c90aff3d827221f016a50b7c8b9906bff23e7267
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm
servo: Merge #14491 - Add domain and path checks for secure cookies eviction (from KiChjang:cookie-checks); r=jdm Fixes #14477. Source-Repo: https://github.com/servo/servo Source-Revision: b13afccb8a4e922f66c77a92914e6505c62ae483
servo/components/net/cookie.rs
servo/components/net/cookie_storage.rs
servo/components/net/http_loader.rs
servo/components/net/resource_thread.rs
servo/tests/unit/net/cookie.rs
servo/tests/unit/net/cookie_http_state.rs
servo/tests/unit/net/http_loader.rs
--- a/servo/components/net/cookie.rs
+++ b/servo/components/net/cookie.rs
@@ -70,17 +70,17 @@ impl Cookie {
         let mut path = cookie.path.unwrap_or("".to_owned());
         if path.chars().next() != Some('/') {
             path = Cookie::default_path(request.path()).to_owned();
         }
         cookie.path = Some(path);
 
 
         // Step 10
-        if cookie.httponly && source != CookieSource::HTTP {
+        if cookie.httponly && source == CookieSource::NonHTTP {
             return None;
         }
 
         Some(Cookie {
             cookie: cookie,
             host_only: host_only,
             persistent: persistent,
             creation_time: now(),
@@ -127,26 +127,21 @@ impl Cookie {
             // character of the request-path that is not included in the cookie-
             // path is a %x2F ("/") character.
             request_path[cookie_path.len()..].starts_with("/")
         ))
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.1.3
     pub fn domain_match(string: &str, domain_string: &str) -> bool {
-        if string == domain_string {
-            return true;
-        }
-        if 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() {
-            return true;
-        }
-        false
+        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())
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.4 step 1
     pub fn appropriate_for_url(&self, url: &ServoUrl, source: CookieSource) -> bool {
         let domain = url.host_str();
         if self.host_only {
             if self.cookie.domain.as_ref().map(String::as_str) != domain {
                 return false;
--- a/servo/components/net/cookie_storage.rs
+++ b/servo/components/net/cookie_storage.rs
@@ -28,46 +28,73 @@ impl CookieStorage {
         CookieStorage {
             version: 1,
             cookies_map: HashMap::new(),
             max_per_host: max_cookies,
         }
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.3
-    pub fn remove(&mut self, cookie: &Cookie, source: CookieSource) -> Result<Option<Cookie>, ()> {
+    pub fn remove(&mut self, cookie: &Cookie, url: &ServoUrl, source: CookieSource) -> Result<Option<Cookie>, ()> {
         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 && url.scheme() != "https" && url.scheme() != "wss" {
+            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(());
+            }
+        }
+
+        // Step 11.1
         let position = cookies.iter().position(|c| {
             c.cookie.domain == cookie.cookie.domain &&
             c.cookie.path == cookie.cookie.path &&
             c.cookie.name == cookie.cookie.name
         });
 
         if let Some(ind) = position {
+            // Step 11.4
             let c = cookies.remove(ind);
 
             // http://tools.ietf.org/html/rfc6265#section-5.3 step 11.2
-            if !c.cookie.httponly || source == CookieSource::HTTP {
-                Ok(Some(c))
-            } else {
+            if c.cookie.httponly && source == CookieSource::NonHTTP {
                 // Undo the removal.
                 cookies.push(c);
                 Err(())
+            } else {
+                Ok(Some(c))
             }
         } else {
             Ok(None)
         }
     }
 
     // http://tools.ietf.org/html/rfc6265#section-5.3
-    pub fn push(&mut self, mut cookie: Cookie, source: CookieSource) {
-        let old_cookie = self.remove(&cookie, source);
+    pub fn push(&mut self, mut cookie: Cookie, url: &ServoUrl, source: CookieSource) {
+        // https://www.ietf.org/id/draft-ietf-httpbis-cookie-alone-01.txt Step 1
+        if cookie.cookie.secure && url.scheme() != "https" && url.scheme() != "wss" {
+            return;
+        }
+
+        let old_cookie = self.remove(&cookie, url, source);
         if old_cookie.is_err() {
             // This new cookie is not allowed to overwrite an existing one.
             return;
         }
 
         // Step 11
         if let Some(old_cookie) = old_cookie.unwrap() {
             // Step 11.3
@@ -78,17 +105,17 @@ impl CookieStorage {
         let domain = reg_host(&cookie.cookie.domain.as_ref().unwrap_or(&"".to_string()));
         let mut cookies = self.cookies_map.entry(domain).or_insert(vec![]);
 
         if cookies.len() == self.max_per_host {
             let old_len = cookies.len();
             cookies.retain(|c| !is_cookie_expired(&c));
             let new_len = cookies.len();
 
-            // https://datatracker.ietf.org/doc/draft-ietf-httpbis-cookie-alone
+            // https://www.ietf.org/id/draft-ietf-httpbis-cookie-alone-01.txt
             if new_len == old_len && !evict_one_cookie(cookie.cookie.secure, cookies) {
                 return;
             }
         }
         cookies.push(cookie);
     }
 
     pub fn cookie_comparator(a: &Cookie, b: &Cookie) -> Ordering {
@@ -114,17 +141,16 @@ impl CookieStorage {
                   c.cookie.value,
                   c.cookie.domain,
                   c.cookie.path);
             info!(" === SENT COOKIE RESULT {}",
                   c.appropriate_for_url(url, source));
             // Step 1
             c.appropriate_for_url(url, source)
         };
-
         // Step 2
         let domain = reg_host(url.host_str().unwrap_or(""));
         let cookies = self.cookies_map.entry(domain).or_insert(vec![]);
 
         let mut url_cookies: Vec<&mut Cookie> = cookies.iter_mut().filter(filterer).collect();
         url_cookies.sort_by(|a, b| CookieStorage::cookie_comparator(*a, *b));
 
         let reducer = |acc: String, c: &mut &mut Cookie| -> String {
@@ -154,16 +180,17 @@ impl CookieStorage {
         let cookies = self.cookies_map.entry(domain).or_insert(vec![]);
 
         Box::new(cookies.iter_mut().filter(move |c| c.appropriate_for_url(url, source)).map(|c| {
             c.touch();
             c.cookie.clone()
         }))
     }
 }
+
 fn reg_host<'a>(url: &'a str) -> String {
     reg_suffix(url).to_string()
 }
 
 fn is_cookie_expired(cookie: &Cookie) -> bool {
     match cookie.expiry_time {
         Some(t) => t.to_timespec() <= time::get_time(),
         None => false,
--- a/servo/components/net/http_loader.rs
+++ b/servo/components/net/http_loader.rs
@@ -276,17 +276,17 @@ fn set_cookie_for_url(cookie_jar: &Arc<R
                       cookie_val: String) {
     let mut cookie_jar = cookie_jar.write().unwrap();
     let source = CookieSource::HTTP;
     let header = Header::parse_header(&[cookie_val.into_bytes()]);
 
     if let Ok(SetCookie(cookies)) = header {
         for bare_cookie in cookies {
             if let Some(cookie) = cookie::Cookie::new_wrapped(bare_cookie, request, source) {
-                cookie_jar.push(cookie, source);
+                cookie_jar.push(cookie, request, source);
             }
         }
     }
 }
 
 fn set_cookies_from_headers(url: &ServoUrl, headers: &Headers, cookie_jar: &Arc<RwLock<CookieStorage>>) {
     if let Some(cookies) = headers.get_raw("set-cookie") {
         for cookie in cookies.iter() {
--- a/servo/components/net/resource_thread.rs
+++ b/servo/components/net/resource_thread.rs
@@ -307,17 +307,17 @@ impl CoreResourceManager {
             cancel_load_map: HashMap::new(),
         }
     }
 
     fn set_cookie_for_url(&mut self, request: &ServoUrl, cookie: cookie_rs::Cookie, source: CookieSource,
                           resource_group: &ResourceGroup) {
         if let Some(cookie) = cookie::Cookie::new_wrapped(cookie, &request, source) {
             let mut cookie_jar = resource_group.cookie_jar.write().unwrap();
-            cookie_jar.push(cookie, source)
+            cookie_jar.push(cookie, request, source)
         }
     }
 
     fn fetch(&self,
              init: RequestInit,
              mut sender: IpcSender<FetchResponseMsg>,
              group: &ResourceGroup) {
         let http_state = HttpState {
--- a/servo/tests/unit/net/cookie.rs
+++ b/servo/tests/unit/net/cookie.rs
@@ -128,33 +128,146 @@ fn test_sort_order() {
     assert!(b.cookie.path.as_ref().unwrap().len() > a.cookie.path.as_ref().unwrap().len());
     assert!(CookieStorage::cookie_comparator(&a, &b) == Ordering::Greater);
     assert!(CookieStorage::cookie_comparator(&b, &a) == Ordering::Less);
     assert!(CookieStorage::cookie_comparator(&a, &a_prime) == Ordering::Less);
     assert!(CookieStorage::cookie_comparator(&a_prime, &a) == Ordering::Greater);
     assert!(CookieStorage::cookie_comparator(&a, &a) == Ordering::Equal);
 }
 
+fn add_cookie_to_storage(storage: &mut CookieStorage, url: &ServoUrl, cookie_str: &str)
+{
+    let source = CookieSource::HTTP;
+    let cookie = Cookie::new_wrapped(cookie_rs::Cookie::parse(cookie_str).unwrap(), url, source).unwrap();
+    storage.push(cookie, url, source);
+}
+
+#[test]
+fn test_insecure_cookies_cannot_evict_secure_cookie() {
+    let mut storage = CookieStorage::new(5);
+    let secure_url = ServoUrl::parse("https://home.example.org:8888/cookie-parser?0001").unwrap();
+    let source = CookieSource::HTTP;
+    let mut cookies = Vec::new();
+
+    cookies.push(cookie_rs::Cookie::parse("foo=bar; Secure; Domain=home.example.org").unwrap());
+    cookies.push(cookie_rs::Cookie::parse("foo2=bar; Secure; Domain=.example.org").unwrap());
+    cookies.push(cookie_rs::Cookie::parse("foo3=bar; Secure; Path=/foo").unwrap());
+    cookies.push(cookie_rs::Cookie::parse("foo4=bar; Secure; Path=/foo/bar").unwrap());
+
+    for bare_cookie in cookies {
+        let cookie = Cookie::new_wrapped(bare_cookie, &secure_url, source).unwrap();
+        storage.push(cookie, &secure_url, source);
+    }
+
+    let insecure_url = ServoUrl::parse("http://home.example.org:8888/cookie-parser?0001").unwrap();
+
+    add_cookie_to_storage(&mut storage, &insecure_url, "foo=value; Domain=home.example.org");
+    add_cookie_to_storage(&mut storage, &insecure_url, "foo2=value; Domain=.example.org");
+    add_cookie_to_storage(&mut storage, &insecure_url, "foo3=value; Path=/foo/bar");
+    add_cookie_to_storage(&mut storage, &insecure_url, "foo4=value; Path=/foo");
+
+    let source = CookieSource::HTTP;
+    assert_eq!(storage.cookies_for_url(&secure_url, source).unwrap(), "foo=bar; foo2=bar");
+
+    let url = ServoUrl::parse("https://home.example.org:8888/foo/cookie-parser-result?0001").unwrap();
+    let source = CookieSource::HTTP;
+    assert_eq!(storage.cookies_for_url(&url, source).unwrap(), "foo3=bar; foo4=value; foo=bar; foo2=bar");
+
+    let url = ServoUrl::parse("https://home.example.org:8888/foo/bar/cookie-parser-result?0001").unwrap();
+    let source = CookieSource::HTTP;
+    assert_eq!(storage.cookies_for_url(&url, source).unwrap(), "foo4=bar; foo3=bar; foo4=value; foo=bar; foo2=bar");
+}
+
+#[test]
+fn test_secure_cookies_eviction() {
+    let mut storage = CookieStorage::new(5);
+    let url = ServoUrl::parse("https://home.example.org:8888/cookie-parser?0001").unwrap();
+    let source = CookieSource::HTTP;
+    let mut cookies = Vec::new();
+
+    cookies.push(cookie_rs::Cookie::parse("foo=bar; Secure; Domain=home.example.org").unwrap());
+    cookies.push(cookie_rs::Cookie::parse("foo2=bar; Secure; Domain=.example.org").unwrap());
+    cookies.push(cookie_rs::Cookie::parse("foo3=bar; Secure; Path=/foo").unwrap());
+    cookies.push(cookie_rs::Cookie::parse("foo4=bar; Secure; Path=/foo/bar").unwrap());
+
+    for bare_cookie in cookies {
+        let cookie = Cookie::new_wrapped(bare_cookie, &url, source).unwrap();
+        storage.push(cookie, &url, source);
+    }
+
+    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");
+
+    let source = CookieSource::HTTP;
+    assert_eq!(storage.cookies_for_url(&url, source).unwrap(), "foo2=value");
+
+    let url = ServoUrl::parse("https://home.example.org:8888/foo/cookie-parser-result?0001").unwrap();
+    let source = CookieSource::HTTP;
+    assert_eq!(storage.cookies_for_url(&url, source).unwrap(), "foo3=bar; foo4=value; foo2=value");
+
+    let url = ServoUrl::parse("https://home.example.org:8888/foo/bar/cookie-parser-result?0001").unwrap();
+    let source = CookieSource::HTTP;
+    assert_eq!(storage.cookies_for_url(&url, source).unwrap(),
+               "foo4=bar; foo3=value; foo3=bar; foo4=value; foo2=value");
+}
+
+#[test]
+fn test_secure_cookies_eviction_non_http_source() {
+    let mut storage = CookieStorage::new(5);
+    let url = ServoUrl::parse("https://home.example.org:8888/cookie-parser?0001").unwrap();
+    let source = CookieSource::NonHTTP;
+    let mut cookies = Vec::new();
+
+    cookies.push(cookie_rs::Cookie::parse("foo=bar; Secure; Domain=home.example.org").unwrap());
+    cookies.push(cookie_rs::Cookie::parse("foo2=bar; Secure; Domain=.example.org").unwrap());
+    cookies.push(cookie_rs::Cookie::parse("foo3=bar; Secure; Path=/foo").unwrap());
+    cookies.push(cookie_rs::Cookie::parse("foo4=bar; Secure; Path=/foo/bar").unwrap());
+
+    for bare_cookie in cookies {
+        let cookie = Cookie::new_wrapped(bare_cookie, &url, source).unwrap();
+        storage.push(cookie, &url, source);
+    }
+
+    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");
+
+    let source = CookieSource::HTTP;
+    assert_eq!(storage.cookies_for_url(&url, source).unwrap(), "foo2=value");
+
+    let url = ServoUrl::parse("https://home.example.org:8888/foo/cookie-parser-result?0001").unwrap();
+    let source = CookieSource::HTTP;
+    assert_eq!(storage.cookies_for_url(&url, source).unwrap(), "foo3=bar; foo4=value; foo2=value");
+
+    let url = ServoUrl::parse("https://home.example.org:8888/foo/bar/cookie-parser-result?0001").unwrap();
+    let source = CookieSource::HTTP;
+    assert_eq!(storage.cookies_for_url(&url, source).unwrap(),
+               "foo4=bar; foo3=value; foo3=bar; foo4=value; foo2=value");
+}
+
 
 fn add_retrieve_cookies(set_location: &str,
                         set_cookies: &[String],
                         final_location: &str)
                         -> String {
     let mut storage = CookieStorage::new(5);
     let url = ServoUrl::parse(set_location).unwrap();
     let source = CookieSource::HTTP;
 
     // Add all cookies to the store
     for str_cookie in set_cookies {
         let bytes = str_cookie.to_string().into_bytes();
         let header = Header::parse_header(&[bytes]).unwrap();
         let SetCookie(cookies) = header;
         for bare_cookie in cookies {
             let cookie = Cookie::new_wrapped(bare_cookie, &url, source).unwrap();
-            storage.push(cookie, source);
+            storage.push(cookie, &url, source);
         }
     }
 
     // Get cookies for the test location
     let url = ServoUrl::parse(final_location).unwrap();
     storage.cookies_for_url(&url, source).unwrap_or("".to_string())
 }
 
--- a/servo/tests/unit/net/cookie_http_state.rs
+++ b/servo/tests/unit/net/cookie_http_state.rs
@@ -16,17 +16,17 @@ fn run(set_location: &str, set_cookies: 
 
     // Add all cookies to the store
     for str_cookie in set_cookies {
         let bytes = str_cookie.to_string().into_bytes();
         let header = Header::parse_header(&[bytes]);
         if let Ok(SetCookie(cookies)) = header {
             for bare_cookie in cookies {
                 if let Some(cookie) = Cookie::new_wrapped(bare_cookie, &url, source) {
-                    storage.push(cookie, source);
+                    storage.push(cookie, &url, source);
                 }
             }
         }
     }
 
     // Get cookies for the test location
     let url = ServoUrl::parse(final_location).unwrap();
     storage.cookies_for_url(&url, source).unwrap_or("".to_string())
--- a/servo/tests/unit/net/http_loader.rs
+++ b/servo/tests/unit/net/http_loader.rs
@@ -547,17 +547,17 @@ fn test_load_sets_requests_cookies_heade
 
     {
         let mut cookie_jar = context.state.cookie_jar.write().unwrap();
         let cookie = Cookie::new_wrapped(
             CookiePair::new("mozillaIs".to_owned(), "theBest".to_owned()),
             &url,
             CookieSource::HTTP
         ).unwrap();
-        cookie_jar.push(cookie, CookieSource::HTTP);
+        cookie_jar.push(cookie, &url, CookieSource::HTTP);
     }
 
     let request = Request::from_init(RequestInit {
         url: url.clone(),
         method: Method::Get,
         body: None,
         destination: Destination::Document,
         origin: url.clone(),
@@ -585,17 +585,17 @@ fn test_load_sends_cookie_if_nonhttp() {
 
     {
         let mut cookie_jar = context.state.cookie_jar.write().unwrap();
         let cookie = Cookie::new_wrapped(
             CookiePair::new("mozillaIs".to_owned(), "theBest".to_owned()),
             &url,
             CookieSource::NonHTTP
         ).unwrap();
-        cookie_jar.push(cookie, CookieSource::HTTP);
+        cookie_jar.push(cookie, &url, CookieSource::HTTP);
     }
 
     let request = Request::from_init(RequestInit {
         url: url.clone(),
         method: Method::Get,
         body: None,
         destination: Destination::Document,
         origin: url.clone(),
@@ -977,24 +977,24 @@ fn  test_redirect_from_x_to_y_provides_y
     {
         let mut cookie_jar = context.state.cookie_jar.write().unwrap();
         let cookie_x = Cookie::new_wrapped(
             CookiePair::new("mozillaIsNot".to_owned(), "dotOrg".to_owned()),
             &url_x,
             CookieSource::HTTP
         ).unwrap();
 
-        cookie_jar.push(cookie_x, CookieSource::HTTP);
+        cookie_jar.push(cookie_x, &url_x, CookieSource::HTTP);
 
         let cookie_y = Cookie::new_wrapped(
             CookiePair::new("mozillaIs".to_owned(), "theBest".to_owned()),
             &url_y,
             CookieSource::HTTP
         ).unwrap();
-        cookie_jar.push(cookie_y, CookieSource::HTTP);
+        cookie_jar.push(cookie_y, &url_y, CookieSource::HTTP);
     }
 
     let request = Request::from_init(RequestInit {
         url: url_x.clone(),
         method: Method::Get,
         destination: Destination::Document,
         origin: url_x.clone(),
         pipeline_id: Some(TEST_PIPELINE_ID),
@@ -1170,17 +1170,17 @@ fn test_cookies_blocked() {
     context.state.blocked_content = Arc::new(Some(parse_list(&blocked_content_list).unwrap()));
     {
         let mut cookie_jar = context.state.cookie_jar.write().unwrap();
         let cookie = Cookie::new_wrapped(
             CookiePair::new("mozillaIs".to_owned(), "theBest".to_owned()),
             &url,
             CookieSource::HTTP
         ).unwrap();
-        cookie_jar.push(cookie, CookieSource::HTTP);
+        cookie_jar.push(cookie, &url, CookieSource::HTTP);
     }
 
     let request = Request::from_init(RequestInit {
         url: url.clone(),
         method: Method::Get,
         body: None,
         destination: Destination::Document,
         origin: url.clone(),