Issue 6538, 6781 - Add code injection wrapper to snippets library

lib/content/snippets.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 40 matching lines @@
     let url = URL.createObjectURL(new Blob([code]));
     script.src = url;
     document.documentElement.appendChild(script);
     URL.revokeObjectURL(url);
   }
 
   document.documentElement.removeChild(script);
 }
 
 /**
- * Safely converts a function and an optional list of arguments into a string

[kzar, 2018/07/17 16:08:22]

("Safely" seems a bit optimistic, when we're talking about evaling code.)

[Manish Jethani, 2018/07/17 16:39:33]

I really want it to be safe, but thanks to your comment I realized it wasn't
fully safe.

Consider this:

  function foo() {}
  foo.toString = function ()
  {
    // do something malicious
  };

Now when we do this:

  stringifyFunctionCall(foo);

It executes the malicious JS in the content script's context, because we are
calling toString directly.

The function foo could still contain malicious JS, but it would get executed in
the document's context, which at least has lower privileges (e.g. cannot send
messages to the background page!).

So now I have updated the patch to do this safely.

"Safe" here means that it won't execute any arbitrary code in the extension.

The safety is only guaranteed for the execution of the function. The function
returns code of course, which could be unsafe to execute, but that's up to the
caller. Only the generation of the code is safe, not its execution.

[kzar, 2018/07/17 16:59:19]

I guess I don't understand if they are able to mess with `func.toString` how
they're not also able to mess with `Function.prototype.toString` (or other
things like `Array.prototype.map`).

[Manish Jethani, 2018/07/17 17:27:02]

You're right, at this point I am merely being overcautious and designing for
some unknown future. Do you think it's overdone? I don't have a strong opinion,
I can roll it back.

But I would still leave the "safely" part in the comment; one of the reasons we
call JSON.stringify is that, at least for the arguments (which come from outside
this module), we want to avoid the possibility of (accidental) arbitrary code
execution.

The problem usually is not intentionally malicious code but rather some
developer who doesn't know what they're doing.

[kzar, 2018/07/18 07:40:08]

Well, I'm all for being cautious, but if we leave other weaknesses there it
seems kind of pointless. From my experience with the other wrappers, hardening
against this kind of stuff properly is a shitload of work too.

- * of code containing a function call. The function is converted to its string
+ * Converts a function and an optional list of arguments into a string of code
+ * containing a function call. The function is converted to its string
  * representation using the <code>Function.prototype.toString</code> method.
  * Each argument is stringified using <code>JSON.stringify</code>. The
  * generated code begins with the <code>"use strict"</code> directive.
  *
  * @param {function} func The function to convert.
  * @param {...*} [params] The arguments to convert.
  *
  * @returns {string} The generated code containing the function call.
  */
 function stringifyFunctionCall(func, ...params)
 {
+  // Call JSON.stringify on the arguments to avoid any arbitrary code
+  // execution.
   return `"use strict";(${func})(${params.map(JSON.stringify).join(",")});`;
 }
 
 /**
  * Wraps a function and its dependencies into an injector. The injector, when
  * called with zero or more arguments, generates code that calls the function,

[kzar, 2018/07/17 16:08:22]

Nit: "with zero or more arguments" can be removed, since you mention "if any"
later.

[Manish Jethani, 2018/07/17 16:39:33]

Acknowledged.

I'll leave it as it is though, if you really don't mind.

[kzar, 2018/07/17 16:59:19]

OK.

  * with the given arguments, if any, and injects the code, along with any
  * dependencies, into the document using a temporary <code>script</code>
  * element.
  *
  * @param {function} injectable The function to wrap into an injector.
  * @param {...(function|string)} [dependencies] Any dependencies of the
  *   function. A dependency may be either a function or a string containing
  *   some executable code.
  *
  * @returns {function} The generated injector.
@@ skipping 26 matching lines @@
  * @param {...*} [args] The arguments to log.
  */
 function trace(...args)
 {
   // We could simply use console.log here, but the goal is to demonstrate the
   // usage of snippet dependencies.
   log(...args);
 }
 
 exports.trace = makeInjector(trace, log);