Opened 8 years ago

Closed 8 years ago

#3766 closed defect (fixed)

securecookie has no effect for cookies set by JavaScript

Reported by: inkerman Owned by: pde
Priority: High Milestone:
Component: HTTPS Everywhere/EFF-HTTPS Everywhere Version:
Severity: Keywords:
Cc: mikeperry, pde Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Securecookie works correctly for cookies set by the HTTP header, but has no effect for cookies set by JavaScript.

Child Tickets

Attachments (1)

https-everywhere-fixed-bug-3766.patch (2.5 KB) - added by haviah 8 years ago.
Patch for bug #3766

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by pde

Status: newaccepted

comment:2 Changed 8 years ago by mikeperry

Can someone post an example site that is failing to get certain cookies secured due to this bug? Would save me some trouble in terms of making one + creating a rule to test a fix.

comment:3 Changed 8 years ago by mikeperry

Ok, an untested fix for this is in mikeperry/javascript-cookies.

comment:4 Changed 8 years ago by haviah

I've tested said commit from https://gitweb.torproject.org/mikeperry/https-everywhere.git/commit/42995e67ac1d9cedc1af1cace7a4b8b821119a6f on FF 7.0.1/Linux/i686. Does not work for cookies set by javascript. Here's the fix. I'll post patch shortly.

  1. Starting from line "subject.QueryInterface(Ci.nsIArray)", there are few typos in https-everywhere.js, the "Ci" in "Ci.nsIArray", "Ci.nsICokie2", etc. should be capitalized to "CI.nsIArray".
  2. in HTTPS.js, handleInsecureCookieEvent should be called handleInsecureCookie (the callers call it by this name)
  3. Some braindead cookies have expiry after end of universe and everything
  4. in HTTPS.js, handleInsecureCookie should use "nsICookieManager2" instead of:
var cookieManager = Components.classes["@mozilla.org/cookiemanager;1"]
                             .getService(Components.interfaces.nsICookieManager);

Use following rule as testcase:

<ruleset name="Reddit.com (custom)">
  <target host="reddit.com" />
  <target host="www.reddit.com" />
  <target host=".reddit.com" />
  <target host="thumbs.reddit.com" />
  <target host="pixel.reddit.com" />
  <target host="static.reddit.com" />

  <securecookie host="^(.*\.)?reddit\.com$" name=".*" />

  <rule from="^http://(www\.)?reddit\.com/" to="https://www.reddit.com/"/>
  <rule from="^http://thumbs\.reddit\.com/" to="https://thumbs.reddit.com/"/>
  <rule from="^http://pixel\.reddit\.com/" to="https://pixel.reddit.com/"/>
  <rule from="^http://static\.reddit\.com/" to="https://static.reddit.com/"/>
</ruleset>

Go to reddit.com (note that it has bad CNs in many of the certs, just add temporary exception for testing sake). Originally only "reddit_session" and "reddit_first" cookies were turned to secure by the HTTP headers rewriting.

Important note: many sites use ".host.net" for domain name of cookies. That breaks rules which have targets like "www.something.com", "blabla.something.com", but not "*.something.com" in the target XML element (notice the line saying <target host=".reddit.com">). That's what brough me to this bug. (FF API javascript simply handles the ".something.com" as cookie domain). I haven't seen notice about such quirk, maybe it should be added to rule creation documentation.

Yay, my first non-trivial FF extension bug fixed! (Rant: I really can't shake the idea that some FF API developers must really hate FF extension developers. At least Venkman helps if one swears a lot at it while enumerating possible ways to get it actually break at the right place and do a proper step-over. So kudos for the effort put into HTTPS Everywhere).

Changed 8 years ago by haviah

Patch for bug #3766

comment:5 Changed 8 years ago by haviah

Resolution: fixed
Status: acceptedclosed

Patch submitted. Note that the patch is for mikeperry's javascript-cookies branch.

I'm marking as fixed (I'm not sure about the bug flow here, so reopen if necessary).

comment:6 Changed 8 years ago by arma

Resolution: fixed
Status: closedreopened

If you close the ticket nobody will ever look at it again.

So if you want anybody to evaluate your patch or apply it, the ticket should remain open.

comment:7 Changed 8 years ago by mikeperry

Cc: mikeperry pde added
Status: reopenedneeds_review

Thanks for cleaning this up, testing it, and providing a patch + test cases everyone. Appreciate the help.

comment:8 Changed 8 years ago by mikeperry

Status: needs_reviewassigned

I have looked this over and merged it into my branch. git-am did not like the attached file, so I had to use patch directly.

I pushed the commit to mikeperry/javascript-cookies. I did not merge because I wasn't sure if we should backport this to 1.0.3 right away, or let it bake for a release in the 2.0 dev series first. I figure pde can make that call.

comment:9 Changed 8 years ago by pde

I'm going to pull it into master, where we can try it in a 2.0 dev release. Then if it seems ok it can be in 1.0.4 or 1.0.5 (or 2.0 stable, if that happens first).

comment:10 Changed 8 years ago by pde

Resolution: fixed
Status: assignedclosed

This patch shipped in 2.0development.3.

Note: See TracTickets for help on using tickets.