npm / cli

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

[BUG] `npm ci` succeeds when `package-lock.json` doesn't match `package.json` #2701

Closed icatalina closed 2 years ago

icatalina commented 3 years ago

Current Behavior:

npm ci does not fail when package.json doesn't match package-lock.json

Expected Behavior:

npm ci refuses to install when the lock file is invalid.

Steps To Reproduce:

  1. Manually bump a major version of a dependency in package.json
  2. Run npm ci
  3. It should fail but performs the whole installation

npm@7

image

npm@6

image

Environment:

ext commented 3 years ago

I've ran into this as well, is there any workarounds until this issue is resolved? Perhaps a flag or a new command could be added to verify the two files are in sync.

A consequence of this is that CI builds (which uses npm ci) now passes (as it installs an old version) even if a dependency would normally cause a build failure (such as when a major version as suggested by OP).

aaronadamsCA commented 3 years ago

This is also affecting workspaces.

We are trying to switch from Yarn workspaces to NPM workspaces, and we were confused why npm ci would tolerate an outdated lockfile (whereas yarn install --immutable would error out). This bug seems to explain it.

This is more or less a blocker for switching back to NPM. The NPM CLI tools for managing workspace dependencies are still rough enough that I expect some mistakes; but without this bugfix, I don't know any other way to protect our main branch against a lockfile mismatch. 😕

RA80533 commented 3 years ago

I see this happen quite often when the version field in package.json is bumped without making a corresponding change to package-lock.json, resulting in the files being out of sync.

iggyzap commented 3 years ago

This bug contradicts description from documentation on how npm ci should work:

In short, the main differences between using npm install and npm ci are:

The project must have an existing package-lock.json or npm-shrinkwrap.json.
If dependencies in the package lock do not match those in package.json, npm ci will exit with an error, instead of updating the package lock.
npm ci can only install entire projects at a time: individual dependencies cannot be added with this command.
If a node_modules is already present, it will be automatically removed before npm ci begins its install.
It will never write to package.json or any of the package-locks: installs are essentially frozen.

https://docs.npmjs.com/cli/v7/commands/npm-ci

legopin commented 3 years ago

@darcyclarke This has been on P1 for a while, is there anything we can do to help move it along?

mrmckeb commented 3 years ago

We just hit this too @darcyclarke, as we're moving more repos to npm@7. I can confirm that there are no errors in the CI.

the-spyke commented 2 years ago

Same issue with npm 8.1.0

rotem-cider commented 2 years ago

Hi, This is MAJOR security issue, especially today with all the dependency attacks..

I just found that even our protections using package-lock is not validated, and this bug is open from February.

TLDR.. How can I help fix this?

virkt25 commented 2 years ago

@rotem-cider I don't think this is a security issue, because as I understand it just ignores what's in the package.json and does exactly what package-lock.json says.

To temporary overcome this issue you could switch to npm i and check if file was changed afterwards (either by git or a checksum)

I do think it's a major security issue as well ...

If your package.json has a version marked as ^1.2.3 and your lockfile is set at 1.2.3 followed by a malicious release of said dependency at version 1.2.4 then running npm ci in it's current form will pick up this vulnerability. In the past running npm ci would've only installed 1.2.3 since that's the version pinned in the lock file.

My team's faced numerous issues with unexpected version bumps because of this bug. Also happy to help with this issue if i can

the-spyke commented 2 years ago

I've re-checked and it indeed ignores what is in the lock file and installs by semver from package.json.

rotem-cider commented 2 years ago

For now, I think the best thing is to run npm6 in ci environments.. is there another workaround which can be recommended?

virkt25 commented 2 years ago

For now, I think the best thing is to run npm6 in ci environments.. is there another workaround which can be recommended?

