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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

globally-installed package overwrites an existing binary in the target install location #7761

Open
koba04 opened this issue Dec 12, 2019 · 8 comments

Comments

@koba04
Copy link

@koba04 koba04 commented Dec 12, 2019

npm has announced vulnerabilities that npm has been fixed.

https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli

One of the vulnerabilities has been fixed at yarn v1.12.1. Thank you! 馃憦馃憦馃憦
But yarn hasn't fixed the other yet.
Do you have any plans to fix this?

Do you want to request a feature or report a bug?

bug?

What is the current behavior?

globally-installed package overwrites an existing binary in the target install location.

If the current behavior is a bug, please provide the steps to reproduce.

What is the expected behavior?

Do not overwrite the symlink.

Please mention your node.js, yarn and operating system version.

  • Node: v10.16.3
  • Yarn: v1.21.1
  • OS: macOS 10.15.1
@arcanis

This comment has been minimized.

Copy link
Member

@arcanis arcanis commented Dec 13, 2019

tldr: The vulnerabilities are fixed. What remains (both in Yarn and npm) is the consequence of the typical Node package management security policies, and closing that the way you intend it would require to simply rethink part of how both package managers and Node security policies are designed. On the bright side, Yarn 2 is doing precisely that (for the package manager part), with the hope that once Node catches up we'll be able to switch painlessly.

So ... what happened? There are three reports (pinging their author, @DanielRuf):

1. Binary planting

This means that, by using a specially crafted bin field, you could write files (or rather symlinks) on the filesystem, in locations that weren't part of the dependency tree. For example:

{
  "bin": {
    "../../../../../../../../../../../../home/arcanis/foo": "foo"
  }
}

This is a very weird behaviour, not a single chance that it would be legit, so we now prevent it. It's still possible to do it using a postinstall script, of course, but at least just running yarn install won't risk modifying your system without even a note that a postinstall script got executed.

To be clear: it's safe to call this behavior a vulnerability, and it's now fixed.

2. Out-of-tree execution

This means that, by using an absolute (or deep relative) path as bin target, you could lead Yarn to install symlinks that would point to any location on the filesystem:

{
  "bin": {
    "foo": "/home/arcanis/foo"
  }
}

Yarn 1.21.1 fixes that by making sure that the target is neither an absolute path nor a back-relative path. So ./foo/bar would be allowed, but not ../bar or /bar, for example.

Now - does it entirely solve the problem? Meh. Let's assume two cases:

  • You have a postinstall script in the malicious package. In this case, well, you're done. This behaviour is peanuts compared to the fact that you'll be running a malicious shellscript that can do whatever it pleases.

  • You don't have a postinstall script in the malicious package, or you disabled them. In this case well done! But it's not enough. See, for the symlink to be executed, you'll have to ... execute it, right? By something like yarn run malicious-bin? By which point you have no control on the script being executed, and it's game over - again. The attacker would just have to simply call the binary themselves using child_process or a similar API.

As you can see, fixing the initial report does little to protect you against a malicious package, as they have many tools at their disposal to mess with your system if you dare execute them. For this reason, I'd class the fix we've done (both Yarn and npm) as a bugfix rather than a vulnerability fix. The only true fix can come from having true per-package security policies, which can only be implemented on the Node side.

3. Binary overlap

This means that a malicious package can "pose" as another by exposing an overlapping binary name. So for example:

{
  "name": "malicious-package",
  "bin": {
    "my-tool": "./script.js"
  }
}

In this case, if you have both packages my-tool and malicious-package in your dependencies, it's not clear which one will "win" and will be called if you run yarn run my-tool.

Let's ignore postinstall scripts for a second - you'll have understood by then that you're in a bad place if they're executed. So what should we do here? It seems likely indeed that we shouldn't allow such conflicts to happen. At the same time:

  • I'm pretty sure some packages are expecting this to work properly. For example I think I remember webpack-cli doing something similar at some point - where webpack was providing the webpack binary, and webpack-cli was overriding it (or maybe it wasn't Webpack? At the very least I seen it once or twice). So changing this behavior would likely be a breaking change.

  • For this to be possible, malicious-package has to be in your dependencies ... by which point it stands to reason that you will execute it, one way or another (and especially if you installed it as a global package!). Maybe via Node, maybe via yarn run, but if it's in your deps it will eventually be executed - and at this point, the path of the binary that you ran when running yarn run webpack doesn't matter very much, since you already [intended to] execute malicious code.

