Https-everywhere: Discussion: securecookie tags

[jeremyn]

Type: other

I want to revisit how we use securecookie tags. I generally dislike these tags. They are hard to reason about, and the most common use with a general .+/.+ can break website functionality. See my comments at https://github.com/EFForg/https-everywhere/pull/7527#issuecomment-262405979 and https://github.com/EFForg/https-everywhere/pull/11475#issuecomment-340901080.

I think securecookies should rarely be used. From my comment at https://github.com/EFForg/https-everywhere/issues/2907#issuecomment-245113096:

1. securecookie tags should only be used in targeted situations where we are trying to fix a identified vulnerability, instead of used as a default catch-all, but
2. we don't need to make special efforts to remove existing securecookies, and
3. casual contributors should not be encouraged to create them.

The underlying theme here is that I prefer to be cautious in rulesets, because it is generally difficult for a regular user to effectively report problems and get them fixed.

However, I've noticed an increase in the number of PRs that include securecookies. I sometimes avoid these PRs because I don't want to have the securecookie argument again. However if the community generally feels that we should be more free with securecookies then I will stop worrying about it as much.

Ping @brainwane @gloomy-ghost @Hainish @J0WI @wonderchook .

[bardiharborow]

I agree with your interpretation in https://github.com/EFForg/https-everywhere/pull/7527#issuecomment-262405979, but per https://github.com/EFForg/https-everywhere/pull/13177#discussion_r144802578 I'm still very unclear how ^\w rules are treated. If you are able to clarify this, it would help inform my ruleset work. I generally only create securecookie rules if all the subdomains seem to support HTTPS but maybe I need to be even more cautious.

[Bisaloo]

However, I've noticed an increase in the number of PRs that include securecookies.

My guess is that this is due to the arrival of new contributors. When I first started out, I read that most cookies were missing the secure flag and that we should add it whenever possible.

I don't recall any warning about the issues associated with it. It's only later, when I saw bug reports about it, that I understood it was a dangerous tool.

My take on this is that we should add a warning in CONTRIBUTING.md using your comments.

[ghost]

One question: With HSTS preloaded domains, do cookies get only accepted from secure domains? If that's the case then emulating their behavior shouldn't be problematic it appears.

[bardiharborow]

With HSTS preloaded domains, do cookies get only accepted from secure domains?

@ihave100openPRs A HSTS domain with the includeSubDomains flag will never be accessed over an insecure connection under any circumstances, so any cookies set for that domain will always be secure. Non-includeSubDomains domains may leak cookies to their unsecured subdomains unless those cookies are marked as Secure.

emulating their behavior shouldn't be problematic it appears

HTTP-Everywhere rulesets with specified subdomains (as opposed to wildcard entries) operate like non-includeSubDomains domains by default, leaking cookies to their unsecured subdomains. securecookie rules plug this hole. The issue discussed by this ticket, is that in many cases this leakage is actually required for the website to function correctly, and in many cases we are too haphazard in applying securecookie rules such that we break websites.

[jeremyn]

@bardiharborow I don't know what's going on in the code, but my instinct for ^\w would be to replace it with .+ as a matter of style. If I saw it in a new ruleset, I would probably ask the contributor to replace it with .+ without thinking about it too much. If there is some significant behavior difference between ^\w and .+, that should be clarified, and we shouldn't be relying on it.

