lirantal / lockfile-lint

Lint an npm or yarn lockfile to analyze and detect security issues
Apache License 2.0
782 stars 35 forks source link

feat(validate package versions): matches versions with resolved fields #137

Closed bozdoz closed 2 years ago

bozdoz commented 2 years ago

Checks the version range with the version in the resolved fields; adds semver as a dependency to verify.

Types of changes

Related Issue

120

Motivation and Context

Someone could alter a lock file to download a different version than was intended (maybe the work of a disgruntled employee, or a hacky dev)

How Has This Been Tested?

Added unit tests, coverage for the new file is 100%.

Checklist:

lirantal commented 2 years ago

Thanks for suggesting another improvement @bozdoz :-)

A few thoughts on this:

  1. the new package-lock.json format (lockfileVersion: 2) doesn't use a name+version (like meow@^0.0.1) but rather omits the version from the package name entry of the object, and instead uses the explicit version key in the object where resolved, integrity, and others exist. This is actually not something you have to worry about though (the range) because the internal lockfile-lint-api logic already makes use of the explicit version tag here. Can you confirm? So, perhaps we should compare the two version fields directly instead? this will also remove the need to use the semver package to satisfy the range.
  2. Based on a few tests I was running locally it seems like it would take more than just changing the version number. At least with npm v8.15.0, it has some internal resolution that has changed from what it was last time I ran a lockfile injection exercise and it copes with resolving changes better. I'll look further on what it needs to actually look for.
bozdoz commented 2 years ago

Do you want to conditionally check for version range, if using lockfileVersion 1? I can rewrite this to check for the version field against the resolved version.

lirantal commented 2 years ago

@naugtur @yoavain would be happy to hear your thoughts on this one.

naugtur commented 2 years ago

I looked at the code, I gave it a thought and while it seems a little odd that we're validating a file against a neighbor file in a repository in case it gets maliciously modified, I get the idea that getting a package-lock change through code review is much easier.
So yes, IMHO this makes sense. I don't like new dependencies popping up in the lint-api package, but this one is not too big.

bozdoz commented 2 years ago

I also wonder about checking version field against the resolved field, since it's actually the same file (a malicious dev could easily alter both); is it possible for a dev to alter both, or would npm ci complain if the package-lock version didn't match? I tend to think it would complain.

lirantal commented 2 years ago

is it possible for a dev to alter both, or would npm ci complain if the package-lock version didn't match? I tend to think it would complain.

Right, that's kind of what I'm getting at. I'm not entirely sure this should land so I'm going to close and defer for now.

Your effort is much appreciated @bozdoz ❤️