search

lirantal / lockfile-lint

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

Question: possible false positives #36

Closed radmen closed 4 years ago

radmen commented 4 years ago

I've read the article about this tool and at first all made sense.

Then, I've looked at the examples and realized that it's simple to trick it to report false positives.

I'll give two scenarios.

1. using the example from the article

  1. the hacker injects link to Github repository (as in the article)
  2. the application already has dependencies pointing to GH
  3. lockfile-lint is used with following options (this is the command taken from examples) 
    lockfile-lint --path yarn.lock --allowed-hosts yarn github.com --validate-https --allowed-schemes "https:" "git+https:"
  4. lockfile-lint will not report any problems

2. Injecting malicious NPM package

  1. hacker creates a new malicious package (with some random name, it doesn't really matter)
  2. the package is published on NPM
  3. hacker injects link to this package in the lockfile
  4. lockfile-lint will not report any problems (the package is hosted by NPM so it should be fine)

Quick and dirty example of such you can find on followin Gist: https://gist.github.com/radmen/0e9ce8706343db28f68bc7ad97910ed3

By all means, I'm not a security expert. I just wanted to ask - are those scenarios possible? I find them highly possible and thus I think that this tool might give people false sense of security that everything works fine.

lirantal commented 4 years ago

Hi 👋

Thanks for chiming in and hope the article was a valuable insight. TLDR; none of these is an expected source of concern for lockfile-lint, see below for elaborate explanation.

To address your scenarios:

  1. GitHub whitelisting - this is actually no different than whitelisting all of npm too, right? just like you mentioned in your 2nd point. We had a good discussion about it here https://github.com/lirantal/lockfile-lint/pull/12 and there's room for improvement there by whitelisting specific URIs, if you wanted to contribute a PR I'd be happy to merge it.
  2. npm malicious packages - that's indeed a possible vector of attack, unrelated to the lockfile and one which I wrote about both in Snyk's state of open source security report as well as in a medium article and this is a scope that lockfile-lint doesn't have a way to validate or at least doesn't assume to be handling. One could also argue that by the time you have a lockfile to lint and if you already installed a malicious package then linting the lockfile is the least of your concerns.

An action item that I'm thinking about with regards to the issues raised above is that perhaps these need to be better documented, so I'm happy to get a PR to update the README with said security disclaimer pointing out these risks which this package doesn't address at this point.

P.S. I believe a false sense of security is a bit aggressive so I updated the title and reflected it in your own words of possible false positives.

radmen commented 4 years ago

P.S. I believe a false sense of security is a bit aggressive so I updated the title and reflected it in your own words of possible false positives.

I'm sorry, I didn't mean to sound harsh. English is not my native language, and it didn't seem aggressive to me.

npm malicious packages - that's indeed a possible vector of attack, unrelated to the lockfile

I've mentioned a scenario of lock file poisoning - a case where a package is replaced by another directly in a lock file. This is something that might go unnoticed by the user.

It's a scenario that goes way beyond host/protocol checking. It would probably require (IMO) a more in-depth analysis of package.json contents and comparing them w/ the lock file.

One could also argue that by the time you have a lockfile to lint and if you already installed a malicious package, then linting the lockfile is the least of your concerns.

Isn't this the purpose of this tool? "Lint an npm or yarn lockfile to analyze and detect security issues" (that's what's written at the top of the repo page).

It covers only basic scenarios. Something a bit more complex won't be noticed and the users won't be aware that their lock files might be compromised. I believe that this should be stated in the docs.

lirantal commented 4 years ago

npm malicious packages - that's indeed a possible vector of attack, unrelated to the lockfile

I've mentioned a scenario of lock file poisoning - a case where a package is replaced by another directly in a lock file. This is something that might go unnoticed by the user.

Agree, it's a form of poisoning but at the level of the trusted source. To an extent, it is the purpose of the tool, but also there's a scope of how much you're able to scope out and what risk you still have to undertake. Otherwise, you could also argue that even if you lint specific package sources then someone can compromise npm or GitHub themselves to manipulate one version with another too.

Like I said, I'm happy to receive a PR to implement URI-specific whitelisting, or any other ideas you have to improve on the current capabilities, as well as a disclaimer for the README.