Also, even if all subdomains for some domain currently support HTTPS, new ones can be added later that don't. There are also sometimes many domains that aren't publicly visible at for example Google, so unless you are exhaustively searching with something like Sublist3r, you might be missing some. (I've started to prefer Google to Sublist3r myself, since I find Sublist3r often presents too many domains that are not worth covering.)

@ihave100openPRs A problem with relying on HSTS preloading and things like that is that it varies by domain and requires careful thought each time. Look at the discussion that @J0WI and I have in PR https://github.com/EFForg/https-everywhere/pull/11475. I don't want to have that sort of discussion for every PR that has a general securecookie and some domains that don't support HTTPS.

---

By the way, just to argue against my own position, in practice it is very rare to see a problem caused by a securecookie. It may be that the increased security of blanket securecookies outweighs these rare usability problems. In that case though, maybe we should make the code "autoinclude" a .+/.+ securecookie, and instead rely on contributors to exclude or override this if there is a problem. Or, we could autoinclude a securecookie that is strictly limited to the given targets.

[Bisaloo]

By the way, just to argue against my own position, in practice it is very rare to see a problem caused by a securecookie.

The flickr neverending login issue was enough for me: https://github.com/EFForg/https-everywhere/issues/239, https://github.com/EFForg/https-everywhere/issues/728, https://github.com/EFForg/https-everywhere/issues/1592

[ghost]

I think the correct procedure would be to require the PR submitter to manually test whether the site's functionality isn't affected due to problems with cookies, and to remove that tag if he can't/doesn't want to test it (of course this shouldn't be required for static sites or ones where problems with securecookie won't arise), and not that securecookie itself should be discouraged as much as possible (and as noted instances of breakage due to securecookie are very rare).

[Bisaloo]

@ihave100openPRs, I agree with this. This is also the rule I follow in my PR.

I still think this should be documented in CONTRIBUTING.md.

[jeremyn]

It is almost impossible to test cookies thoroughly. Suppose you see a cookie set by a.example.com with name lasksjddflkasda and value 2093080948320483208403. What is it supposed to do on b.example.com? You probably don't know. As a maintainer who is supposed to at least theoretically be able to double-check you, I definitely don't know.

(Also, contributors should be manually testing their rulesets before submitting them anyway.)

[Hainish]

This problem is probably exacerbated by our example here:

https://github.com/EFForg/https-everywhere/blob/c7da7b7eb704a697cc3d44e406c1912e45b954a9/CONTRIBUTING.md#L266-L272

This is a clarification on regex styling, but we perhaps don't draw out some of the difficulties with ensuring that a cookie won't be used by an insecure subdomain.

I've looked to our git history in order to determine the original inclusion the securecookie tag, since looking for where it first solved a real problem should illuminate its ideal use case. It was introduced in 43b1536, where we add it to the Facebook.xml file:

diff --git a/src/chrome/content/rules/Facebook.xml b/src/chrome/content/rules/Facebook.xml
index eee23f1..3a9f053 100644
--- a/src/chrome/content/rules/Facebook.xml
+++ b/src/chrome/content/rules/Facebook.xml
@@ -1,10 +1,16 @@
 <ruleset name="Facebook">
-  <target host="www.facebook.com" />
   <target host="facebook.com" />
-  <target host="m.facebook.com" />
+  <target host="*.facebook.com" />
   <target host="www.v6.facebook.com" />

+  <securecookie host=".*\.facebook.com" name="c_user" />
+  <securecookie host=".*\.facebook.com" name="lu" />
+  <securecookie host=".*\.facebook.com" name="sct" />
+  <securecookie host=".*\.facebook.com" name="xs" />
+
   <rule from="^http://(www\.)?facebook\.com/" to="https://www.facebook.com/"/>
   <rule from="^http://m\.facebook\.com/" to="https://m.facebook.com/"/>
+  <rule from="^http://ssl\.facebook\.com/" to="https://ssl.facebook.com/"/>
+  <rule from="^http://login\.facebook\.com/" to="https://login.facebook.com/"/>
   <rule from="^http://www\.v6\.facebook\.com/" to="https://www.v6.facebook.com/"/>
 </ruleset>

In this context, it makes sense. We were preventing Firesheep-style attacks from leaking login sessions. See how we're using it here - we have specific name attributes enumerated, so I presume we were fairly certain how these cookies were being used.

Since then, we've liberalized how we use this tag. Effectively, using it with a wildcard (either ^\w or .+, effectively the same in this context) causes cookies set on the eTLD+1 to be siloed by protocol. At worst, this can cause problems where insecure cookies are presumed to be set in a secure environment, then used in other contexts, as @jeremyn has described previously in this thread.

---

Creating a hard-and-fast rule for the securecookies flag seems to draw a relatively arbitrary line in the sand. If the above Facebook.xml rule, instead of enumerating all cookies, simply set all cookies to be secure, would that pass for an acceptable rule? It would protect users, but may impede usability. This tradeoff between usability and security would be particular to each site. In an attack scenario, where Facebook login sessions were being stolen in real time, that tradeoff would, I argue, be worth it. But how are we to know if it's worth it for any given site?

In the creation of rulesets, in general, we (perhaps ironically) air on the side of usability rather than security. See our discussion about when to disable rulesets. Any new ruleset is checked for broken functionality before it is merged. This can be a pretty tricky process, but that's another issue. It seems to me that usability should be the guide here, too. We don't want HTTPS Everywhere to impede functionality of a site.

I think if a wildcard securecookie tag is included, we could require the submitter to justify why. Anyone submitting a rule should be able to justify why all cookies should be secure for this entire eTLD+1, such as asserting that there is sensitive data behind an HTTPS-only logged-in portal. This justification doesn't need to pass some high bar, but just having it referenceable (perhaps requiring a comment above the securecookie tag line) may be enough. Down the line, if the securecookie does start causing problems, at least then we can have a reasonable reference point to weigh whether it's worth removal.

[jeremyn]

@Hainish The Facebook example is consistent with my item (1) in the first comment: "securecookie tags should only be used in targeted situations where we are trying to fix a identified vulnerability".

Facebook is not a typical example because the site is super high visibility, and you can probably count on at least some of the maintainers having Facebook accounts. A more typical example is a top-100k site in a language I don't know, with some invalid domains listed, maybe with a forum or a shop or something else that you can expect is especially sensitive to cookies but that can't be completely tested without making an account or buying something.

An advantage of a hard-and-fast rule is that we don't have to argue about it after we make the rule. Requiring contributors to justify the tag each time is not going to work. People will say too much or not enough. It will be an argument. They will need repeated reminding. For security reasons we shouldn't trust things we can't test anyways.

Regarding usability vs. security, in a discussion last year about mixed content blocking (MCB) (issue https://github.com/EFForg/https-everywhere/issues/6629), we collectively agreed to be permissive with MCB, which supports security over usability. So I don't think we favor one or the other in all cases.

[Hainish]

@jeremyn

Regarding usability vs. security, in a discussion last year about mixed content blocking (MCB) (issue #6629), we collectively agreed to be permissive with MCB, which supports security over usability. So I don't think we favor one or the other in all cases.

We agreed to allow forcing users into mixed content blocking as long as it doesn't impede usability, as defined specifically under the phrase "feature breakage": https://github.com/EFForg/https-everywhere/issues/6629#issuecomment-243884432. I do view this as favoring usability over security. Ultimately, if a site is very secure but breaks features of the website, usability is the deciding factor.

---

An advantage of a hard-and-fast rule is that we don't have to argue about it after we make the rule. Requiring contributors to justify the tag each time is not going to work. People will say too much or not enough. It will be an argument. They will need repeated reminding. For security reasons we shouldn't trust things we can't test anyways.

Requiring a justification is a hard-and-fast rule, in my opinion. Within that context, we can instruct contributors that we favor excluding the securecookie tag. Unless a user feels particularly compelled to include it, they'll probably be willing to work with you and simply remove it. But if inclusion of a wildcard securecookie is important, we should give them a mechanism to include it.

[Hainish]

On my point:

Requiring a justification is a hard-and-fast rule, in my opinion. Within that context, we can instruct contributors that we favor excluding the securecookie tag. Unless a user feels particularly compelled to include it, they'll probably be willing to work with you and simply remove it. But if inclusion of a wildcard securecookie is important, we should give them a mechanism to include it.

If this is a policy we decide on, we should definitely include it in CONTRIBUTING.md.

[jeremyn]

Requiring a justification may be a hard-and-fast rule to you. As a maintainer it means hundreds of individual conversations about why

    <!-- insecure cookies present -->
    <securecookie host=".+" name=".+" />

on the one hand or

    <!-- [insert several paragraphs here] -->
    <securecookie host=".+" name=".+" />

on the other is or is not appropriate. A hard-and-fast rule to me means a red X or a green check in Travis.

[J0WI]

I feel like we already discussed this in #2907. Maybe GitHub issues are the wrong place for such decisions, because it's quite hard to find them once they have been closed/resolved.

1584 and #6964 may also be worth to look at.

[cschanaj]

Personally, I would like to avoid adding securecookie when creating new rulesets since

• it is always difficult to check if there any undesired breakage of the websites/ no way to perform automated testing, e.g. login issues

• once the cookies are marked secure, disabling the extension might not fix broken site;

https://github.com/EFForg/https-everywhere/blob/71452b9c077202b8f0026179d300e9a6686c1ed0/chromium/rules.js#L635-L637

• the code/ extension behavior seem very unclear to me, e.g. why are we excluding some cookie when a random URL is excluded??? How often does this really work as expected? Besides, how are we going to handle host=".example.com" with leading dots?

https://github.com/EFForg/https-everywhere/blob/71452b9c077202b8f0026179d300e9a6686c1ed0/chromium/rules.js#L650-L654

IMHO, it is much better if we only secure cookies in the "Block all unencrypted requests" mode AND secure all cookies by default (possibly with a handful number of exceptions, e.g. legacy/ popular rulesets). This helps to reduce the maintenance overhead and fix the usability issue in the default mode.

[jeremyn]

@cschanaj If someone turns on "Block all unencrypted requests", visits a site where the extension secures a cookie, and then turns off "Block all unencrypted requests", the cookie will still be secure and possibly breaking things. This is like what you said with "once the cookies are marked secure, disabling the extension might not fix broken site;".

[cschanaj]

@jeremyn it was a quick thought, thanks for correcting me. It sound like that securecookie are bad anyway ... Maybe we should avoid creating new PRs with securecookies tag being used unless it is strictly necessary.

A hard-and-fast rule to me means a red X or a green check in Travis.

We shall extend ruleset-whitelist.csv to require whitelisting for securecookie if we want to discourage casual contributors creating them. In this way, we don't need additional effort to remove the existing securecookie too.

[Hainish]

@cschanaj

We shall extend ruleset-whitelist.csv to require whitelisting for securecookie if we want to discourage casual contributors creating them. In this way, we don't need additional effort to remove the existing securecookie too.

The current ruleset-whitelist.csv has 3878 rulesets, rather small in comparison to the 23,226 total number of rulesets. Rulesets which include securecookie number 13,255, which is a lot more. This will make scripts like utils/duplicate-whitelist-cleanup.sh (which is run at release-time) much slower, which I'd like to avoid.

I still think securecookie can be valuable in certain circumstances, and would like to give ruleset contributors the opportunity to justify its usage. In addition to the possibility of leaking tracking cookies as pointed out in https://github.com/EFForg/https-everywhere/issues/2907#issuecomment-245365536, a network attacker can force login sessions to be divulged if those cookies are not properly secured by redirecting a client to http://nonexistant.example.com/. I could easily see this happening in the context of an IOT device that is controlled from a publicly accessible site, which a vendor has no interest in upgrading.

[jeremyn]

@Hainish So to answer you, and also @J0WI 's totally legitimate observation in https://github.com/EFForg/https-everywhere/issues/13762#issuecomment-348588436 that we've already discussed securecookies:

The ideal dynamic for me as a maintainer is that a given contributor submits a PR, I make comments on it, they update the PR based on the comments in a timely way, and I merge the PR. Eventually this contributor's PRs require less discussion and reviewing their PRs becomes less difficult.

However a certain bad dynamic/failure case can happen when you have two maintainers with different approaches. In this case a contributor submits a PR that, for whatever reason, another maintainer passes with minimal comments but which, if I reviewed it, would require more discussion. Eventually I just avoid these PRs entirely and they effectively become reviewable only by the other maintainer.

That's what's happening with these securecookie PRs: they are problematic for me but @J0WI is okay with them, so I avoid them and now @J0WI is stuck with them. I want to resolve that tension, particularly before the backlog of these PRs gets much larger.

There are different ways we can resolve this problem: deciding that I'm too strict, or @J0WI is not strict enough, or that we are both doing it wrong, or something else. However, any resolution which significantly increases the tedium of resolving a PR -- for example, being obligated to have a "opportunity to justify its usage" discussion -- means, just being honest here, that I'm probably going to ignore that PR. Maybe we should be having these discussions, but the reality is that there are a lot of other things I'd rather do than argue with a contributor about the details of some cookie behavior.

[gopherit]

Speaking only for myself (as a low-volume contributor), I'm very careful with any securecookie tags in rulesets and meticulously/OCDly test each cookie's functionality. If all cookies test fine, only then do I use the wildcard .+; otherwise I get granular.

I've definitely glossed over some rulesets with securecookie wildcard tags "thrown in" (just a gut feeling) where I questioned whether the contributor _really_ tested their ruleset or if they just use some cookie-cutter [Edit: pun not intended] template or script and blast out a ton of rulesets. I definitely commiserate with the ruleset maintainers, since it's not always clear how much effort and QA a given PR has been through prior to being submitted.

I'm not sure if the all-or-nothing approach is ideal. I see value in securing session cookies in particular and still see value in the functionality. Maybe some process improvements can be made?

1. Update CONTRIBUTING.md to clearly describe how the tags should be used and pitfalls to functionality should they be misused.
2. Update Travis testing? I'm not sure where to start with this.

[jeremyn]

@gopherit The potential securecookie problem that I've described, where a HTTPS subdomain a sets a secure cookie for ^ which cannot be read by HTTP-only subdomain b, can't be tested for completely, because it is always possible for the site owner to add a new HTTP-only subdomain b after you do your testing.

I feel maintainers should theoretically treat PRs as not just untested but actively hostile. Perhaps what appears to be a sloppy securecookie tag is actually a deliberate attempt to break a site in a plausibly-deniable way. This includes PRs from long-term contributors who may, for all I know, be deep-cover moles, or recently compromised. Of course in practice I don't believe this is actually true for most PRs or contributors, but this sort of "security first" mindset means among other things that maintainers should try to keep things as simple and obvious as possible. It also means that I give very little weight to what a contributor says is true, if I can't test it myself.

[J0WI]

I would like to bump #1584 again: What aims the securecookie rule to secure? Can we mitigate potential issues? Is the current behavior suitable for most rules?

I think "most rules" are simple redirects with only a few targets and no relevant [1] non-working targets.
Risky scenarios are hosts that use different subdomains as "shortcuts" [2] for the main site and also contains features like account login. We need to take care there, but it's sometimes hard to understand the whole structure of a site.

A user friendly solution would be a checkbox (opt in or out) to control if we should try to secure cookies for the user. This might be also useful for debugging and you can still secure HTTP requests.

[1] Means common external services like blogs, mail, status page, link shortener etc. that are not mixing any cookies from the main site.
[2] Means the subdomains are controlled by the same service (e.g. a CMS).

[jeremyn]

A very large number of sites share Google Analytics cookies like _ga using the ^ domain. I don't really care about protecting a site's analytics but it means these sites require extra effort to determine what "mixing" is going on, and I want to reduce our effort.

Also, even sites that are simple for external users can have public-facing login pages for internal users, like for managing a CMS or logging into corporate webmail. These internal-user domains might require Sublist3r to find but they often exist.

We shouldn't complicate the UI to add a cookie toggle. Most users will have no idea what it does or when to use it. In fact I guess right now most users only interact with the extension to enable or disable it entirely.

[jeremyn]

This discussion seems to have ended.

[pipboy96]

I wonder if we even need to modify cookies to add Secure flag to them, given that such addition is pretty much irreversible (we do not record which cookies we have rewritten), and if user disables or uninstalls HTTPS Everywhere or we remove the rule due to the website no longer supporting HTTPS, the user will lose their cookies.

One of the reasons I ask is bugs like #1584, but there are more reasons. Can we add a (default on) switch for securing cookies instead of it happening for everyone no matter if they want to disable it or not?