process: initial impl of feature access control #22112

addaleax · 2018-08-03T20:51:19Z

Just playing around with something here … this doesn’t need to happen, or can happen in a completely different way or by somebody else. I’d love to get ideas and feedback on this, though.

/cc @nodejs/security @nodejs/collaborators

Implement process.accessControl, a simple API for restricting usage of certain in-process APIs,
and similarly an accessControl option when setting up workers.

Refs: #22107

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-08-03T20:51:22Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/418/pipeline

devsnek · 2018-08-03T20:57:49Z

this looks pretty cool. I've been playing around with some stuff that can be applied on a per-script/per-module granularity. I don't think per-process is always good enough.

addaleax · 2018-08-03T20:59:44Z

@devsnek Would love to hear more details. In particular, i feel like it might end up being pretty hard to make sure nothing messes with node’s internals, if it’s a solution in JS land?

benjamingr · 2018-08-03T21:03:25Z

This is really cool! Would it be possible to place restrictions on npm install scripts and third-party modules in the future? It could be really cool if the package.json file could specify restrictions on different dependencies although I have no idea how tracking them would look like (like running modules in vm instances?)

devsnek · 2018-08-03T21:05:14Z

@addaleax basically I had to hack into v8 internal execution context tracking to see where calls from from. from there I keep a big map of applied permissions and check them in order of lowest to highest by doing a graph traversal from the node the rule applies to up to the entry. that way the highest request always overrides lower ones so permissions can change but not overridden.

benjamingr · 2018-08-03T21:06:06Z

doc/api/process.md

+
+<!-- eslint-skip -->
+```js
+{ accessInspectorBindings: true,


Missing some stuff like http2 and dns? (dns is under net outgoing?)

dns currently falls under netOutgoing, but I missed that in the doc here (updated with that).

HTTP/2 is a purely computational thing, so I don’t think there’s anything to restrict here (although that could of course be implemented)? For now, that’s also covered by the net flags

addaleax · 2018-08-03T21:07:07Z

@benjamingr I think what @devsnek is hacking around on would go into that direction.

It’s a lot easier for npm install scripts, though; something I had in mind is that we could maybe refine this to allow for connections to whitelisted hosts/writing to whitelisted files/directories (these are not easy to implement, though).

jasnell · 2018-08-03T21:08:55Z

This is a very interesting start to something that will likely be a long running discussion. I've been working on details for a command line based approach that could easily parallel this. Essentially something like:

node --deny=fs.write,net

node --allow=fs.write

The are some tricky bits to all this and having a number of different options on the table would be great to work things out

addaleax · 2018-08-03T21:14:10Z

@jasnell Yeah, I thought about a CLI API too… totally open for suggestions and happy to implement them :)

(I haven’t spent much all that much time on this, tho – this PR is literally hacked together within a single afternoon so far.)

jasnell · 2018-08-03T21:15:07Z

For example of how these could parallel with one another...

Command-line:

$ node --deny=fs.read,fs.write,workers --allow=setV8Flags

Runtime API (with a slightly different syntax that could provide more flexibility):

process.accessControl({
  deny: ['fs.read', 'fs.write', 'workers'],
  allow: ['setV8Flags']
})

FWIW, I would think that ability to spawn child processes and loading addons would need to be automatically denied by default if certain policies such as fs.read, fs.write, etc are denied, since a child process can be used as an easy workaround to those policies. We need to give some thought to sensible defaults.

addaleax · 2018-08-03T21:18:02Z

@jasnell One reason I didn’t look into using arrays/sets for this was that I’d like to keep the possibility open to use more complex settings than booleans, e.g. whitelists for specific directories or so. (Once again, hard to implement).

jasnell · 2018-08-03T21:20:01Z

Yeah, I definitely get that but I'd much rather keep the API and the policy definition as simple as possible. The more options we provide, the more we increase the complexity, cost, and likelihood of getting it wrong.

vsemozhetbyt

Just some doc nits)

vsemozhetbyt · 2018-08-03T21:06:09Z

doc/api/process.md

 [`promise.catch()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch
 [`require()`]: globals.html#globals_require
 [`require.main`]: modules.html#modules_accessing_the_main_module
 [`require.resolve()`]: modules.html#modules_require_resolve_request_options