Not ideal but you can pin version in package.json by removing ^ / ~ / etc. and use the newer versions (but it's not ideal imo)

valentjn commented 2 years ago

@virkt25 What about transitive dependencies (where the version can't be pinned)? Do we already know this bug doesn't affect those as well?

To make it explicit, let's say our package.json depends on version 1.0.0 of module A, which in turn depends on ^2.0.0 of module B. Our package-lock.json contains versions 1.0.0 for A and 2.0.0 for B. Now, a new release 2.0.1 of B is published. Which version of B does npm ci install: 2.0.0 or 2.0.1?

rotem-cider commented 2 years ago

Hi @valentjn, I checked the same scenario now, and as I tested it and understood, if there is a package-lock.json it does take the versions from it. but if there was a new dependency it downloads the latest by the semantic versioning.

So with npm ci I wasn't able to upgrade the lower dependency, but I feel like if one will add a dependency that wasn't previously it will be added. Needs more testing and understanding of how it works

the-spyke commented 2 years ago

Or use script to fail if lock-file doesn't match:

CKSUM_BEFORE=$(cksum package-lock.json)

npm i

CKSUM_AFTER=$(cksum package-lock.json)

if [[ $CKSUM_BEFORE != $CKSUM_AFTER ]]; then
    echo "package-lock.json is outdated"
    exit 1
fi
valentjn commented 2 years ago

@the-spyke If there is a new unwanted release of a dependency, wouldn't npm i execute post-install scripts, which have been leveraged in some of the latest attacks?

the-spyke commented 2 years ago

@valentjn in this case you could add --ignore-scripts and use this as a separate job to just check that package-lock.json matches package.json before actually installing and building

virkt25 commented 2 years ago

@virkt25 What about transitive dependencies (where the version can't be pinned)? Do we already know this bug doesn't affect those as well?

To make it explicit, let's say our package.json depends on version 1.0.0 of module A, which in turn depends on ^2.0.0 of module B. Our package-lock.json contains versions 1.0.0 for A and 2.0.0 for B. Now, a new release 2.0.1 of B is published. Which version of B does npm ci install: 2.0.0 or 2.0.1?

I haven't tested this but it's a valid concern. npm v7/8 aren't production ready imo but were made the default with Node 16 which we've upgraded to for some of the newer features.

If you have the choice then @rotem-cider's suggestion is the best one -- use v6.

Tried comparing the code between v6/7 to see if it was a regression that could be quickly fixed but v7 changed the underlying engine entirely so it's likely a feature implementation (don't quote me -- need to dig into the new engine code more to understand what's happening)

the-spyke commented 2 years ago

More testing.

npm ci of version 6 doesn't care if you change ^4.11.2 to ^4.11.0 or 4.11 in package.json. It just installs what package-lock.json has if it matches that version specifier by semver .

But changing to a different version like ^4.12.0 raises an error.

valentjn commented 2 years ago

I just skimmed over the Bug Bounty Program of GitHub (owner of npm). One example of a “medium severity issue” is explicitly given as:

  • package integrity compromise, i.e., downloading a package that does not match the integrity as defined in package-lock.json

Isn't the bug in this issue here doing exactly this? npm downloads a package of a different version than the one defined in package-lock.json. That downloaded package must have a different integrity/hash than the one stored in package-lock.json, because the stored integrity belongs to a different version than that of the downloaded package.

I think this is a major security problem. This issue needs the Security label and a fix ASAP, also in light of all the recent attacks. In the meantime, as an immediate hotfix, npm should IMO disable the npm ci subcommand (or at least print a warning that it's not doing what it's supposed to do, although not many will read that in CI contexts).

icatalina commented 2 years ago

Isn't it the opposite? it just ignores package.json

valentjn commented 2 years ago

@icatalina It ignores package-lock.json and follows package.json when I do the following:

  1. Create new module that depends on version X of some module, for which there is actually a newer version Y available.
  2. Run npm i.
  3. Change the version of dependency in package.json from X to Y.
  4. Verify that package-lock.json still contains version X for the dependency.
  5. Delete node_modules/, but not package-lock.json.
  6. Run npm ci.
  7. Observe that no error occurred and version Y has been installed (look in node_modules/.../package.json).

It did install version X when I changed the dependency version in step 3 from X to ^X (even though Y is available and matches ^X).

icatalina commented 2 years ago

Oh, that's worse than my experience... I thought it wouldn't even touch package.json...

rotem-cider commented 2 years ago

I opened a CVE for proper reference of this issue

https://nvd.nist.gov/vuln/detail/CVE-2021-43616

icatalina commented 2 years ago

@valentjn, just gotten around to testing this. It is pretty bad 🤦

I created a repo with the issue: https://github.com/icatalina/CVE-2021-43616

dominykas commented 2 years ago

There definitely seems to be a bug where npm ci sometimes (not always! I did get a failure in some of my local tests) proceeds with installation, rather than fails, when package-lock.json and package.json are in disagreement.

However, I was not able to reproduce a case where it installs something that matches the semver in package.json, which is different to what is defined in package-lock.json, i.e. I don't see how this is a security issue, since having control of package.json implies control of package-lock.json, i.e. there is no point to make them disagree when you can just modify both to install whatever malicious thing you want installed.

valentjn commented 2 years ago

However, I was not able to reproduce a case where it installs something that matches the semver in package.json, which is different to what is defined in package-lock.json

@dominykas Refer to these steps to have npm install the version given in package.json, ignoring the version given in package-lock.json.

Furthermore, it's not a question of maliciously modifying package.json and/or package-lock.json. It's a question of maliciously publishing updates of dependencies via compromised accounts, without modifying the files. At least, that's how the recent attacks worked:

  1. package.json refers to non-malicious version ~1.0.0 of dependency A
  2. Attacker compromises account of maintainer of A and publishes malicious version 1.0.1
  3. npm i now installs malicious version 1.0.1 and compromises the system (usually, post-install scripts are leveraged to compromise the system at this point, but there are of course more possibilities)

The question is, can a malicious dependency somehow be installed via npm ci when a lockfile exists that doesn't contain the malicious dependency (because the lockfile was created before the malicious dependency was published)? If not, why not (I'd like to understand what npm ci exactly does in this bugged state)? This bug might make this possible, but I haven't had the time to do the proper testing.

dominykas commented 2 years ago

@valentjn you yourself said this:

It did install version X when I changed the dependency version in step 3 from X to ^X (even though Y is available and matches ^X).

npm ci installs the correct thing - I have not seen any examples of bypassing that when package.json and package-lock.json agree with each other.

styfle commented 2 years ago

@valentjn It might be best to give an example with an explicit package name, package version, and npm version.

Kirill89 commented 2 years ago

At Snyk we currently have a lot of noise related to the CVE-2021-43616. We decided to have a closer look at the issue.

We believe that this issue is a software bug rather than vulnerability. We think so because we can't imagine possible attack scenarios where a malicious actor will be able to hijack package.json but not a package-lock.json. And even in this case hijacking package.json means that a malicious actor can do much more damage than just change the version of a package (i.e. change scripts section).

Although we understand that this bug can cause vulnerabilities in some exotic cases (if package.json is a user input of an application).

So, we don't think it is correct to assign a CVE and recommend revoking the CVE-2021-43616.

rotem-cider commented 2 years ago

Hey @Kirill89, First great that Snyk is looking at this vector and researching this closer. I also researched this vector and I also didn't find a way yet for attacking as malware in an external dependency as it does respect the package-lock same as the npm install functionality.

The issue here is that when using "npm ci" I am expecting the npm CLI to verify that the package is strictly written in package-lock and that it is the same hash integrity as before. I hadn't had the time or enough resources to verify all the paths happening inside the arborist package which is in charge of verifying the tree, but sure enough the functionality that we had in npm6 is not the same as in npm7 and leaves loopholes that may be abused by attackers.

One scenario I can think of depends on the CODEOWNERS file, pipelines can be automatically run and even merged, depending on if the package or lock files are in it. The malicious actor can add a rogue package to package.json or reset the package-lock file to something like {} which will still work.

As this is clearly against what is written about the functionality of "npm ci" in the documentation and security teams are relying on it for their security audits and checks I do think this should stay as a CVE and should be addressed.

About the impact, I think that it is per usage and personally feel it should be a medium-high and not critical.

the-spyke commented 2 years ago

@Kirill89 There is no talk about somebody hijacking package.json or package-lock.json. It's much simpler.

People sometimes just forget to check in the updated lock file because it's machine generated and they're humans. This is exactly why we have ci command. And with current "behavior" it will install an unknown version on every CI run instead of failing. And not just every, but the newest version on every run.

It will be as always: somebody hijacks Lodash/whatever popular, publishes a patch version with a backdoor, 1 minute after it will be already sucked into some build job without devs even doing anything.

virkt25 commented 2 years ago

@Kirill89 thanks for chiming in. I get that it's a bug BUT I think it has massive security implications for production environments. Would love to be explained how the scenario below isn't a critical security issue:


npm@6 guaranteed package installs to be "frozen" (as per their documentation) based on the contents of package-lock.json when using npm ci. This made it ideal for CI/CD deployments since it guaranteed the same build based on package-lock.json. It is common for developers to pick up minor/patch bumps by running npm i locally -- this would normally result in changes to package-lock.json BUT not package.json.

Ex: package.json has shortid: "^2.0.0" and so does package-lock.json. Runningnpm iwill leave package.json untouched BUT update package-lock.json so it now hasshortid: "^2.0.4"`.

The new lock file is tested and committed to the application. CI/CD systems always use npm ci since it is supposed to guarantee a frozen install that matches exact versions as defined in package-lock.json.

Version 7 and up

Following the above scenario running npm ci in CI/CD environments can result in a NEW artifact if shortid releases versions 2.0.5 even though package-lock.json doesn't specific this version (and package.json is still at ^2.0.0).

The whole point of npm ci is to guarantee a "frozen" install which it no longer does which in my opinion is a significant security risk and adds instability even otherwise for production applications.

the-spyke commented 2 years ago

Step-by-step:

Prepare a repo. Let's pretend that we installed lodash@4.11.0 some time ago:

$ mkdir npm-2701
$ cd npm-2701
$ npm -v
8.1.0
$ npm init -y
$ npm i lodash@4.11.0
$ npx json -I -f package.json -e 'this.dependencies.lodash="^4.11.0"'
$ npm i

Let's see what we have as initial setup:

$ cat package.json | npx json 'dependencies.lodash'
^4.11.0
$ cat package-lock.json | npx json 'dependencies.lodash'
{
  "version": "4.11.0",
  "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.11.0.tgz",
  "integrity": "sha1-Qo9xcqXpqC6aRZtUOBZImBDsuK8="
}
$ cat ./node_modules/lodash/package.json | npx json 'version'
4.11.0

Okay. It's time to update Lodash to a newer version ^4.11.0 >>> ^4.11.1 which recently came out:

$ npx json -I -f package.json -e 'this.dependencies.lodash="^4.11.1"'
$ git commit -m "Update Lodash to the latest and greatest"
$ git push origin

I have a feeling that I forgot something... Meanwhile on my CI build:

$ git clone
$ cat package.json | npx json 'dependencies.lodash'
^4.11.1
$ cat package-lock.json | npx json 'dependencies.lodash'
{
  "version": "4.11.0",
  "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.11.0.tgz",
  "integrity": "sha1-Qo9xcqXpqC6aRZtUOBZImBDsuK8="
}
$ ls node_modules
ls: cannot access 'node_modules': No such file or directory

Now the fun part. Output below is exact copy from my terminal:

$ npm ci

added 1 package, and audited 2 packages in 939ms

found 0 vulnerabilities
$ cat package.json | npx json 'dependencies.lodash'
^4.11.1
$ cat package-lock.json | npx json 'dependencies.lodash'
{
  "version": "4.11.0",
  "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.11.0.tgz",
  "integrity": "sha1-Qo9xcqXpqC6aRZtUOBZImBDsuK8="
}
$ cat ./node_modules/lodash/package.json | npx json 'version'
4.17.21

You're welcome.

tangobravo commented 2 years ago

Just bitten by this too.

Precondition: A version of a dependency listed in package-lock.json is no longer a semver match for the dependency in package.json Expected behaviour: npm ci should fail with an error. Actual behaviour: npm ci installs the latest version matching the semver in package.json and ignores the lockfile entirely.

Regardless of whether it's a CVE or not, it's clearly a big bug and really worrying that there's no word on anyone working on a fix for it despite affected versions of npm now being the default in the current "Recommended for most users" Node.js installers.

FWIW I'm in the camp it is a security issue - it's very easy through human error to get package.json out-of-sync with a lockfile - eg just one developer manually editing the package.json file and committing without running npm install. Now any future CI builds using npm ci will pull in the latest versions for that dependency at that particular moment (and presumably its transitive deps too). All of the protections the developers thought they were getting through lockfiles and npm ci have been effectively sidestepped.

iggyzap commented 2 years ago

Hi,

Yes, this is good summary - when a user forgets to update package-lock.json or npm-shrinkwrap.json after updating a dependency in package.json manually, then npm ci will pass a build as if lock files were in sync with package.json

And this behaviour goes completely against description of what nom ci is supposed to do!

On Wed 24 Nov 2021 at 16:04, tangobravo @.***> wrote:

Just bitten by this too.

Precondition: A version of a dependency listed in package-lock.json is no longer a semver match for the dependency in package.json Expected behaviour: npm ci should fail with an error. Actual behaviour: npm ci installs the latest version matching the semver in package.json and ignores the lockfile entirely.

Regardless of whether it's a CVE or not, it's clearly a big bug and really worrying that there's no word on anyone working on a fix for it despite affected versions of npm now being the default in the current "Recommended for most users" Node.js installers.

FWIW I'm in the camp it is a security issue - it's very easy through human error to get package.json out-of-sync with a lockfile - eg just one developer manually editing the package.json file and committing without running npm install. Now any future CI builds using npm ci will pull in the latest versions for that dependency at that particular moment (and presumably its transitive deps too). All of the protections the developers thought they were getting through lockfiles and npm ci have been effectively sidestepped.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/npm/cli/issues/2701#issuecomment-978015203, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCQXX7J2UUHR35ZDRZ776DUNUEI7ANCNFSM4XT6MXKA .

-- With regards, Ignat Zapolsky

Kirill89 commented 2 years ago

Sorry for late response,

I don't have any objections about the bug itself – it is indeed a serious security concern. I only want to emphasize that it is not a security vulnerability because by definition a vulnerability has to be exploitable. I still don't see an attack vector for this issue. @rotem-cider CODEOWNERS example maybe the closest possible example to count it as a vulnerability, but I would count it more like a vulnerable configuration of a repository because the same could happen for example if you simply not use package-lock.json in your project and run npm install in the CI/CD.

I know this discussion is a bit tedious and I agree with most of you that this bug has to be fixed ASAP. But at Snyk we also care to keep the vulnerability database clean and meaningful, hence all my concerns are only around CVE – not about the bug itself.

cc @the-spyke @virkt25

the-spyke commented 2 years ago

@Kirill89 I wasn't advocating for making it a CVE as I don't know how this process works.

But now I'm curious, because by your reasoning SolarBurn didn't exist as its victims had no exploitable vulnerabilities. Using npm ci and not using npm ci are two different things. It's like having a lock and not having a lock. In our case we have a lock, but it stopped to care which keys are inserted. Also I assume that from the point of view of Snyk customers (which are mitigating supply chain attacks by using npm ci) there is absolutely no need to know about this "bug" and they are not exploitable, it's just fresh packages.

virkt25 commented 2 years ago

I think the CVE is important and Snyk should be flagging this to others because you could just trust npm ci in the past (and most production systems I've seen rely on that) and you can no longer do that. Comments like "this bit me" are perfect examples of people expecting one thing but it doing another and hence being exposed to unexpected upgrades to versions which may be compromised / haven't been tested.

And if it helps expedite a solution to this issue, great but that doesn't seem to be the case.

Either way I think a CVE is important for awareness even if this isn't a direct vulnerability

ljharb commented 2 years ago

@virkt25 that logic would make everything a CVE, and make CVEs even more useless than they already are due to false positives. CVEs are not solely for mismatched expectations.

rotem-cider commented 2 years ago

As far as I understand CVE’s are supposed to tell about security problems in products, and to affect the versions that are vulnerable so companies can upgrade to non vulnerable versions.

in this case companies using npm7 and 8 are vulnerable In cases they have a drift between the lock file and the package.json file, same happens in other CVE’s where there is a need for a specific configuration of Apache or Nginx - context is everything in every CVE that comes out and our mission is to understand fast if this affects us

Because CVE’s are not specific to usage yet, (maybe in future there will be a way to define the use case specifically in the format and will save us lots of trouble) Then we have no choice but to flag the whole version vulnerable without going into how is it used, are we really running it in isolated environments or giving it access to secrets for deployment to our cloud services.

This is a very valid CVE from my perspective, I know there is a lot of heat lately on npm specifically and they are handling this as much as they can, hope this is a wake up call for them to step up their game, and for us as an industry to understand that the weakest link can harm our whole environment.

sebastian-fredriksson-bernholtz commented 2 years ago

I just came across this and I'm shocked. In my tests, package-lock.json is completely disregarded and npm ci seems to act just like npm i, except it doesn't update the package-lock.json, so you don't even realise that a different version has been installed. What's the point of having package-lock.json then?

npm ci now seems like at least as dangerous an operation in CI/CD as npm i. It seems like the only use case for the command is no longer fulfilled. But maybe it does indeed work sometimes, as some people have indicated.

patcon commented 2 years ago

Yeesh, this bug. Haven't tested it, but this might help for CI in the meantime: https://github.com/RocketChat/package-lock-check

icatalina commented 2 years ago

It is sort of ridiculous that a Priority 1 bug has been opened for almost a year now... 😔

ricardobeat commented 2 years ago

Can anyone provide more context on what just happened here? The ci command was introduced as

npm ci bypasses a package’s package.json to install modules from a package’s lockfile

If ci installs directly from a lockfile, why is the extra validation step in 457e0ae61bbc55846f5af44afa4066921923490f necessary?

ljharb commented 2 years ago

@ricardobeat the lockfile is only reliable if it satisfies the package.json; it's a bug that that validation was ever omitted imo.

aaronadamsCA commented 2 years ago

The docs have always been clear too:

If dependencies in the package lock do not match those in package.json, npm ci will exit with an error, instead of updating the package lock.

ckirbybigdog commented 2 years ago

Oh OK I apologize my bad

On Fri, Apr 8, 2022 at 4:56 AM Aaron Adams @.***> wrote:

The docs have always been clear too:

If dependencies in the package lock do not match those in package.json, npm ci will exit with an error, instead of updating the package lock.

— Reply to this email directly, view it on GitHub https://github.com/npm/cli/issues/2701#issuecomment-1092783626, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYQ6CPSOXPCA2C4TJP33RL3VEANGVANCNFSM4XT6MXKA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

dominykas commented 2 years ago

This was meant to be fixed in 8.4.1, if I'm following the tags correctly, but I can still npm ci and it will happily install things even if package.json disagrees? Can someone please confirm this?

I'm also wondering about the correct behavior with deep dependencies - should there be a failure if the shrinkwrap overrides what's in their parent's package.json?

patcon commented 2 years ago

Maybe of interest to others here, re: npm ci failure modes: (malformed integrity hashes ignored) https://github.com/npm/cli/issues/4460#issuecomment-1095653142