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

process: initial impl of feature access control #22112

Closed
wants to merge 3 commits into from
Closed

process: initial impl of feature access control #22112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8537311
process: initial impl of feature access control
addaleax Aug 3, 2018
b3cfaed
worker: add feature access control API
addaleax Aug 3, 2018
b47f3eb
cli: add flags for access control
addaleax Aug 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ If this flag is passed, the behavior can still be set to not abort through
[`process.setUncaughtExceptionCaptureCallback()`][] (and through usage of the
`domain` module that uses it).

### `--disable=...`
<!-- YAML
added: REPLACEME
-->

Disable certain features of Node.js at startup. See [`process.accessControl`][]
for more details.

### `--enable-fips`
<!-- YAML
added: v6.0.0
Expand All @@ -60,6 +68,15 @@ added: v6.0.0
Enable FIPS-compliant crypto at startup. (Requires Node.js to be built with
`./configure --openssl-fips`.)

### `--experimental-access-control`
<!-- YAML
added: REPLACEME
-->

Allow disabling certain features of Node.js at runtime.
See [`process.accessControl`][] for more details.
The `--disable` flag implies this flag.

### `--experimental-modules`
<!-- YAML
added: v8.5.0
Expand Down Expand Up @@ -688,6 +705,7 @@ greater than `4` (its current default value). For more information, see the
[`--openssl-config`]: #cli_openssl_config_file
[`Buffer`]: buffer.html#buffer_class_buffer
[`SlowBuffer`]: buffer.html#buffer_class_slowbuffer
[`process.accessControl`]: process.html#process_access_control
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[Chrome DevTools Protocol]: https://chromedevtools.github.io/devtools-protocol/
[REPL]: repl.html
Expand Down
9 changes: 9 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,14 @@ found [here][online].
<a id="nodejs-error-codes"></a>
## Node.js Error Codes

<a id="ERR_ACCESS_DENIED"></a>
### ERR_ACCESS_DENIED

> Stability: 1 - Experimental

This error is thrown when an attempt was made to use a feature of Node.js
which was previously disabled through the [`process.accessControl`][] mechanism.

<a id="ERR_AMBIGUOUS_ARGUMENT"></a>
### ERR_AMBIGUOUS_ARGUMENT

