Issue 1727 - Prevent circumvention via WebSocket

include.preload.js

Index: include.preload.js
diff --git a/include.preload.js b/include.preload.js
index 949d039663f9130843b4d8d0a3ddfcb434522066..685f20970919f737b4682d8d1e6b8fccc2e796cc 100644
--- a/include.preload.js
+++ b/include.preload.js
@@ -17,6 +17,7 @@
 
 var MutationObserver = window.MutationObserver || window.WebKitMutationObserver;
 var SELECTOR_GROUP_SIZE = 200;
+var id = Math.random().toString(36).substr(2);
 
 var typeMap = {
   "img": "IMAGE",
@@ -349,49 +350,150 @@ function reinjectStyleSheetWhenRemoved(document, style)
   return observer;
 }
 
+function injectJS(f)

[Sebastian Noack, 2016/08/08 22:09:15]

I just noticed that this function is redundant with runInPage() in
safari/include.youtube.js. Functions defined here can be reused there.

FWIW, note how that implementation is also somewhat simpler by always expecting
exactly one argument which is all we'd need.

[kzar, 2016/08/09 12:08:05]

Done.

+{
+  var args = JSON.stringify(Array.prototype.slice.call(arguments, 1));
+  args = args.substring(1, args.length - 1);
+  var codeString = "(" + f.toString() + ")(" + args + ");";
+
+  var script = document.createElement("script");
+  script.async = false;
+  script.textContent = codeString;
+  document.documentElement.appendChild(script);
+  document.documentElement.removeChild(script);
+}
+
 function protectStyleSheet(document, style)
 {
-  var id = Math.random().toString(36).substr(2)
   style.id = id;
 
-  var code = [
-    "(function()",
-    "{",
-    '  var style = document.getElementById("' + id + '") ||',
-    '              document.documentElement.shadowRoot.getElementById("' + id + '");',
-    '  style.removeAttribute("id");'
-  ];
+  var protector = function(id)
+  {
+    var style = document.getElementById(id) ||
+                document.documentElement.shadowRoot.getElementById(id);
+    style.removeAttribute("id");
+
+    var i;

[Sebastian Noack, 2016/08/08 22:09:15]

It seems that you don' reuse the variable i anymore. So put the declaration in
the loop header?

[kzar, 2016/08/09 12:08:05]

Done.

+    var disableables = [style, style.sheet];
+    for (i = 0; i < disableables.length; i++)
+      Object.defineProperty(disableables[i], "disabled",
+                            {value: false, enumerable: true});
+
+    var methods = ["deleteRule", "removeRule"];

[Sebastian Noack, 2016/08/08 22:09:15]

How about ["deleteRule", "removeRule"].forEach(), without creating a variable?

[kzar, 2016/08/09 12:08:05]

Done.

+    methods.forEach(function(method)
+    {
+      var original = CSSStyleSheet.prototype[method];
+      CSSStyleSheet.prototype[method] = function(index)
+      {
+        if (this != style.sheet)
+          original.call(this, index);
+      };
+    });
+  };
 
-  var disableables = ["style", "style.sheet"];
-  for (var i = 0; i < disableables.length; i++)
+  injectJS(protector, id);
+}
+
+// Neither Chrome[1] nor Safari allow us to intercept WebSockets, and therefore
+// some ad networks are misusing them as a way to serve adverts and circumvent
+// us. As a workaround we wrap WebSocket, preventing blocked WebSocket
+// connections from being opened.
+// [1] - https://bugs.chromium.org/p/chromium/issues/detail?id=129353
+function wrapWebSocket()
+{
+  if (typeof WebSocket == "undefined")
+    return;
+
+  var eventName = "abpws-" + id;
+
+  document.addEventListener(eventName, function(event)
   {
-    code.push("  Object.defineProperty(" + disableables[i] + ', "disabled", '
-                                         + "{value: false, enumerable: true});");
-  }
+    ext.backgroundPage.sendMessage({
+      type: "websocket-request",
+      url: event.detail.url
+    }, function (block)
+    {
+      document.dispatchEvent(
+        new CustomEvent(eventName + "-" + event.detail.url, {detail: block})
+      );
+    });
+  });
 
-  var methods = ["deleteRule", "removeRule"];
-  for (var j = 0; j < methods.length; j++)
+  function wrapper(eventName)
   {
-    var method = methods[j];
-    if (method in CSSStyleSheet.prototype)
+    // As far as possible we must track everything we use that could be
+    // sabotaged by the website later in order to circumvent us.
+    var RealWebSocket = WebSocket;
+    var closeWebSocket = RealWebSocket.prototype.close;
+    var addEventListener = document.addEventListener.bind(document);
+    var removeEventListener = document.removeEventListener.bind(document);
+    var dispatchEvent = document.dispatchEvent.bind(document);
+    var CustomEvent = window.CustomEvent;
+    var boundCall = Function.prototype.call.bind(Function.prototype.call);
+    var stringToString = String.prototype.toString;
+    // (These two functions are usually the same, but since Safari 9 considers
+    //  WebSocket to be an object rather than a function we must track both.)
+    var functionToString = Function.prototype.toString;
+    var WebSocketString = RealWebSocket.toString();

[Sebastian Noack, 2016/08/08 22:09:15]

I suppose this variable should rather be lowercase?

[kzar, 2016/08/09 12:08:05]

Done.

+
+    function checkRequest(url, callback)
     {
-      var origin = "CSSStyleSheet.prototype." + method;
-      code.push("  var " + method + " = " + origin + ";",
-                "  " + origin + " = function(index)",
-                "  {",
-                "    if (this != style.sheet)",
-                "      " + method + ".call(this, index);",
-                "  }");
+      var incomingEventName = eventName + "-" + url;
+      function listener(event)
+      {
+        callback(event.detail);
+        removeEventListener(incomingEventName, listener);
+      }
+      addEventListener(incomingEventName, listener);
+
+      dispatchEvent(new CustomEvent(eventName, {
+        detail: {url: url}
+      }));
     }
-  }
 
