search

lirantal / lockfile-lint

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

feat(cli arg): validate package names #114

Closed fabioberger closed 2 years ago

fabioberger commented 2 years ago

Description

Instead of modifying the lockfile to resolve a dependency to a Github repo, they could simply modify it to point to a malicious package that they've published on a trusted repository.

Example of attack:

ms@^2.1.1:
  version "2.1.2"
  resolved "https://registry.yarnpkg.com/malicious-package/-/malicious-package-2.1.2.tgz#d09d1f357b443f493382a8eb3ccd183872ae6009"
  integrity sha512-sGkPx+VjMtmA6MX27oA4FBFELFCZZ4S4XqeGOXCv68tT+jb3vk/RyaKWP0PTKyWtmLSM0b+adUTEvbs1PEaH2w==

Types of changes

Related Issue

Fixes #113

Sorry I didn't wait for discussion before implementing this 😅

Motivation and Context

It makes it much harder for an attacker to point a legitimate dependency to a repository they control (even if on an official repository).

How Has This Been Tested?

I've extended the existing tests to cover the new functionality.

Screenshots (if appropriate):

Checklist:

lirantal commented 2 years ago

Hi @fabioberger, thanks for working on this enhancement and adding the pull request with tests. I highly appreciate that ❤️

A few things I'd like to raise before we move forward with this suggested enhancement:

  1. Are the package names consistent with the URL?
ms@^2.1.1:
  version "2.1.2"
  resolved "https://registry.yarnpkg.com/malicious-package/-/malicious-package-2.1.2.tgz#d09d1f357b443f493382a8eb3ccd183872ae6009"

Meaning, can you expect that all packages on the registry conform to their package name string such as ms in this case, will have the exact same URL parameter name of ms? Could you run some tests on bigger package lock files to ensure it?

  1. We should also take into account package names using scoped namespaces. Can you update the code here to support these, and also add relevant test cases for them?
fabioberger commented 2 years ago

Thanks for the quick response @lirantal 🙏

Re: supporting namespaced packages, this is already implemented. I've commented on the relevant lines above. I've also gone ahead and tested this feature on three separate Yarn.lock files with ~18000 lines and did not hit any further edge-cases.

As for whether all resolved URLs will have the package name as the first part of the URL path, this is the case for the official Yarn and NPM package registries. It does not hold for arbitrary resolved URLs since it can contain any valid URL (e.g., https://codeload.github.com/webpack/tooling/tar.gz/2e849403e5608b110e869b599442f89d7004e920 from Webpack's yarn.lock). It might make sense to enforce that the user specify --allowed-hosts to be either npm or yarn in order to use this feature. Happy to add this if you feel it's necessary.

Relevant documentation for Yarn and npmjs:

fabioberger commented 2 years ago

Friendly bump on this @lirantal 🙏

lirantal commented 2 years ago

Noted. I haven't read yet through the replies but will do my best to get to it tomorrow. Appreciate the patience here.

lirantal commented 2 years ago

Thank you for all of the work spent to implement this @fabioberger, wonderful job!

It might make sense to enforce that the user specify --allowed-hosts to be either npm or yarn in order to use this feature. Happy to add this if you feel it's necessary.

I'd appreciate if we could indeed add this extra check when someone uses --validate-package-names to make sure users aren't left confused why does it fail in case they have some packages resolved out of GitHub gists, artifactory remote URLs or something else that's a bit out of the regular conventions.

lirantal commented 2 years ago

@fabioberger I'm commuting now home so will merge it later tonight.

lirantal commented 2 years ago

looks like npmjs is down, GitHub Actions CI fails to publish: image

I'll give it a spin tomorrow.

fabioberger commented 2 years ago

Thanks for merging @lirantal! Please let me know once you've been able to publish 🙏

lirantal commented 2 years ago

New versions are out :-)