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

Explore notebook trust + workspace trust #118584

Closed
roblourens opened this issue Mar 9, 2021 · 43 comments
Closed

Explore notebook trust + workspace trust #118584

roblourens opened this issue Mar 9, 2021 · 43 comments
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders notebook verification-needed Verification of issue is requested verified Verification succeeded workspace-trust Trusted workspaces
Milestone

Comments

@roblourens
Copy link
Member

Continuing the discussion from #106747

@roblourens roblourens added under-discussion Issue is under discussion for relevance, priority, approach notebook workspace-trust Trusted workspaces labels Mar 9, 2021
@roblourens roblourens added this to the March 2021 milestone Mar 9, 2021
@roblourens roblourens self-assigned this Mar 9, 2021
@DonJayamanne
Copy link
Contributor

Let us know if there's anything you need. Thanks

@roblourens
Copy link
Member Author

First, what are we trying to do by observing workspace trust?

  • It sounds like in vscode's workspace trust model, if the user starts a debug session, that is an explicit action by the user, and we will not make them trust the workspace to do this. Is that right @sbatten? If so it seems like executing a notebook/cell is essentially the same, so do we have any reason to require the workspace to be trusted to do this?
  • Do we care about renderers executing code in outputs, when they are sandboxed in the webview?
    • To be clear we are differentiating between "extension trust" and "workspace trust", only talking about "workspace trust" here. So I am not thinking about evil or compromised renderers. But if a renderer loads code from your workspace and executes it in order to render, then that extension is responsible for checking workspace trust, just as the TS extension is responsible for checking workspace trust before executing code out of node_modules/typescript/....
    • The question is, outside of that case, is there any reason for vscode to prevent a renderer from running in an untrusted workspace where it is loading data (not code) from a cell output, processing it, and rendering it in the webview?
    • It's possible that some carefully constructed output data could trigger a vulnerability in a renderer and make it do bad things. Is that in our threat model?

@sbatten
Copy link
Member

sbatten commented Mar 11, 2021

There are currently 2 thought models on workspace trust and explicit actions.

  1. Explicit actions are out of scope. We only talk about browsing a repository and any explicit action is known to the user, so we don't request trust. (i.e. we allow it and do nothing)
  2. Explicit actions are signals of workspace trust and we take this as a signal to trust the workspace. In this model we still must confirm the user wants to mark the workspace as trusted and is only presented with a limited set of options to continue or cancel.

At present, one is not chosen over the other, but hopefully this scopes the discussion.

@jrieken
Copy link
Member

jrieken commented Mar 12, 2021

