Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make npm install scripts opt-in #488

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

@tolmasky
Copy link

@tolmasky tolmasky commented Nov 5, 2021

Install scripts that can run just about anything by default pose some pretty serious security considerations, and these are inreasingly moving out of the theoretical realm and becoming actively exploited. See for example here: https://therecord.media/malware-found-in-coa-and-rc-two-npm-packages-with-23m-weekly-downloads/.

At the same time, we have developed better techniques for many of the most common use cases for install scripts in the many years since npm originally included then. In particular, N-API offers a compelling alternative to binary packages that are built on the users' computer. However, even before this, many packages are choosing to just pre-build for multiple platforms ahead of time to handle most of the common installation targets and make the install process easier on the user in general.

Instead of by default always running the install scripts (preinstall, install, postinstall, prepublish, preprepare, prepare, postprepare) if they are present during the install process, provide flags to require users to explicitly allow them to run, either whoelsale as "one big switch", or on a package by package (and optionally version by version) basis. Also provide matching npm config options to do the same globally and permanently instead of on every install.

… pretty serious security considerations, and these are inreasingly moving out of the theoretical realm and becoming actively exploited. See for example here: https://therecord.media/malware-found-in-coa-and-rc-two-npm-packages-with-23m-weekly-downloads/.

At the same time, we have developed better techniques for many of the most common use cases for install scripts in the many years since npm originally included then. In particular, [N-API](https://nodejs.org/api/n-api.html) offers a compelling alternative to binary packages that are built on the users' computer. However, even before this, many packages are choosing to just pre-build for multiple platforms ahead of time to handle most of the common installation targets and make the install process easier on the user in general.

Instead of by default always running the install scripts (`preinstall`, `install`, `postinstall`, `prepublish`, `preprepare`, `prepare`, `postprepare`) if they are present during the install process, provide flags to require users to explicitly allow them to run, either whoelsale as "one big swtich", or on a package by package (and optionally version by version) basis. Also provide matching `npm config` options to do the same globally and permanently instead of on every install.

Reviewed by @tolmasky.
@tolmasky tolmasky changed the title Install scripts that can run just about anything by default pose some… Transition to a model where install scripts must be explicitly allowed during the install process Nov 5, 2021
@tolmasky tolmasky changed the title Transition to a model where install scripts must be explicitly allowed during the install process Make install scripts opt-in Nov 5, 2021
@tolmasky tolmasky changed the title Make install scripts opt-in Make npm install scripts opt-in Nov 5, 2021
@WebReflection
Copy link

@WebReflection WebReflection commented Nov 5, 2021

Hello new comers from the mailing list 👋

This is a gently reminder that discussions and decisions are not taken after thumbs up and down or reactions, but after respectful conversations to align, eventually agree, and move forward.

I'll save you some time, this is exactly what happened here too, so you might want to skip right 'till the end of this discussion to screen latest chats and see were we've headed, thank you 👍


Install scripts that can run just about anything by default pose some pretty serious security considerations

This is nothing new in the software world, to be honest, when you install pretty much any software in Windows it asks you if it's OK to give this software the rights to configure the OS on your behalf and everyone is like "sure thing, whatever mate, let me move on" ... and here there's no difference ... except we miss the TRUST around packages.

What is trust? The fact a package has every allowed publisher behind 2FA to start with, so that publishers are accountable.

Every example to date shows that everyone can steal and publish on behalf of everyone else any kind of crapware, and yet the proposal here is to "provide yet another flag"?

What is a flag good for, when it can become a configurable default? It would solve nothing around the well known issue.

If there's anything npm should do, in my opinion, is to promote 2FA modules any way you can:

  • npm info module shows a special star, flag, check, about security/accountability because of 2FA
  • the site itself down-votes any search if the module is not 100% behind 2FA (or its developer/s)
  • a warning is shown on install ... or ... no pre/post install script is allowed if the module has not 100% 2FA publishers

Now this is thinking in a way that will stop causing the same accident over and over, where developers reputation also could play a role, so that trust become the way to move forward, as opposite of limiting all npm functiomalities because "evil happens".

Thanks for listening, and I hope yet another flag won't be the only move here 👋


P.S. we've been there before with HTTP (not S) and credit card payments or any sort of man in the middle attack, I don't understand why it's so difficult to see how much value a 2FA only ecosystem could bring to the npm's plate

Loading

@tolmasky
Copy link
Author

@tolmasky tolmasky commented Nov 5, 2021

This is nothing new in the software world, to be honest, when you install pretty much any software in Windows it asks you if it's OK to give this software the rights to configure the OS on your behalf and everyone is like "sure thing, whatever mate, let me move on" ... and here there's no difference ... except we miss the TRUST around packages.

This is actually a very different situation, as you are comparing the behavior of non-technical users installing non-technical software, vs. developers who do generally care (if allowed to) about the software they use. The other critical difference this analogy misses is that a very large portion of package installs are done not by humans, but in an automated fashion. So it is not a bunch of people saying "OK whatever," it is a fleet of CI machines repeatedly pulling packages and by default running scripts that could compromise them. A package going from not running an install script to running an install script between CI runs should IMO absolutely fail the test: there is a very bizarre change in side effects taking place, and test failures are an expected part of test suites (it's the whole reason they exist), and at that point you can issue another commit that adds the explicit approval of the package, or, as with what happened earlier today, say "wait a minute, none of these should be installing", and avoid having a bunch of junk infect your CI machines.

Another important piece of context is that vast majority of people will actually see no change, as a tremendously small amount of packages on npm today use this feature. If this was some critical package.json field that 10% of packages used or something, then that would be one thing, but the reality is that when we last checked every package (literally every package on npm) at RunKit, the percentage was vanishingly small.

Additionally, many of the packages that do use this legitimately now have better alternatives. A clear example is transpilation that happens on install instead of on publish (which additionally means we're wasting a bunch of energy retranspiling the same code over and over instead of shipping the "built" package). As mentioned in the PR, N-API also provides a better solution to actual binary packages.

What is a flag good for, when it can become a configurable default? It would solve nothing around the well known issue.

If you read the actual configuration options provided, you will see that there isn't a blanket option, only allowing you to globally allow "safely" a specific version of a package, and "unsafely" any version (or range) of a package. In other words, if you encounter that one random package that you know and love that has an install script, you can enable that one "from now on", which makes sense, you are saying "I've checked this package version, I'm good with it". This is very different than saying "I don't care about scripts running just do whatever with anything".

Thinking about the distinction between "automated" installs vs. "user drive installs" also informs the thinking around configuration vs. flags, and why both are provided to some extent: it makes total sense that one would have a different risk model for an individual developer's computer vs. a CI or production machine. If a developer accidentally installs something malicious on their computer, that could be relatively contained. However, that can be a huge profit loss if it manages to install on a production machine. So it makes a lot of sense to use the most explicit flags in the repo, but for individual users to be able to globally turn on certain packages for convenience.

Incidentally, you can even imagine a model where when running npm install interactively it can ask you about packages instead of failing (the same way apt asks you about disk usage), that way this process has even less friction.

Every example to date shows that everyone can steal and publish on behalf of everyone else any kind of crapware, and yet the proposal here is to "provide yet another flag"?

The proposal is to make unexpected behavior default-off. It's like getting angry that browsers now require a iframe attributes to explicitly enable certain features -- no, it should have always been that way, and the people working on it (developers, not end users) are perfectly equipped to understand and update their code. Except again, most people will not even need to use this flag, as the legitimate use cases are small, and only growing smaller. Again, to the point where if this were implemented with a grace period to update packages, very quickly there would be very few packages that "need" this flag. I would argue that this would for example take much less time than the ESM transition that is currently underway. Also, as mentioned in the PR, if the flag is really that bothersome, you can globally allow packages to install.

Now this is thinking in a way that will stop causing the same accident over and over, where developers reputation also could play a role, so that trust become the way to move forward, as opposite of limiting all npm functiomalities because "evil happens".

Trust doesn't fundamentally solve the problem of purposefully malicious actors. If someone steals your 2FA keys and publishes a malicious package, no amount of "ratings" or "stars" in npm info can account for that, especially not before hand. There are two issues at play here: accountability (how to deal with things once they go wrong), and security (how to prevent things from going wrong in the first place). There doesn't exist an accountability system on Earth that would make me feel comfortable with giving browser JavaScript access to my filesystem by default. I want it sandboxed, no matter what other measures are taken.

Loading

@WebReflection
Copy link

@WebReflection WebReflection commented Nov 5, 2021

you are comparing the behavior of non-technical users installing non-technical software, vs. developers who do generally care

beside the fact we don't have data around developers that really care VS developers that copy and paste whatever they find on SO or elsewhere, this proposal would break genuine software like this which needs mandatory postinstall, and everything not having it allowed by default would just break.

A breaking change like this in npm needs to take into account all use cases to date (Electron based Apps too), not just CI, but I'll read the rest later, as I think the first line is already a no-go to me, sorry.

Loading

@wesleytodd
Copy link

@wesleytodd wesleytodd commented Nov 5, 2021

While this is a laudable goal, this is not the first time this has been brought up, and very little has changed now compared to before. This would be a MAJOR breaking change for the ecosystem and despite agreeing that opt-in would have been a better default, all it would do is shift the attack to the next easiest way to compromise the supply chain (post-install scripts currently being the easiest right now in the npm ecosystem). There are no easy fix-all's when it comes to running third-party code, and I am still not sold on the juice being worth the squeeze on this one.

IIRC, the last time this was brought up it was also brought up that the current config is a "all or nothing" thing (although I seem to recall some changes to that recently, so I might be wrong now). IMO, a much better starting point for this discussion would be incremental improvements which would not break the entire ecosystem all at once. I have not been following the RFC calls or PR's much recently, but I am sure someone who has could point to a few others in this space which have been discussed.

Loading

@WebReflection
Copy link

@WebReflection WebReflection commented Nov 5, 2021

I am proposing "2FA or nothing" thing instead, which seems way more sensible to me, still as breaking change.

Can anyone please read my suggestion as a long time needed extra, not just as a blocker? Thank you!

Loading

@wesleytodd
Copy link

@wesleytodd wesleytodd commented Nov 5, 2021

I am proposing "2FA or nothing" thing instead, which seems way more sensible to me, still as breaking change.

Have you opened an RFC for this? I would love to see a --require-2fa config which would fail install if any packages were not published with 2FA as long as it had a good set of tools for ignoring things. Seems like a better way to respond to a credential theft as is the source of these recent issues.

Loading

@tolmasky
Copy link
Author

@tolmasky tolmasky commented Nov 5, 2021

A breaking change like this in npm needs to take into account all use cases to date (Electron based Apps too), not just CI, but I'll read the rest later, as I think the first line is already a no-go to me, sorry.

That's certainly unfortunate, considering that had you read the RFC before this, it explicitly discusses a strategy of having a warning transition period to give everyone time to update (Electron apps can trivially take this time to add the flag and avoid any hiccups whatsoever, completely transparently to their users. I'm also not even sure this is an issue since Electron apps can ship "built" and don't have to necessarily themselves npm install on users' machines, but again, that would itself be mitigated completely with a one-line change). Anyways, your goal of showing me disrespect has succeeded, by declaring that a huge post that tried to in good faith address your comments isn't even worth reading since you didn't like the opening sentence for a reason that was covered in the original RFC. This however does not prevent you from continuing to criticize.

Loading

@wesleytodd
Copy link

@wesleytodd wesleytodd commented Nov 5, 2021

Hot button issues with folks passionate in the ecosystem can get tough to navigate, but I recommend taking a moment to listen. There will always be someone who disagrees in any sufficiently populated OSS community, and that is a good thing. In this case, I think this issue has made SOOO many rounds, and this solution has been recommended and talked through before, that it might lead to folks who have been involved in them in the past to jump to conclusions.

I think we can all agree that you did a great job writing up the RFC @tolmasky, and I highly doubt that going back in time and being able to change things before it was so entrenched, anyone would disagree with you. I would recommend letting folks comment on the RFC, and then attending the next RFC call. The folks who regularly attend are great, and really open to discussion.

Loading

@peey
Copy link

@peey peey commented Nov 5, 2021

Install scripts that can run just about anything by default pose some pretty serious security considerations

Since this presupposes that the package you're installing might have malicious code, how do you propose to handle the threat posed by running your own trusted code which imports from these untrusted libraries?

Loading

@tolmasky
Copy link
Author

@tolmasky tolmasky commented Nov 5, 2021

While this is a laudable goal, this is not the first time this has been brought up, and very little has changed now compared to before. This would be a MAJOR breaking change for the ecosystem and despite agreeing that opt-in would have been a better default, all it would do is shift the attack to the next easiest way to compromise the supply chain (post-install scripts currently being the easiest right now in the npm ecosystem). There are no easy fix-all's when it comes to running third-party code, and I am still not sold on the juice being worth the squeeze on this one.

A number of things here, hopefully some of which ew at RunKit can be helpful with. We have to regularly analyze the usage of install scripts on RunKit, since we try to install every combination of every version of every package on npm, specially if we can join forces with npm's side of the data. I feel fairly confident that this can be done in a non "break the world" fashion, especially because I think many of the remedies can be handled by authors vs. the developers using the packages. So, in particular:

  1. We should of course no matter what have a transition period where npm simply warns about install scripts.
  2. I think there are also some "common sense" user-driven heuristics that could mitigate many of the problems completely. For example, if a package-lock.json is present then you can don't need to require the flag. This is because there is no danger of picking up a random patch update in this case, you are only installing items you've already installed in the past, and even checking against the checksum. If you wanted to be super safe, you could only apply this rule for package-lock.jsons generated before the "opt-in" transition (this can be determined by the presence of a property in the package lock) This means that you are truly catching the most egregious cases of installing unlocked packages that may pick up unintended script additions.
  3. I think we could fairly reasonably establish the "affected radius" of users by combining download data and dependency-chain info. We could thus establish a fairly good metric of "safety" to reach before pulling the trigger on anything that would have any actual end-user affects.
  4. During this period we can assist package authors in getting their packages updated. We've also done a lot of work in trying to group "install script usage", so we could fold in some "declarative" alternatives for a lot of the "piggy-backing off of install just because its the only way to do it" use cases. For example:
    • Providing an official way to ask for donations, so that doesn't have to go through install (I can't remember if this was already possible, apologies if this is already the case, but if so, then certainly letting people know that if in a year they don't switch to the official declarative support.md format or whatever their packages will require an install flag will probably go a long way to getting everyone transitioned).
    • Providing a system for declaring and installing "outside of npm" dependencies, either as a check, or as an install. We've actually implemented this ourselves for our own packages internally, something like "aptDependencies" and "aptRequirements", where npm is responsible for either making sure you have certain dependencies or fetching them. This would also just be a great ecosystem move to further "surface" the dependency structure of npm, and make it way easier for people to convey that their packages require certain tools to be available, etc.

With regard to the "bar" we're trying to reach with this strategy, I think the goal is of course not to fix everything, which isn't realistically possible. I do however think we can make reasonable progress to make "passive" attacks considerably harder to execute (where packages don't even need to be run for an attack to be successful).

Loading

@madjam002
Copy link

@madjam002 madjam002 commented Nov 5, 2021

Seems like a lot of work for the entire ecosystem when all malicious authors would have to do is just switch to running malware on first run via require/import instead which would probably happen as soon as this was implemented.

Perhaps a flag instead to refuse to install packages that have been published in a recent time period or at least flag them interactively? That way each individual user or organisation could configure their preference on how long packages have to have been in the wild before they even get downloaded onto their system.

Loading

@rauchg
Copy link

@rauchg rauchg commented Nov 5, 2021

Very supportive of this proposal. .*(install|prepare) scripts not only decrease security, they slow down installations, complicate caching and remove reproducibility by introducing side effects, and pose challenges for cross-platform compatibility.

These scripts can't be part of the "happy path" of npm. They need to be painful to use and consume and phased out of the default path of installation.

I thoroughly agree with the sentiment that the ecosystem having developed an excellent alternative for installing pre-built binary addons targeting multiple platforms means "this time is different" (I'm cognizant this has been proposed many times before)

Loading

@peey
Copy link

@peey peey commented Nov 5, 2021

For example, if a package-lock.json is present then you can don't need to require the flag. This is because there is no danger of picking up a random patch update in this case, you are only installing items you've already installed in the past, and even checking against the checksum.

I like this idea. I think defaulting to trusting the scripts when a package-lock is present and not trusting them when it's absent is sensible.

@tolmasky I'd also encourage you to file a proposal with yarn and pnpm. npm has been conservative about accepting innovations unless they're already popularized by competing package managers.

Loading

@tolmasky
Copy link
Author

@tolmasky tolmasky commented Nov 5, 2021

Since this presupposes that the package you're installing might have malicious code, how do you propose to handle the threat posed by running your own trusted code which imports from these untrusted libraries?

This is a great question and I touched on it briefly in the response above, but I think it's worth addressing clearly, if only to establish expectations: this isn't meant to target those threats (but there are other ideas out there that do!). The goal of this RFC is explicitly to handle a unique class of "passive" threats that exist by simply being present in the dependency chain. This can make them particularly nefarious precisely because they don't need to be imported or run. This fact can make them easier to sneak into codebases (since people often don't review "metadata" files as thoroughly as code files, so the addition of an innocuous package may not raise any eyebrows, or the even harder to find two step approach of initially getting a "safe" package in, only to then modify its internal dependencies afterwards), and can also make these attacks "resistant" against mitigations targeting imported code, since something like code isolation won't help you if the problem is happening during the install. Ultimately, these different kinds of attacks will require different strategies, but I think the install script trick has made the barrier to entry to create these kinds of hard-to-spot "passive" attacks very low.

Loading

@mattisdada
Copy link

@mattisdada mattisdada commented Nov 5, 2021

I think this is a great proposal and have no real issue with it. But, an alternative that would move the burden from users back to NPM is that packages with install must be reviewed and approved by NPM for every publish and probably require an additional cost for said review and 2FA.

I like this proposal more, but if people really don't want breaking changes, this would not cause any breaking changes for end users and motivate publishers to use alternative methods... Just a thought

Loading

@dominykas
Copy link

@dominykas dominykas commented Nov 5, 2021

I'm not sure I agree with the "next goal post" thinking as a reason to stay put. We do not have an easy way to evaluate package contents without installing them (side note: something to fetch the tarball without installing would be super useful as well). Yes, you can run every install with --ignore-scripts, but that's crap DX. So yeah, the next goal post does exist, but that doesn't mean we need to continue suffering and preventing actual improvements in the process for people who do care.

A blanket "always ignore install scripts" as opposed to the currently available "always ignore all scripts" (which does not allow anything npm run, last I checked) would be useful.

It doesn't have to start as opt-in. A more comfortable opt-out would be better than what we have now.

And yes, 2fa would be nice. I wish we could revive the staging discussions (#92, one approach for 2fa for automation). I also have nodejs/package-maintenance#282 parked for a very long time.

Not going to hold my breath, though, see you back here in a year. Notice how it's always around budget planning season that these things are more common? Maybe someday the budget for securing things will actually happen...

Loading

@peey
Copy link

@peey peey commented Nov 5, 2021

The goal of this RFC is explicitly to handle a unique class of "passive" threats that exist by simply being present in the dependency chain. This can make them particularly nefarious precisely because they don't need to be imported or run. ... This fact can make them easier to sneak into codebases (since people often don't review "metadata" files as thoroughly as code files,...

I see. If I understand correctly, the attack vector here is code which is contributed by an attacker who does not have commit rights to repo or publishing rights to the package, and then the contribution is merged by a maintainer.

Do you have any recent examples of this so it's easier to understand how such an attack might be carried out? In the examples you have given (coa, rc) the attacker had access to package developer's account (publishing rights).

Loading

@totten
Copy link

@totten totten commented Nov 5, 2021

A slightly biased/tangential anecdote: PHP's composer went the other way from the start. They never allowed library-packages to run their own installation steps. (Only root-level/consumer-packages could define install steps.) It was a recurring request for a few years (eg composer/composer#1193), and (from my POV, as a downstream dev) I felt it held composer back from doing some useful things. But I do appreciate that it reduces the attack-surface and has probably discouraged some supply-chain attacks.

Similar proposals were raised to allow installation logic with some kind of whitelist. The ship for that probably sailed a few years ago, but... eventually (late last year) I implemented a version of it. So you can see an example of such behavior here:

Screenshot

For installers/upgraders, it did introduce an extra prompt. I was a little concerned about potential for user-pushback. (Available signals suggest ~1000 of our deployments do composer-based administration.) So far nobody has complained about the extra workflow step being onerous.

Loading

@lewisrobbins
Copy link

@lewisrobbins lewisrobbins commented Nov 5, 2021

@totten Doesn't that have the same problem as Windows' UAC prompt -- every single user just clicks yes without a second's thought. Appreciate composer has more options that just yes and no but I think the point still applies.

Loading

@MylesBorins
Copy link

@MylesBorins MylesBorins commented Nov 5, 2021

Hey all. Myles from the npm team here. I've been trying to think through how we get to a place where we can disable install / postinstall scripts from running by default.

The biggest gap imho, as others have pointed out, is lazy loading platform specific binary assets. This is something I think we should figure out how to support in the client in a static and consistent manner.

I think with that behavior in place we could safely change defaults in a Semver Major.

I'm just stepping away for the weekend and plan to come back to this when back in the office next week.

It's exciting to see all the momentum for this type of change, even just 2 years ago I don't think we'd see such wide support for such a large breaking change

Loading

@mirek
Copy link

@mirek mirek commented Nov 5, 2021

Wouldn't signing npms on publish with 2FA protected allowed public keys solve this class of problems?

Loading

@totten
Copy link

@totten totten commented Nov 5, 2021

Doesn't that have the same problem as Windows' UAC prompt -- every single user just clicks yes without a second's thought. Appreciate composer has more options that just yes and no but I think the point still applies.

Good point. I don't have data for a systemic answer. Personally, I find it analogous to TOFU in SSH. During the first invocation, you don't really know what to expect, and many folks would blindly accept the suggestion. But then the message goes away (decision is saved into JSON); for most day-to-day tasks (git pull && composer install or git clone && composer install or even composer update), the validation is invisible. The message should only pop-up if something changes, eg

  • (SSH) If a host starts advertising a different public-key, then that is suspicious. The client prompts for confirmation.
  • (Package manager) If a package wants to increase its permissions, then that is suspicious. The client prompts for confirmation.

So trying to apply to the rc example - any downstreams newly adopting rc@1.2.9 would be liable to blind-acceptance. But all existing downstream upgrading from rc@1.2.8=>1.2.9 would get an unusual prompt.

Perhaps your question ultimately turns on "how frequently do SSH hosts change their keys" or "how frequently do packages try to increase their permissions"?

Loading

@ivarprudnikov
Copy link

@ivarprudnikov ivarprudnikov commented Nov 6, 2021

For those who mentioned 2FA, there is still an issue with the automated publishing process that would usually use an access token. A token can be leaked, thus 2FA does not solve everything.

Loading

@Okeanos
Copy link

@Okeanos Okeanos commented Nov 6, 2021

Perhaps a flag instead to refuse to install packages that have been published in a recent time period or at least flag them interactively? That way each individual user or organisation could configure their preference on how long packages have to have been in the wild before they even get downloaded onto their system.

I am not sure I understand how this would solve this issue? Wouldn't that mean that in case of (critical) security patches everybody using this flag would've to disable it or else run insecure software? Another question I am asking myself here is: if this flag is opt-in instead of opt-out … the attack surface of install scripts isn't really reduced because a lot of people wouldn't know about the flag or use it. Additionally, if everyone waited for say, a week, before installing it would technically increase the window of time in which malicious scripts etc. can be found. But by whom? Who'd do the auditing of this? I currently imagine this would only postpone the inevitable and recreate a status quo "7 days later" (using the example wait window).

I am a casual user of npm/yarn and wouldn't like this burden shifted to me if a better alternative (e.g. like SSH TOFU as previously mentioned) appears to be feasible. 2FA publishing also sounds like a good idea to me, however, as an additional improvement not as an alternative to requiring explicit consent for install scripts by default.

Loading

@ngudbhav
Copy link

@ngudbhav ngudbhav commented Nov 6, 2021

What if npm maintains an anti-malware database, just like a regular antivirus? Any install can then be directly halted if the package is marked as unsafe.
To make it a non breaking change, anti-malware can be used as a separate package. npm can issue a warning if installation is triggered without this anti-malware package.
Regular definition updates headache can be removed if the database is maintained online.

A single source of truth for all the vulnerabilities. This can help notify people via tweet/mail about recent alerts.
Organisations can ping the database regularly to find if the package is vulnerable or not.

To make it more streamlined, a simple set of rules like 2FA publish, author's information, last publish source can be kept.

Loading

@mikl
Copy link

@mikl mikl commented Nov 10, 2021

Does preventing these scripts really make that much of a difference?

The thing people usually do after installing an npm package is running said package in a Node.js environment, where a malicious script would have more or less the same access to do Bad Things™ as they would have had with an install script.

Unless I’m missing something, only in cases where the npm package is not run in Node.js (ie. dependency management for browser code or npm packages containing fonts or other non-code assets) would the elimination of install-scripts effectively prevent malicious code from running.

Unless we completely rethink Node.js and start limiting what code loaded into the runtime can do (a bit like Deno does), I don’t see how limiting install scripts would make a serious security improvement. It would just be a tiny speed-bump for bad actors.

Edit: not asking to be contrarian. This would be a significant BC break that would inconvenience a lot of users and package maintainers. If it’s not a significant security improvement, that would generate a lot of frustration and extra effort for very little gain.

Loading

@tolmasky
Copy link
Author

@tolmasky tolmasky commented Nov 10, 2021

Does preventing these scripts really make that much of a difference?

Hi Mikkel, yes there are a great number of circumstances where this makes a big difference, that have been outlined in various comments here, but I'll quickly resummarize some of them here:

  1. Per your own post, if you are using npm for purely frontend work, this opens a completely unnecessary attack surface. It might be easy to forget here in node land, but many teams that use completely different languages for their backend work only fall precisely in this category. It doesn't make sense to be exposing Ruby or Python devs to install scripts, when otherwise the code they'd install would never run on their own systems and be confined completely to the browser sandbox.
  2. Install scripts may run under different privileges than the node apps run in. Do a search for "--unsafe-perm" and you'll see that for better or worse this is a suggested "fix all" for various npm install woes, which thus exacerbates this problem.
  3. Install scripts run in different environments than node apps. As mentioned above, install scripts may be uniquely positioned to be the only attack vector on build machines, that otherwise wouldn't run code at all (some companies for example separate build machines and CI machines, etc.) This is made worse by the fact that it is not uncommon to have builds and CI take place automatically in response to a pull request, and thus all it takes to infect the machine is issuing a pull request with the package.
  4. Install scripts allow for "typo squatting" where you can register names close to popular packages such that if you accidentally type "npm install canvsa" the install script immediately kicks off before you can even realize and hit ctrl-c and its already spawned a process to start doing something malicious.
  5. There is also the fact that install scripts can be easier to introduce precisely because its not code that needs to be imported and run to attack. As such, it is possible to sneak a package into the package-lock.json as part of a larger change, since github will often automatically hide package-lock diffs, and no one really takes the time to read them since they're not really meant to be human-readable. Compare this with was malicious code that needs to be run, where you have a higher chance of noticing it in a diff since the actual code is where people usually pay attention when reviewing code and they'd ask "why are we importing this library we didn't need before?". Again, it doesn't make it impossible, it's just much easier with scripts.

To be clear, there are many more security vulnerabilities outside of this. This has been repeatedly actively exploited though, and the bar is super low (people now just straight up copy old attacks and insert them in new places). When you couple this with the fact that it is infrequently used and many of the use cases could be standardized, it seems like a good direction to merely require these (hopefully increasingly) rare uses to be approved by users. This comment is also helpful with respect to this question: #488 (comment)

Loading

@matthiasg
Copy link

@matthiasg matthiasg commented Nov 10, 2021

around modules behind 2FA, or provide at least that detail when info is used, or a module surfed through the site.

How would 2FA offer any more trust when the second factor is not at least linked to a telephone number ?

Currently npm asks for an authenticator app code based on rfc6238 not even a physical key. It can be used on a number of 'anonymous' devices. So it protects a tiny bit more and can mitigate effects from 'hacked' passwords, but the typical attacks are too different. Too readily accepted pull requests, maintainer change, misspellings of package names etc.

I would hesitate to put too much emphasis on a verification based on 2FA, though I would agree that it might be a base layer.

In general security of the executing code would need to be improved. It should for example be limited to their own package source and each package should also by default not get access to communication features unless specifically allowed by the main author.

I would follow @tolmasky 's arguments and default to use an opt-in to limit the attacks in general and put pressure on the package owners to remove any install time scripts (I haven't used any in years).

Loading

@ljharb
Copy link
Contributor

@ljharb ljharb commented Nov 10, 2021

Isn't linking 2FA to a phone number pretty insecure, given SIM swaps and whatnot? I'm pretty sure an authenticator app is much more secure than a phone number.

Loading

@WebReflection
Copy link

@WebReflection WebReflection commented Nov 10, 2021

I would follow @tolmasky 's arguments and default to use an opt-in to limit the attacks in general

in multiple times I've stated these two are not mutually exclusive, but 2FA is either useful or it's not, and we can just remove it as it provides zero extra guards, as you state, right?

Well no. Password leaks are very common still in 2021, and without 2FA a lot of modules, including mine, in the past, could've been hacked with ease. I added 2FA after last global leak from known site and never looked back. I suggest you, and everyone else, to do the same.

Loading

@matthiasg
Copy link

@matthiasg matthiasg commented Nov 10, 2021

@WebReflection I am not arguing against 2FA :) It does offer a some extra protection, but the emphasis should be on not running package scripts, not matter whether they use 2FA or something else.

Requiring 2FA or higher validation levels for package dependencies emitted as warnings or errors in general would make sense as an independent setting of course and could put some positive pressure on the ecosystem.

Loading

@WebReflection
Copy link

@WebReflection WebReflection commented Nov 10, 2021

@matthiasg so we kinda agree then ... me pushing for any way to empathize 2FA is being used is because now there's literally zero knowledge whatsoever about 2FA behind any module, even those with million downloads per week.

I am looking forward to see emphasis around 2FA instead, for that positive pressure on the ecosystem you mentioned.

Loading

@thescientist13
Copy link

@thescientist13 thescientist13 commented Nov 10, 2021

@WebReflection
Not sure if it already exists, but would it make sense to spin up / contribute to a new RFC about 2FA hints / metadata for packages? Could then get into the queue for the weekly NPM calls and continue the thread in parallel to this one.

Maybe it could even be metadata in package.json that could display an icon on npmjs.org like it does for projects that use TypeScript?
Screen Shot 2021-11-10 at 10 04 52 AM

Loading

@WebReflection
Copy link

@WebReflection WebReflection commented Nov 10, 2021

@thescientist13 I am not familiar with the RFC process in here, but yeah, I guess that makes sense, although self proclaiming a package 2FA enabled with package.json is not really what I am after there ... the TS case is just "point at some index.d.ts and icon happens", which is equivalent to zero extra trust to me. I will stop mentioning 2FA though, it's just that this whole topic to me should result into a --trust flag, as opposite of --allow-scripts-unsafe-modulename specially because @group/name are also module names, and a config flag such as --allow-scripts-unsafe-@group/name looks super awkward, while an explicit trust flag would be clearer, as trust is the kind of fix we're proposing here (or better, this is about not trusting by default, but trust is the keyword that matters to me).

Will try to find time to check how to contribute with a new RFC around this topic.

Loading

@thescientist13
Copy link

@thescientist13 thescientist13 commented Nov 10, 2021

No problem, just also felt it might a good thread worth tugging at in any of its forms. The README should hopefully provide a comprehensive overview of the process, and then meetings happen weekly and are posted as issues in this repo.
https://github.com/npm/rfcs/issues?q=is%3Aissue+is%3Aclosed

Loading

@everett1992
Copy link

@everett1992 everett1992 commented Nov 10, 2021

@WebReflection I think you're over indexing on 2FA tho I'm in favor of your suggestion


Password leaks are very common still in 2021

This qr code decodes to otpauth://totp/test-account?secret=4OWWKH4IFOARZABD7K6RQ3JPFBFLHC5I.

oathtest-qr

If this were a real account npm would store or be able to generate 4OWWKH4IFOARZABD7K6RQ3JPFBFLHC5I to validate a one time pass. If the secret is leaked by npm or the user an attacker could generate valid one time passes. npm's 2FA is rfc6238 a time based extension of rfc4226 which is a shared secret algorithm. Section 7.5 covers management of shared secrets. If npm (or any other rfc6238 provider) implements Random Generation they have a table of all users 2fa secrets which can be used to generate one time passwords. Unlike passwords these cannot be hashed or slated.

Loading

1. `npm config set allow-scripts-canvas@1.0.0 true`: This will allow canvas version 1.0.0 to install scripts whenever `npm install` is used.
2. `npm config set allow-scripts-unsafe-canvas true`: This will allow any version of canvas to install scripts whenever `npm install` is used. It should also probably warn on each case.

## Rationale and Alternatives

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a scope section, your comment here is a good starting place
#488 (comment)

Loading


Two open questions for me:

1. Having npm ship with a grandfathered list of "known safe" package versions. For example, every currently shipping version of canvas. That way, for important large packages, this is a "from now on" requirement. Alternatively, there could just be a transition period where npm just warns about script, and then afterward we make it actually refuse to run them without the proper flags.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N.B. grandfather clause has a pretty nasty history, I try to use 'legacy'.

grandfather clause
a provision in several southern state constitutions designed to enfranchise poor white people and disenfranchise Black people by waiving high voting requirements for descendants of men voting before 1867

Loading


## Summary

Instead of by default always running the install scripts (`preinstall`, `install`, `postinstall`, `prepublish`, `preprepare`, `prepare`, `postprepare`) if they are present during the install process, provide flags to require users to explicitly allow them to run, either whoelsale as "one big swtich", or on a package by package (and optionally version by version) basis. Also provide matching `npm config` options to do the same globally and permanently instead of on every install.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify if scripts are ignored or fail the install if they are not allowed to run.

Loading

- `npm install canvas --allow-scripts-unsafe canvas`: Installs the latest version of canvas to the current `package.json`, and allows it to run install scripts.
- `npm install canvas --allow-scripts-unsafe canvas --allow-unsafe-scripts ramda`: Installs the latest version of canvas to the current `package.json`, and allows it to run install scripts for canvas and any version of ramda it encounters.

### npm config

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set of packages that can run scripts should be tracked in source control so developers or CI agents can install the projects dependencies without making decisions about which scripts to run. Choosing scripts to run should be done when adding or updating dependencies, not when installing packages that are already dependencies.

I think package.json is the best location for this option.

  • package-lock.json is ephemeral and may be deleted to get a fresh install.
  • .npmrc may or may not be tracked in source. 1

Footnotes

  1. If it is checked into source developers need to avoid checking in personal changes. For example a dev needs a proxy for one project but not another. If they set the config in project npmrc they must not commit that change, if they set the config in their user npmrc they must remember to remove it before working on another project. I should write a RFC for multiple project npmrc's.

Loading

@WebReflection
Copy link

@WebReflection WebReflection commented Nov 16, 2021

@everett1992 about this

I think you're over indexing on 2FA tho I'm in favor of your suggestion

apparently it's not just me believing is a better way forward ;-)

Screenshot from 2021-11-17 00-25-08

Loading

@bmeck
Copy link

@bmeck bmeck commented Nov 17, 2021

Could we get a STDEV on the total number of transitive dependent packages on packages with scripts?

Loading

Two open questions for me:

1. Having npm ship with a legacy list of "known safe" package versions. For example, every currently shipping version of canvas. That way, for important large packages, this is a "from now on" requirement. Alternatively, there could just be a transition period where npm just warns about script, and then afterward we make it actually refuse to run them without the proper flags.
2. I think the automatic node-gyp stuff should be under this umbrella as well. That is to say, node-gyp would only run when gypfiles are detected if the `allow-scripts` flag allows it, since you can also techncially put whatever you want in there I believe, and not doing this would simply mean that any malicious behavior would be transitioned there.
Copy link

@naugtur naugtur Nov 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
2. I think the automatic node-gyp stuff should be under this umbrella as well. That is to say, node-gyp would only run when gypfiles are detected if the `allow-scripts` flag allows it, since you can also techncially put whatever you want in there I believe, and not doing this would simply mean that any malicious behavior would be transitioned there.
2. I think the automatic node-gyp stuff should be under this umbrella as well. That is to say, node-gyp would only run when gypfiles are detected if the `allow-scripts` flag allows it, since you can also techncially put whatever you want in there I believe, and not doing this would simply mean that any malicious behavior would be transitioned there.
## Prior art
- run scripts from allowlist [allow-scripts](https://www.npmjs.com/package/allow-scripts)
- run scripts from allowlist [allow-scripts from lavamoat](https://www.npmjs.com/package/@lavamoat/allow-scripts)
- tools to prevent/detect scripts running when not wanted [preinstall-always-fail](https://www.npmjs.com/package/@lavamoat/preinstall-always-fail) [pentest-my-ci](https://www.npmjs.com/package/@naugtur/pentest-my-ci)
- reviewing which scripts need to run to help build allowlists [can-i-ignore-scripts](https://www.npmjs.com/package/can-i-ignore-scripts)

Loading


Two open questions for me:

1. Having npm ship with a legacy list of "known safe" package versions. For example, every currently shipping version of canvas. That way, for important large packages, this is a "from now on" requirement. Alternatively, there could just be a transition period where npm just warns about script, and then afterward we make it actually refuse to run them without the proper flags.
Copy link
Contributor

@ljharb ljharb Nov 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a hardcoded package list is a very bad idea - this incentivizes authors to never publish updates, since existing known-good versions are privileged.

Loading

@naugtur
Copy link

@naugtur naugtur commented Nov 28, 2021

If we disable scripts by default, we need means to run compilation etc. for some packages. npm rebuild could do that.
I have a poc of running arbitrary code if you run npm rebuild on something trustworthy.
Who should I talk to?

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet