search

npm / cli

the package manager for JavaScript
https://docs.npmjs.com/cli/
Other
8.21k stars 3k forks source link

[BUG] NPM allows insecure code execution by configuration file #4101

Open rotem-cider opened 2 years ago

rotem-cider commented 2 years ago

Is there an existing issue for this?

This issue exists in the latest npm version

Current Behavior

Intro

NPM is a package manager used in our CI/CD environments, We are using it to download and install all our different dependencies inside our building phase for production.

In order to use it safely, we use some precautions, We try to minimize as much as possible its exposure to secrets but still sometimes we have no choice but to add them.

Reading the documentation here and best practices using the cli around the internet it is best to run in ci environments with npm ci --ignore-scripts which should not run any scripts, additionally, we saw that npm sees bypassing this command as a high severity

Researching this subject we found out that a developer or malicious actor with access to the codebase can in fact force npm to run scripts although we configured it explicitly to ignore scripts. Talking with the bounty team the response was that this is not eligible for bounty and is not considered a risk as this is the intended behavior of npm.

We agree that this is a design issue, but nevertheless, this can be used as an attack vector by actors, we want to raise the issue here and understand what mitigations can we have in the meantime, and if this design will be changed in the near future?

The Issue

When adding a .npmrc file to our repo, npm will pick up the file and read its configuration. This means that any command using npm will automatically pick up the configuration file.

It is possible to inject a configuration that will then execute code in the installation context and attempt to attack the installation environment by two vectors:

  1. Adding git=${PWD}/rce.sh to .npmrc and adding an installation from git to the package.json, such as "dependencies": { "ini":"git://github.com./npm/ini.git#v2.0.0" } will execute the rce.sh script when running npm install version 7,8
  2. Adding onload-script=${PWD}/rce to .npmrc will invoke the rce.js script in the library when running ANY npm version 6 command

Expected Behavior

As we are not expecting to run any scripts when running with "--ignore-scripts" this is can and will affect our devops installations if a malicious actor gains access to parts of our codebase.

I understand that this was not the intended behavior of the flag, but it became the only security measure in use and implies that there will be no unintended scripts running

Steps To Reproduce

Using npm version 7,8

  1. npm init
  2. npm i --save "git://github.com./npm/ini.git#v2.0.0"
  3. echo "git=\${PWD}/test.sh" > .npmrc
  4. echo 'echo "HACKED!!!" > poc.txt && git $@' > test.sh
  5. rm -rf node_modules

Attack phase

  1. npm ci --ignore-scripts
  2. cat poc.txt

Using npm version 6

  1. npm init
  2. echo "onload-script=\${PWD}/rce" > .npmrc
  3. echo "console.log('Hacked") > rce.js

Attack phase

  1. npm ci --ignore-scripts

Environment

; "project" config from fuzz/npm-lion/.npmrc git = "npm-lion/test.sh"

ljharb commented 2 years ago

How is this any different than running npm run foo and defining the "foo" script to run insecure code? If someone has control of the local filesystem, they can always run whatever code they want (albeit with constrained permissions)

ntindle commented 2 years ago

I would argue that to be true if it wasn’t a CI command. Even if this isn’t changed, it’s a good thing to document.

rotem-cider commented 2 years ago

Hey @ljharb , this is different in the case where the developer does not control the ci environment.

in a scenario where I know builds will run with “npm ci —ignore-scripts” (so all the normal scripts won’t run) then the developer should not be able to force the build scripts to execute his code.

because of this scenario the developer can always bypass with this method and execute his own code.

if for instance in the pipeline there was a npm run foo, then the developer would be able to change the foo script and run it. Then best practice would be to run it in a subshell without any env variables which may leak.

worst in npm6 any command can be hijacked, so even “npm publish” that usually has write permissions will execute the developers code

aviadhahami commented 2 years ago

I agree with @rotem-cider. While I'm aware this is a feature, a user who's unfamiliar with .npmrc files may run npm x and the shell will be executed.

The major difference between having a malicious script foo and this case, is that foo will run iff one explicitly runs the foo script(npm run foo), while here even a basic npm ci will result in RCE (regardless of --ignore-scripts specification) This is unexpected behavior for the untrained eye.

while I know that scripts can be called using the pre/post hooks, they can be disabled using --ignore-scripts. In the custom binary case the flags are not respected

ntindle commented 2 years ago

https://docs.npmjs.com/cli/v8/commands/npm-ci#ignore-scripts

The way I read this is that is lifecycle script behavior is undefined according to the docs.

DanielRuf commented 2 years ago

I found this issue while reading the article at https://snyk.io/blog/visibly-invisible-malicious-node-js-packages-when-configuration-niche-meets-invisible-characters/

That reminds me of the log4j CVE labeled as RCE which was an ACE and was about something similar (configuration file changes or code changes could allow arbitrary code execution).

I agree with @ljharb.

