Issue 29829569: Issue 6538, 6781 - Add code injection wrapper to snippets library (Closed)
|
Created:
July 13, 2018, 2:09 p.m. by Manish Jethani Modified:
July 18, 2018, 12:09 p.m. Reviewers:
kzar CC:
Sebastian Noack Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionThere will be at least two kinds of snippets: those that run in the content script context and those that run in the document context. For the latter kind, we need some common functions that make it easy to export a snippet with a wrapper that injects it into the document.
The makeInjector function does this.
exports.trace = makeInjector(trace, log);
Here the "trace" snippet is injected and executed in the document context. It depends on the log function, so we add that as a dependency as well.
Patch Set 1 #Patch Set 2 : Add trace snippet #
Total comments: 18
Patch Set 3 : Add JSDoc comments for injectCode, stringifyFunctionCall, and makeInjector #Patch Set 4 : Add JSDoc comments for log and trace snippets #
Total comments: 8
Patch Set 5 : Use Function.prototype.toString for safety #Patch Set 6 : Remove "safely" from comment, remove unnecessary checks #MessagesTotal messages: 12
Patch Set 1 Patch Set 2: Add trace snippet See description. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:69: function trace(...args) I had to do this, both because it helps to have an example but mainly also because otherwise ESLint complains that makeInjector is unused.
https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:56: function makeInjector(injectable, ...dependencies) Please could you add a JSDoc comment for this function too? https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:58: return (...args) => injectCode(stringifyFunctionCall(injectable, ...args), I'd prefer we did the work of `stringifyFunctionCall` here, and made the code a bit easier to grok whilst we're at it. Something like this (untested): return (...args) => { injectCode(`"use strict"; ($(injectable)(...${JSON.stringify(args)}));`, dependencies); }; Also, perhaps injecting the "use strict" without a surrounding function is a bad idea. Unless I'm mistaken that will enable strict mode for the entire document. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:67: exports.log = log; Why not just export `console.log`? I don't think it even needs to be bound. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:69: function trace(...args) On 2018/07/13 14:17:33, Manish Jethani wrote: > I had to do this, both because it helps to have an example but mainly also > because otherwise ESLint complains that makeInjector is unused. Please could you add a JSDoc comment explaining that this is an example and what it does? https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:71: log(...args); Why not call `console.log(...args);` here directly?
https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:56: function makeInjector(injectable, ...dependencies) On 2018/07/16 14:13:56, kzar wrote: > Please could you add a JSDoc comment for this function too? Will do. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:58: return (...args) => injectCode(stringifyFunctionCall(injectable, ...args), On 2018/07/16 14:13:56, kzar wrote: > I'd prefer we did the work of `stringifyFunctionCall` here, and made the code a > bit easier to grok whilst we're at it. I would really prefer if stringifyFunctionCall is a separate function. It performs a well defined task, which I can document. We are going to have to reuse the function elsewhere, mostly likely, and in any case if it's well defined and generic it's better to have it as a separate function. [...] > Also, perhaps injecting the "use strict" without a surrounding function is a bad > idea. Unless I'm mistaken that will enable strict mode for the entire document. No, it will only enable strict mode for the script element, and that's what we want. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:67: exports.log = log; On 2018/07/16 14:13:56, kzar wrote: > Why not just export `console.log`? I don't think it even needs to be bound. Because console.log.toString() is "function log() { [native code] }" which is not executable. The snippet's string form should be executable JS code. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:69: function trace(...args) On 2018/07/16 14:13:57, kzar wrote: > On 2018/07/13 14:17:33, Manish Jethani wrote: > > I had to do this, both because it helps to have an example but mainly also > > because otherwise ESLint complains that makeInjector is unused. > > Please could you add a JSDoc comment explaining that this is an example and what > it does? Will do. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:71: log(...args); On 2018/07/16 14:13:57, kzar wrote: > Why not call `console.log(...args);` here directly? Because the goal is to demonstrate the use of the dependencies argument to makeInjector. Here log is a dependency of trace. It is deliberately contrived, to be used as an example. I will add some documentation to this patch.
https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:58: return (...args) => injectCode(stringifyFunctionCall(injectable, ...args), On 2018/07/17 10:44:41, Manish Jethani wrote: > I would really prefer if stringifyFunctionCall is a separate function. It > performs a well defined task, which I can document. We are going to have to > reuse the function elsewhere, mostly likely, and in any case if it's well > defined and generic it's better to have it as a separate function. Alright, but what about my comment about making it easier to read? return `"use strict"; ($(injectable)(...${JSON.stringify(args)}));`; VS return `"use strict";(${func})(${params.map(JSON.stringify).join(",")});`; Well, up to you anyway. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:67: exports.log = log; On 2018/07/17 10:44:41, Manish Jethani wrote: > On 2018/07/16 14:13:56, kzar wrote: > > Why not just export `console.log`? I don't think it even needs to be bound. > > Because console.log.toString() is "function log() { [native code] }" which is > not executable. The snippet's string form should be executable JS code. Good point. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:71: log(...args); On 2018/07/17 10:44:42, Manish Jethani wrote: > On 2018/07/16 14:13:57, kzar wrote: > > Why not call `console.log(...args);` here directly? > > Because the goal is to demonstrate the use of the dependencies argument to > makeInjector. Here log is a dependency of trace. It is deliberately contrived, > to be used as an example. > > I will add some documentation to this patch. Acknowledged.
Patch Set 3: Add JSDoc comments for injectCode, stringifyFunctionCall, and makeInjector Patch Set 4: Add JSDoc comments for log and trace snippets https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:56: function makeInjector(injectable, ...dependencies) On 2018/07/17 10:44:42, Manish Jethani wrote: > On 2018/07/16 14:13:56, kzar wrote: > > Please could you add a JSDoc comment for this function too? > > Will do. Done. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:58: return (...args) => injectCode(stringifyFunctionCall(injectable, ...args), On 2018/07/17 11:11:12, kzar wrote: > On 2018/07/17 10:44:41, Manish Jethani wrote: > > I would really prefer if stringifyFunctionCall is a separate function. It > > performs a well defined task, which I can document. We are going to have to > > reuse the function elsewhere, mostly likely, and in any case if it's well > > defined and generic it's better to have it as a separate function. > > Alright, but what about my comment about making it easier to read? > > return `"use strict"; > ($(injectable)(...${JSON.stringify(args)}));`; > > VS > > return `"use strict";(${func})(${params.map(JSON.stringify).join(",")});`; > > Well, up to you anyway. That would inject code like: foo(...["x", "y", "z"]) Rather than: foo("x", "y", "z") I think I would prefer that the injected code is cleaner so it's easier to debug and find problems, even if the code that injects it is a little bit more complex. https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:69: function trace(...args) On 2018/07/17 10:44:41, Manish Jethani wrote: > On 2018/07/16 14:13:57, kzar wrote: > > On 2018/07/13 14:17:33, Manish Jethani wrote: > > > I had to do this, both because it helps to have an example but mainly also > > > because otherwise ESLint complains that makeInjector is unused. > > > > Please could you add a JSDoc comment explaining that this is an example and > what > > it does? > > Will do. Done.
Otherwise LGTM https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29829572/lib/content/snippet... lib/content/snippets.js:58: return (...args) => injectCode(stringifyFunctionCall(injectable, ...args), On 2018/07/17 15:42:36, Manish Jethani wrote: > On 2018/07/17 11:11:12, kzar wrote: > > On 2018/07/17 10:44:41, Manish Jethani wrote: > > > I would really prefer if stringifyFunctionCall is a separate function. It > > > performs a well defined task, which I can document. We are going to have to > > > reuse the function elsewhere, mostly likely, and in any case if it's well > > > defined and generic it's better to have it as a separate function. > > > > Alright, but what about my comment about making it easier to read? > > > > return `"use strict"; > > ($(injectable)(...${JSON.stringify(args)}));`; > > > > VS > > > > return `"use strict";(${func})(${params.map(JSON.stringify).join(",")});`; > > > > Well, up to you anyway. > > That would inject code like: > > foo(...["x", "y", "z"]) > > Rather than: > > foo("x", "y", "z") > > I think I would prefer that the injected code is cleaner so it's easier to debug > and find problems, even if the code that injects it is a little bit more > complex. Acknowledged. https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... lib/content/snippets.js:61: * Safely converts a function and an optional list of arguments into a string ("Safely" seems a bit optimistic, when we're talking about evaling code.) https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... lib/content/snippets.js:79: * called with zero or more arguments, generates code that calls the function, Nit: "with zero or more arguments" can be removed, since you mention "if any" later.
Patch Set 5: Use Function.prototype.toString for safety https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... lib/content/snippets.js:61: * Safely converts a function and an optional list of arguments into a string On 2018/07/17 16:08:22, kzar wrote: > ("Safely" seems a bit optimistic, when we're talking about evaling code.) 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. https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... lib/content/snippets.js:79: * called with zero or more arguments, generates code that calls the function, On 2018/07/17 16:08:22, kzar wrote: > Nit: "with zero or more arguments" can be removed, since you mention "if any" > later. Acknowledged. I'll leave it as it is though, if you really don't mind.
https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... lib/content/snippets.js:61: * Safely converts a function and an optional list of arguments into a string On 2018/07/17 16:39:33, Manish Jethani wrote: > On 2018/07/17 16:08:22, kzar wrote: > > ("Safely" seems a bit optimistic, when we're talking about evaling code.) > > 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. 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`). https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... lib/content/snippets.js:79: * called with zero or more arguments, generates code that calls the function, On 2018/07/17 16:39:33, Manish Jethani wrote: > On 2018/07/17 16:08:22, kzar wrote: > > Nit: "with zero or more arguments" can be removed, since you mention "if any" > > later. > > Acknowledged. > > I'll leave it as it is though, if you really don't mind. OK.
https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... lib/content/snippets.js:61: * Safely converts a function and an optional list of arguments into a string On 2018/07/17 16:59:19, kzar wrote: > On 2018/07/17 16:39:33, Manish Jethani wrote: > > On 2018/07/17 16:08:22, kzar wrote: > > > ("Safely" seems a bit optimistic, when we're talking about evaling code.) > > > > 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. > > 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`). 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.
Patch Set 6: Remove "safely" from comment, remove unnecessary checks OK, I have come around. Let's keep it simple.
> OK, I have come around. Let's keep it simple. Makes sense, LGTM. https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29829569/diff/29832569/lib/content/snippet... lib/content/snippets.js:61: * Safely converts a function and an optional list of arguments into a string On 2018/07/17 17:27:02, Manish Jethani wrote: > On 2018/07/17 16:59:19, kzar wrote: > > 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`). > > 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. 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. 
|