Changing that would make you feel safer, maybe, but you wouldn't really be. And it could trigger compatibility problems for some projects that would then be unable to keep upgrading Yarn and profit from future security updates.

Conclusion - What to do?

  • Yarn 2 will only run bin scripts through Node. Shellscripts won't be run anymore (you'll have to use child_process to spawn a shellscript if you really want to). This will make it possible for Node to implement sandboxing mechanisms on their side, ensuring that binaries cannot do anything they haven't been given express permission beforehand.

  • Node should implement a security policy akin to what I just mentioned. It's much easier to recommend it than to implement it, of course, but I know this is something in the back of their mind so I would expect to see it happen in the next few years. In the meantime...

  • Postinstall scripts should be disabled by default. This is probably not something we'll be able to do for Yarn v2, since it would break a significant part of the ecosystem (still, you'll be able to do it manually and even whitelist specific packages you will want to allow running postinstall scripts). It's very much on the table for Yarn v3, though.

    • If you're a package author, drop postinstall scripts now. We will make your life intentionally harder on the long term if you don't. You have other choices:

      • If you're looking for native perfs, use WebAssembly instead.
      • If you're looking for funding requests, use funding instead.
      • If you're looking to access native APIs that Node doesn't expose by default ... well, for now keep using native packages, but don't forget to express your interest to the WebAssembly people to get access to dlopen / dlsym or similar mechanisms.
@koba04

This comment has been minimized.

Copy link
Author

@koba04 koba04 commented Dec 16, 2019

@arcanis
Thank you for the details!!
I'm looking forward to yarn v2 and v3!! 馃憤

Changing that would make you feel safer, maybe, but you wouldn't really be. And it could trigger compatibility problems for some projects that would then be unable to keep upgrading Yarn and profit from future security updates.

I agree with you. But I feel that printing a warning for binary overlap is helpful to detect the case because printing a warning won't break anything and developers sometimes install npm packages without confirming what the package does by copy-paste from blog posts.

Postinstall scripts should be disabled by default. This is probably not something we'll be able to do for Yarn v2, since it would break a significant part of the ecosystem (still, you'll be able to do it manually and even whitelist specific packages you will want to allow running postinstall scripts). It's very much on the table for Yarn v3, though.

I sometimes concern about the risk of postinstall scripts. I don't actually recognize how many postinstall scripts are executed while yarn install. I can imagine that disabling postinstall will break our builds but I don't want to allow any scripts without recognizing them. So I'm a huge fan of whitelist approach or if it would be nice if I can confirm all postinstall scripts that have run while yarn install through a console output.
If yarn provides a way to protect from the risk of postinstall, which is a huge advantage for me to choose yarn.

@koba04

This comment has been minimized.

Copy link
Author

@koba04 koba04 commented Dec 18, 2019

I've created an npm package to disclose install and postinstall scripts in dependencies.
https://www.npmjs.com/package/install-scripts

I think it would be nice if yarn supports this feature, which makes it easy to use --ignore-scripts and then run scripts that we need.

@DanielRuf

This comment has been minimized.

Copy link
Contributor

@DanielRuf DanielRuf commented Dec 18, 2019

@koba04 it seems your package reads the local files and checks if they contain postinstall and so on but that does not help during or before the actual npm install.

It has to be done after the tarballs are unpacked but before description. Yarn v2 might support this through plugins / hooks.

@koba04

This comment has been minimized.

Copy link
Author

@koba04 koba04 commented Dec 18, 2019

@DanielRuf Yeah, in this case, we should run npm install --ignore-scripts.

@koba04

This comment has been minimized.

Copy link
Author

@koba04 koba04 commented Dec 18, 2019

Does yarn have a plan to support fund command like npm?

@arcanis

This comment has been minimized.

Copy link
Member

@arcanis arcanis commented Dec 18, 2019

Yes, although new features will be implemented in the v2 trunk (we may backport them on a case-by-case basis, but this will only be once the v2 is stable).

@koba04

This comment has been minimized.

Copy link
Author

@koba04 koba04 commented Dec 18, 2019

@arcanis Thank you!!! That sounds amazing!!馃槏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.