GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — wasi: clearly document sandboxing & file system security status by guybedford · Pull Request #50396 · nodejs/node

Conversation

@guybedford

@guybedford guybedford commented Oct 25, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Currently we use the term "sandbox" in the node:wasi documentation, which can be misconstrued as forming a security model.

This PR firstly removes the usage of the term "sandbox" and then also updates the docs to include a warning that WASI in Node.js is not a secure capability system and that this is not part of the Node.js threat model. This is very similar to what we already do for the VM module in being clear it does not provide a comprehensive security model for running untrusted code.

Finally, this PR adds a new section on Security to the WASI docs. If we improve the security properties in future, this versioned security documentation can form part of the contract of the implementation so that we can treat the addition of any security guarantees in future as a feature. Perhaps we never get there but it at least leaves the door open to that.

In terms of what would be required to claim our implementation does in fact provide secure sandboxing - the main issue right now is that the filesystem sandboxing is escapable via timing approaches with symlinks. The way to solve this from a security perspective would be to expose the openat primitive in libuv and use that to build a secure model. I created a discussion issue for this in libuv/libuv#4167. This would have a slight performance cost, but would form the correct primitive to provide a secure filesystem sandbox.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/wasi
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. wasi Issues and PRs related to the WebAssembly System Interface. labels Oct 25, 2023
mhdawson
mhdawson approved these changes Oct 25, 2023
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
UlisesGascon
UlisesGascon approved these changes Oct 26, 2023
View reviewed changes
RafaelGSS
RafaelGSS reviewed Oct 26, 2023
View reviewed changes
Comment thread doc/api/wasi.md Outdated

@RafaelGSS RafaelGSS Oct 26, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could mention it in the SECURITY.md file

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@guybedford guybedford Oct 30, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this is already covered by the existing thread model definition in SECURITY.md for Node.js, unless we want to explicitly call out WASI there?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell approved these changes Oct 28, 2023
View reviewed changes
@guybedford guybedford added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 31, 2023
Guy Bedford
wasi: clearly document sandboxing & file system security status
51b029d 
PR-URL: nodejs#50396
@guybedford
guybedford force-pushed the wasi-security branch from f3d1496 to 51b029d Compare October 31, 2023 19:32
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Oct 31, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 31, 2023

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/50396
✔  Done loading data for nodejs/node/pull/50396
----------------------------------- PR info ------------------------------------
Title      wasi: clearly document sandboxing & file system security status (#50396)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     guybedford:wasi-security -> nodejs:main
Labels     doc, wasi
Commits    1
 - wasi: clearly document sandboxing & file system security status
Committers 1
 - Guy Bedford 
PR-URL: https://github.com/nodejs/node/pull/50396
Reviewed-By: Michael Dawson 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50396
Reviewed-By: Michael Dawson 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - wasi: clearly document sandboxing & file system security status
   ℹ  This PR was created on Wed, 25 Oct 2023 18:50:44 GMT
   ✔  Approvals: 2
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/50396#pullrequestreview-1698108476
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/50396#pullrequestreview-1702811418
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/6713160160
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
guybedford pushed a commit that referenced this pull request Oct 31, 2023
Guy Bedford
wasi: document security sandboxing status
ffb326c 
PR-URL: #50396
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@guybedford

guybedford commented Oct 31, 2023

Copy link
Copy Markdown
Contributor Author

Landed in ffb326c.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford guybedford closed this Oct 31, 2023
@guybedford
guybedford deleted the wasi-security branch October 31, 2023 23:08
@guybedford guybedford removed the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label Oct 31, 2023
targos pushed a commit that referenced this pull request Nov 11, 2023
Guy Bedford @targos
wasi: document security sandboxing status
8cab236 
PR-URL: #50396
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Nov 12, 2023
v21.2.0 release proposal #50681
Merged
targos pushed a commit that referenced this pull request Nov 14, 2023
Guy Bedford @targos
wasi: document security sandboxing status
2f86d50 
PR-URL: #50396
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
Guy Bedford @UlisesGascon
wasi: document security sandboxing status
c6c0a74 
PR-URL: #50396
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
v20.11.0 proposal #51124
Merged
@RafaelGSS RafaelGSS mentioned this pull request Dec 19, 2023
doc: mention node:wasi in the Threat Model #51211
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RafaelGSS RafaelGSS RafaelGSS left review comments
@jasnell jasnell jasnell approved these changes
@UlisesGascon UlisesGascon UlisesGascon approved these changes
+1 more reviewer
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. wasi Issues and PRs related to the WebAssembly System Interface.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@guybedford @nodejs-github-bot @jasnell @UlisesGascon @mhdawson @RafaelGSS
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.