Detect static securecookies to trivialize more rules #16029

cschanaj · 2018-07-06T12:34:19Z

This PR modify trivialize-rules.js to detect static securecookie to trivialize more rules. It also trivialize the securecookie tags if all the securecookie domain and target domain are strictly identical. This will further trivializes about 100+ rulesets when ran against 8cfabe5

cc @RReverser @Hainish @Bisaloo @J0WI

RReverser · 2018-07-09T09:56:49Z

@cschanaj Can you please explain in prose what rules you're using / when it's going to assume that cookies are ok? I believe this is quite tricky to detect statically but I might be wrong and hard to understand the nuances from code.

cschanaj · 2018-07-09T14:05:59Z

@RReverser This PR aims to trivialize rulesets that would have been modified if securecookies are ignored. It is assumed that if all securecookies are static, they can be safely ignored.

A securecookie is called to be static either it is a trivial securecookie or ALL of the following conditions are satisfied:

securecookie.host match cookie.host from the beginning ^ to the end $. Otherwise, it might match subdomains/ partial patterns, thus a non-trivial securecookie.
securecookie.host will not throw an error when passed to explodeRegExp(). Otherwise, it might match patterns too complicated for our interests.
Each exploded securecookie.host should be included in ruleset.target/ exploded target. Otherwise, this ruleset is likely problematic itself. It is dangerous for a rewrite.

P.S. I have force-pushed 917e627 because there was an logic error in the previous implementation. Sadly we cannot trivialize the securecookies anyway.

P.S.2. When running this commit against the master, it is found that the script is problematic for AllianceBernstein.xml, namely a trivial domain is gone from ruleset.target; but I am guessing this a bug from the upstream...

Bisaloo · 2018-07-10T15:12:16Z

@cschanaj, it would be useful to have this info somewhere else than a comment in this PR. It would be great if you could copy your text from #16029 (comment) in either the commit message or a standalone README.

This PR aims to trivialize rulesets that would have been modified if securecookies are ignored. It is assumed that if all securecookies are static, they can be safely ignored. A securecookie is called to be static either it is a trivial securecookie or ALL of the following conditions are satisfied: 1. securecookie.host match cookie.host from the beginning ^ to the end $. Otherwise, it might match subdomains/ partial patterns, thus a non-trivial securecookie. 2. securecookie.host will not throw an error when passed to explodeRegExp(). Otherwise, it might match patterns too complicated for our interests. 3. Each exploded securecookie.host should be included in ruleset.target/ exploded target. Otherwise, this ruleset is likely problematic itself. It is dangerous for a rewrite.

cschanaj · 2018-07-11T12:14:43Z

@Bisaloo I've added the comment in the commit message of 77f08cf

RReverser · 2018-07-11T12:16:14Z

@cschanaj I see that you, for example, assume that securecookie.host === '.+' && securecookie.name === '.+' means that domain list can be trivialised; I think the opposite, it means that cookies might cover hosts not covered by rule expressions and so any current targets have to be preserved.

Also I wonder how you treat cookies on subdomains? In general, subdomain1.cookie.com can set cookies for subdomain2.cookie.com as well as cookie.com itself and so just the fact that cookie host doesn't appear in explicit targets, doesn't mean that targets can be trivialised.

cschanaj · 2018-07-11T12:26:43Z

@RReverser For cookies to be covered, there must be at least one rule covering the same domain. This is guarantee in safeToSecureCookie(cookie). In which a random URL is generated to test if we will HTTPSify it before securing a cookie.

Trivial securecookie implies that domain list can be trivialized. This also prevent the case where subdomain1 set cookies for subdomain2 which is not covered. In fact, subdomain2 cannot be secured by HTTPSE under the current implementation.

https-everywhere/chromium/background-scripts/rules.js

Lines 633 to 659 in a24cc40

    
           // If we passed that test, make up a random URL on the domain, and see if 
        
           // we would HTTPSify that. 
        
           var nonce_path = "/" + Math.random().toString(); 
        
           var test_uri = "http://" + domain + nonce_path + nonce_path; 
        
           // Cap the size of the cookie cache (limit chosen somewhat arbitrarily) 
        
           if (this.cookieHostCache.size > 250) { 
        
             // Map.prototype.keys() returns keys in insertion order, so this is a FIFO. 
        
             this.cookieHostCache.delete(this.cookieHostCache.keys().next().value); 
        
           } 
        
           util.log(util.INFO, "Testing securecookie applicability with " + test_uri); 
        
           var potentiallyApplicable = this.potentiallyApplicableRulesets(domain); 
        
           for (let ruleset of potentiallyApplicable) { 
        
             if (!ruleset.active) { 
        
               continue; 
        
             } 
        
             if (ruleset.apply(test_uri)) { 
        
               util.log(util.INFO, "Cookie domain could be secured."); 
        
               this.cookieHostCache.set(domain, true); 
        
               return true; 
        
             } 
        
           } 
        
           util.log(util.INFO, "Cookie domain could NOT be secured."); 
        
           this.cookieHostCache.set(domain, false); 
        
           return false;

RReverser · 2018-07-11T12:32:45Z

For cookies to be covered, there must be at least one rule covering the same domain.

Ah, that I didn't know. It does change the perspective and optimisation opportunities. Thanks!

cschanaj · 2018-07-11T12:36:10Z

When running this commit against the master, it is found that the script is problematic for AllianceBernstein.xml, namely a trivial domain is gone from ruleset.target; but I am guessing this a bug from the upstream...

@RReverser could you please confirm this? It seem that not all covered domains are enumerated by explodeRegExp() for certain type of RegExp. You might remove the securecookie tag to run trivialize-rules in master to see the bug. More trivialization can be made if this got fixed.

RReverser · 2018-07-11T17:25:20Z

@cschanaj Do you mean this?

diff --git a/src/chrome/content/rules/AllianceBernstein.xml b/src/chrome/content/rules/AllianceBernstein.xml
index 766b024548..db07742ff9 100644
--- a/src/chrome/content/rules/AllianceBernstein.xml
+++ b/src/chrome/content/rules/AllianceBernstein.xml
@@ -11,16 +11,14 @@
 -->
 <ruleset name="AllianceBernstein (partial)">

+       <target host="bernstein.com" />
        <target host="alliancebernstein.com" />
        <target host="www.alliancebernstein.com" />
-       <target host="bernstein.com" />
-       <target host="www.bernstein.com" />


        <securecookie host="^(?:www\.)?alliancebernstein\.com$" name=".+" />


-       <rule from="^http://(www\.)?(alliance)?bernstein\.com/"
-               to="https://$1$2bernstein.com/" />
+       <rule from="^http:" to="https:" />

 </ruleset>

RReverser · 2018-07-11T17:35:21Z

Okay, yes, I found the bug.

RReverser · 2018-07-11T17:43:02Z

Fix is here: #16051

cschanaj · 2018-07-11T22:30:52Z

@RReverser yes, thanks for filing a fix!!

Hainish

Looks good. Please include a comment in the code as per @Bisaloo's suggestion on what a "static" cookie is, since we're introducing new terminology.

Hainish · 2018-07-12T01:01:37Z

utils/trivialize-rules/trivialize-rules.js

