From 046ac8973062f36c090a6637fbbb11a4ec1cb30b Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 23 Dec 2016 15:26:46 -0800 Subject: [PATCH] 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 | 17 ++-- servo/components/net/cookie_storage.rs | 45 +++++++-- servo/components/net/http_loader.rs | 2 +- servo/components/net/resource_thread.rs | 2 +- servo/tests/unit/net/cookie.rs | 115 +++++++++++++++++++++- servo/tests/unit/net/cookie_http_state.rs | 2 +- servo/tests/unit/net/http_loader.rs | 10 +- 7 files changed, 164 insertions(+), 29 deletions(-) diff --git a/servo/components/net/cookie.rs b/servo/components/net/cookie.rs index 768017ec6f64..b47246440caa 100644 --- a/servo/components/net/cookie.rs +++ b/servo/components/net/cookie.rs @@ -75,7 +75,7 @@ impl Cookie { // Step 10 - if cookie.httponly && source != CookieSource::HTTP { + if cookie.httponly && source == CookieSource::NonHTTP { return None; } @@ -132,16 +132,11 @@ impl Cookie { // 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::().is_err() && - string.parse::().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::().is_err() && + string.parse::().is_err()) } // http://tools.ietf.org/html/rfc6265#section-5.4 step 1 diff --git a/servo/components/net/cookie_storage.rs b/servo/components/net/cookie_storage.rs index 21c47e1f17b4..cf2779daa04e 100644 --- a/servo/components/net/cookie_storage.rs +++ b/servo/components/net/cookie_storage.rs @@ -33,11 +33,32 @@ impl CookieStorage { } // http://tools.ietf.org/html/rfc6265#section-5.3 - pub fn remove(&mut self, cookie: &Cookie, source: CookieSource) -> Result, ()> { + pub fn remove(&mut self, cookie: &Cookie, url: &ServoUrl, source: CookieSource) -> Result, ()> { 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 && @@ -45,15 +66,16 @@ impl CookieStorage { }); 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) @@ -61,8 +83,13 @@ impl CookieStorage { } // 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; @@ -83,7 +110,7 @@ impl CookieStorage { 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; } @@ -119,7 +146,6 @@ impl CookieStorage { // 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![]); @@ -159,6 +185,7 @@ impl CookieStorage { })) } } + fn reg_host<'a>(url: &'a str) -> String { reg_suffix(url).to_string() } diff --git a/servo/components/net/http_loader.rs b/servo/components/net/http_loader.rs index 7904e9f995f9..4091486597b5 100644 --- a/servo/components/net/http_loader.rs +++ b/servo/components/net/http_loader.rs @@ -281,7 +281,7 @@ fn set_cookie_for_url(cookie_jar: &Arc>, 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); } } } diff --git a/servo/components/net/resource_thread.rs b/servo/components/net/resource_thread.rs index a1b0e72905b2..108bc01d8cf4 100644 --- a/servo/components/net/resource_thread.rs +++ b/servo/components/net/resource_thread.rs @@ -312,7 +312,7 @@ impl CoreResourceManager { 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) } } diff --git a/servo/tests/unit/net/cookie.rs b/servo/tests/unit/net/cookie.rs index aa6519571b87..fbeebab2c5f1 100644 --- a/servo/tests/unit/net/cookie.rs +++ b/servo/tests/unit/net/cookie.rs @@ -133,6 +133,119 @@ fn test_sort_order() { 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], @@ -149,7 +262,7 @@ fn add_retrieve_cookies(set_location: &str, 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); } } diff --git a/servo/tests/unit/net/cookie_http_state.rs b/servo/tests/unit/net/cookie_http_state.rs index 819aaabb5f27..7d5df8b658ca 100644 --- a/servo/tests/unit/net/cookie_http_state.rs +++ b/servo/tests/unit/net/cookie_http_state.rs @@ -21,7 +21,7 @@ fn run(set_location: &str, set_cookies: &[&str], final_location: &str) -> String 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); } } } diff --git a/servo/tests/unit/net/http_loader.rs b/servo/tests/unit/net/http_loader.rs index c2b720517f75..f31098e793a6 100644 --- a/servo/tests/unit/net/http_loader.rs +++ b/servo/tests/unit/net/http_loader.rs @@ -552,7 +552,7 @@ fn test_load_sets_requests_cookies_header_for_url_by_getting_cookies_from_the_re &url, CookieSource::HTTP ).unwrap(); - cookie_jar.push(cookie, CookieSource::HTTP); + cookie_jar.push(cookie, &url, CookieSource::HTTP); } let request = Request::from_init(RequestInit { @@ -590,7 +590,7 @@ fn test_load_sends_cookie_if_nonhttp() { &url, CookieSource::NonHTTP ).unwrap(); - cookie_jar.push(cookie, CookieSource::HTTP); + cookie_jar.push(cookie, &url, CookieSource::HTTP); } let request = Request::from_init(RequestInit { @@ -982,14 +982,14 @@ fn test_redirect_from_x_to_y_provides_y_cookies_from_y() { 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 { @@ -1175,7 +1175,7 @@ fn test_cookies_blocked() { &url, CookieSource::HTTP ).unwrap(); - cookie_jar.push(cookie, CookieSource::HTTP); + cookie_jar.push(cookie, &url, CookieSource::HTTP); } let request = Request::from_init(RequestInit {