zkat / npx

execute npm package binaries (moved)
https://github.com/npm/npx
Other
2.63k stars 105 forks source link

Consider a useful prompt to mitigate security issues #66

Open wbinnssmith opened 7 years ago

wbinnssmith commented 7 years ago

Even without the global fallback (which nicely requires the @version specifier), it's still pretty easy to goof up a name and download and execute something completely unexpected (for example "create-react-appp" with three ps is still available on the registry at this time).

A prompt of some kind could give the user time to review what they've just requested to be downloaded from the internet and executed with all the permissions of their user on their computer.

The prompt could force a complete 'yes' response in the same way you first agree to a fingerprint when using ssh.

npx could even display author information and popularity stats like download or star counts so the user can be very aware they're downloading what they think they are.

And of course none of this could be necessary if there's no need to download anything in the case the user's already agreed to this or the binary is available in node_modules/.bin.

Thanks for considering!

zkat commented 7 years ago

I really like this idea. I think I'm gonna hack something together that adds both a confirmation prompt, as well as a whitelist (and blacklist?) to npx and see how it feels. This would only happen when installing remote packages, and after you confirm once, you won't get prompted for that package again. Thanks for this issue!

How's that sound?

junosuarez commented 7 years ago

This would only happen when installing remote packages, and after you confirm once, you won't get prompted for that package again.

sounds great to me for interactive use on my machine.

For scripting use, I'd want to be able to include a flag (--force? --yes? ) to avoid this prompt

wbinnssmith commented 7 years ago

😍😍😍

Whitelist and blacklist sounds awesome. I wonder if it could even be a shared resource (published to npm itself?). For example has similar behavior for yarn create.

@js-n, something like that for environments without a tty like CI sounds like a good idea to me!

zkat commented 7 years ago

@js-n what I want to try is make it so, if you're using it non-interactively and it's going to install something, you must use --whitelist <pkg>. So you need to explicitly type it twice, in your scripts. When running either as a non-tty, or in an isCI environment, npx should just crash instead of prompt for confirmation.

If you're using this in CI, the thing I πŸ’― expect you to do is install it as a devDependency, in which case npx will never prompt you.

junosuarez commented 7 years ago

(sidebar, and not really related to the issue)

in CI, the thing I πŸ’― expect you to do is install it as a devDependency

Yep, yep- I've got a variety of non-ci things that are nonetheless scripted across different environments where a prior npm install step - or even the idea of a package.json don't make sense: things like one-off administrative tasks, some data integrity stuff, etc.

Something like npx --whitelist foobar foobar works fine

zkat commented 7 years ago

I'm gonna cross-link to https://github.com/yarnpkg/yarn/issues/3909 in case there's interesting/useful stuff in discussions over there, and hopefully so this is discoverable from there as well.

Martinspire commented 7 years ago

