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.

Sign up for GitHub

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

Jump to bottom

Promise's domain property allows escaping VM #15673

Closed
TimothyGu opened this issue Sep 29, 2017 · 2 comments
Closed

Promise's domain property allows escaping VM #15673

TimothyGu opened this issue Sep 29, 2017 · 2 comments
Labels
domain Issues and PRs related to the domain subsystem. vm Issues and PRs related to the vm subsystem.

Comments

@TimothyGu
Copy link
Member

  • Version: v8.x / master
  • Platform: Linux debian-x240s 4.9.0-3-amd64 SMP Debian 4.9.30-2+deb9u5 (2017-09-19) x86_64 GNU/Linux
  • Subsystem: vm
> vm.runInNewContext('Promise.resolve().domain') instanceof Object
true
> vm.runInNewContext('Promise.resolve().domain instanceof Object')
false

A question of a larger scope is if the promise hooks should be enabled at all for VM contexts.
@TimothyGu TimothyGu added domain Issues and PRs related to the domain subsystem. vm Issues and PRs related to the vm subsystem. labels Sep 29, 2017
@bnoordhuis
Copy link
Member

bnoordhuis commented Sep 29, 2017 •
edited
Loading

Doesn't really strike me as an issue. vm is not a security mechanism.

edit: no opinion on whether promise hooks should be enabled or not.

@TimothyGu
Copy link
Member Author

vm is not a security mechanism.

... and I never said it was, but it has been the case until now that objects from another context can only be present in a vm context if the user explicitly adds things into the sandbox from another sandbox.

I just realized the domain property is only added if domains are enabled, which is the case in the REPL. That softens the blow for me, but this still sounds like a bug.

@TimothyGu TimothyGu mentioned this issue Sep 30, 2017
Closed
4 tasks
@parasyte parasyte mentioned this issue Oct 1, 2017
Closed
@bnoordhuis bnoordhuis mentioned this issue Oct 2, 2017
Closed
3 tasks
TimothyGu added a commit to TimothyGu/node that referenced this issue Oct 4, 2017
@TimothyGu
src: do not add .domain to promises in VM contexts
fd18704 
The promises are still tracked, and their handlers will still execute in
the correct domain. The creation domain is simply hidden.

Fixes: nodejs#15673
addaleax pushed a commit to addaleax/ayo that referenced this issue Oct 12, 2017
@TimothyGu @addaleax
src: do not add .domain to promises in VM contexts
8d727f1 
The promises are still tracked, and their handlers will still execute in
the correct domain. The creation domain is simply hidden.

PR-URL: nodejs/node#15695
Fixes: nodejs/node#15673
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@TimothyGu TimothyGu mentioned this issue Oct 16, 2017
Closed
4 tasks
TimothyGu added a commit to TimothyGu/node that referenced this issue Oct 22, 2017
@TimothyGu
src: do not add .domain to promises in VM contexts
3a7411f 
The promises are still tracked, and their handlers will still execute in
the correct domain. The creation domain is simply hidden.

Backport-PR-URL: REPLACEME
PR-URL: nodejs#15695
Fixes: nodejs#15673
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
evanlucas pushed a commit that referenced this issue Oct 23, 2017
@TimothyGu @evanlucas
src: do not add .domain to promises in VM contexts
0b04869 
The promises are still tracked, and their handlers will still execute in
the correct domain. The creation domain is simply hidden.

Backport-PR-URL: #16074
PR-URL: #15695
Fixes: #15673
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@domenic domenic mentioned this issue Jan 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
domain Issues and PRs related to the domain subsystem. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bnoordhuis @TimothyGu