Closed Bug 1941780 Opened 1 year ago Closed 1 month ago

script.evaluate and script.callFunction should bypass CSP

Categories

(Remote Protocol :: WebDriver BiDi, defect, P2)

defect
Points:
5

Tracking

(firefox147 fixed)

RESOLVED FIXED
147 Branch
Tracking Status
firefox147 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [webdriver:m18][webdriver:relnote])

Attachments

(4 files, 2 obsolete files)

STRs:

  • start webdriver bidi session
  • open tab on page with CSP preventing eval (eg Content-Security-Policy: script-src 'none';)
  • use script.evaluate with expression = eval(1+1)

ER:
Should return 2
AR:
CSP exception is thrown

Blocks: 1770417
Blocks: 1770418

Points might be wrong, hard to know how complicated this will be on the platform side.

Severity: -- → S3
Points: --- → 3
Priority: -- → P2
Whiteboard: [webdriver:backlog]

Note that we have the same issue in DevTools at the moment: Bug 1580514

See Also: → 1580514

Probably a question for SpiderMonkey at this point.

Arai, hi! Do you know if eval/evalWithBindings can be used to evaluate code which bypasses CSPs defined by the content page?

Flags: needinfo?(arai.unmht)

I have 3 questions in order to understand the situation:

  • (a) Is this specific to eval() in the code?
  • (b) Can you provide more details around how to reproduce it, or the exact error and some more details?
  • (c) What's the relation between script.evaluate and Debugger's eval/evalWithBindings right now?

For (b), is it "EvalError: call to eval() blocked by CSP" error?
If that's the case, it's the error thrown by EvalKernel, and the callback used there is nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction.
So, if there's some special handling needed, either of the following will be a solution:

  • if the specific situation is already represented in the nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction's parameter, handle it accordingly there
  • otherwise, somehow add a parameter to the related codepath, and handle it in nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction or perhaps in EvalKernal.

For (a) and (c), is script.evaluate already using the Debugger API's eval/evalWithBindings? If so, it shouldn't be using EvalKernel directly, unless there's eval() call inside the script. So I would assume this issue is specific to eval() being inside the code?

Flags: needinfo?(arai.unmht) → needinfo?(jdescottes)

Thanks Arai!

(a) Is this specific to eval() in the code?

Sorry I should have given more context. So WebDriver BiDi provides two commands to automation clients to evaluate JS in a content page: script.evaluate and script.callFunction. They slightly differ in terms of semantics for the end user, but otherwise the underlying implementation is very similar. And it's also very similar to evaluating code with the DevTools' Webconsole.

Those commands use either executeInGlobal or executeInGlobalWithBindings (and not eval/evalWithBindings, not sure why I had those names in mind) on the debugger global (created via makeGlobalObjectReference).

The use case here is that a client uses script.evaluate to execute JS which contains eval or new Function. And if the page is defining a CSP preventing this, calling executeInGlobal with code containing eval/new Function will indeed throw with "EvalError: call to eval() blocked by CSP". So yes, it's not an issue overall when using script.evaluate on a page with CSPs, it's only triggered if the code we try to run contains eval/new Function.

I could provide a BiDi test case, but the easiest way to reproduce the same issue is with the DevTools webconsole:

Does that change something for the suggestions you provided already?

Flags: needinfo?(jdescottes) → needinfo?(arai.unmht)

Thank you for the details!
It makes sense now.

And I think the suggested way would be the same as above. Modify either the CSP's hook or the eval handling, and somehow use the information about Debugger API being used, and bypass the CSP restriction in that situation.

