Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#7491 closed defect (fixed)

[FIREFOX] We sometimes flag cookies as "secure" even though they are from HTTP origins

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

Description

While investigating this bug report I realised that HTTPS Everywhere will actually flag cookies as secure from within HTTP-only pages/origins. Needless to say, this can interact very badly with *.example.com target host rules!

This is something we explicitly avoid in the handleSecureCookies() path that deals with cookies set in HTTP headers.

But we have another path which I think was added to fix #3766 which does not perform the same check.

Child Tickets

Change History (9)

comment:1 Changed 7 years ago by pde

Component: - Select a componentEFF-HTTPS Everywhere

comment:2 Changed 7 years ago by pde

It looks as though we may not be able to get a reference to the document or window from within the cookie-changed observer. In which case perhaps the best we can do is before securing a cookie for foo.example.com, ask whether we would have rewritten http://foo.example.com to https (and also that it isn't in our redirects-back-to-HTTP blacklist).

comment:3 Changed 7 years ago by pde

Summary: We sometimes flag cookies as "secure" even though they are from HTTP origins[FIREFOX] We sometimes flag cookies as "secure" even though they are from HTTP origins

#7492 is the same bug, but for Chrome.

comment:4 Changed 7 years ago by pde

Status: newneeds_review

There's an attempt at a fix in the fixing cookies branch of my remote. Mike, it would be awesome if you could review this.

I'm wondering whether a more satisfactory workaround to the limitations of the cookie-changed observer might be writing something into the DOM of HTTPS pages only, in order to police JS cookie events.

comment:5 Changed 7 years ago by pde

Cc: schoen added

This is merged into master (but not 3.0, yet) and will probably ship in 4.0development.3 today for field testing.

comment:6 Changed 7 years ago by pde

We have a minimal amount of field testing in 4.0dev3, so this is going into 3.1 stable now.

comment:7 Changed 7 years ago by pde

Resolution: fixed
Status: needs_reviewclosed

comment:8 Changed 7 years ago by pde

Turns out this "fix" was buggy, and probably caused a regression of #3766, as well as the new #7855. Probably still less bad than this bug, though.

I've committed some repairs here, but this is actually hard to test correctly for all the cases I can think of.

It's time for us to get a Marionette test suite.

comment:9 Changed 7 years ago by pde

Well, actually, maybe an xpcshell test suite too.

Note: See TracTickets for help on using tickets.