[$csp2 adblockpluscore] Issue 6329 - Add the CSP filter type

lib/filterClasses.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 80 matching lines @@
 /**
  * Regular expression that RegExp filters specified as RegExps should match
  * @type {RegExp}
  */
 Filter.regexpRegExp = /^(@@)?\/.*\/(?:\$~?[\w-]+(?:=[^,\s]+)?(?:,~?[\w-]+(?:=[^,\s]+)?)*)?$/;
 /**
  * Regular expression that options on a RegExp filter should match
  * @type {RegExp}
  */
 Filter.optionsRegExp = /\$(~?[\w-]+(?:=[^,]+)?(?:,~?[\w-]+(?:=[^,]+)?)*)$/;
+/**
+ * Regular expression that matches an invalid Content Security Policy
+ * @type {RegExp}
+ */
+Filter.invalidCSPRegExp = /(;|^) ?(base-uri|referrer|report-to|report-uri|upgrade-insecure-requests)\b/i;
 
 /**
  * Creates a filter of correct type from its text representation - does the
  * basic parsing and calls the right constructor then.
  *
  * @param {string} text   as in Filter()
  * @return {Filter}
  */
 Filter.fromText = function(text)
 {
@@ skipping 54 matching lines @@
  * Removes unnecessary whitespaces from filter text, will only return null if
  * the input parameter is null.
  * @param {string} text
  * @return {string}
  */
 Filter.normalize = function(text)
 {
   if (!text)
     return text;
 
-  // Remove line breaks and such
+  // Remove line breaks, tabs etc
   text = text.replace(/[^\S ]+/g, "");
 
   // Don't remove spaces inside comments
   if (/^ *!/.test(text))
     return text.trim();
 
   // Special treatment for element hiding filters, right side is allowed to
   // contain spaces
   if (Filter.elemhideRegExp.test(text))
   {
     let [, domain, separator, selector] = /^(.*?)(#@?#?)(.*)$/.exec(text);
     return domain.replace(/ +/g, "") + separator + selector.trim();
   }
 
-  // For most regexp filters we strip all whitespace.
+  // For most regexp filters we strip all spaces, but $csp filter options
+  // are allowed to contain single (non trailing) spaces.
   let strippedText = text.replace(/ +/g, "");
   if (!strippedText.includes("$") || !/\bcsp=/i.test(strippedText))
     return strippedText;
 
   let optionsMatch = Filter.optionsRegExp.exec(strippedText);
   if (!optionsMatch)
     return strippedText;
 
-  // But since the values of $csp filter options are allowed to contain single
-  // (non trailing) spaces we have to be more careful if they might be present.
+  // For $csp filters we must first separate out the options part of the
+  // text, being careful to preserve its spaces.
   let beforeOptions = strippedText.substring(0, optionsMatch.index);
-  let optionsText = text;

[Manish Jethani, 2018/03/15 11:13:25]

By the way, I was wondering if we could avoid creating the array here. I tried
this one:

  let strippedDollarIndex = -1;
  let dollarIndex = -1;

  do
  {
    strippedDollarIndex = beforeOptions.indexOf("$", strippedDollarIndex + 1);
    dollarIndex = text.indexOf("$", dollarIndex + 1);
  }
  while (strippedDollarIndex != -1);

  let optionsText = text.substring(dollarIndex + 1);

This is consistently faster (~1.2s vs. ~1.6s for 10 million iterations here)
than the one that creates an array using String.split.

[Manish Jethani, 2018/03/16 06:47:09]

Out of curiosity, what do you think about this? It performs better. There's no
array created, there's no substring operation. I find it easier to grok as well,
but I guess that's subjective.

It could be a separate function:

  function findOptionsIndex(stripped, original) 
  {                                            
    let strippedIndex = -1; 
    let originalIndex = -1;

    do
    {
      strippedIndex = stripped.indexOf("$", strippedIndex + 1);
      originalIndex = original.indexOf("$", originalIndex + 1);
    }
    while (strippedIndex != -1);

    return originalIndex;
  }

[kzar, 2018/03/16 11:29:05]

Well I think that what we have already is plenty good enough, this is a rarely
used code-path. While this kind of thing is interesting, the effort would be
better spent elsewhere (e.g. removing the split when we parse the options string
later). I mean we're expecting a user to have maybe 5 or 10 of these filters, so
with your numbers and being generous and saying 10 csp filters we're talking 1.6
microseconds vs 1.2 microseconds here, or in other words a saving of 400
nanoseconds.

Since we're already playing code-golf I thought I'd test it[1], seems my numbers
match yours. Your approach is nearly a quarter faster and the difference seems
to be down to the `split().length` operation rather than the `substr` calls.

[1] - https://jsperf.com/findoptionstext

[Manish Jethani, 2018/03/16 12:05:06]

But the effort has already been spent, the code has already been written, right?
I mean there's nothing to do here but change a few lines of code.

It's a rarely used code path now, what if there's a new option like $csp that's
actually used very often? Then somebody may remove that /\bcsp=/i check in the
future, but they won't think about optimizing the part that follows because they
would assume it's already optimal (because why would the ABP team write
suboptimal code?).
This assumption about only 10 $csp filters may not hold in the future.

Honestly, I'm not seeing the downside of choosing code that's both more
efficient and easier to understand, but I'll leave it here.
The current version is 60% slower here on Chrome 65 and 76% slower on Firefox 58
(both running on macOS Sierra).

[kzar, 2018/03/17 14:35:06]

Then we have bigger problems, e.g. we ran the options regexp on the string twice
for those filters.
Wasn't an assumption, I looked at the uBlock filter list which is where this
filter option is already being used. OK I got it wrong and it's 24 not 10... but
sure perhaps that number will change in the future.
You said yourself that it's subjective which is easier to understand and FWIW I
don't agree with you here. Either way the downside is that time spent talking
endlessly about something that *does not matter* is wasted, there's an
opportunity cost associated - we could have spent that time talking about how to
optimise a code path that does matter instead.
I can't reproduce that, I see it as almost 25% slower.

[sergei, 2018/03/19 15:47:30]

I actually agree with Manish here, in both the performance concerns and the
readability. In particular the `split`ing and `substring`s are way less
efficient than just indexes. One has to figure out that the trick with split is
actually to merely count the number of occurrence of the dollar sign and it also
requires to pay attention where the stripped string and the original one.

Since right now it's not an issue on practice I would not insist on anything
what blocks the change from landing. However could someone please create the
issue with the concerns on https://issues.adblockplus.org/? Additionally I'm
recalling hub#7471, perhaps we could measure the performance of emscripten
parsing code as the part of this issue and if it makes sense to bring from the
emscripten merely the parsing functionality instead of full filter classes and
Co. JIC, I would like to remind that not fast enough parsing of the filters is
currently a serious issue for us.

[kzar, 2018/03/19 16:38:58]

Well I still disagree on the readability aspect, but it seems I'm now
outnumbered there. Done.
That's half true. `.split("$").length` is slower than the  `indexOf("$", ...)`
approach. But I did not find using `substring` made much of a difference at all.
As of Patch Set 13 (which still used substring) the performance was approx 2%
slower than Manish's suggestion.
Well IMO if this is a concern we'd be better off:

1.  Making practical compromises with filter normalisation / parsing. For
example just doing `.replace(/ +/, " ").trim()` at normalisation instead of all
this $csp related stuff. See my point of view here[1]. We could also go with
Manish's suggestion of remove support for option values containing "$"s if it
helps.
2. Focusing on more critical parts of the code, e.g. how we do a .split of the
options string in the fromText function.

If you agree about point 1 let us know which parts are especially painful for
performance, perhaps there are other small tweaks with what we allow that could
improve performance further.

[1] - https://codereview.adblockplus.org/29680684/#msg7

[Manish Jethani, 2018/03/19 18:03:38]

Acknowledged.

-  for (let i = beforeOptions.split("$").length; i > 0; i--)
-    optionsText = optionsText.substr(optionsText.indexOf("$") + 1);
-
+  let strippedDollarIndex = -1;
+  let dollarIndex = -1;
+  do
+  {
+    strippedDollarIndex = beforeOptions.indexOf("$", strippedDollarIndex + 1);
+    dollarIndex = text.indexOf("$", dollarIndex + 1);
+  }
+  while (strippedDollarIndex != -1);
+  let optionsText = text.substr(dollarIndex + 1);
+
+  // Then we can normalize spaces in the options part safely
   let options = optionsText.split(",");
   for (let i = 0; i < options.length; i++)
   {
     let option = options[i];
     let cspMatch = /^ *c *s *p *=/i.exec(option);
     if (cspMatch)
     {
       options[i] = cspMatch[0].replace(/ +/g, "") +
                    option.substr(cspMatch[0].length).trim().replace(/ +/g, " ");
     }
@@ skipping 558 matching lines @@
         option = option.substr(0, separatorIndex);
       }
       option = option.replace(/-/, "_").toUpperCase();
       if (option in RegExpFilter.typeMap)
       {
         if (contentType == null)
           contentType = 0;
         contentType |= RegExpFilter.typeMap[option];
 
         if (option == "CSP" && typeof value != "undefined")
-        {
-          if (csp)
-            csp.push(value);
-          else
-            csp = [value];
-        }
+          csp = value;
       }
       else if (option[0] == "~" && option.substr(1) in RegExpFilter.typeMap)
       {
         if (contentType == null)
           ({contentType} = RegExpFilter.prototype);
         contentType &= ~RegExpFilter.typeMap[option.substr(1)];
       }
       else if (option == "MATCH_CASE")
         matchCase = true;
       else if (option == "~MATCH_CASE")
@@ skipping 12 matching lines @@
         sitekeys = value.toUpperCase();
       else
         return new InvalidFilter(origText, "filter_unknown_option");
     }
   }
 
   try
   {
     if (blocking)
     {
-      if (csp)
-      {
-        csp = csp.join("; ").toLowerCase();
-
-        // Prevent filters from injecting report-uri or report-to directives
-        // since they are a privacy concern. Regexp based upon reBadCSP[1].
-        // [1] - https://github.com/gorhill/uBlock/blob/67e06f53b4d73df6179f6d320553a55da4ead40e/src/js/static-net-filtering.js#L1362

[kzar, 2018/03/15 16:09:47]

Note: I no longer attribute uBlock here since we pretty much completely rewrote
the regexp by this point.

[Manish Jethani, 2018/03/15 16:55:34]

Acknowledged.

-        if (/(;|^)\s*report-(to|uri)\b/.test(csp))

[Manish Jethani, 2018/03/15 13:24:44]

Since we allow single spaces in the CSP, we should strip them here before
testing the regular expression:

  if (/(;|^)report-(to|uri)\b/.test(csp.replace(/ /g, "")))

Reason: even though the browsers that we know today might ignore something like
"r e p o r t-uri=http://example.com", we should cover the case where either by
design or by accident the browser starts accepting spaces here (browsers do tend
to be liberal in the kinds of inputs they'll accept and try to interpret
correctly). It would be a shame if because of a browser change somebody was able
to violate our users' privacy.

From the security and privacy point of view I have the following
recommendations:

 1.  It seems there's an outdated "referrer" directive [1]. This is bad for
privacy if exploited, similar to report-uri/report-to, in particular if it is
set to "unsafe-url". We should strip it out as well.

 2.  Since we don't know which directives in the future that would be introduced
in browser would be bad for privacy and security, this should be a whitelist
rather than a blacklist. We should allow the following directives only:
connect-src, default-src, font-src, frame-src, img-src, media-src, object-src,
script-src, style-src, base-uri, plugin-types, sandbox, form-action,
frame-ancestors, navigation-to, block-all-mixed-content, require-sri-for, and
upgrade-insecure-requests (note: no manifest-src, no worker-src).

 3.  'unsafe-eval' should be stripped out from the policy. I can't think of a
single reason why a filter author should be able to add 'unsafe-eval' to the
policy.

Further, none of the directives that are actually injected into the headers
should make the policy more liberal than what it would be otherwise. i.e. if the
response says "Content-Security-Policy: sandbox;" and we are about to inject
"Content-Security-Policy: sandbox allow-scripts;", we should not inject it. This
should be taken care of on the adblockpluschrome side.

[1]:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Po...

[kzar, 2018/03/15 13:44:45]

I don't think that's a problem, Sebastian noticed previously[1] that CSPs are
combined exclusively[2] and so if one policy blocks something that trumps the
other that allows the thing.

(Will reply to the other points later.)

[1] - https://codereview.adblockplus.org/29356231/#msg24
[2] - https://www.w3.org/TR/CSP2/#enforcing-multiple-policies

[Manish Jethani, 2018/03/15 13:47:33]

Thanks, I was just wondering about this. In that case we're OK.

By the way I wonder if whitelisting the directives (1) should be part of the
normalization step, or (2) should be here, or (3) should be one level up in the
stack at the time of header injection (to allow more flexibility).

[Manish Jethani, 2018/03/15 13:50:48]

Also, in the interest of landing this (so we can start playing with it), we
could do the whitelisting/blacklisting part as a separate patch. Then we can
take our time to decide where, what, and how. We'll just have to be careful
about not accidentally shipping it as it is.

[kzar, 2018/03/15 14:13:37]

I'd prefer not to land this without the security / privacy concerns addressed.
If you want to play with it all I try to keep my feature branches up to date in
GitHub[1][2][3].

[1] - https://github.com/kzar/adblockpluschrome/tree/5241-csp-option
[2] - https://github.com/kzar/adblockpluscore/tree/6329-csp-option
[3] - https://github.com/kzar/adblockplusui/tree/6451-csp-filters

[kzar, 2018/03/15 14:13:37]

It should be here at parse time IMO.

[kzar, 2018/03/15 14:13:38]

Well spotted, there's a non-outdated version called Referrer-Policy too[1]. I
think we should only allow "no-referrer" for both, since while omitting the
referrer information might be useful for adblocking anything else could be a
privacy concern.
I disagree about stripping spaces in the off-chance that a browser introduces a
bug in the future. As for the directive whitelist I see your point, but I can
see arguments against. Do you have an opinion here Sebastian?
I'm curious, why did you omit those? worker-src in particular could be useful
for preventing circumvention.
True there's not much point, but also there's not much harm. If our policy can't
allow anything that's already forbidden then who cares right?

[1] - https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referrer-Policy

[Manish Jethani, 2018/03/15 14:28:31]

Referrer-Policy doesn't really concern us since we're not setting that header,
right? If I understand correctly, the $csp filter option is only supposed to
affect the Content-Security-Policy header. if the site sets its own
Referrer-Policy header then so be it, it's not on us.

[Manish Jethani, 2018/03/15 14:28:31]

My only concern was to not allow filter authors to make a document's CSP more
permissive than it already is. Since that's apparently not possible, I'm OK with
these.
Same as above. It's not an issue, we can allow it.

[Sebastian Noack, 2018/03/15 19:03:05]

Agreed. In the end, there is nothing we can effectively do to account for any
possibly future change in browser's behavior. And this particular change seems
especially obscure/unlikely. On the other hand, as more complexity we add, as
more likely we introduce bugs effecting current browser behavior.
The list of directives (relevant to ad blocking) will likely keep growing. So
maintaining a whitelist will be a permanent effort. Also it will keep filter
list authors from using the newest directives, as they have to wait for us to
release a new version with the directive added to the whitelist, and for users
to update their ad blocker.

Directives that could leak information are quite a special case and (unless
they change the semantics of report-uri/report-to again, which seems unlikely),
I don't see a case to add yet another one in the future. Though its still
possible. In the same way, it is possible that CSS selectors could leak
information in the future, yet we don't consider whitelisting safe CSS syntax.

Also it isn't that easy for an attacker to get a malicious filter to be used on
a target machine. The filter lists used by default, come from trusted sources
and we are in the control of the mirrors. So an attacker would have to host
their own filter list and then trick the user in subscribing to this filter
list. In the end, it might be more effective, while not much more difficult, to
trick the user into installing malware instead. I don't think that this
invalidates all security concerns about filter lists, but the attack vector
seems smaller than when dealing with input from most other external sources.

So I think a blacklist is appropriate and sufficient.
Absolutely, we need worker-src. Abuse of SharedWorker/ServiceWorker for
circumvention (since they cannot be associated with the tab/frame they are
related to), was the main motivation for the $csp filter in the first place.

On 2018/03/15 13:24:44, Manish Jethani wrote:
There is a case for 'unsafe-eval'. If a website relies on dynamic code
evaluation, but you want to block other script sources that cannot be blocked by
other means, e.g. inline scripts, you would inject a policy like "script-src
http: 'unsafe-eval'". On the other hand, as already concluded, there is no harm
that can be caused with 'unsafe-eval' since the policy cannot make the document
more permissive, anyway.

On 2018/03/15 13:47:33, Manish Jethani wrote:
I think validation is preferable over normalization here. If we'd just strip
those directives silently, it will be more difficult to figure out why they
don't work, or worse filter list authors won't notice and add them to the filter
list anyway causing potential security issues with other ad blockers that are
less careful. If we mark the filter as invalid, however, this will trigger a
visible error message when adding it as a custom filter (which filter list
authors generally do first to test their filters).

[Manish Jethani, 2018/03/16 12:25:05]

Ack.
Maintaining a blacklist is also a permanent effort, isn't it? Unless we don't
care as much about security/privacy, which we do (?).

So we have two options:

 1.  Maintain a whitelist, let filter authors report missing directives ("It's
not working for me!"), add the directives as and when, and do a new release
after sufficient testing

 2.  Maintain a blacklist, let somebody report a security/privacy issue
(hopefully found on a beta version of the browser, but possibly later when the
browser is already stable and widely deployed and somebody has found a way to
exploit ABP and made the news), add the directive to the blacklist, do an
emergency patch release

CSP not working for filter authors is far more acceptable to me than a
vulnerability that's already being exploited in the wild (possibly without
anybody's knowledge) because of ABP.
The comparison with CSS is not fair, an HTTP header is far more powerful,
especially one that literally has "security" in its name
(Content-Security-Policy).
A filter doesn't have to be malicious for there to be a security issue, it can
be (and usually is) a genuine oversight.

But OK, it really depends on the tradeoffs, I see your point here.
Ack.
Ack.

-          return new InvalidFilter(origText, "filter_invalid_csp");
-      }
+      if (csp && Filter.invalidCSPRegExp.test(csp))
+        return new InvalidFilter(origText, "filter_invalid_csp");
 
       return new BlockingFilter(origText, text, contentType, matchCase, domains,
                                 thirdParty, sitekeys, collapse, csp);
     }
     return new WhitelistFilter(origText, text, contentType, matchCase, domains,
                                thirdParty, sitekeys);
   }
   catch (e)
   {
     return new InvalidFilter(origText, "filter_invalid_regexp");
@@ skipping 238 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });