[security] Hide require('fs') API method calls behind permission & add vscode.FileSystem arbitrary directory whitelist

[zm-cttae]

• As VS Code ecosystem stakeholders, we need to get the Node.js userland secure
• This makes users feel safer per [Feature Request] Extension Permissions, Security Sandboxing & Update Management Proposal #52116
• This also lets marketplace and core team focus on more productive beneficial work

This feature request is part of an "epic" suggestion in #52116 (comment)

Proposals:

• Add contributes.permissions.filesystem.readFile and contributes.permissions.filesystem.writeFile array to define extra IO capacities for vscode.workspace.fs.
• Add a UI workflow in the extension installer to request permission before install - see [Feature Request] Extension Permissions, Security Sandboxing & Update Management Proposal #52116 description.
• Add a migration workflow via 1 startup modal for a user to re-enable all the extensions that require extra permission.
• Also block VSIX cli installs with a prompt to make the extension enabled when extra permissions are needed. Put in a --force CLI command that skips the prompt but keeps the extension off until the UI okays it.
  We can add a secret in vscode-test package and a API vscode.Extension method that accepts the token & enables the ext.
• Discuss whether honoring PATH as an opt-in would be beneficial. Ideally we should be honoring PATH by default, adding some kind of OS sandbox so those can't be used.
• If the user hasn't granted a "read all files" unsafe permission, error require('fs') and any aliases+submodules after these changes - also disable the extension.
  Probably link to a contributes.permissions.*File API tracking issue in the console error.
  Also trigger some kind of workflow to re-enable the 1 extension that's unsafe - presumably one top-right 🧩 Extension alert like in Chromium.
  Debounce permissions requests by about 5-10 seconds and if there are multiple (possibly @ startup event), do a modal.

Keywords: security, fs, filesystem, IO, extension, API, exthost, node, nodejs, permissions, sandbox, readwrite, readfile, writefile

[joyceerhl]

@sbatten, assigning to you to start for workspace trust.

[worksofliam]

Throw an error on the code require('fs') and any aliases+submodules after these changes

• Does this imply that VS Code is going to completely deny extensions of making use of Node.js fs? It's easy enough for me to change my extensions to use workspace.fs, but I include modules that use fs. This would have a large impact.
• I am use fs in conjunction with os.tmpdir() a lot. I would hope you'd honour this value as well as the $PATH values.
• Would I have to polyfill fs for the modules I include?
• Does workspace.fs work for files outside the workspace? Most of my users don't have a workspace open when they are working with remote file systems over a FileSystem implementation.

[zm-cttae]

• I didn't think about dependencies and polyfilling.. this makes a vscode-specific cached polyfill that enforces the permissions system more important. It can throw errors against permission system (shell out to vscode.workspace.fs gave up on this suggestion)
• There actually should be temporary storage location option via vscode.ExtensionContext..
• A polyfill you included would be an attempt to defeat the lockdown, which I suppose could happen down the line but wouldn't be very nice of you!
• Unsure.. thought that ( ...workspace.textDocuments ).getText() was the correct API there. The new API is intended to open up the filesystem API vs the current impl.

NB: ideally everything in opt-in permissions system works before any errors are thrown!

[zm-cttae]

I had an idea for unsafe/compatibility mode, especially for the following scenario:

• say that vs code app is a newish version that has locked down require('fs')
• the value of engines.vscode is super low so we can't use new permissions system

Ideally in this scenario (supported lower vscode version / no extension version lock... combined with locked down vscode API), we don't error out on each run
We just need a "read all files on your system (unsafe)" permissions requirement

What we could do:

• fs is blocked in the cache by permission-checking fake shim that errors out if no permission
• the extension host worker (extension instance) posts back a implicit permissions request also
• these are debounced by 8 seconds then collated into one prompt (modal if +1, top-right alert if 1)
• if "read all files" permission is granted, we give fs back

UX workflow to use for workspace.fs perms error:

• the extension is disabled and a "read all files" permission is added to the list for next time
• the user gets a showErrorMessage asking if they want to re-enable their extension
• if they dismiss that, they get 1 dismissable message dialog that can "show permission changes in extensions pane" on the next startup..

[vscodenpa]

This feature request is now a candidate for our backlog. The community has 60 days to upvote the issue. If it receives 20 upvotes we will move it to our backlog. If not, we will close it. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

[zm-cttae]

Adjusted strat proposal for backwards compatibility - #174715 (comment) - thoughts @worksofliam @mattseddon

[segevfiner]

Not allowing require('fs') will break so many extensions, many can’t work with vscode.workspace.fs, due to using separate processes (LSPs that index files directly, many official heavily used ones do that), or due to it being highly incompatible to the standard fs (It’s a whole different API), or even just external modules that want to access the filesystem, Heck, this includes many official Microsoft extensions. Of course, such extensions don't support virtual workspaces.

