[RRFC] Security: audit lockfiles for injection

[fritzy]

Motivation ("The Why")

A user was concerned that a malicious person could edit a lockfile to replace packages. This attack is interesting, because the default diff view in tools like GitHub commits view sometimes hide package-lock.json/shrinkwrap.json changes.

npm/cli#4447

They opened similar issues to pnpm and yarn.

pnpm/pnpm#4361
yarnpkg/berry#4136

Example

Complete examples are listed in npm/cli#4447

How

We currently use package-lock.json as a trusted state for which exact packages should be installed.

Desired Behaviour

There are specific types of inconsistencies that could be analyzed or caught when reifying, like validating registry mapping with config.

References

npm/cli#4447
pnpm/pnpm#4361
yarnpkg/berry#4136

[fritzy]

Since many layers of security has failed if someone has edited your lockfiles, and once someone has file access to your repo, there are infinite numbers of possible attacks, there's not any guarantees that we could make. However, I think it's worthy of discussion.

[ljharb]

If someone does that, then wouldn't the package.json and the lockfile no longer agree? That seems like something npm should be checking.

[bnb]

A user was concerned that a malicious person could edit a lockfile to replace packages. This attack is interesting, because the default diff view in tools like GitHub commits view sometimes hide package-lock.json/shrinkwrap.json changes.

This has been written about before, and IIRC has happened, though I don't recall the explicit instance. There's a writeup from Snyk here that goes into some detail about how you could also abuse GitHub's diff review to hide these changes.

Since many layers of security has failed if someone has edited your lockfiles,

I actually disagree with this. I think it's fairly common in module upgrades in open source if you're checking in a lockfile.

IMO adding npm audit lockfile would be great :)

If someone does that, then wouldn't the package.json and the lockfile no longer agree? That seems like something npm should be checking.

hence this RRFC? :P

[ljharb]

I would think that npm install should just fail straight away if the lockfile disagrees with package.json, by default.

[nlf]

I would think that npm install should just fail straight away if the lockfile disagrees with package.json, by default.

this is exactly the work i'm starting this week.

[ljharb]

maybe npm ls can check that too? :-D

[webdevelopland]

If versions inside package.json and package-lock.json don't match, then npm ci throws and error. It's good.
But if integrity doesn't match version inside package-lock.json nothing happens. It's a problem.
Let's say package.json has a package:

"my-package": "^2.0.0"

After npm install package-lock.json will have this text:

"node_modules/my-package": {
  "version": "2.0.0",
  "resolved": "https://registry.npmjs.org/my-package/-/my-package-2.0.0.tgz",
  "integrity": "sha512-NeoXwyk6rBYhZIIHuMH0sRanoa5t4hm32nObB5Go/H+XOU6a7hGhWfFlQ3yej6QCcDkgxSye2nHaNTw8tLeNOg=="
}

Now we can manually update the integrity to an integrity of previous version with vulnurability.
Next npm ci will install not 2.0.0 version, but version of the new integrity. (Let's say 1.3.0)
And it's almost impossible to notice while review.

It'd be great, if npm ci checks such things.

[nlf]

as of [email protected] we validate that the shape of your dependency tree is valid when we reify, this means that if a dependency were removed entirely or the wrong version is somewhere, we will throw an error. this lays the groundwork for adding some further verifications, like resolved and integrity values

[darcyclarke]

Closing as we now validate - @fritzy feel free to reopen if there's further validation we should be doing & can backlog work for (&/or if the work @nlf has done does not cover everything in this thread).

[webdevelopland]

@nlf @darcyclarke Does that mean that integrity vulnerability won't be fixed in near future?