+[`tracing.disable()`]: tracing.html#tracing_tracing_disable
+[`tracing.enable()`]: tracing.html#tracing_tracing_enable


Nit: both after [`setTimeout(fn, 0)`] ?

vsemozhetbyt · 2018-08-03T21:13:09Z

doc/api/process.md

+Please report bugs at https://github.com/nodejs/node/issues.
+
+Operations scheduled before this call are not undone or stopped.
+Features that were previously disabled through this call can not be re-enabled.


Nit: can not -> cannot?

vsemozhetbyt · 2018-08-03T21:15:13Z

doc/api/process.md

+Features that were previously disabled through this call can not be re-enabled.
+Child processes do not inherit these restrictions, whereas [`Worker`][]s do.
+
+The following code removes some permissions of this Node.js instance:


Nit: maybe this -> current?

vsemozhetbyt · 2018-08-03T21:23:58Z

doc/api/process.md

+added: REPLACEME
+-->
+
+Returns an object representing a set of permissions for the current Node.js


Replace with the formal * Returns: {Object} Description... signature?

vsemozhetbyt · 2018-08-03T21:25:53Z

doc/api/process.md

+
+Currently only boolean options are supported.
+
+In particular, thes settings affect the following features of the Node.js API:


Nit: thes -> these.

doc/api/process.md