The question is, outside of that case, is there any reason for vscode to prevent a renderer from running in an untrusted workspace where it is loading data (

  • 💯 I share this. Event tho that data can be JavaScript/Html source code it is still running in a sandbox (webview or iframe) and that is isolated from the renderer and has no "dangerous", like NodeJS, apis. To me that makes it similar to the embedded browser that we are building and I don't believe we wrap that into a trust model

@roblourens
Copy link
Member Author

Renderer code can send messages outside of the webview which gives it a potential real impact, but still it seems like the responsibility is on the renderer - if it's going to send a message which could result in a bad impact based on data in the output, it should check workspace trust.

@jrieken
Copy link
Member

jrieken commented Mar 12, 2021

Renderer code can send messages outside of the webview which gives

Ideally those messages go straight to the extension host and aren't rerouted via the renderer. I believe there was some exploration to use message channels that enables that.

@sbatten
Copy link
Member

sbatten commented Mar 12, 2021

@lszomoru for awareness

@roblourens
Copy link
Member Author

roblourens commented Mar 15, 2021

Talked to @sbatten, the current plan is that for explicit user actions like debugging or running a command that requires trust, a dialog will show asking the user to "continue" and trust the workspace, or "cancel" and not trust it. For notebooks, we can hook into the same system to show a dialog when you first run a cell.

You could argue that the extension should be involved in this rather than vscode enforcing it, because trust doesn't matter for some notebooks. For example, I don't care about running a github issues query from an untrusted workspace. But most notebooks are not like that, and this is so similar to debugging, and it will be much simpler to have vscode handle it in the same way.

Then also, given the discussion above, I think that we should not automatically block outputs/renderers from loading in an untrusted workspace. The extension can require workspace trust if it is necessary for that extension (like if it is loading and executing code in the workspace) and it's the responsibility of the extension to determine that. cc @connor4312 and @DonJayamanne do you think that's acceptable?

@DonJayamanne
Copy link
Contributor

@roblourens Thanks this makes a lot of sense, and from what I can tell, there's no need to implement a custom trust model in Jupyter extension.

  • If user has a trusted workspace, then we don't prompt users to trust the notebook
  • If user does not have a trusted workspace, and they open a notebook
    • Jupyter extension to decide whether we render the output or not
    • If we don't display the output, then we can optionally notify the user that they can trust the worksapce if they want to see the output
  • If user does not have a trusted workspace, and they attempt to run a notebook
    • Jupyter extension can ask the user to trust the workspace, if they trust it, we let them run the notebook, else nothing happens (cell does not run)

I guess the only question for Jupyter extension is whether we display the output or not (when not trusted).

@claudiaregio @greazer /cc

@roblourens
Copy link
Member Author

Jupyter extension can ask the user to trust the workspace, if they trust it, we let them run the notebook, else nothing happens (cell does not run)

VS Code will handle this part of it, the extension doesn't have to do it.

Jupyter extension to decide whether we render the output or not

Yes, and we can discuss this more. I'm not sure there is really any need to do anything with output renderers but it probably needs more thinking.

@connor4312
Copy link
Member

connor4312 commented Mar 16, 2021

👍 for allowing renderers through. We just need to be aware that any of the FromWebview messages may be coming from a untrusted (or, malicious) context. Looking at what we have today, the worst an extension could do it mess up layout/focus and show a 'save' dialog. Arguably the latter could be dangerous if users blindly allow saving, but I don't think there's a way around that.

@jrieken
Copy link
Member

jrieken commented Mar 16, 2021

You could argue that the extension should be involved in this rather than vscode enforcing it, because trust doesn't matter for some notebooks. For example, I don't care about running a github issues query from an untrusted workspace. But most notebooks are not like that, and this is so similar to debugging, and it will be much simpler to have vscode handle it in the same way.

Sounds like a kernel property to me, like a kernel can say if it requires trust

@claudiaregio
Copy link

@roblourens Hi Rob, right now the Jupyter extension prompts the user to trust a notebook if not created on the user's machine (ex. they download it). What happens in this scenario where a user opens a stand-alone notebook where it did not come from a trusted workspace? (ex. opened from downloads folder)

@roblourens
Copy link
Member Author

@sbatten how does trust work in a window that doesn't have a folder open? Or for files that are not in the current workspace?

@sbatten
Copy link
Member

sbatten commented Mar 19, 2021

Current implementation is that empty workspaces (no folder) are trusted by default. However, opening files in an empty workspace will be treated as the "loose file" scenario. If the loose file is not from a known trusted path, we prompt the user once to inform them that they are bringing untrusted content into a trusted workspace. From then on, any additional untrusted content will not receive a prompt for the remainder of the session.

@roblourens
Copy link
Member Author

Command links in outputs and markdown cells could pose a threat in untrusted workspaces. We implemented this in #98100. Is the Jupyter extension still relying on these in outputs and markdown cells?

We'd like to think about removing them since they are a vscode-only feature. Or, we have to either

  • disable command links in untrusted workspaces
  • or disable scripts in outputs in untrusted workspaces
  • or just disable all rich outputs in untrusted workspaces

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Apr 25, 2021

Jupyter extension still relying on these in outputs and markdown cells?

Yes.

or just disable all rich outputs in untrusted workspaces

I'd think this is preferred.

or disable scripts in outputs in untrusted workspaces

If we can have this, that's even better, but can we guarantee that this will not be open to exploits in some form?
At the end of the day the requirement is to protect the user from malicious content running without the users consent.

@roblourens
Copy link
Member Author

  • We decided to hide rich outputs in an untrusted workspace, and show some placeholder output
  • There has been some conversation about asking a debug adapter whether it requires trust, maybe I would do the same for notebooks, although I don't really like it https://github.com/microsoft/vscode-internalbacklog/issues/2049
  • On Friday I said that there wouldn't be a popup prompt if you try to run a cell in an untrusted workspace. Actually that is not true, there is a prompt to trust the workspace at that moment, and I have implemented that. FYI @claudiaregio

@roblourens roblourens added the verification-needed Verification of issue is requested label Jun 1, 2021
@rzhao271 rzhao271 added the verification-steps-needed Steps to verify are needed for verification label Jun 1, 2021
@rzhao271
Copy link
Contributor

rzhao271 commented Jun 1, 2021

@roblourens looks like there's a lot going on here. Can you write some verification steps, please?

@roblourens
Copy link
Member Author

  • Open an untrusted workspace
  • Open a python notebook
  • You should be able to see the notebook cells, but you should not be able to execute a cell
  • If you try to execute a cell, you should get the prompt to trust the workspace
  • Open a python notebook that has outputs saved
  • You should not be able to see rich html outputs, but it should fall back to simple outputs (markdown, plain text, etc) if they exist

@rzhao271
Copy link
Contributor

rzhao271 commented Jun 2, 2021

I've verified almost all of the points above except for the last one.
I'm not familiar with how rich html output works, and how one writes a notebook to produce that output, though, so I'm unable to tell when the output switches from a rich html one to a simple one.

@RandomFractals
Copy link

@rzhao271 you can try it with a restbook ... tanhakabir/rest-book#90 (comment)

@RandomFractals
Copy link

@roblourens why should not I see previously generated rich html output in a loaded notebook? I think you are taking this 'untrusted' workspaces a bit too far.

If I had a renderer that generated a chart as a cell output and saved it. Are you saying I should not be able to see it with your notion of an untrusted workspace? I most certainly disagree with you and your teams decisions on this.

@roblourens
Copy link
Member Author

roblourens commented Jun 2, 2021

Custom renderers should still work in an untrusted workspace. HTML outputs don't. Even though they are "sandboxed" in an iframe, the iframe has special privileges such that we believe it isn't safe to run explicitly untrusted scripts in it. In the html output case, this is untrusted content from the document, not from an extension.

@rchiodo
Copy link
Contributor

rchiodo commented Jun 2, 2021

I believe Google collab and jupyter run something that scrapes out javascript from any html outputs by default. Might we be able to do something similar?

@roblourens
Copy link
Member Author

I don't see why we would show a possibly broken output. And I don't really trust any html parsing code I write for security purposes. There might be a fallback output mimetype.

@rchiodo
Copy link
Contributor

rchiodo commented Jun 2, 2021

Wouldn't custom renderers be the same thing? The data is json that drives the custom renderer. It could potentially be a security risk too?

@rchiodo
Copy link
Contributor

rchiodo commented Jun 2, 2021

I think the only truly safe output would be text/plain. Although I still think you could remove script tags pretty easy. Everything else is potentially running code on the output.

@rzhao271
Copy link
Contributor

rzhao271 commented Jun 2, 2021

I switched the workspace to untrusted, and then opened up a notebook that renders an HTML output. The HTML output seemed to still be there, but the only option when it came to switching renderers was plain text:

html-output

@RandomFractals
Copy link

RandomFractals commented Jun 2, 2021

close, but I think you should still show built-in html render and render it. not sure I like the stripping out of js idea either. do it if you must.

Those cell outputs run in an iframe in a notebook webview and is already sandboxed with CSP and rest that comes with it. what attack vectors are you trying to prevent there?

Sounds like premature optimization to me at this point until we have some more rich output renders to play with and finalize this.

@meganrogge meganrogge added verification-found Issue verification failed and removed verification-steps-needed Steps to verify are needed for verification labels Jun 3, 2021
@connor4312 connor4312 reopened this Jun 3, 2021
@roblourens
Copy link
Member Author

@connor4312 this regressed in d6d9200#diff-54b1e3297f88bd6925ca5fe088edaf93b8aa1832936da6350d69fec68375504eL54 where we pick mimetype 0, even if it's not trusted. I don't quite follow, can you take a look?

@connor4312 connor4312 removed the verification-found Issue verification failed label Jun 3, 2021
@roblourens
Copy link
Member Author

Thanks @connor4312.

@rchiodo it's the difference between html that comes from workspace content and html that comes from extensions.

@RandomFractals an example is that html outputs can have links that trigger vscode commands

@RandomFractals
Copy link

ok. I did likes and dislikes on this feature implementation, ideas, and proposed approaches with my thumbs up and down.

Please view them as agree or disagree. Thanks!

@bpasero bpasero added the verification-steps-needed Steps to verify are needed for verification label Jun 4, 2021
@rchiodo
Copy link
Contributor

rchiodo commented Jun 4, 2021

The json in the outputs that drives the html from the renderer can be anything though. And the renderer could theoretically be doing anything possible in a renderer (like say sending messages back to the extension to run a cell). That's why I think any json (read by a renderer, or causes code execution) in an untrusted notebook should be deemed malicious.

@rzhao271 rzhao271 added verified Verification succeeded and removed verification-steps-needed Steps to verify are needed for verification labels Jun 4, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders notebook verification-needed Verification of issue is requested verified Verification succeeded workspace-trust Trusted workspaces
Projects
None yet
Development

No branches or pull requests

13 participants