Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move template engine and brick evaluation to Chrome sandbox #105

Closed
Tracked by #4387
twschiller opened this issue Nov 5, 2020 · 8 comments · Fixed by #4701
Closed
Tracked by #4387

Move template engine and brick evaluation to Chrome sandbox #105

twschiller opened this issue Nov 5, 2020 · 8 comments · Fixed by #4701
Assignees
Labels

Comments

@twschiller
Copy link
Contributor

twschiller commented Nov 5, 2020

@twschiller twschiller changed the title Move template engine and brick evaluation to sandbox Move template engine and brick evaluation to Chrome sandbox Mar 16, 2021
@fregante
Copy link
Contributor

fregante commented Jun 6, 2021

I think you're suggesting to run that code in the sandbox in Chrome and as a userScript in Firefox. I'll look into how these two can be commanded through the same interface, but it's possible that we might need/want to stabilize our messaging API first.

Related: #430

There are a couple of things that will make a little awkward:

  • userScripts need to be registered via background page, but they will run on the current page (thankfully there's an event that we receive in the content script, in which we can provide the messaging API)
  • the sandbox needs to be loaded via iframe, it seems, hopefully that won't be a problem on sites with strict CSP. If it is, we might have to run it in the background page itself, adding an extra messaging step (and it isn't possible in MV3 Identify gaps preventing migration to Chrome Manifest V3 #287)

@fregante
Copy link
Contributor

fregante commented Jun 6, 2021

I'm not yet familiar with the capabilities of bricks, but the sandbox won't have access to the webpage at all (unlike the user scripts), would that be a problem? Would it require bricks to run in 2 places at once?

Given that TamperMonkey is already able to inject user scripts, I wonder if I can make a browser.userScript.register polyfill of sorts in order to avoid the wildly-different sandbox/userScript APIs.

Edit: From a quick look, it appears that TamperMonkey injects the user script with a plain script, so that's not enough.


Related links with possibly more details/context:

Scripting in chrome, especially in MV3: https://groups.google.com/a/chromium.org/g/chromium-extensions/c/q9H8KwFLkMs

SO question with links related to communication between user scripts and content scripts:
https://stackoverflow.com/questions/63814566/can-my-tampermonkey-script-or-other-userscript-call-my-chrome-extension

@twschiller
Copy link
Contributor Author

twschiller commented Jun 6, 2021

Thanks for starting to look into this! This issue isn't urgent, as we're still in a world where people are creating for themselves or running bricks made by their company's development team.

I need to write up a wiki page on formally detailing the threat/security model so we can determine best way to leverage sandbox. The main potential attack vectors from a malicious brick creator would be:

  1. Prototype pollution
  2. XSS
  3. Data exfiltration
  4. Privilege escalation (e.g., by someone running methods on background page, or in the site's JS context)

For each, the surface area pretty well-defined because we don't support arbitrary JS in bricks. Therefore, each brick/class of brick can have its own mitigation mechanisms. For example, the markdown brick runs DOMPurify.sanitize on the resulting html.

When I originally wrote this issue, the implementation sketch I had in mind for initial sandboxing was:

  1. Run all calls to mapArgs in the sandbox worker (because it can run Nunjucks/handlebars templates)
  2. Modify the @pixiebrix/jq brick to run jq in the sandbox worker
  3. Double-check whether mustache is actually logic-less, and if not, also make sure all calls the Mustache.render are run via the sandbox worker. If we're just worried about prototype pollution on this one, we might be able to get away with Object.freeze

These don't require access to the page or network connection, as they're just transforming data.

On iframe and MV3:

@fregante
Copy link
Contributor

You mentioned this in the other issue but I want to clarify here: Do you intend to use the sandbox only in Chrome for now? And then allow the code to run locally in Firefox?

@twschiller
Copy link
Contributor Author

And then allow the code to run locally in Firefox?

I think we have to, as I'm not sure Firefox has a sufficient analog. (It has a UserScripts API, but IIRC that isn't a good fit for this granular of call)

@fregante
Copy link
Contributor

fregante commented Jan 6, 2022

My understanding is that you want to only bring the execution of specific parts of the bricks to the sandbox, for example:

  1. Create sandbox
  2. Request "compute this nunjuck template"
  3. Sandbox responds with serializable result
  4. Sandbox is shut down

If so, we'll have to create a sandbox.js bundle with just the specific handlers, rather than moving the whole engine there.

Can you list these sandboxable bricks so we can move this ticket forward?

@twschiller
Copy link
Contributor Author

My understanding is that you want to only bring the execution of specific parts of the bricks to the sandbox

Correct, the things that need to be moved to sandbox are:

  1. Template evaluation as part of runtime argument passing
  2. Brick execution: JQ, and see below
  3. RJSF Form Validation

For a POC of this ticket, I think we should modify engineRenderer to use the sandbox under the hood. This will allow us to see what the impact on brick execution speed.

Can you list these sandboxable bricks so we can move this ticket forward?

Runtime:

  1. engineRenderer should return method that uses Sandbox under the hood
  2. Templates for service authentication currently use mustache, so we would not need to move it to the sandbox

Bricks:

  1. jq
  2. template

@fregante fregante removed their assignment Jun 8, 2022
@fregante
Copy link
Contributor

fregante commented Aug 31, 2022

Sandboxing coming to the web 😮 (safe eval). Already at stage 3.

https://github.com/tc39/proposal-shadowrealm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants