Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add domain and path checks for secure cookies eviction #14491

Merged
merged 3 commits into from Dec 24, 2016
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Add domain and path checks for secure cookies eviction

  • Loading branch information
KiChjang committed Dec 21, 2016
commit 63a7e8efdf19d607d7a48ebba771ae20cde8a252
@@ -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::<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'.' &&

This comment has been minimized.

@frewsxcv

frewsxcv Dec 9, 2016

Member

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

This comment has been minimized.

@jdm

jdm Dec 9, 2016

Member

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

string.parse::<Ipv4Addr>().is_err() &&
string.parse::<Ipv6Addr>().is_err())
}

// http://tools.ietf.org/html/rfc6265#section-5.4 step 1
@@ -33,10 +33,31 @@ impl CookieStorage {
}

// 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![]);

// 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 &&
@@ -62,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;
@@ -120,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![]);
@@ -281,7 +281,7 @@ fn set_cookie_for_url(cookie_jar: &Arc<RwLock<CookieStorage>>,
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);
}
}
}
@@ -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)
}
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.