It will basically just end up being that most extensions need to request this permission and any security benefit from it will be rather small to non existent, but in return, annoying every single extension developer with this, and every single user with pointless security prompts.

An OS level transparent sandbox makes more sense (Like Chromium does), as it won't require changing the code completely if it doesn't access anything it is not supposed to, or just add the small amount of additional stuff it does need.

[zm-cttae]

Not allowing require('fs') will break so many extensions, many can’t work with vscode.workspace.fs, due to using separate processes (...), or due to it being highly incompatible to the standard fs

This change will have a significant impact. When extensions were locked down in Chrome 70 behind a website-specific permissions system, it basically brought the most useful extensions in the web store to ground 0. But the change was celebrated because people could reject "read all files" extensions & otherwise click "OK" once.

As explained in #174715 (comment) these requests can be batched as much as possible and borrow as much as possible from the existing permissions UX in browsers.

It will basically just end up being that most extensions need to request this permission and any security benefit from it will be rather small to non existent, but in return, annoying every single extension developer with this.

UI extensions will be unaffected. People will use native APIs instead of Node.js ones. node API access is generally going to be scaled down in the ROADMAP to some degree - this request is about increasing that and then putting the absolute minimum number of prompts to get things working again.

annoying every single extension developer with this, and every single user with pointless security prompts.

I see. Well the security concern described in this issue is big IMHO. I also don't think such security prompts are pointless - my previous employer had an IT policy that banned marketplace access in Visual Studio Code, just to enforce a sane level of filesystem restriction.

Hopefully this kind of feature can be well made - people never complain about Chrome extension security prompts. With good UI and UX work (and learning from Chromium's extension perms as a UI-UX success), this will be something the extension devs can be happy about, or just indifferent to.

Heck, this includes many official Microsoft extensions. Of course, such extensions don't support virtual workspaces.

EDIT: Just noting that the use of fs isn't supported or documented officially. Which is a shame because fs and child_process has given us linting, compilers and every other feature that turns VS Code into an IDE

[segevfiner]

They are not pointless only when its not everyone asking for that permission, VS Code's primary strength come from LSPs and running other linters and so on, so most useful extensions will ask for that, once an extension can do that it breaks any sort of security this achieve in the first place...

After all a native process separate from the extension host can do whatever...

[zm-cttae]

I don't think that's VS Code's primary strength.. its just one of them. VS Code is for web technologies and first and foremost.. one of its biggest USPs is the browser support and even language service features (example 1, example 2).

Will sandboxing protect documents and source codes in the filesystem? I didn't think sandbox at the OS level would scale to all flavours of OS and environment either. This ticket is meant to support the server codespace described in microsoft/vscode-remote-release#6608 - how can we ensure the filesystem's integrity?

The important thing is that users consent with <=1 popup ever to the extension using API methods from require('fs'). 90% of avoiding any "annoying" UX is debouncing the permissions requests to 5-10 seconds.

The VS Code app can register a conditional permission wrapper for the FileSystem Node module in the extension host worker for each extension. The change would put the user on two decision paths:

1. this is a "already safe" extension - they can readwrite in extension, workspace dir and source dirs which is usually safe
2. the extension hits Node FS module so we need to ask the user for security permissions once

EDIT: I notice that the extension host process is already sandboxed so that won't be a technical blocker RE: app resources here.. each extension will have its own set of permissions and readwrite access

[mattseddon]

This won't even lock down access to the file system. Consider the following line of code:

node -p -e "let fs=require('fs');let result = fs.readdirSync(process.cwd());console.error(result)"

which can easily be run using child_process.

[zm-cttae]

That would be patched by #174714.
Imaginably whatever was done for fs could be copypasta'd to child_process.
The only change is that core team would need to introduce child_process alternative with same permissions setup

[zm-cttae]

---

It occurs to me that the solution involves wrapping every exported function in the fs+fs/promises module and the fs.promises prop with a VS Code check.
We can add an "early pass" path to to the permission check in the case of "safe" or no file paths:

• iterate across any arguments fed to the function
• detect whether any input arguments could be a filepath (a string that contains / at indexOf !== 0)
• If all filepath(s) are whitelisted in the permissions system, we can let it through
• node_modules in extension dir can automatically be considered safe

This will significantly reduce the impact of this issue. Thoughts?

[zm-cttae]

I've given the thread a temperature test - it's clear there is little appetite in the extension developer community for it. Will Unsubscribe and let it run passively

edit: @segevfiner could you make a feature request for an OS-level transparent sandbox per #174715 (comment) done!

[vscodenpa]

This feature request has not yet received the 20 community upvotes it takes to make to our backlog. 10 days to go. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

[zm-cttae]

Replaced by #180233 per collective user request.