Remove blocking HTTP header listener

semenko · semenko · commit 2bf1c2930c48 · 2014-02-22T14:29:16.000-06:00
After thinking about this more, this function seems insane. I think we should
remove it for a lot of reasons. Here are the big ones:

1. Performance

We want wider HTTPS Everywhere adoption, and performance is the big limiting
factor here. The biggest complaints are still "it's slow," and blocking all
HTTP request headers to loop over &amp; reconstruct cookies is nuts.

Testing on a CR-48 &amp; other CPU-limited machines, HTTPS Everywhere still causes
a significant performance/battery hit (*especially* this single function).

2. It doesn't really work right

If the comments are any indication ("XXX I have no idea..."):

The if(ruleset) code doesn't actually secure any cookies -- it just adds the
ruleset to the tab, but doesn't append a new secure cookie to the newCookies
array.

3. The race condition this "prevents" seems just silly

The old justification for this function is it prevents a race-condition, but
wow, that's insanely hard to replicate. After running with some alert()s for
some time, I can't even get the warning in question to trigger.

To really steal a cookie here, you'd need:
- A site that supports HTTPS
- and is in our rulesets
- and has appropriately scoped, secureable cookies
- and sets them without the HTTPS Only flag
- and they're interesting enough to steal

AND

- An active, MITM attacker that controls a subdomain of interest
- with the ability force the user to visit an HTTP page
- with millisecond-level timing precision

This just seems silly to me. If you're controlling HTTP subdomains &amp; any
sites the end-user is visiting, this seems like it's simultaneously the most
complex and also most meaningless attack you could mount.

Signed-off-by: Nick Semenkovich &lt;semenko@alum.mit.edu&gt;
diff --git a/chromium/background.js b/chromium/background.js
@@ -342,47 +342,6 @@ function onCookieChanged(changeInfo) {
   }
 }
 
-// This event is needed due to the potential race between cookie permissions
-// update and cookie transmission (because the cookie API is non-blocking).
-// Without this function, an aggressive attacker could race to steal a not-yet-secured
-// cookie if they controlled & could redirect the user to a non-SSL subdomain.
-// WARNING: This is a very hot function.
-function onBeforeSendHeaders(details) {
-  // TODO: Verify this with wireshark
-  for (var h in details.requestHeaders) {
-    if (details.requestHeaders[h].name == "Cookie") {
-      // Per RFC 6265, Chrome sends only ONE cookie header, period.
-      var uri = new URI(details.url);
-      var host = uri.hostname();
-
-      var newCookies = [];
-      var cookies = details.requestHeaders[h].value.split(";");
-
-      for (var c in cookies) {
-        // Create a fake "nsICookie2"-ish object to pass in to our rule API:
-        var fake = {domain:host, name:cookies[c].split("=")[0]};
-        // XXX I have no idea whether the knownHttp parameter should be true
-        // or false here.  We're supposedly inside a race condition or
-        // something, right?
-        var ruleset = all_rules.shouldSecureCookie(fake, false);
-        if (ruleset) {
-          activeRulesets.addRulesetToTab(details.tabId, ruleset);
-          log(INFO, "Woah, we lost the race on updating a cookie: "+details.requestHeaders[h].value);
-        } else {
-          newCookies.push(cookies[c]);
-        }
-      }
-      details.requestHeaders[h].value = newCookies.join(";");
-      log(DBUG, "Got new cookie header: "+details.requestHeaders[h].value);
-
-      // We've seen the one cookie header, so let's get out of here!
-      break;
-    }
-  }
-
-  return {requestHeaders:details.requestHeaders};
-}
-
 function onResponseStarted(details) {
 
   // redirect counter workaround
@@ -394,11 +353,6 @@ function onResponseStarted(details) {
 
 wr.onBeforeRequest.addListener(onBeforeRequest, {urls: ["https://*/*", "http://*/*"]}, ["blocking"]);
 
-// This watches cookies sent via HTTP.
-// We do *not* watch HTTPS cookies -- they're already being sent over HTTPS -- yay!
-wr.onBeforeSendHeaders.addListener(onBeforeSendHeaders, {urls: ["http://*/*"]},
-                                   ["requestHeaders", "blocking"]);
-
 wr.onResponseStarted.addListener(onResponseStarted,
                                  {urls: ["https://*/*", "http://*/*"]});
 
@@ -416,8 +370,7 @@ chrome.tabs.onReplaced.addListener(function(addedTabId, removedTabId) {
     displayPageAction(addedTabId);
 });
 
-// Listen for cookies set/updated and secure them if applicable. This function is async/nonblocking,
-// so we also use onBeforeSendHeaders to prevent a small window where cookies could be stolen.
+// Listen for cookies set/updated and secure them if applicable. This function is async/nonblocking.
 chrome.cookies.onChanged.addListener(onCookieChanged);
 
 function disableSwitchPlannerFor(tabId) {