-  code.push("})();");
+    function wrappedToString()
+    {
+      if (this === WebSocket)

[Sebastian Noack, 2016/08/08 22:09:16]

As per Mozilla's coding practices, we prefer == over ===.

[kzar, 2016/08/09 12:08:04]

Sure but here we want to check that `this` points to the same function as
WebSocket. We don't want any type conversion.

[Sebastian Noack, 2016/08/09 14:53:29]

I think, if none of the values has a primitive type, == wouldn't do any
conversion, or do I miss something?

[kzar, 2016/08/09 16:11:39]

Well to tell you the truth I'm not 100% sure if type conversion is ever a
problem here, but since we really are checking for identity I think it makes
more sense to write that explicitly. No chance for mistake that way and it also
more accurately illustrates what we're doing.

+        return WebSocketString;
+      if (this === wrappedToString)

[Sebastian Noack, 2016/08/08 22:09:15]

This special case is unneccessary if you simply assign
Function.prototype.toString = wrappedToString.bind() below.

[kzar, 2016/08/09 12:08:04]

(I tried it out but found I would get the exception "VM2877:39 Uncaught
TypeError: Function.prototype.toString is not generic(…)" when doing things like
`Function.prototype.toString.call(WebSocket)` or
`Function.prototype.toString.call(Function.prototype.toString)` which worked
before.)

[Sebastian Noack, 2016/08/09 14:53:29]

Even better, why not just |WebSocket = function(...) {...}.bind()|? That would
make the whole Function.prototype.toString hack redundant.

[kzar, 2016/08/09 16:11:39]

Well nice idea but then WebSocket.toString() gives the wrong values:
Chrome: "function WebSocket() { [native code] }" -> "function () { [native code]
}"
Safari: "[object WebSocketConstructor]" -> "function WebSocket() { [native code]
}"

(Newlines omitted.)

+        return boundCall(functionToString, functionToString);
+      return boundCall(functionToString, this);
+    };
+    Function.prototype.toString = wrappedToString;
 
-  var script = document.createElement("script");
-  script.async = false;
-  script.textContent = code.join("\n");
-  document.documentElement.appendChild(script);
-  document.documentElement.removeChild(script);
+    WebSocket = function(url, protocols)
+    {
+      // Ensure that `new WebSocket();` throws the correct exception
+      if (!url)
+        return new RealWebSocket();

[Sebastian Noack, 2016/08/08 22:09:15]

You still get a different error when null or undefined is given. Not sure
whether it's worth to be considered. But then I wonder why you don't simply call
|websocket = new RealWebSocket(url, protocols)| unconditionally. IMO, the
string-to-string dance should happen afterwards anyway as its just for the case
if a crazy object is given that can be converted to a string by the WebSocket
constructor but isn't automatically converted through event messaging.

[kzar, 2016/08/09 12:08:05]

Hmm good point and we can even just use `websocket.url` since then the WebSocket
constructor does the work of normalising the url for us! Done.

[Sebastian Noack, 2016/08/09 14:53:29]

Even better, nice!

+
+      // First ensure url isn't a URL object, then make sure it's a real String.
+      // This is necessary to prevent circumvention, without breaking anything.
+      url = boundCall(stringToString, url.toString());

[Sebastian Noack, 2016/08/08 22:09:15]

8Why) do you have to call both, stringToString() and .toString()?

+
+      var websocket = new RealWebSocket(url, protocols);
+
+      checkRequest(url, function(blocked)
+      {
+        if (blocked)
+          boundCall(closeWebSocket, websocket);
+      });
+
+      return websocket;
+    };
+
+    var properties = Object.getOwnPropertyNames(RealWebSocket);
+    for (var i = 0; i < properties.length; i++)
+    {
+      var name = properties[i];
+      var desc = Object.getOwnPropertyDescriptor(RealWebSocket, name);
+      Object.defineProperty(WebSocket, name, desc);
+    }
+
+    RealWebSocket.prototype.constructor = WebSocket;
+  }
+
+  injectJS(wrapper, eventName);
 }
 
 function init(document)
@@ -401,6 +503,8 @@ function init(document)
   var observer = null;
   var tracer = null;
 
+  wrapWebSocket();
+
   function getPropertyFilters(callback)
   {
     ext.backgroundPage.sendMessage({