if for instance in the pipeline there was a npm run foo, then the developer would be able to change the foo script and run it

How is this different from simply changing the code in your own fork or with GitHub Codespaces?

There are some security recommendations regarding pipelines.

Also on GitHub Actions you can define some further settings (who is allowed to run the actions, which actions are allowed to run, who can run the actions for PRs, ...).

Bildschirmfoto 2022-03-01 um 23 29 00 Bildschirmfoto 2022-03-01 um 23 29 10 Bildschirmfoto 2022-03-01 um 23 29 19

Further information: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#good-practices-for-mitigating-script-injection-attacks https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#modifying-the-contents-of-a-repository

https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-nodejs-or-python

I didn't verify this but shouldn't the highest verbosity level of npm make such tricks visible?

rotem-cider commented 2 years ago

@DanielRuf you have some valid points but are reliant on the fact that all companies work with strict configurations.

First about log4j, it is a full easy to create and exploit remote code execution which affected thousands of companies by default configuration. I think the comparison was wrong, also that this vulnerability is not critical like log4j but very important.

About the ability to code execute in the pipeline, as the pipeline is a very sensitive area which directly controls what will be in our final production build but also has Access to many secrets a company should protect it with proper security measures

adding the security measures you mentioned In GitHub is super important, especially to make sure no untrusted developer can execute code in secret aware areas in the ci.

Unfortunately these measures are not always used, not enough and companies have many different ci systems which have different security measures (jenkins, circleci, codefresh, …)

as for the GitHub security measures:

  1. As this bypass happens inside the GitHub action, the measure you mentioned about when running GitHub actions is not relevant
  2. Not running for first time contributors may help, but bypassing it is super easy, just request to fix a comma or dot in the repo, and you become a validated commuter
  3. Read permissions give you a lot, especially when your goal is to read secrets and use them externally

When separating properly the different actions and giving them proper permissions we can avoid such issues but when we have a wildcard that allows users to submit a file and wait for shell our job as security engineers gets much more complicated

you can read more implications about different perspectives of attacking the ci in Omer’s excellent blog post here - https://www.cidersecurity.io/blog/research/ppe-poisoned-pipeline-execution/

ljharb commented 2 years ago

@rotem-cider if you use a dependency's code, you are trusting it. You run your dependencies' code in dev, CI, and often prod. Someone who wants to use this as an attack vector can also just write code to do it, instead of relying on npm to do it - and npm typically never runs in prod, making it safer than "node itself" in this regard.

DanielRuf commented 2 years ago

@rotem-cider regarding log4j, I do not mean the original CVE but this one: CVE-2021-44832 (ACE caused by manipulated configuration, where attackers need access to your sourcecode)

Regarding the other points: you can always make it more strict, please see my links and screenshots.

Still, someone who has access to the sourcecode can do anything. That has nothing to do with npm and any other program with a config and even env variables can be theoretically abused. This is by design and technically no real vulnerability.

lirantal commented 2 years ago

I think the nuance here, at least with regards to Aviad's article on employing both trojan source and configuration overwriting is that this doesn't have to be a code/dependency issue at all. If a collaborator is able to somehow trick you into making a change to the project's .yarnrc / .npmrc and also to somehow sneak in the malicious code as part of the repository's source code then they put the project maintainers at risk.

I do agree that at this is however not much of a higher elevated risk than anyone just proposing a pull request with malicious code/intent behind it. That said, the interesting use of trojan source attacks (invisible characters, etc) for high-impact insecure defaults is quite interesting. At the very least, this reminds me my research about lockfile insecurities which will mostly go unnoticed to anyone.

So, to be more practical, I'd ask not whether we want to spend more time to convince each other if it's a vulnerability or not, but what actions can we drive forward to advance security for end-users. Are there changes we can drive to package managers with regards to secure defaults?

ljharb commented 2 years ago

That still requires us to agree on what defaults are more secure. What do you suggest here?

DanielRuf commented 2 years ago

Rereading the first post I wonder how this would help when someone would export or create an alias (using bash files in the repo, for example for husky / git hooks, ...) to use some other binary than the original npm or git, which would ultimately lead to the same outcome (and is harder to track, at least environment related stuff, and could remove itself from the file using sed or some self-destruct method).

For any solution I think about (disable loading of config files, disable git usage, disable programmatic usage of npm) there are other ways to achieve the same. GitHub and others would need some "protected files" feature similar to "protected branches" - but maybe it's too similar to the code owners solution (https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners).

Or maybe this for package managers:

lirantal commented 2 years ago

That still requires us to agree on what defaults are more secure. What do you suggest here?

One idea I can think of is as follows:

  1. The entry for yarnPath: “./evil.sh” also includes a hash signature
  2. The package manager keeps a history of that hash
  3. Anytime that config changes, or is a new one, the user is prompted to validate the config and the executable before it is actually used