Expand Down Expand Up @@ -1867,6 +1875,7 @@ A module file could not be resolved while attempting a [`require()`][] or
[`net`]: net.html
[`new URL(input)`]: url.html#url_constructor_new_url_input_base
[`new URLSearchParams(iterable)`]: url.html#url_constructor_new_urlsearchparams_iterable
[`process.accessControl`]: process.html#process_access_control
[`process.send()`]: process.html#process_process_send_message_sendhandle_options_callback
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`require()`]: modules.html#modules_require
Expand Down
163 changes: 163 additions & 0 deletions doc/api/process.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1996,6 +1996,148 @@ Will generate an object similar to:
tz: '2016b' }
```

## Access Control

> Stability: 1 - Experimental

This feature is experimental and needs to be enabled by passing
the `--experimental-access-control` flag, or the
`--disable=restrictionA,restrictionB,...` flag to Node.js.

The `--disable` flag takes a comma-separated list of identifiers as described
below. For example, it can be used as `--disable=fsRead,fsWrite`.

### process.accessControl.apply(restrictions)
<!-- YAML
added: REPLACEME
-->

* `restrictions` {Object} A set of access restrictions that will be applied
to the current Node.js instance. The format should be the same as the one
returned by [`process.accessControl.getCurrent()`][]. Omitted keys will
default to `true`, i.e. to retaining the relevant permissions.

*Warning*: This does not provide a full isolation mechanism. Existing resources
and communication channels may be used to circumvent these measures, if they
have been made available before the corresponding restrictions have been put
into place.

This API is recent and may not be complete.
Please report bugs at https://github.com/nodejs/node/issues.

Operations started before this call are not undone or stopped.
Features that were previously disabled through this call cannot be re-enabled.
Child processes do not inherit these restrictions, whereas [`Worker`][]s do.
Copy link
Member

Choose a reason for hiding this comment

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

This should be clarified. For instance, if I use fs.createWriteStream() now to open a file for writing, then set the access control policy, will I be able to use the write stream or no?

Another question: if the Node.js process stdout/stderr are redirected to a pipe and fsWrite is denied, what happens when the process attempts to write out?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell This is a very good point.

For instance, if I use fs.createWriteStream() now to open a file for writing, then set the access control policy, will I be able to use the write stream or no?

You will not, because of the way fs streams are implemented – as individual fs.read and fs.write operations in JS.

However, existing network sockets can still be used, because we implement them in a very abstract way in C++ that is not specific to the kind of resource (e.g. net & tty all use the same code). We can align behaviours if we think that that makes sense, though.

I’ve pushed docs & test changes to account for these scenarios.

Another question: if the Node.js process stdout/stderr are redirected to a pipe and fsWrite is denied, what happens when the process attempts to write out?

Assuming you meant “redirected to a file”: An exception occurs. console.log and friends silence those, but using raw process.stdout.write() will throw.


The following code removes some permissions of the current Node.js instance:

```js
process.accessControl.apply({
childProcesses: false,
createWorkers: false,
fsRead: false,
fsWrite: false,
loadAddons: false,
setV8Flags: false
});
```

Keys not listed in the object, or with values not set to `false`,
are unaffected.

If a key that is not `childProcesses` or `loadAddons` is set to `false`,
the `childProcesses` and `loadAddons` feature set will also be disabled
automatically.

See [`process.accessControl.getCurrent()`][] for a list of permissions.

### process.accessControl.getCurrent()
<!-- YAML
added: REPLACEME
-->

* Returns: {Object} An object representing a set of permissions for the
current Node.js instance of the following structure:

<!-- eslint-skip -->
```js
{ accessInspectorBindings: true,
Copy link
Member

@benjamingr benjamingr Aug 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@addaleax addaleax Aug 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

childProcesses: true,

This comment was marked as resolved.

Sign in to view

createWorkers: true,
fsRead: true,
fsWrite: true,
loadAddons: true,
modifyTraceEvents: true,
netConnectionless: true,
netIncoming: true,
netOutgoing: true,
setEnvironmentVariables: true,
setProcessAttrs: true,
setV8Flags: true,
signalProcesses: true }
```

Currently only boolean options are supported.

In particular, these settings affect the following features of the Node.js API:
Copy link
Contributor

@davisjam davisjam Aug 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I find fsRead and fsWrite a bit confusing, since there are already APIs of this name.

Since this is for access control, perhaps we should use the naming convention <verb> [<preposition>] <noun>, e.g. writeToFs and readFromFs. This would affect the childProcesses, the net*, and perhaps others.

Thoughts on a consistent convention for "write" vs. "read" operations. e.g. setProcessAttrs -> changeProcessAttrs, fsWrite -> changeFs, that kind of thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, interesting … is the only reason for this that these names refer to specific functions? The current naming scheme seems a bit more useful if we have multiple keys that we want to appear as groups when sorted alphabetically (e.g. net*).

Copy link
Contributor

Choose a reason for hiding this comment

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

is the only reason for this that these names refer to specific functions?

In the specific case of fsRead and fsWrite it is confusing, yes.

But keeping naming consistent within an API helps users. Especially since you say:

Keys not listed in the object, or with values not set to `false`, are unaffected.

This means if I mis-type a name then I will have a false sense of security. I would rather have a little verbosity than a security issue.

if we have multiple keys that we want to appear as groups

I imagine this being useful primarily for APIs in the same module. You might address that with an increasingly verbose naming convention <module>_<verb>[<preposition>]<noun>. Are there cases where you would want to group keys that are part of different modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

So … what’s the noun for fs then? :) fs_writeFilesystem? 😄

My gut still says that we want something a) short/succinct and b) with the module coming first.

Maybe fsObserve/fsModify?


- `accessInspectorBindings`:
- [`inspector.open()`][]
- [`inspector.Session.connect()`][]
- `childProcesses`:
- [`child_process`][`ChildProcess`] methods
- `createWorkers`:
- [`worker_threads.Worker`][`Worker`]
- `fsRead`:
- All read-only [`fs`][] operations, including watchers
- This always includes `fs.open()` and `fs.openSync()`, even when
writing to files.
- Existing `fs.ReadStream` instances will not continue to work.
- [`os.homedir()`][]
- [`os.userInfo()`][]
- [`require()`][]
- [`process.stdin`][] if this stream points to a file.
- Note that `fsRead` and `fsWrite` are distinct permissions.
- `fsWrite`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would be worthwhile to remark on open explicitly, since it's a vectored API whose behavior depends on the flags (and the access check looks at the flags)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, makes sense. Done!

- All other [`fs`][] operations
- `fs.open()` and `fs.openSync()` when flags indicate writing or appending
- Existing `fs.WriteStream` instances will not continue to work.
- [`net`][] operations on UNIX domain sockets
- [`process.stdout`][] and [`process.stderr`][], respectively, if those
streams point to files.
- `loadAddons`:
- [`process.dlopen()`][] and [`require()`][] in the case of native addons
- `modifyTraceEvents`:
- [`tracing.disable()`][] and [`tracing.enable()`][]
- `netConnectionless`:
- [UDP][] operations, including sending data from existing sockets
- `netIncoming`:
- [`server.listen()`][`net.Server`]
- Receiving or sending data on existing sockets is unaffected.
- `netOutgoing`:
- [`socket.connect()`][`net.Socket`]
- [`dns`][] requests
- Receiving or sending data on existing sockets is unaffected.
- `setEnvironmentVariables`:
- Setting/deleting keys on [`process.env`][]
- `setProcessAttrs`:
- [`process.chdir()`][]
- [`process.initgroups()`][]
- [`process.setgroups()`][]
- [`process.setgid()`][]
- [`process.setuid()`][]
- [`process.setegid()`][]
- [`process.seteuid()`][]
- [`process.title`][] setting
- `setV8Flags`:
- [`v8.setFlagsFromString()`][]
- `signalProcesses`:
- [`process.kill()`][]
- Debugging cluster child processes

This function always returns a new object. Modifications to the returned object
will have no effect.

## Exit Codes

Node.js will normally exit with a `0` status code when no more async
Expand Down Expand Up @@ -2057,24 +2199,44 @@ cases:
[`Worker`]: worker_threads.html#worker_threads_class_worker
[`console.error()`]: console.html#console_console_error_data_args
[`console.log()`]: console.html#console_console_log_data_args
[`dns`]: dns.html
[`domain`]: domain.html
[`end()`]: stream.html#stream_writable_end_chunk_encoding_callback
[`fs`]: fs.html
[`inspector.open()`]: inspector.html#inspector_inspector_open_port_host_wait
[`inspector.Session.connect()`]: inspector.html#inspector_session_connect
[`net`]: net.html
[`net.Server`]: net.html#net_class_net_server
[`net.Socket`]: net.html#net_class_net_socket
[`os.constants.dlopen`]: os.html#os_dlopen_constants
[`os.homedir()`]: os.html#os_os_homedir
[`os.userInfo()`]: os.html#os_os_userinfo_options
[`process.accessControl.getCurrent()`]: #process_process_accesscontrol_getcurrent
[`process.argv`]: #process_process_argv
[`process.chdir()`]: #process_process_chdir_directory
[`process.dlopen()`]: #process_process_dlopen_module_filename_flags
[`process.env`]: #process_process_env
[`process.execPath`]: #process_process_execpath
[`process.exit()`]: #process_process_exit_code
[`process.exitCode`]: #process_process_exitcode
[`process.hrtime()`]: #process_process_hrtime_time
[`process.hrtime.bigint()`]: #process_process_hrtime_bigint
[`process.initgroups()`]: #process_process_initgroups_user_extragroup
[`process.kill()`]: #process_process_kill_pid_signal
[`process.setegid()`]: #process_process_setegid_id
[`process.seteuid()`]: #process_process_seteuid_id
[`process.setgid()`]: #process_process_setgid_id
[`process.setgroups()`]: #process_process_setgroups_groups
[`process.setuid()`]: #process_process_setuid_id
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`process.title`]: #process_process_title
[`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
[`setTimeout(fn, 0)`]: timers.html#timers_settimeout_callback_delay_args
[`tracing.disable()`]: tracing.html#tracing_tracing_disable
[`tracing.enable()`]: tracing.html#tracing_tracing_enable
Copy link
Contributor

