Issue 1727 - Prevent circumvention via WebSocket

include.preload.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 332 matching lines @@
   var observer = new MutationObserver(function()
   {
     if (style.parentNode != parentNode)
       parentNode.appendChild(style);
   });
 
   observer.observe(parentNode, {childList: true});
   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 + ");";
-
+function runInPage(fn, arg)
+{
   var script = document.createElement("script");
+  script.type = "application/javascript";
   script.async = false;
-  script.textContent = codeString;
+  script.textContent = "(" + fn + ")(" + JSON.stringify(arg) + ");";
   document.documentElement.appendChild(script);
   document.documentElement.removeChild(script);
 }
 
 function protectStyleSheet(document, style)
 {
   style.id = id;
 
-  var protector = function(id)
+  runInPage(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++)
+    for (var 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)
+    ["deleteRule", "removeRule"].forEach(function(method)
     {
       var original = CSSStyleSheet.prototype[method];
       CSSStyleSheet.prototype[method] = function(index)
       {
         if (this != style.sheet)
           original.call(this, index);
       };
     });
-  };
-
-  injectJS(protector, id);
+  }, 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)
   {
     ext.backgroundPage.sendMessage({
       type: "websocket-request",
       url: event.detail.url
     }, function (block)
     {
       document.dispatchEvent(
         new CustomEvent(eventName + "-" + event.detail.url, {detail: block})
       );
     });
   });
 
-  function wrapper(eventName)
+  runInPage(function(eventName)
   {
     // 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 closeWebSocket = Function.prototype.call.bind(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 incomingEventName = eventName + "-" + url;
       function listener(event)
       {
         callback(event.detail);
         removeEventListener(incomingEventName, listener);
       }
       addEventListener(incomingEventName, listener);
 
       dispatchEvent(new CustomEvent(eventName, {
         detail: {url: url}
       }));
     }
 
-    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;
-
-    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()?

+    WebSocket = function WrappedWebSocket(url, protocols)
+    {
+      // Throw correct exceptions if the constructor is used improperly.
+      if (!(this instanceof WrappedWebSocket)) return RealWebSocket();
+      if (arguments.length < 1) return new RealWebSocket();
 
       var websocket = new RealWebSocket(url, protocols);
 
-      checkRequest(url, function(blocked)
+      checkRequest(websocket.url, function(blocked)
       {
         if (blocked)
-          boundCall(closeWebSocket, websocket);
+          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);
-    }
+    }.bind();
+
+    Object.defineProperties(WebSocket, {
+      CONNECTING: {value: RealWebSocket.CONNECTING, enumerable: true},
+      OPEN: {value: RealWebSocket.OPEN, enumerable: true},
+      CLOSING: {value: RealWebSocket.CLOSING, enumerable: true},
+      CLOSED: {value: RealWebSocket.CLOSED, enumerable: true},
+      prototype: {value: RealWebSocket.prototype}
+    });
 
     RealWebSocket.prototype.constructor = WebSocket;
-  }
-
-  injectJS(wrapper, eventName);
+  }, eventName);
 }
 
 function init(document)
 {
   var shadow = null;
   var style = null;
   var observer = null;
   var tracer = null;
 
   wrapWebSocket();
@@ skipping 161 matching lines @@
   }, true);
 
   return updateStylesheet;
 }
 
 if (document instanceof HTMLDocument)
 {
   checkSitekey();
   window.updateStylesheet = init(document);
 }