+    try {
+      explodeRegExp(securecookie.host, domain => {
+        if (domain.startsWith('.')) {
+          domain = domain.slice(1);


Is this sufficient munging to ensure it matches the target domains?
If not, at worst it will not match and result in a false result, which isn't terrible but not desirable.

@Hainish this should be good enough. As mentioned in #16029 (comment), securecookie.host only match cookie.domain if there is a rule covers cookie.domain. Given the target are trivial, cookie.domain cannot be anything other than domain.example.com and .domain.example.com for a securecookie rule to works. With condition (1) effective, securecookie.host should explode to either one of them. Otherwise, the securecookie will never be applied (dangling rules?) under the current implementation.

With the above logic in mind, I have also add 4bbd731 to remove some of these securecookie rules that will never be effective.

IMHO, it would be great to have #15998 and #16000 merged before running this against the master.

cschanaj · 2018-07-19T00:52:22Z

@Hainish would you merge this code before the next release if the changes I made after your first review look good to you? So that I can file a separate PR running the script and fix the failing tests.

Hainish · 2018-07-19T16:10:40Z

@cschanaj yes - releases will be made every other month, so this should be deployed next month. I'll have more time to review next week. No promises if there is a security release, though.

Hainish · 2018-07-19T16:11:47Z

Perhaps @bcyphers has some time to review?

bcyphers · 2018-07-25T00:50:01Z

Looking at it now

Hainish · 2018-08-03T19:11:29Z

@bcyphers?

cschanaj · 2018-08-04T00:57:55Z

@RReverser my apologies, I have forgotten to rebase this PR before running the script. the issue is a duplicate of the fixed one. sorry for pinging you. please ignore my previous comment.

bcyphers · 2018-08-04T05:36:05Z

Hi, sorry, still working on this -- had to catch up on the rest of the static simplifications first. I'll have some comments this weekend.

bcyphers · 2018-08-06T22:46:09Z

utils/trivialize-rules/trivialize-rules.js

+    // safely.
+    if (unsupportedDomains.size > 0) {
+      if (unsupportedDomains.size === localDomains.size) {
+        shouldRemoveSecurecookies = true;


If any one of the securecookies is unsupported, this will cause all of them to be removed. I don't think that's the intended behavior.

For example, for the following XML, the script will trivialize the file and remove all securecookies:

<target host="a.foo.com" /> <supercookie host="a.foo.com" /> <supercookie host="b.foo.com" /> ...

But if the two securecookie entries are combined into one (<securecookie host="^(a|b)\.foo\.com$" />), the script will refuse to trivialize the XML.

It seems like we should remove one securecookie entry at a time, not drop them all at once. If that's risky (not sure why it would be), we can just refuse to trivialize any XML with unsupported securecookie domains.

See comments in code for more details.

cschanaj · 2018-08-07T13:23:42Z

@bcyphers That's indeed unexpected, thanks for catching the bugs 😃 5c149aa should work, I have tested it with the following rulesets; the diff is the result from running the script.

<ruleset name="Foo.com">
	<target host="*.foo.com" />

	<securecookie host="^www\.foo\.com$" name=".+" />
	<securecookie host="^a\.foo\.com$" name=".+" />

	<rule from="^http://www\.foo\.com/" to="https://www.foo.com/" />
</ruleset>

gives

11c11
< 	<target host="*.foo.com" />
---
> 	<target host="www.foo.com" />
14,16c14
< 	<securecookie host="^a\.foo\.com$" name=".+" />
< 
< 	<rule from="^http://www\.foo\.com/" to="https://www.foo.com/" />
---
> 	<rule from="^http:" to="https:" />

and (noted that the script cannot remove partial dangling securecookie rules)

<ruleset name="Bar.com">
	<target host="*.bar.com" />

	<securecookie host="^(www|a)\.bar\.com$" name=".+" />

	<rule from="^http://www\.bar\.com/" to="https://www.bar.com/" />
</ruleset>

gives

11c11
< 	<target host="*.bar.com" />
---
> 	<target host="www.bar.com" />
15c15
< 	<rule from="^http://www\.bar\.com/" to="https://www.bar.com/" />
---
> 	<rule from="^http:" to="https:" />

RReverser · 2018-08-07T14:37:09Z

utils/trivialize-rules/explode-regexp.js

@@ -4,8 +4,8 @@ const { parse } = require('regulex');

 class UnsupportedRegExp extends Error {}

-function explodeRegExp(re, callback) {
-  (function buildUrls(str, items) {
+function explodeRegExp (re, callback) {


Hmm, this file was previously formatted with prettier. Did it add these whitespaces or did you manually? Looks quite odd.

May I know which prettier options did you use? I can revert the changes in a37df21

P.S. a37df21 was mostly done automatically using semistandard from standardjs

I think just single-quotes plus semicolons.

It seems to me that prettier will create some strange changes to trivialize-rules.js, e.g. spliting the back-tick quoted string to the following

+ let scReSrc = `\n([\t ]*)<securecookie\\s*host=\\s*"${escapeStringRegexp( + securecookie.host + )}"(\\s*)name=\\s*"${escapeStringRegexp( + securecookie.name + )}"\\s*?/>[\t ]*\n`;

So I guess it is better not to run prettier on it. Would you mind if I leave the file as-is in e8f27ee?

This reverts commit a37df21.

cschanaj · 2018-08-09T12:21:43Z

ping @Hainish @bcyphers @RReverser

bcyphers · 2018-08-13T22:44:04Z

utils/trivialize-rules/trivialize-rules.js

+      if (unsupportedDomains.size === localDomains.size) {
+        return [true, true];
+      }
+      // TODO: Can we remove partial dangling securecookie rules???


Don't think so. This means that the author intended at least some of the target domains to use secure cookies, we shouldn't throw out that information

cschanaj · 2018-08-16T22:49:59Z

@bcyphers Does e5ad3c5 looks good to you?

bcyphers · 2018-08-16T23:39:11Z

Yeah, I can't find any other issues. I think this is good to go.

cschanaj · 2018-08-17T14:42:51Z

@bcyphers @RReverser @Hainish Thanks for the effort reviewing this PR. linking #16122

cschanaj · 2018-08-22T02:57:24Z

@bcyphers would you or @Hainish merge this PR? I would like to run this script before the next release and submit a PR to a downstream project in https://github.com/brave/https-everywhere-builder

Hainish · 2018-08-22T23:00:58Z

@cschanaj sorry this didn't make it in to 2018.8.22. This can be part of the ruleset release next week.

Hainish reviewed Jul 12, 2018

View reviewed changes

Chan Chak Shing added 3 commits July 12, 2018 20:45

Add comments on static cookie terminology

ca2844f

Remove securecookie tags if they are not effective

4bbd731

Add comments on the safety removing dangling securecookies

ca52ba2

cschanaj closed this Jul 13, 2018

cschanaj reopened this Jul 13, 2018

This was referenced Jul 16, 2018

Enhance explode regexp to support \d #16064

Closed

Batch trivialize some cdn rewrites with trivialize-cdn-rules #16067

Closed

bcyphers self-requested a review July 19, 2018 17:35

cschanaj mentioned this pull request Jul 23, 2018

Add support for 112.2o7.net rewrites #16071

Merged

bcyphers suggested changes Aug 6, 2018

View reviewed changes

Fix a bug for handling the dangling securecookie rules

5c149aa

See comments in code for more details.

semistandard --fix explode-regexp.js trivialize-rules.js

a37df21

RReverser reviewed Aug 7, 2018

View reviewed changes

Revert "semistandard --fix explode-regexp.js trivialize-rules.js"

db2b8b6

This reverts commit a37df21.

Chan Chak Shing added 2 commits August 9, 2018 23:17

Remove unused variables and fix syntax manually

e8f27ee

Update comments and remove unnecessary checks

6e82204

bcyphers reviewed Aug 13, 2018

View reviewed changes

Update comments on partial dangling securecookie

e5ad3c5

bcyphers approved these changes Aug 16, 2018

View reviewed changes

Hainish merged commit 7b1cb57 into EFForg:master Aug 22, 2018

cschanaj deleted the trivialize-js-1 branch August 27, 2018 23:07

This was referenced Sep 13, 2018

Fix WebExtension Excessive Memory Usage #12232

Closed

Add a script to trivialize a special form of securecookies #16540

Merged

cschanaj mentioned this pull request May 1, 2019

Add script to trivialize some left-wildcard targets #17855

Merged

Detect static securecookies to trivialize more rules #16029

Detect static securecookies to trivialize more rules #16029

Conversation

cschanaj commented Jul 6, 2018

RReverser commented Jul 9, 2018

cschanaj commented Jul 9, 2018

Bisaloo commented Jul 10, 2018

cschanaj commented Jul 11, 2018

RReverser commented Jul 11, 2018

cschanaj commented Jul 11, 2018 • edited Loading

RReverser commented Jul 11, 2018

cschanaj commented Jul 11, 2018 • edited Loading

RReverser commented Jul 11, 2018

RReverser commented Jul 11, 2018

RReverser commented Jul 11, 2018

cschanaj commented Jul 11, 2018

Hainish left a comment

Choose a reason for hiding this comment

Hainish Jul 12, 2018

Choose a reason for hiding this comment

cschanaj Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

cschanaj Jul 12, 2018

Choose a reason for hiding this comment

cschanaj commented Jul 19, 2018 • edited Loading

Hainish commented Jul 19, 2018

Hainish commented Jul 19, 2018

bcyphers commented Jul 25, 2018

Hainish commented Aug 3, 2018

cschanaj commented Aug 4, 2018

bcyphers commented Aug 4, 2018

bcyphers Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

cschanaj commented Aug 7, 2018 • edited Loading

RReverser Aug 7, 2018

Choose a reason for hiding this comment

cschanaj Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

RReverser Aug 9, 2018

Choose a reason for hiding this comment

cschanaj Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

cschanaj commented Aug 9, 2018

bcyphers Aug 13, 2018

Choose a reason for hiding this comment

cschanaj commented Aug 16, 2018

bcyphers commented Aug 16, 2018

cschanaj commented Aug 17, 2018

cschanaj commented Aug 22, 2018

Hainish commented Aug 22, 2018

cschanaj commented Jul 11, 2018 •

edited

Loading

cschanaj commented Jul 11, 2018 •

edited

Loading

cschanaj Jul 12, 2018 •

edited

Loading

cschanaj commented Jul 19, 2018 •

edited

Loading

bcyphers Aug 6, 2018 •

edited

Loading

cschanaj commented Aug 7, 2018 •

edited

Loading

cschanaj Aug 7, 2018 •

edited

Loading

cschanaj Aug 9, 2018 •

edited

Loading