Choose a reason for hiding this comment

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

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

[`v8.setFlagsFromString()`]: v8.html#v8_v8_setflagsfromstring_flags
[Android building]: https://github.com/nodejs/node/blob/master/BUILDING.md#androidandroid-based-devices-eg-firefox-os
[Child Process]: child_process.html
Expand All @@ -2089,4 +2251,5 @@ cases:
[Signal Events]: #process_signal_events
[Stream compatibility]: stream.html#stream_compatibility_with_older_node_js_versions
[TTY]: tty.html#tty_tty
[UDP]: dgram.html
[Writable]: stream.html#stream_writable_streams
13 changes: 13 additions & 0 deletions doc/api/worker_threads.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,13 +305,21 @@ if (isMainThread) {
```

### new Worker(filename[, options])
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/???
description: The `accessControl` option was added.
-->

* `filename` {string} The path to the Worker’s main script. Must be
either an absolute path or a relative path (i.e. relative to the
current working directory) starting with `./` or `../`.
If `options.eval` is true, this is a string containing JavaScript code rather
than a path.
* `options` {Object}
* `accessControl` {Object} A set of access restrictions that will be applied
to the worker thread. See [`process.accessControl.apply()`][] for details.
* `eval` {boolean} If true, interpret the first argument to the constructor
as a script that is executed once the worker is online.
* `workerData` {any} Any JavaScript value that will be cloned and made
Expand All @@ -327,6 +335,10 @@ if (isMainThread) {
* stderr {boolean} If this is set to `true`, then `worker.stderr` will
not automatically be piped through to `process.stderr` in the parent.

Note that if `accessControl.fsRead` is set to `false`, the worker thread will
not be able to read its main script from the file system, so a source script
should be passed in instead and the `eval` option should be set.

### Event: 'error'
<!-- YAML
added: v10.5.0
Expand Down Expand Up @@ -473,6 +485,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
[`port.on('message')`]: #worker_threads_event_message
[`process.exit()`]: process.html#process_process_exit_code
[`process.abort()`]: process.html#process_process_abort
[`process.accessControl.apply()`]: process.html#process_process_accesscontrol_apply_restrictions
[`process.chdir()`]: process.html#process_process_chdir_directory
[`process.env`]: process.html#process_process_env
[`process.stdin`]: process.html#process_process_stdin
Expand Down
12 changes: 12 additions & 0 deletions doc/node.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ the next argument will be used as a script filename.
.It Fl -abort-on-uncaught-exception
Aborting instead of exiting causes a core file to be generated for analysis.
.
.It Fl -disable Ns = Ns Ar featureA,featureB,...
Disables certain features of Node.js at startup.
A full list of available restriction identifiers can be shown by running:
.Pp
.Sy ./node --experimental-access-control --print 'process.accessControl.getCurrent()'
.
.It Fl -enable-fips
Enable FIPS-compliant crypto at startup.
Requires Node.js to be built with
Expand All @@ -83,6 +89,12 @@ Requires Node.js to be built with
.It Fl -experimental-modules
Enable experimental ES module support and caching modules.
.
.It Fl -experimental-access-control
Allow disabling certain features of Node.js at runtime.
The
.Fl -disable
flag implies this flag.
.
.It Fl -experimental-repl-await
Enable experimental top-level
.Sy await
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/bootstrap/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@
perThreadSetup.setupMemoryUsage(_memoryUsage);
perThreadSetup.setupKillAndExit();

if (process.binding('config').experimentalAccessControl)
process.accessControl = internalBinding('access_control');

if (global.__coverage__)
NativeModule.require('internal/process/write-coverage').setup();

Expand Down
6 changes: 6 additions & 0 deletions lib/internal/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,12 @@ class Worker extends EventEmitter {
publicPort: port2,
hasStdin: !!options.stdin
}, [port2]);

if (typeof options.accessControl === 'object' &&
options.accessControl !== null) {
this[kHandle].applyAccessControl(options.accessControl);
}

// Actually start the new thread now that everything is in place.
this[kHandle].startThread();
}
Expand Down
1 change: 1 addition & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@
'src/js_stream.cc',
'src/module_wrap.cc',
'src/node.cc',
'src/node_access_control.cc',
'src/node_api.cc',
'src/node_api.h',
'src/node_api_types.h',
Expand Down
3 changes: 3 additions & 0 deletions src/cares_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1800,6 +1800,7 @@ class GetHostByAddrWrap: public QueryWrap {
template <class Wrap>
static void Query(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_IF_INSUFFICIENT_PERMISSIONS(env, netOutgoing);
ChannelWrap* channel;
ASSIGN_OR_RETURN_UNWRAP(&channel, args.Holder());

Expand Down Expand Up @@ -1949,6 +1950,7 @@ void CanonicalizeIP(const FunctionCallbackInfo<Value>& args) {

void GetAddrInfo(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_IF_INSUFFICIENT_PERMISSIONS(env, netOutgoing);

CHECK(args[0]->IsObject());
CHECK(args[1]->IsString());
Expand Down Expand Up @@ -2000,6 +2002,7 @@ void GetAddrInfo(const FunctionCallbackInfo<Value>& args) {

void GetNameInfo(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_IF_INSUFFICIENT_PERMISSIONS(env, netOutgoing);

CHECK(args[0]->IsObject());
CHECK(args[1]->IsString());
Expand Down
Loading