Page MenuHomePhabricator

Add some form of static analysis for package-lock.json
Open, In Progress, MediumPublic

Description

See https://snyk.io/blog/why-npm-lockfiles-can-be-a-security-blindspot-for-injecting-malicious-modules/

I usually skim changes in package-lock.json, but I could see myself very much missing the type of injection described in the linked blog post during CR of a few hundred line diff.

Are there other types of checks we should institute?

Event Timeline

Adding a npx lockfile-lint --path package-lock.json --type npm -validate-https --allowed-hosts registry.npmjs.org step into quibble could work?

Adding a npx lockfile-lint --path package-lock.json --type npm -validate-https --allowed-hosts registry.npmjs.org step into quibble could work?

I think we'd also want to whitelist github.com/wikimedia as well. I'm thinking it might be easier to just re-implement lockfile-lint in a few lines of Python to do just the checks we want instead of installing yet another npm dependency tree...

Adding a npx lockfile-lint --path package-lock.json --type npm -validate-https --allowed-hosts registry.npmjs.org step into quibble could work?

I think we'd also want to whitelist github.com/wikimedia as well.

Sure, though it's rare.

I'm thinking it might be easier to just re-implement lockfile-lint in a few lines of Python to do just the checks we want instead of installing yet another npm dependency tree...

Oh, hmm, I assumed we'd have npx installed in the node base images, but indeed not. Yeah.

Note that we have both

"version": "github:wikimedia/eslint-plugin-jquery#2fa39abd3c8167bde49841a726794359416c1bd3",

and

"version": "git+https://github.com/wikimedia/kad.git#96f8f5c8e5a88f5dffed47abc20756e93e16387e",

… to support, if you're re-implementing.

Adding a npx lockfile-lint --path package-lock.json --type npm -validate-https --allowed-hosts registry.npmjs.org step into quibble could work?

I think we'd also want to whitelist github.com/wikimedia as well.

Sure, though it's rare.

Unfortunately it seems to be common in services with swagger-ui, kad, service-runner all pulling from GitHub (at least from the repos I have checked out locally). Here's all the non-github.com/wikimedia deps (again, from what I have checked out locally):

Processing /home/km/gerrit/data-values/value-view/package-lock.json
[src/main.rs:107] &deps = [
    "jquery-ui@https://github.com/jquery/jquery-ui/archive/1.9.2.tar.gz: version comes from a non-Wikimedia GitHub account: https://github.com/jquery/jquery-ui/archive/1.9.2.tar.gz",
]
Processing /home/km/gerrit/mediawiki/services/parsoid/package-lock.json
[src/main.rs:107] &deps = [
    "gc-stats@git+https://github.com/dainis/node-gcstats.git#5be60dfd24293d6cefbc8a459c1537611373fac5: version comes from a non-Wikimedia GitHub account: git+https://github.com/dainis/node-gcstats.git#5be60dfd24293d6cefbc8a459c1537611373fac5",
    "service-runner@git+https://github.com/cscott/service-runner.git#4393231942cdb0918de896fe439ef9e24da20722: version comes from a non-Wikimedia GitHub account: git+https://github.com/cscott/service-runner.git#4393231942cdb0918de896fe439ef9e24da20722",
]
Processing /home/km/gerrit/mediawiki/services/push-notifications/package-lock.json
[src/main.rs:107] &deps = [
    "request@git+https://github.com/mdholloway/request.git#a059694bb48ca16317fd607d55f5c038fc0c2b78: version comes from a non-Wikimedia GitHub account: git+https://github.com/mdholloway/request.git#a059694bb48ca16317fd607d55f5c038fc0c2b78",
    "http2@https://github.com/node-apn/node-http2/archive/apn-2.1.4.tar.gz: version comes from a non-Wikimedia GitHub account: https://github.com/node-apn/node-http2/archive/apn-2.1.4.tar.gz",
]
Processing /home/km/gerrit/ooui/package-lock.json
[src/main.rs:107] &deps = [
    "grunt-promise-q@git+https://github.com/jdforrester/grunt-promise-q.git#d4e856ef5189c5d6a1089eb239bf29dc0f7c14c6: version comes from a non-Wikimedia GitHub account: git+https://github.com/jdforrester/grunt-promise-q.git#d4e856ef5189c5d6a1089eb239bf29dc0f7c14c6",
]

I see basically two options forward:

  • Globally mandate that all deps must be under github.com/wikimedia or some legacy/grandfathered exception
  • Allow for a .package-lock-lint.json config in each repo to override the list of allowed hosts.

That said, I'm not really sure how much security this would win us, given that the barrier to uploading malicious stuff to npmjs.org probably isn't going to be much higher than uploading it to GitHub.