I'm not sure if a prompt would even be secure enough. If something isn't downloading directly but installes some other package that does, it would still be a security risk. Also being able to run npx from every folder seems like a risk as well. Preventing to run it everywhere by having a line in your package.json (like allowNpx: true or something, might also improve it a bit, but I feel there is still a major security risk to handle. Especially if you are going to ship it by default.

zkat commented 7 years ago

@Martinspire as soon as you start talking about transitive dependencies, the risk profile (imo) becomes identical to npm's.

I think it's important not just to have specific attacks to demonstrate, but to show that they're actually different from the risks imposed by using npm at all.

I'm also wondering about this allowNpx: true suggestion. I don't really understand the point of this? What's the attack here?

Martinspire commented 7 years ago

Well, I'm still thinking about it, but I can imagine hacked dependencies becoming a thing. Or that virusses will use npx if it is available to download and run code. A prompt would probably require a way to override that too, as to make it usable on automated systems as well.

A big nono would be to override system files or to go outside of its own folder to access files (or even write). Or how about a keylogger that activated by downloading some hacked app.

AllowNpx would probably make no sense as a task could write it anyways and thus ignore the whole security, but I still feel it should not be so easy for every folder to execute something.

Also take into account that some apps get shipped with node nowadays (to make desktop apps or mobile apps) and that many subdependencies get bundled, without the user looking at whether they contain something fishy.

tunderdomb commented 7 years ago

npx could even display author information and popularity stats

I can imagine this as part of the confirmation prompt proposed by @zkat something like this:

$ npx say-hi Josh
< Execute author/say-hi@6.0.5 (5 stars 10k downloads)? (Y/n)
< y
$ Hi Josh!

Or force it and ignore the propt:

$ npx say-hi --force
$ Hi Josh! # no prompt, just the output
zkat commented 7 years ago

Update: I've talked to registry folks about ways to efficiently get some of this metadata, what metadata would be appropriate, etc.

Turns out we have some actual quality metrics, too, since npm's search runs ~the same code as npms.io. We just don't display it in the search pages yet, but it's there πŸ‘

vpicone commented 7 years ago

I wonder if it would be helpful to simply confirm any package that has less than 100 weekly downloads.

trisys3 commented 7 years ago

@wbinnssmith, Just wanted to note that create-react-appp (with 3 p's) doesn't exist anymore, 3 days later. I guess your criticism and this issue's massive view count spurred instantaneous action to purge it from npm. Great job if that's what happened!

junosuarez commented 7 years ago

any package that has less than 100 weekly downloads.

The problem here is it's really trivial for a malicious user to just download their package 101 times.

I like the idea of reusing some of the work and thoughts around "quality metrics" that the npm registry (and others) has already done with search.

zkat commented 7 years ago

I had a thought recently: Should trust be based on user instead of package? Like, if you thought a package by @zkat was trustworthy before, does it make sense to assume you don't mind the rest of their packages getting installed?

Martinspire commented 7 years ago

@zkat and what if a package of their dependencies or their dependencies dependencies gets hacked? How would you know?

The problem is probably not the top packages, its the nested children.

I think there should be something that defines what you can and cannot execute (without overrides) and that perhaps NPM should watch what files are being changed in order to make sure it isn't something fishy. I think its mostly fine to run something within your project folder, but outside should be a no-go (perhaps except for some exceptions like python, c++, etc). Though even then you are not paying attention to a virus that is executed or something.

Just running anything anywhere doesn't sound like a good idea, so rules should be put in place and enforced. I feel if you just let this happen, NPM would be the delivery system of many of new viruses. It runs on almost every platform, so it would be easy to send around the place.

ScottFreeCode commented 7 years ago

having a line in your package.json (like allowNpx: true or something

Anything that requires legitimate users to write something into package.json in order to run it would defeat at least half the purpose of npx and probably closer to the whole purpose.

(And probably wouldn't stop any malicious hackers anyway, as all it does is make usage less convenient; unless the convenience itself is the vulnerability, in which case we may as well delete npx since convenience is all it adds as far as I can tell.)


On a related note, I'd suggest that protection against typosquatting belongs in npm itself.

The whole job of npm is to download code off the internet and the whole reason to download code is to run it (whether through the CLI, through require, or automatically through its lifecycle scripts); if there's some way that we're at risk of getting malicious code instead of what we wanted (and I think the original issue of typosquatting is one such way), surely it applies to use the package manager in the first place.

zkat commented 7 years ago

@ScottFreeCode I agree, in re typosquatting being better off in npm itself. Part of the reason we never did this before is after seeing the results of a researcher running analysis on the registry and finding that typos, specially typosquatting, were pretty rare overall in the registry (I don't have the numbers on me -- this was before my time, but it was the response when I asked).

As far as getting something like this into npm, I like the idea, and my thinking here was that I could hack this into npx to try out the UX, since it's less likely to be disruptive, and if we find something that works really well, we can look into moving that workflow over to npm itself (and have npx do things through that). I'd love to prevent small typos from affecting users like this πŸ‘

gabrielcsapo commented 7 years ago

Could a simple Regex to check to replicated values be simple enough to warn users that they have β€œmisspelled” a package? In the example of create-react-appp would yield good results for this but a package called crete-react-app would have to bring more actual language processing to define errors in grammar or spelling. It is an interesting problem, almost like a social engineering problem with any system that deals with these issues of centralized trust.

zkat commented 7 years ago

@gabrielcsapo the registry has APIs that provide information about package quality, popularity, maintenance, and authorship, and includes a pretty efficient full-text search endpoint that can be nudged into being useful. This is the route I'm planning to take.

zkat commented 7 years ago

to expand a bit more: I think something like levenshtein is more appropriate for that sort of matching but I'm not quite sure how to get that specifically out of our current APIs. (ideally, we'd do a levenshtein search and prompt the user if, say, there's a much more popular module with better quality metrics at a very close distance) /cc @bcoe because search

dotchev commented 7 years ago

As a start you can do it like ssh does. Upon first install ask the user if he trusts that thing (and show some relevant metrics as discussed above). Then remember his choice.

bcoe commented 7 years ago

@zkat it looks like this could potentially give you what you need:

https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-fuzzy-query.html

perhaps you could figure out on a test data-set of package names what an appropriate edit distance would be, and we could try exposing a new API endpoint that implements fuzziness on name.

dotchev commented 6 years ago

See how it is done in direnv