Not saying that's ideal, but potentially adds a bit more friction before users are executing arbitrary commands automatically.

ljharb commented 2 years ago

What is “yarnPath”?

lirantal commented 2 years ago 
yarn-path "./bin/yarn"
Instructs yarn to defer to another Yarn binary for execution. Useful if you want to bundle Yarn into your repository and have everyone use the same version for consistency. This was introduced in Yarn 1.0, so all developers must have Yarn >= 1.0 installed.

Value must be a relative file path, or false to disable (default).

Source: https://classic.yarnpkg.com/lang/en/docs/yarnrc/#toc-yarn-path

ljharb commented 2 years ago

This is npm’s repo.

lirantal commented 2 years ago

I was "riding" on the example that Daniel pointed out from the Snyk article on trojan source + yarn config. Similar issues exist for npm, as Rotem pointed out in Cider's article.

ljharb commented 2 years ago

ah, i hadn’t read the links. What would be a concrete example for npm that wouldn’t be outweighed by the DX cost? (For example, retaining and updating checksums any time scripts are edited is typically untenably frictionful)

rotem-cider commented 2 years ago

For me the main issue here is when an organization is separating permissions between regular developer capabilities to Devops capabilities bypasses to execute code are very easy by using these methods

Aliasing the npm or any other tool needs command execution permissions.

The jump from static configuration files which we do want to give the developers control over and allowing them to execute code in sensitive ci/cd environments can be dangerous.

codeowners is a nice mechanism but does not control if the pipeline should run, but more if it should require approval after it already ran.

Some suggestions just throwing ideas here:

  1. Status check for specific files each time they change - can be an overhead for some organizations
  2. Split the configuration files
  3. Sign and check configuration files by external approval, will need another process to decide if they are valid
  4. Deprecate the usage of default configuration or disable using those aliased commands when loading default configuration.
  5. As mentioned earlier, hashing the config file but will need to maintain state of approved hashes which needs a whole new system
ljharb commented 2 years ago

You could certainly build a system that required approval before running a pipeline; that’s well beyond the scope of a cli tool.

Hashing the config file doesn’t solve the problem unless you add an untenable amount of friction on top; options 1-3 there have nothing to do with npm. What default configuration are you thinking of in option 4?

rotem-cider commented 2 years ago

About the 4th option, a big part of the problem is that cli tools pick up the default configuration file from the current working directory. This has many pros and almost all cli tools do that. But it does introduce the option for unexpected configuration overrides.

The solution here can be to deprecate the picking up of .npmrc automatically and add the need to specifically request it’s usage by adding a flag

As this is a breaking change, I think if going down this road will need to do it in multiple phases.

first add a warning to the cli that it may pick up unintended configuration files while recommending users to select a specific configuration file

Then next phase can be to break if no config file or more practically stop processing certain dangerous commands if configuration file is not explicitly set.

Unfortunately I think this will be a complicated decision in this stage

ljharb commented 2 years ago

It's more than just a breaking change; it's far too much friction - especially since, as you indicated, virtually every tool does this by default, which means that's what users almost universally expect.

rotem-cider commented 2 years ago

I agree, maybe just adding a warning when executing will help with user education and a comment in documentation and revisiting this in next major version

see similar changes done in other tools

https://github.com/bridgecrewio/checkov/commit/d5ca203b586582f77e227c2757d77f23f1ab8cf7

ljharb commented 2 years ago

Sure - but "a file in the repo" is a trusted source.

DanielRuf commented 2 years ago

first add a warning to the cli that it may pick up unintended configuration files while recommending users to select a specific configuration file

Normally they are merged / combined.,

I agree with @ljharb that this is too much friction. We have config files on a few more levels like the user, the global, project root, subfolder (monorepo) and so on.

If some of your coworkers introduces a malicious change in your code and commits this, then you have a bigger problem and this is people-related - a malicious actor who has direct access to the sourcecode can do anything in theory. Code owner configurations, protected branches, merge request policies, CI policies and so on mitigate some scenarios.

aviadhahami commented 2 years ago

Budging in here 👋 [disclosure - I wrote the article @DanielRuf refd to]

What if we put a verification system behind a flag(?) and if the dev opted in - then perform the verification?

assuming opted in - all opts start falsy, and we can allow a granular pick of features

ie:

{
    "strictRC": true, // <-- the feature flag
    "rcOpts":{ 
        "allowCustomBinary": false,
        "allowCustomGitBinary": true
    }
}
// disregard namings ;)

this way it's: 0/ less to no friction (behind a flag the defaults to false) 1/ allows granular control over rc files (regardless of which one was picked) 2(optional)/ allows future introduction of this system as a breaking change as a double security gate (rc file + explicit allowance of feature)

DanielRuf commented 2 years ago

As long as your environment (bashrc, profile, ...) is there you can bypass these "checks" by pointing the aliases / binaries (git and more) to something else.