At least for the initial scope of this tool I am thinking of enforcing only:

My code so far: https://gitlab.com/legoktm/package-lock-lint

I plan to add this to LibUp and then if that goes well, propose it for including in the npm test CI. I ended up writing it in Rust instead of Python primarily so it would be easier to deploy in containers so it wouldn't be dependent on an entire Python runtime.

Here's all the non-github.com/wikimedia deps (again, from what I have checked out locally):

Processing /home/km/gerrit/data-values/value-view/package-lock.json
[src/main.rs:107] &deps = [
    "jquery-ui@https://github.com/jquery/jquery-ui/archive/1.9.2.tar.gz: version comes from a non-Wikimedia GitHub account: https://github.com/jquery/jquery-ui/archive/1.9.2.tar.gz",
]
Processing /home/km/gerrit/mediawiki/services/parsoid/package-lock.json
[src/main.rs:107] &deps = [
    "gc-stats@git+https://github.com/dainis/node-gcstats.git#5be60dfd24293d6cefbc8a459c1537611373fac5: version comes from a non-Wikimedia GitHub account: git+https://github.com/dainis/node-gcstats.git#5be60dfd24293d6cefbc8a459c1537611373fac5",
    "service-runner@git+https://github.com/cscott/service-runner.git#4393231942cdb0918de896fe439ef9e24da20722: version comes from a non-Wikimedia GitHub account: git+https://github.com/cscott/service-runner.git#4393231942cdb0918de896fe439ef9e24da20722",
]
Processing /home/km/gerrit/mediawiki/services/push-notifications/package-lock.json
[src/main.rs:107] &deps = [
    "request@git+https://github.com/mdholloway/request.git#a059694bb48ca16317fd607d55f5c038fc0c2b78: version comes from a non-Wikimedia GitHub account: git+https://github.com/mdholloway/request.git#a059694bb48ca16317fd607d55f5c038fc0c2b78",
    "http2@https://github.com/node-apn/node-http2/archive/apn-2.1.4.tar.gz: version comes from a non-Wikimedia GitHub account: https://github.com/node-apn/node-http2/archive/apn-2.1.4.tar.gz",
]
Processing /home/km/gerrit/ooui/package-lock.json
[src/main.rs:107] &deps = [
    "grunt-promise-q@git+https://github.com/jdforrester/grunt-promise-q.git#d4e856ef5189c5d6a1089eb239bf29dc0f7c14c6: version comes from a non-Wikimedia GitHub account: git+https://github.com/jdforrester/grunt-promise-q.git#d4e856ef5189c5d6a1089eb239bf29dc0f7c14c6",
]

Those are all good things to flag; except for data-values/value-view, these are each local forks we've made of upstream to work around issues, but didn't want to make them 'official' Wikimedia forks. Such things should be reviewed regularly to see if upstream has moved (and if not, possible prompt actual forks).

I see basically two options forward:

  • Globally mandate that all deps must be under github.com/wikimedia or some legacy/grandfathered exception
  • Allow for a .package-lock-lint.json config in each repo to override the list of allowed hosts.

That said, I'm not really sure how much security this would win us, given that the barrier to uploading malicious stuff to npmjs.org probably isn't going to be much higher than uploading it to GitHub.

Fair. I'd quite like the second option in the very rare cases that it's needed, but I worry about such over-rides proliferating.

At least for the initial scope of this tool I am thinking of enforcing only:

Lots of people are moving to npm 7 etc. already; given that we except CI to never change package-lock, only follow it, that seems an over-restriction?

At least for the initial scope of this tool I am thinking of enforcing only:

Lots of people are moving to npm 7 etc. already; given that we except CI to never change package-lock, only follow it, that seems an over-restriction?

I thought we wanted to force people to stay on npm6 for now? Anyways, I can add lockfileVersion 2 support pretty easily I think.

Change 693016 had a related patch set uploaded (by Legoktm; author: Legoktm):

[labs/libraryupgrader@master] Lint package-lock.json after installing npm stuff

https://gerrit.wikimedia.org/r/693016

Change 693016 merged by jenkins-bot:

[labs/libraryupgrader@master] Lint package-lock.json after installing npm stuff

https://gerrit.wikimedia.org/r/693016

Anyways, I can add lockfileVersion 2 support pretty easily I think.

I implemented this now and did a bunch of other cleanup and edge-case handling in package-lock-lint 0.2.0. I think we should be ready to enable this on all CI builds, I'll propose it on wikitech-l.

sbassett changed the task status from Open to In Progress.Jul 6 2022, 5:32 PM
sbassett triaged this task as Medium priority.