If the information is not yet directly there (that's what I can see for now), the possible options would be something like the following:

  • use the enclosing frame information (the caller parameter below) to determine if it's inside Debugger API's execution
    • this would work if we can somehow mark the outer-most frame as Debugger API's execution. but this won't work in promise reactions etc, given the frame won't retain the initial one
  • add a new flag about the Debugger API execution, either only in SpiderMonkey side or both in SpiderMonkey and Gecko side, where the flag is propagated through promises in the same way as JS::PromiseUserInputEventHandlingState, and refer it from the CSP handling to by pass the check

We should be careful not to leak the "bypass the CSP check" mode to webpage's code, and the promise handling part might be too risky unless there's demand for that usage.

https://searchfox.org/mozilla-central/rev/345ec3c55ddda5f0ce37168f0644dcdcc4834409/js/src/builtin/Eval.cpp#233-235

static bool EvalKernel(JSContext* cx, HandleValue v, EvalType evalType,
                       AbstractFramePtr caller, HandleObject env,
                       jsbytecode* pc, MutableHandleValue vp) {

I'll see if there's some other solution, or some flag or execution mode (if any) which can be used or used as a reference.

Flags: needinfo?(arai.unmht)
Duplicate of this bug: 1650112

WebExtension content scripts are also exempted from the page CSP, to achieve this they currently use ChromeUtils.compileScript and script.executeInGlobal. It would be good to share some code here, so that it's easier to audit the places that might bypass CSPs.

Fixing this bug will let us enforce a CSP for the files in chrome://remote/content/marionette/. These are currently exempt: https://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityUtils.cpp.

This is important because we can disable the other testing exceptions outside of tests but the marionette tests don't have the automation flag xpc::IsInAutomation() set.

Just a quick note: a fix here won’t immediately resolve issues with chrome://remote/content/marionette/ files, but it would demonstrate how we could address it for Marionette as well. An alternative approach would be to move those files into an extension, as suggested in bug 1344267, rather than bundling them with the Firefox release build.

Nevertheless WebDriver BiDi will likely face a similar situation soon, once we support evaluating scripts in the parent process, we’ll also need a way to serve test files in that context.

Lets discuss this bug in our next triage meeting.

Whiteboard: [webdriver:backlog] → [webdriver:backlog][webdriver:triage]

We are going to try to get bug 1344267 fixed so that we are not shipping these files.

But nevertheless it would be good to know how to reproduce the problem with those chrome URLs. Simon, do you have some steps that we could run locally to see those CSP assertions? Thanks.

Flags: needinfo?(sfriedberger)

Add a CSP to the test files and it will fail. Here is the patch I would like to apply to enforce a CSP: https://phabricator.services.mozilla.com/D247045

Here is an example try run: https://treeherder.mozilla.org/jobs?repo=try&revision=2f0a0f8b2ea89d9efeb00d676bc8469eb507dcd4 have a look at "unit".

Flags: needinfo?(sfriedberger)

Thanks, Simon. One last question though:

If we were to package those XHTML files within an add-on, similar to how Mochitest does, would we still need to apply the CSP changes to them?

Also does it mean we’re enforcing CSP even in the chrome scope? It most likely means that we would also fail with script evaluation for certain JS code unless this bug is fixed, right?

Flags: needinfo?(sfriedberger)

We are working on enforcing CSPs in all chrome and about pages. We're almost finished though, so if it isn't failing yet, maybe you don't have a problem.

Flags: needinfo?(sfriedberger)
See Also: → 1964043
Whiteboard: [webdriver:backlog][webdriver:triage] → [webdriver:backlog]
No longer blocks: 1963356

Fix should be straightforward, let's table it for m18.

Whiteboard: [webdriver:backlog] → [webdriver:m18]
Points: 3 → 5
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

Starting to look at this, it seems the async scenario is not handled by Chrome. Let's discuss in https://github.com/w3c/webdriver-bidi/issues/1024, but I'm going to assume we can focus on handling sync scenarios only for now.

Simon, we are working on allowing to bypass CSPs when calling scripts via WebDriver script evaluation commands.

I know this is already something already allowed for WebExtensions, and I need to look at how it's handled/implemented there. From a security perspective do you have an opinion on how this bypass should behave in scenarios more complicated than merely calling eval/new Function from a WebDriver BiDi evaluated script. The ones we identified so far are:

  • calling eval asynchronously (which implies the eval is scheduled by the content page, even if the script comes from BiDi)
  • nested evals
  • calling other scripts which contain eval/new Function (either scripts coming from other WebDriverBiDi commands, or from the content page itself)

I wrote some tentative test cases at https://github.com/web-platform-tests/wpt/pull/55853 if that helps to get more context.

Flags: needinfo?(sfriedberger)
See Also: → 1591983, 1731913

In WebExtensions, the relevant logic to execute code in the web page's context despite CSP was added in bug 1736575 (with source code/filename redaction implemented in bug 1900410), and the relevant fragments live at:

FYI, not super critical, but note that there is currently a recurring debug-only crash (bug 1940852) because the underlying AsyncScriptCompiler needs to be hooked up to GC in a better way.

Just to make sure I didn't misunderstand something: WebDriver BiDi is purely for testing and the testing harness is trusted, right? So it's not like we actually have to be concerned about attacks here, correct?

(In reply to Simon Friedberger [:simonf] from comment #22)

Just to make sure I didn't misunderstand something: WebDriver BiDi is purely for testing and the testing harness is trusted, right? So it's not like we actually have to be concerned about attacks here, correct?

Correct, BiDi is purely for driving the browser remotely for test automation, it's only enabled if a command line argument is used when starting Firefox. The question is more about what would be the most sensible from a user perspective.

Thanks for the pointers about the webextensions implementation Rob, I'm wondering if we can rely on CompileScript here, given we also use executeInGlobalWithBindings in order to call functions with parameters, which feels orthogonal to pre compiling scripts. I'll modify the tests a bit to see how this behaves in the edge cases around nested eval, async eval, calling eval from other scripts etc.

In general the code that WebExtensions are using looks good. As Tom mentioned above, please make sure to share this code. I've also added a comment to your tests on GitHub. As written I think people would not be able to fully test their CSP because we might be more permissive in the WebDriver case than when actually using the page.

Flags: needinfo?(sfriedberger)

(In reply to Simon Friedberger [:simonf] from comment #24)

In general the code that WebExtensions are using looks good. As Tom mentioned above, please make sure to share this code.

I feel like using CompileScript is not something which can apply to us, especially for the scenarios where we need to use executeInGlobalWithBindings.

Also looking at the test cases added for WebExtensions in https://phabricator.services.mozilla.com/D211869, it seems that we are not talking about the same thing? Our goal here is to be able to use eval and new Function in scripts evaluated via WebDriver BiDi, regardless of the page CSP. And the tests added for webextensions are explicitly checking that the page CSP still applies to user scripts from webextensions. So I think on the webextension's side, the only thing which is supported is to be able to evaluate scripts in WORLD=MAIN regardless of CSPs, but not to use eval/new Function regardless of CSP.

rob: can you confirm? Are you actually able to use eval and new Function in webextension user scripts regardless of the page CSP? I think https://searchfox.org/firefox-main/source/toolkit/components/extensions/test/mochitest/test_ext_scripting_executeScript_world.html#1 shows the opposite.

I've also added a comment to your tests on GitHub. As written I think people would not be able to fully test their CSP because we might be more permissive in the WebDriver case than when actually using the page.

Thanks for the comment! As commented there, the tests are here to illustrate the various scenarios where we need to take a decision about bypassing CSP or not. I don't want all of them to pass ultimately. In particular the one you highlighted on should fail IMO, but passes on Chrome at the moment :(

Flags: needinfo?(rob)

(In reply to Julian Descottes [:jdescottes] from comment #25)

rob: can you confirm? Are you actually able to use eval and new Function in webextension user scripts regardless of the page CSP? I think https://searchfox.org/firefox-main/source/toolkit/components/extensions/test/mochitest/test_ext_scripting_executeScript_world.html#1 shows the opposite.

In WebExtensions, eval and such cannot directly be used to bypass CSP. We intentionally restrict string-based code execution to the extension framework (using compileScript for the lack of anything better), in order to not inadvertently degrade the security capabilities of the web page. Code running in the same execution environment cannot be trusted, it is nearly impossible to effectively enforce the CSP when there is no isolation.

E.g. even if you were inclined to only check the caller, then a hostile web page could override any method used by the "trusted" script, and point it to eval instead. The trusted caller could then be tricked into calling eval. For example:

// Untrusted:
console.log = eval;
String.prototype.toLowerCase = () => "alert('Hahaha')";

// Fooling the "trusted" script:
console.log("UUU".toLowerCase());
Flags: needinfo?(rob)

Alright, thanks. So that's different from what we are trying to achieve here.

Based on https://github.com/w3c/webextensions/blob/main/proposals/user-scripts-api.md#:~:text=Exempt%20from%20the%20page%27s%20CSP%2C I thought that meant user scripts are meant to run in the context of the page but still be exempt from CSP, but maybe I'm missing some nuance. And anyway we don't seem to be implementing this at the moment.

Looking at the current usage in Playwright, it seems important to support nested use cases (although it's difficult to get clear feedback on their requirements).

So that means I should probably go forward with a flag propagated with promises.

I added a new isDebuggerExecution field in JSContext in order to keep track of the fact that we are in a debugger evaluation.
Then in Frame.cpp EvaluateInEnv I set the field to true.
When a promise is created I read the field from the context and I set a new promise flag, which I propagate in the same spots as JS::PromiseUserInputEventHandlingState. At this point however I'm not sure how to propagate my JSContext field to the ... new JSContext corresponding to the new promise (assuming there's a new JSContext at each new async step?). The implementation seems to work fine for all sync + nested cases, but fails as soon as there is any async step.

I will try to cleanup my patch and upload it, might be easier to understand.

Flags: needinfo?(arai.unmht)

(In reply to Julian Descottes [:jdescottes] from comment #28)

When a promise is created I read the field from the context and I set a new promise flag, which I propagate in the same spots as JS::PromiseUserInputEventHandlingState. At this point however I'm not sure how to propagate my JSContext field to the ... new JSContext corresponding to the new promise (assuming there's a new JSContext at each new async step?). The implementation seems to work fine for all sync + nested cases, but fails as soon as there is any async step.

There is only one JSContext for things where the flag needs to be propagated.

You need to flip the flag on and off depending on what you're executing.

So, if you're executing a code from debugger, flip the JSContext's flag on, and bypass the CSP, and also propagate the flag to the promise reactions and any tasks created by there, and flip the flag off after executing such code.
if you're executing a promise reaction jobs with the flag, flip the JSContext's flag on, and do the same as above.
if you're executing a task (e.g. setTimeout task), flip the JSContext's flag on, and do the same as above.
For all other cases, you should keep the JSContext's flag turned off.

Flags: needinfo?(arai.unmht)

I didn't check the patch but saw the comment with the patch title, so I have a question: is the patch above restricting the CSP bypass to the specific use case only, and not applying it by default to other DevTools clients? Bypassing CSP by default is concerning and should be limited to only the cases that really need it. In particular I would not want this by default for WebExtensions (browser.devtools.inspectedWindow.eval), and it would also be unrealistic when used from the DevTools console.

The WebDriver BiDi spec draft doesn't mention anything about CSP right now. Is there cross browser consensus that the CSP should be bypassed by default? (See below for another CSP bypass option that is opt in)

The CDP's Runtime.evaluate method appears to have an allowUnsafeEvalBlockedByCSP option that defaults to true. If the consensus is to replicate this behavior, it would be nice to also have a similar option in WebDriver, at least as an opt out. I guess that disabling the CSP by default is convenient, but it unfortunately also translates to potential abuse, even when the evaluate() call itself does not directly intend to use eval(). If I were to design the API from scratch, I would make it opt in, but I understand that this may not be feasible for backwards compatibility reasons in Chrome.


Since you mentioned Playwright, I took a look and also found that there is also a bypassCSP option, but that applies to the Page, not individual script evaluation calls: https://playwright.dev/docs/api/class-browser#browser-new-context-option-bypass-csp

I also see that there is a Page.setBypassCSP CDP method that presumably backs that option: https://chromedevtools.github.io/devtools-protocol/tot/Page/#method-setBypassCSP

I checked the implementation of that option in Chromium, and what it does is ignoring CSP headers and meta tags for document loads associated with the Page instance.

Flags: needinfo?(jdescottes)

(In reply to Rob Wu [:robwu] from comment #31)

I didn't check the patch but saw the comment with the patch title, so I have a question: is the patch above restricting the CSP bypass to the specific use case only, and not applying it by default to other DevTools clients? Bypassing CSP by default is concerning and should be limited to only the cases that really need it. In particular I would not want this by default for WebExtensions (browser.devtools.inspectedWindow.eval), and it would also be unrealistic when used from the DevTools console.

Chrome script evaluation (from DevTools or CDP in general) bypasses CSPs because allowUnsafeEvalBlockedByCSP defauls to true, as you mentioned below. My goal here is to align us with Chrome where it makes sense, because this is considered a blocker for Playwright adoption of WebDriver BiDi, and it has been a long standing issue in DevTools.

It should be easy to make this fully optional, and only triggered by WebDriver BiDi script evaluation if there are concerns for other APIs.

it would also be unrealistic when used from the DevTools console.

We've had several bug reports over the years asking for this as chrome parity, eg Bug 1580514. I don't mind enabling it only for BiDi, but DevTools and Security team (comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1731913#c2) didn't express concerns here.

I would not want this by default for WebExtensions (browser.devtools.inspectedWindow.eval)

I didn't think about this entry point. Happy to restrict my patch to only impact BiDi. Out of curiosity did you check how Chrome works there? Since they seem to consistently bypass CSP in all cases I wouldn't be surprised if they do it in this case too.

IMO this API should behave the same as using the DevTools console, so if we don't enable it for webextensions, then we probably shouldn't enable it for devtools either. We can leave this discussion to a followup.

The WebDriver BiDi spec draft doesn't mention anything about CSP right now. Is there cross browser consensus that the CSP should be bypassed by default? (See below for another CSP bypass option that is opt in)

Since this is Chrome's default behavior, tools built on CDP have been built assuming this was the case yes.

The CDP's Runtime.evaluate method appears to have an allowUnsafeEvalBlockedByCSP option that defaults to true. If the consensus is to replicate this behavior, it would be nice to also have a similar option in WebDriver, at least as an opt out. I guess that disabling the CSP by default is convenient, but it unfortunately also translates to potential abuse, even when the evaluate() call itself does not directly intend to use eval(). If I were to design the API from scratch, I would make it opt in, but I understand that this may not be feasible for backwards compatibility reasons in Chrome.

We can have a spec discussion to add a flag to the command.


Since you mentioned Playwright, I took a look and also found that there is also a bypassCSP option, but that applies to the Page, not individual script evaluation calls: https://playwright.dev/docs/api/class-browser#browser-new-context-option-bypass-csp

I also see that there is a Page.setBypassCSP CDP method that presumably backs that option: https://chromedevtools.github.io/devtools-protocol/tot/Page/#method-setBypassCSP

I checked the implementation of that option in Chromium, and what it does is ignoring CSP headers and meta tags for document loads associated with the Page instance.

This is one of the issues I'm trying to get feedback on (https://github.com/w3c/webdriver-bidi/issues/1033). Note that setBypassCSP has no impact on script evaluation in Playwright because 1/ Chrome bypasses CSPs by default as discussed, 2/ they patched their Firefox version to bypass CSP unconditionally (in a limited way, but which covers 100% of their use case): https://github.com/microsoft/playwright/blob/d92591a9d79a0ff67241923ceb041b880d765909/browser_patches/firefox/patches/bootstrap.diff#L2013

The bypassCSP command allows to fully ignore CSPs for a given user context, but we don't have this in the spec for BiDi at the moment. The current bug should only be about script evaluation from WebDriver BiDi.

Flags: needinfo?(jdescottes)

(In reply to Julian Descottes [:jdescottes] from comment #32)

(In reply to Rob Wu [:robwu] from comment #31)

it would also be unrealistic when used from the DevTools console.

We've had several bug reports over the years asking for this as chrome parity, eg Bug 1580514. I don't mind enabling it only for BiDi, but DevTools and Security team (comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1731913#c2) didn't express concerns here.

My concern is also expressed in the comment you linked: "If commands issued on the console command-line behave differently than when used in the page itself it will make experimentation and debugging very frustrating."

Chrome also recognized this issue at some point, which is why they now have the allowUnsafeEvalBlockedByCSP option (interestingly, to evaluate only, but not in callFunction). This was added in this commit associated with crbug 40693194. From the discussion there, the initial inclination was to fully match Firefox's (strict) behavior, but that direction was not pursued for Runtime.evaluate due to implementation issues that would arise from dropping the relaxation. I don't see any evidence of the intent for the relaxation to apply transitively to web page-defined code that calls into eval. The code change there still lives on today, and opts out of CSP bypassing behavior in code run from the devtools console.

I just tested calling eval("1") from the DevTools console in Chrome 142, and it blocks eval if the page has a CSP that restricts unsafe-eval.

I didn't think about this entry point. Happy to restrict my patch to only impact BiDi. Out of curiosity did you check how Chrome works there? Since they seem to consistently bypass CSP in all cases I wouldn't be surprised if they do it in this case too.

I haven't tested the devtools.inspectedWindow.eval API, but it looks like they transform the API call to a Runtime.evaluate command without setting allowUnsafeEvalBlockedByCSP: https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/panels/common/ExtensionServer.ts;l=1574;drc=78bd63326c0a2489e6b016140aa4d84014af077a

I have not seen any feature request on BMO from devtools extension developers asking for the ability to bypass CSP in their eval calls specifically. Consistency between the devtools UI and thedevtools.inspectedWindow.eval behavior is desirable. Given the lack of demand for eval bypass, and the fact that the devtools respects CSP in Chrome and Firefox, I'm recommending to also be strict in Firefox.

I wondered whether this is a Chrome-only thing. Although Safari does not support BiDi yet, it does have a DevTools UI that is likely going to be a good indicator of expected behavior. Surprisingly, Safari allows the devtools console to bypass CSP:

data:text/html,<meta http-equiv=content-security-policy content="default-src 'unsafe-inline'"><script>function isEvalBlocked() { try { return eval("false"); } catch { return true; } }function test(){console.log(isEvalBlocked());}test()</script>See console (expect true). Run test() for comparison

code snippet that checks eval (from console, unless stated otherwise) is eval blocked in Safari?
test() by web page true
test() false
setTimeout(test) true
Promise.resolve().then(test) false
queueMicrotask(test) false
document.body.setAttribute("onclick", "(" + test + ")()");document.body.click() false

I would not want this by default for WebExtensions (browser.devtools.inspectedWindow.eval)

IMO this API should behave the same as using the DevTools console, so if we don't enable it for webextensions, then we probably shouldn't enable it for devtools either. We can leave this discussion to a followup.

Do you want to enable it first and then have a discussion on whether to revert, or do you want to preserve the current behavior and then have a discussion on whether to enable, e.g. in one of the bugs you mentioned?

It is easier to lift restrictions later, than to start with being too permissive first and then having to restrict later if it turns out that we are over-permissive. That is why I am asking these questions now; if other browser vendors are also aligned on working towards this being opt-in, then now would be the greatest time to do so.

Thanks for checking Chrome DevTools again, I assumed they were still bypassing CSPs and didn't test recently (not in the last 5 years at least). Interesting to see they switched gears and now behave like Firefox.

Do you want to enable it first and then have a discussion on whether to revert, or do you want to preserve the current behavior and then have a discussion on whether to enable, e.g. in one of the bugs you mentioned?

I will only enable this for BiDi and on the BiDi side, I think I agree we better make it optional. I don't want to block progress on spec discussions here, so I think I will rather rely on a mozilla only flag for now.

Duplicate of this bug: 2001377
Attachment #9524205 - Attachment is obsolete: true
Attachment #9524203 - Attachment is obsolete: true
Attachment #9527754 - Attachment description: Bug 1941780 - Bypass CSP restrictions for code evaluated from the debugger API → Bug 1941780 - Implement bypassCSP option for Frame.eval
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 147 Branch
Whiteboard: [webdriver:m18] → [webdriver:m18][webdriver:relnote]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: