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.

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

Integrity checks via policy #23834

Closed
wants to merge 5 commits into from
Closed

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Oct 23, 2018

This is an initial effort to introduce integrity checks into code loading paths for Node.js . It is meant to cover the requirements of Subresource Integrity Checks as laid out previously. I have left the generation of manifests outside of the scope of Node.js for this PR and think this is the minimal viable way to introduce integrity checks in a sufficient manner.

There have been concerns about using absolute paths for file integrity, but I have not found a suitable alternative as of yet and feel that this matches Subresource Integrity where location is related to integrity. A separate mechanism for location-less integrity could be introduced falling more under the style of script-src and is not under the scope of this PR. In particular I lay out why I find starting with location based integrity more conservative and less likely to introduce the same surface as location less with a couple examples.

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

@bmeck bmeck added wip Issues and PRs that are still a work in progress. security Issues and PRs related to security. experimental Issues and PRs related to experimental features. labels Oct 23, 2018
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 23, 2018
@vdeturckheim
Copy link
Member

cc @nodejs/security-wg

@@ -0,0 +1,42 @@
# Policies

This comment was marked as resolved.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Good work! I would an explicit experimental warning if the option is used.

@@ -22,8 +22,9 @@
'use strict';

const { NativeModule } = require('internal/bootstrap/loaders');
const util = require('util');
const { manifest } = require('internal/process/policy');
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer it this file was loaded only if policies are in use. Every module we add the longer it gets Node.js to start.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not convinced this has significant impact. Is there some metric we are trying to keep within? If startup time is more critical we could also take other approaches than avoiding loading code such as taking snapshots. Is this a nit or something that you wish to block on?

Copy link
Member

Choose a reason for hiding this comment

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

+1 on code caches, but it's something that is kind of far in the future as far as I understand.

Is there some metric we are trying to keep within?

@jasnell or @BridgeAR might have some number. However every file adds some millis to startup time.

Considering that it takes very little effort to load it at runtime, why not doing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

extra complexity from my point of view to do so for little gain from my understanding. even just bundling core into a single source text would have bigger savings than this I would think. the less complex the system via conditional branching the easier i find it to reason about and maintain.

Copy link
Member

Choose a reason for hiding this comment

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

This PR gaves us 10%: #20567. Most of those where added with the same reasoning, but as Node.js gets more feature, those pile up.

Copy link
Member

Choose a reason for hiding this comment

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

I am definitely for lazy loading this! It is nothing that is required all the time. Adding snapshots is great and we want to do that on top of this. The current startup time is way to high and adding things as we used to doesn't make things any better. Instead we should always lazy load most files. This will also reduce the memory overhead for a simple Node.js application that does not use these features.

Copy link
Member Author

Choose a reason for hiding this comment

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

lazy loaded now, rebased though

return entries;
};

module.exports = Object.freeze({
Copy link
Member

Choose a reason for hiding this comment

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

why are you freezing? this is internal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Defensive programming practice, habit / no real downside. I'd just leave it as is unless you really want it to change.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer it changed, this likely sit on a hot path for startup, and as far as I know V8 didn't really like freezed object when optimizing.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

}
```

In order to generate integrity strings, a script such as `printf "sha384-$(cat checked.js | openssl dgst -sha384 -binary | base64)"` can be used.
Copy link
Member

Choose a reason for hiding this comment

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

This is not really friendly for people on Windows. We should provide a tool on npm to automate this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can agree to that but a generalized manifest tool seems something that we aren't ready to make a design for while we still are working out the design of manifests themselves.

Copy link
Member

Choose a reason for hiding this comment

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

This is not really a blocker, but something that we should put into the roadmap for this to get out of experimental.

doc/api/errors.md Outdated Show resolved Hide resolved
doc/api/policy.md Outdated Show resolved Hide resolved
doc/api/policy.md Outdated Show resolved Hide resolved
doc/api/policy.md Outdated Show resolved Hide resolved
@vsemozhetbyt
Copy link
Contributor

Memo to not forget) For --experimental-policy flag, these docs need to be updated:

https://github.com/nodejs/node/blob/master/doc/api/cli.md
https://github.com/nodejs/node/blob/master/doc/node.1

lib/internal/policy/manifest.js Show resolved Hide resolved
lib/internal/policy/sri.js Show resolved Hide resolved
return o;
});
manifest = new Manifest(json);
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not lose this error.

Copy link
Member Author

Choose a reason for hiding this comment

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

do we have a recommended practice for nesting errors, we don't to my knowledge

Copy link
Member

Choose a reason for hiding this comment

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

We don't, but I think a user would like to know where the JSON is malformed, reading the file errors or what else could happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

i can add the error.message but don't know where i should put the error.code, adding both would be a bit confusing.

Copy link
Member

Choose a reason for hiding this comment

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

How about not wrapping at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

seems fine, removed the wrapper

lib/internal/errors.js Outdated Show resolved Hide resolved
@@ -733,6 +748,12 @@ Module._extensions['.json'] = function(module, filename) {

// Native extension for .node
Module._extensions['.node'] = function(module, filename) {
var content = fs.readFileSync(filename);
Copy link
Member

Choose a reason for hiding this comment

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

these two lines should definitely be inside the condition

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

src/node_config.cc Outdated Show resolved Hide resolved
@@ -722,6 +731,12 @@ Module._extensions['.js'] = function(module, filename) {
// Native extension for .json
Module._extensions['.json'] = function(module, filename) {
var content = fs.readFileSync(filename, 'utf8');

const moduleURL = pathToFileURL(filename);
Copy link
Member

Choose a reason for hiding this comment

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

should seemingly be inside condition

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -662,6 +663,11 @@ function normalizeReferrerURL(referrer) {
// Returns exception, if any.
Module.prototype._compile = function(content, filename) {

const moduleURL = pathToFileURL(filename);
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Once this has been set, all modules must conform to a policy file passed to the flag:

```sh
node --experimental-policy policy.json app.js
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer syntax like --experimental-policy=policy.json app.js (note the =)

Copy link
Member Author

@bmeck bmeck Oct 23, 2018

Choose a reason for hiding this comment

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

using = doesn't mesh with other cli options like --require

Copy link
Member

Choose a reason for hiding this comment

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

@jasnell @bmeck Fun fact: Because we were previously inconsistent about this w.r.t. different options, both work with the new options parser. Like, you can also currently use --require=... instead of --require …, if you prefer :)

Copy link
Member Author

Choose a reason for hiding this comment

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

changed doc, though autocomplete won't work with the = form, curious why the preference but not really anything of note. I do have some concerns and need to think of if we ever want separate flags for policies if this occupies the = space.


### Integrity Checks

Policy files must use integrity checks with Subresource Integrity strings compatible with the [integrity attribute as present in browsers](https://www.w3.org/TR/SRI/#the-integrity-attribute) associated with absolute URLs.
Copy link
Member

Choose a reason for hiding this comment

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

nit: line wrap at <= 80 chars

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, had to reword a bit

doc/api/policy.md Outdated Show resolved Hide resolved
@deian
Copy link
Member

deian commented Oct 23, 2018

I've expressed my concerns about this on patch on the security-wg slack. Will reach out to chat via hangouts more about this and will circle back here to explain the concerns/resolutions (have other things on my plate now).

@bnoordhuis
Copy link
Member

@bmeck Can you speak more on use cases? Who would use this when and why?

@bmeck
Copy link
Member Author

bmeck commented Oct 23, 2018

@bnoordhuis the main intent here is part of a larger story of forming an idea of an auditable level of trust in code being run on Node.js as laid out in the drive document in the description. As such, this PR addresses a singular concern of the goals laid out.

By introducing the concept of integrity to files, we open a way for users to run code without fear of running code that was not audited (either by tooling or human). The main use cases for this are for running command line applications, not library concerns. When running an application installed via some tool or through an image Node.js does not have a mechanism to check if the source of an application has been tampered with (intentional or not). This PR introduces a means for applications to be run that prevents alteration of source code from what was installed from being loaded through the module system.

Anytime an unaudited application runs it can open the client to malicious behavior as we saw with eslint earlier this year. In the case of eslint, the behavior was limited to eval() but it could just as easily have run in a way that permanently modified files on disk. This PR seeks to address the later concern, as the former is a different topic within the drive document. The intent is to allow a mechanism persistent guarantees that files are what were originally installed, and the ability to audit for potentially bad files.

The majority of installations of CLI applications and usage of 3rd party modules could benefit from this feature by preventing some malicious behaviors from altering files on disk. In particular a focus should be placed on how files can replace existing parts of an application like OWASP's Unrestricted File Upload concerns for HTTP servers. However, anytime arbitrary writes could occur within an application directory this also is of concern even without file uploads.

The means by which these are generated is left open for now while experimentation can occur. Tools related to generating these files need to have a high level of trust. They might wish to defer to extra information like if a known bad integrity exists due to a CVE and audit if that integrity is anywhere in a manifest, similar to checks of package versions. Unlike package version checks which are only concerned with installation time guarantees this PR would be checked whenever a module is loaded, even if someone managed to alter the contents on disk via some persistent attack.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Great work! Look forward to seeing the policy supported through the package.json.

@guybedford
Copy link
Contributor

Re the URLs, can we support relative URLs in the manfiest, taken to be relative to the URL of the manifest itself? That is basically what package maps do.

Object: {
defineProperty
}
} = global;
Copy link
Member

Choose a reason for hiding this comment

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

These double destructures can get a bit weird… wouldn’t const { defineProperty } = Object; do the trick as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

weird? that works and I can simplify but double destructuring should be understandable.

src/node_config.cc Outdated Show resolved Hide resolved
src/node_options.cc Show resolved Hide resolved
@bmeck
Copy link
Member Author

bmeck commented Oct 25, 2018

@guybedford

Great work! Look forward to seeing the policy supported through the package.json.

We should not give the actual policy the ability to be placed in the application directory. It is an easy misconfiguration problem if the application directory is writable by the user running the application. The policy should not be able to be mutated by running a program:

  1. Run program that modifies package.json#policy
  2. On second run use corrupted policy

In addition, ideally policies could be used across multiple applications and things like https://github.com/yarnpkg/pnp-sample-app and https://github.com/npm/tink would be placing the dependencies outside of the application directory and be shared integrities amongst all the places running things.

The ability to request permissions should ideally be placed in a location that tooling such as package managers can pick it up, but it should be a point that is tamper resistant or encourages designs that are less prone to attack.

We should not completely remove the ability to modify a policy from a node application though. The ability to run prompts to modify a policy is a powerful tool that should be looked into (not doable through this PR). Things like prompts about integrity mismatch and missing privileges that can prompt to update permissions of a policy are powerful and help the user understand why something is failing and if they wish to proceed anyway.

@guybedford
Copy link
Contributor

We should not give the actual policy the ability to be placed in the application directory. It is an easy misconfiguration problem if the application directory is writable by the user running the application. The policy should not be able to be mutated by running a program:

So to understand your expectation is that the policy would be in a trusted file without write permissions on the system, separate to the application folder?

Perhaps this should be documented somewhere as it wasn't clear to me that was a strong constraint of the security model being advocated here.

I don't quite follow your argument though as I imagine in most cases the policy would always be in the application folder - and ideally without write permissions, but the folder location seems a separate concern and therefore not a difference in security to me.

@bmeck
Copy link
Member Author

bmeck commented Oct 25, 2018

So to understand your expectation is that the policy would be in a trusted file without write permissions on the system, separate to the application folder?

Perhaps this should be documented somewhere as it wasn't clear to me that was a strong constraint of the security model being advocated here.

The location isn't a constraint, but this is following the fact that most applications have write permissions to their working directory or could chmod files within them to get permissions. Design here is trying to encourage best practice not force it as I don't see a reasonable way to enforce such practice from Node.

I don't quite follow your argument though as I imagine in most cases the policy would always be in the application folder - and ideally without write permissions, but the folder location seems a separate concern and therefore not a difference in security to me.

The difference is 2 fold.

  1. encouraging the ability to share integrity across multiple applications.
  2. encouraging an easy way to move the policy out of a directory via copy or other mechanism to help users get to best practices. This point is based upon current ways of running Node applications and how they often are in mutable directories, which would be problematic for any security manifest.

doc/api/cli.md Outdated Show resolved Hide resolved
@bmeck
Copy link
Member Author

bmeck commented Jan 10, 2019

All comments appear resolved to me, if anything is still missing let me know, otherwise going to merge tomorrow morning.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

@bmeck I see the docs now, not sure what I was doing when I looked before sorry for any confusion.

@mhdawson
Copy link
Member

And to confirm, it was intended to be informative to the TSC, not blocking so at this point you should be good to go.

This enables code loaded via the module system to be checked for
integrity to ensure the code loaded matches expectations.
@bmeck
Copy link
Member Author

bmeck commented Jan 11, 2019

@bmeck
Copy link
Member Author

bmeck commented Jan 11, 2019

@bmeck
Copy link
Member Author

bmeck commented Jan 14, 2019

Need to debug why Windows is failing.

@bmeck
Copy link
Member Author

bmeck commented Jan 15, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/20149/ should be passing now

bmeck added a commit that referenced this pull request Jan 17, 2019
This enables code loaded via the module system to be checked for
integrity to ensure the code loaded matches expectations.

PR-URL: #23834
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@bmeck
Copy link
Member Author

bmeck commented Jan 17, 2019

Landed in 9d5fbeb.

@bmeck bmeck closed this Jan 17, 2019
@bnoordhuis
Copy link
Member

For something that has pretty far reaching implications, this PR only got three sign-offs, even though there have been plenty (partial?) reviewers. Am I the only one who feels somewhat uncomfortable about that? Did it really get all the scrutiny and discussion it needed?

@bmeck
Copy link
Member Author

bmeck commented Jan 22, 2019

@bnoordhuis It was up for quite some time, got tagged for TSC and security, got some TSC comments/review, and was talked about in the slack channel for the security WG, the Realms calls for TC39, and node-dev IRC. If there is something more we could have done, we can continue to iterate on what is required to land pull requests. As it stands, it only is getting scrutiny after landing, even though I tried to advocate in common communication channels for several weeks before landing. If there are objections to the PR we can revert it, and if there are security concerns we certainly should fix them or document them. All in all, landing this seemed appropriate given the time and amount of publicity it had.

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 23, 2019
targos pushed a commit that referenced this pull request Jan 24, 2019
This enables code loaded via the module system to be checked for
integrity to ensure the code loaded matches expectations.

PR-URL: #23834
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
MylesBorins added a commit that referenced this pull request Jan 24, 2019
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687
MylesBorins added a commit that referenced this pull request Jan 25, 2019
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687
@sam-github
Copy link
Contributor

For others searching to understand the context of this better, some of the PR discussion ocurred in slack, starting with this comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory. security Issues and PRs related to security. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.