+<!-- eslint-skip -->
+```js
+{ accessInspectorBindings: true,
+  childProcesses: true,


vsemozhetbyt · 2018-08-03T21:42:07Z

doc/api/process.md

+  - All other [`fs`][] operations
+  - [`net`][] operations on UNIX domain sockets
+- `loadAddons`:
+  - [`process.dlopen()`][] and [`require()`][] in the case of native addons.


Nit: inconsistent period at the end of an incomplete sentence?

vsemozhetbyt · 2018-08-03T21:48:06Z

doc/api/process.md

+- `setEnvironmentVariables`:
+  - Setting/deleting keys on [`process.env`][]
+- `setProcessAttrs`:
+  - [`process.title`][] setting


Nit: maybe it is worth to sort this sublist?

addaleax · 2018-08-03T21:55:12Z

@vsemozhetbyt Done, but please keep in mind that this PR is still at its very beginning :)

bengl · 2018-08-03T21:57:24Z

We had a long discussion about this sort of feature at the IRL @nodejs/security-wg meeting in SF last week. We came to the conclusion that without actual isolation, this can be done so long as user code in the Node process is assumed to be trusted (that is, there's no malicious code, but code could be buggy). @addaleax based on the docs you've added, it seems you've implemented this with the same assumption, which is great! Sadly, I don't think anyone took notes. Sorry! That being said, there's some more discussing here: nodejs/security-wg#327

Also FWIW, Intrinsic builds a commercial product around this kind of feature: https://intrinsic.com. Intrinsic also provides multiple policy sets per process (i.e. multiple actually-isolated environments), though by default they're organized by inbound HTTP routes, rather than by module. (Disclaimer: I work for Intrinsic.)

addaleax · 2018-08-03T22:07:23Z

We came to the conclusion that without actual isolation, this can be done so long as user code in the Node process is assumed to be trusted (that is, there's no malicious code, but code could be buggy). @addaleax based on the docs you've added, it seems you've implemented this with the same assumption, which is great!

@bengl I would personally love it if we could get actual sandboxing in a runtime that is written for a language which in turn was designed with sandboxing in mind and whose primary use case still is sandboxing.

But yes, this PR doesn’t provide any protections against user code leading to crashes or resource exhaustion or whatever.

jasnell · 2018-08-03T22:16:52Z

Intrinsic builds a commercial product around this kind of feature

@bengl ... this really is a critical point. We need to make sure that whatever we do here is additive to whatever additional experience commercial vendors may provide and does not compete or run counter to such efforts. Ideally, anything we do here should make it easier for companies like Intrinsic to either continue providing such services or easier to build new capabilities on top.

On the sandboxing bit, the point was raised at the face to face that the only guaranteed way to ensure proper sandboxing here is to rely on the container within which the Node.js process is running. The goal for us should be to make it more difficult to use Node.js as an attack vector while retaining the current assumption that code running within the typical Node.js process is trusted.

not-an-aardvark · 2018-08-03T22:21:00Z

Could you elaborate on the intended use case for this? To me, it seems like the main value-add of "access control" is typically the improved security (since it restricts the level of impact that a compromised component could have). But if this can be bypassed by spawning a child process, then it's not clear how it's useful for security, because an attacker that would have originally called fs.write could now just use require('child_process')... and spawn a new process that does the same thing.

bengl · 2018-08-03T22:24:08Z

@jasnell Because Intrinsic assumes a malicious attacker model, there's not as much overlap as it may seem.

addaleax · 2018-08-03T22:24:25Z

Could you elaborate on the intended use case for this?

I’d love it if we could get to a point where we can run npm install scripts without worrying too much about that, as an example.

But if this can be bypassed by spawning a child process, then it's not clear how it's useful for security, because an attacker that would have originally called fs.write could now just use require('child_process')... and spawn a new process that does the same thing.

This PR does provide the option to disable spawning child processes / loading addons through Node’s APIs for that.

Qard · 2018-08-03T22:44:54Z

I would disagree slightly with avoiding features competitive with products built on Node.js in favor of only being additive. If a feature is useful to a significant portion of users for it to be built into core, I don't think we should prevent that. I would, however, be hesitant to support more niche features around it, which may not be useful to as many users.

In this particular case, I would not want a desire to avoid competition to result in a less capable or less usable feature, potentially compromising the security advantage of the feature, given that is the main purpose of it.

not-an-aardvark · 2018-08-03T22:45:25Z

I’d love it if we could get to a point where we can run npm install scripts without worrying too much about that, as an example.

That makes sense, and to be clear, I'm definitely on board with allowing things like this to be done safely -- it's just not clear to me whether the security policy mechanism in this PR would be an improvement for that case.

For example, regarding this note:

[The Security WG] came to the conclusion that without actual isolation, this can be done so long as user code in the Node process is assumed to be trusted (that is, there's no malicious code, but code could be buggy).

Is there a particular category of buggy code that this is intended to protect against? For example, it seems unlikely to me that there are common bugs which would cause a module to write to the filesystem if the module is not supposed to ever write to the filesystem. Either a module's source code has a call like fs.write() (in which case it's clearly supposed to write to the filesystem and it wouldn't be possible to block its filesystem access while preserving intended functionality), or it does not have any calls like that (in which case it seems like an attacker would need to have arbitrary code execution in order to create that call).

But in cases where the attacker can execute arbitrary code, then it seems like the "trusted code assumption" is violated, so at that point there are probably other things they can exploit even without spawning a child process (such as issues like #9820 which previously were not considered to have security impact).

gabrielschulhof · 2018-08-03T22:52:36Z

For example, it seems unlikely to me that there are common bugs which

would cause a module to write to the filesystem if the module is not supposed to ever write to the filesystem. At the very least, having access control would mean that if, initially, writing to the file system is disabled, then, when someone (inadvertently?) adds a `fs.write()` call in some subsequent version of the package, it will initially fail, until the policy is updated. This should make adding a capability an explicit step, creating a process which hopefully results in justified and explicit modifications of the policy, rather than implicit access grant.

…

On Fri, Aug 3, 2018 at 3:47 PM, Teddy Katz ***@***.***> wrote: I’d love it if we could get to a point where we can run npm install scripts without worrying too much about that, as an example. That makes sense, and to be clear, I'm definitely on board with allowing things like this to be done safely -- it's just not clear to me whether the security policy mechanism in this PR would be an improvement for that case. For example, regarding this note: [The Security WG] came to the conclusion that without actual isolation, this can be done so long as user code in the Node process is assumed to be trusted (that is, there's no malicious code, but code could be buggy). Is there a particular category of buggy code that this is intended to protect against? For example, it seems unlikely to me that there are common bugs which would cause a module to write to the filesystem if the module is not supposed to ever write to the filesystem. Either a module's source code has a call like fs.write() (in which case it's clearly supposed to write to the filesystem and it wouldn't be possible to block its filesystem access while preserving intended functionality), or it does not have any calls like that (in which case it seems like an attacker would need to have arbitrary code execution in order to create that call). But in cases where the attacker can execute arbitrary code, then it seems like the "trusted code assumption" is violated, so at that point there are probably other things they can exploit even without spawning a child process (such as issues like #9820 <#9820> which previously were not considered to have security impact). — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#22112 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0cd-GL_e-rcRAjD20vuMHgqIGHmrks5uNNL6gaJpZM4VumUO> .

addaleax · 2018-08-08T22:12:04Z

Full CI: https://ci.nodejs.org/job/node-test-pull-request/16283/

@mhdawson

Even guarded with experimental I'm thinking we need to have some performance analysis to make sure we are comfortable there is no significant performance impact when its turned off as those checks will still be in place.

I don’t expect this to have a noticeable impact since the check compiles down to a single bit-test + a conditional jump (with a "not taken" hint when available), but in any case, here’s a benchmark CI for an affected subsystem (dgram):

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/222/

(edit: 4 significant results, which is about the number of false postives we’d expect to see – and all of the results are actually increases. 🙂)

We probably would also want an assessment of code coverage before/after.

Yes, definitely. 👍

I also wonder if this is something worth incubating in a fork (like we did for N-API) until there is general consensus that we have the right API, approach and coverage of the important use cases?

A fork has advantages if we think that we want a separate place for discussion (in this case that might as well be the security WG’s repo?) and/or expect this to be implemented incrementally in more than a couple steps. I’m not sure these conditions are met, though.

addaleax · 2018-08-08T23:13:12Z

~~New CI: https://ci.nodejs.org/job/node-test-pull-request/16286/~~
New CI: https://ci.nodejs.org/job/node-test-pull-request/16287/ (edit: ✔️)

arcanis · 2018-08-14T19:48:14Z

Would it be possible to place restrictions on npm install scripts and third-party modules in the future? It could be really cool if the package.json file could specify restrictions on different dependencies although I have no idea how tracking them would look like (like running modules in vm instances?)

This is something that I think would be super valuable. Third party modules are currently given full access to the machine, but they really shouldn't in 99% of the cases. It would make that much harder for the next eslint-scope to happen, and would have added benefits (if the package managers know that a given package will never access the filesystem, new optimizations become possible).

If we had such an API available, I believe we would be super interested into implementing in Yarn an interface that would let the user review the permissions granted to their packages 🙂

Implement `process.accessControl`, a simple API for restricting usage of certain in-process APIs. Refs: https://github.com/nodejs/node/issues/22107

Similarly to the previous commit, this API allows prohibiting usage of certain features inside a worker thread. Refs: https://github.com/nodejs/node/issues/22107

Add `--disable=featureA,featureB,...` and `--experimental-access-control` flags that control (and enable) `process.accessControl` at startup.

ryzokuken · 2018-10-27T14:22:22Z

@addaleax is this abandoned? Would you like someone else to pick this up or sth?

fhinkel · 2019-10-26T23:06:49Z

ping @addaleax Should we close this? Please re-open if it's still needed.

cactysman · 2022-02-28T23:43:23Z

Java has something similar where you can limit what's possible to do. Imagine a kind of sandbox where you deny any kind of network request, just like iframes on the web can be limited.

Of course you could use the vm module to create a new context with network stuff stripped, but Node offering you ways to control that stuff on a lower level might make more sense.

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. discuss Issues opened for discussions and feedbacks. experimental Issues and PRs related to experimental features. labels Aug 3, 2018

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 3, 2018

addaleax added process Issues and PRs related to the process subsystem. worker Issues and PRs related to Worker support. labels Aug 3, 2018

benjamingr reviewed Aug 3, 2018

View reviewed changes

vsemozhetbyt reviewed Aug 3, 2018

View reviewed changes

addaleax force-pushed the access-control branch 2 times, most recently from e2631eb to bf473c2 Compare August 8, 2018 22:09

addaleax force-pushed the access-control branch from bf473c2 to 58f4334 Compare August 8, 2018 23:05

addaleax force-pushed the access-control branch from 9526757 to 1038a8f Compare August 8, 2018 23:19

addaleax mentioned this pull request Aug 9, 2018

Discussion Towards a Security Model nodejs/security-wg#376

Closed

arcanis mentioned this pull request Aug 14, 2018

Proposal to provide package safety check yarnpkg/rfcs#96

Open

addaleax added 3 commits August 19, 2018 12:43

process: initial impl of feature access control

8537311

Implement `process.accessControl`, a simple API for restricting usage of certain in-process APIs. Refs: https://github.com/nodejs/node/issues/22107

worker: add feature access control API

b3cfaed

Similarly to the previous commit, this API allows prohibiting usage of certain features inside a worker thread. Refs: https://github.com/nodejs/node/issues/22107

cli: add flags for access control

b47f3eb

Add `--disable=featureA,featureB,...` and `--experimental-access-control` flags that control (and enable) `process.accessControl` at startup.

addaleax force-pushed the access-control branch from 1038a8f to b47f3eb Compare August 19, 2018 10:44

bmeck mentioned this pull request Sep 14, 2018

Security Policy Tracking Issue nodejs/security-wg#404

Closed

10 tasks

targos mentioned this pull request Dec 8, 2018

Feature Request: Package Permissions #24908

Closed

mcollina mentioned this pull request Dec 13, 2018

Suggestion: add standard methods to limit APIs dependencies can use nodejs/package-maintenance#92

Closed

lirantal mentioned this pull request Jan 29, 2019

Yarn's Future - v2 and beyond yarnpkg/yarn#6953

Open

Trott mentioned this pull request May 6, 2019

repl: properly handle uncaughtException #27151

Closed

4 tasks

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:52

lirantal mentioned this pull request Sep 29, 2019

Userspace network access control experiment nodejs/security-wg#580

Closed

fhinkel closed this Oct 26, 2019

addaleax mentioned this pull request Nov 8, 2019

Is there a flag in nodejs runtime that is set when the bootstrap process is over and it starts executing userland code? nodejs/help#2259

Closed

jasnell mentioned this pull request May 21, 2020

[WIP] src,lib: policy permissions #33504

Closed

4 tasks

addaleax mentioned this pull request Jun 23, 2020

Need helping finding some key areas of the source code (for sandboxing) nodejs/help#2789

Closed

tniessen mentioned this pull request Aug 4, 2021

Add a global Navigator object #39540

Closed

RafaelGSS mentioned this pull request Apr 20, 2022

Permission Model nodejs/security-wg#791

Closed

addaleax mentioned this pull request Jul 27, 2022

src,process: initial permission model implementation #44004

Merged


		Currently only boolean options are supported.

		In particular, thes settings affect the following features of the Node.js API:

process: initial impl of feature access control #22112

process: initial impl of feature access control #22112

Conversation

addaleax commented Aug 3, 2018

Checklist

nodejs-github-bot commented Aug 3, 2018

devsnek commented Aug 3, 2018

addaleax commented Aug 3, 2018

benjamingr commented Aug 3, 2018

devsnek commented Aug 3, 2018

benjamingr Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

addaleax Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

addaleax commented Aug 3, 2018

jasnell commented Aug 3, 2018

addaleax commented Aug 3, 2018

jasnell commented Aug 3, 2018 • edited Loading

addaleax commented Aug 3, 2018

jasnell commented Aug 3, 2018

vsemozhetbyt left a comment

Choose a reason for hiding this comment

vsemozhetbyt Aug 3, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 3, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 3, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 3, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 3, 2018

Choose a reason for hiding this comment

This comment was marked as resolved.

vsemozhetbyt Aug 3, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 3, 2018

Choose a reason for hiding this comment

addaleax commented Aug 3, 2018

bengl commented Aug 3, 2018

addaleax commented Aug 3, 2018

jasnell commented Aug 3, 2018

not-an-aardvark commented Aug 3, 2018

bengl commented Aug 3, 2018

addaleax commented Aug 3, 2018

Qard commented Aug 3, 2018

not-an-aardvark commented Aug 3, 2018

gabrielschulhof commented Aug 3, 2018 via email

addaleax commented Aug 8, 2018 • edited Loading

addaleax commented Aug 8, 2018 • edited Loading

arcanis commented Aug 14, 2018 • edited Loading

ryzokuken commented Oct 27, 2018

fhinkel commented Oct 26, 2019

cactysman commented Feb 28, 2022

benjamingr Aug 3, 2018 •

edited

Loading

addaleax Aug 3, 2018 •

edited

Loading

jasnell commented Aug 3, 2018 •

edited

Loading

addaleax commented Aug 8, 2018 •

edited

Loading

addaleax commented Aug 8, 2018 •

edited

Loading

arcanis commented Aug 14, 2018 •

edited

Loading