Postinstall scripts are security liabilities

[ysbaddaden]

The postinstall script is known to cause portability issues (#468) or to be kinda broken (#498), but it's also a security liability!

See for example shai-hulud and sha1-hulud in NPM packages for example:

Those releases embedded a postinstall script (bundle.js) that attempted to harvest developer tokens (e.g., npm, GitHub, cloud credentials) and exfiltrate secrets. Treat any system that installed these versions as fully compromised.

---

This is easy to reproduce:

name: "nice"
version: "1.0.0"
scripts:
  postinstall: "sh build.sh"

Seems fine... until build.sh runs rm -rf $HOME.
Install the nice shard and you'll wipe your HOME directory 💥

---

Running whatever must require explicit user approval.

At the very least Shards should run the postinstall script on demand only, and take a list of authorized shards:

• the --skip-postinstall argument shall become a NOOP
• add --run-postintall=<allow,list> for example --run-postinstall=protobuf,grpc,interro

Since installing executables is tightly coupled with the postinstall script having build said executables (#498):

• the --skip-executables argument shall become a NOOP
• add --install-executables=<allow,list> argument

This would solve all the issues around the postinstall script and make it harder to use and thus harder to shoot yourself in the foot (or worse).

EDIT: ideally Shards should drop support for the postinstall script altogether, making the entry in shard.yml informational only.

[crysbot]

This issue has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/shards-postinstall-considered-harmful/3910/116

[straight-shoota]

100% agree on stopping postinstall running by default.

But I'm not convinced about the --run-postinstall option. It seems rather inconvenient to have to pass that explicitly to every shards install or shards update command. And I'm wondering what good it actually does? It still executes code before being able to inspect it. Even if you generally trust a shard, there's no way to tell whether a newly installed version might be compromised.

So I don't think postinstall execution should ever be implicit.

Going a step further, we could consider dropping command execution form shards entirely. I think this would be better in multiple ways: less complexity, fewer liabilities, less surprises. And I'd argue that many shards will be better off without postinstall anyway.

We could keep the postinstall property, but it would be informational only. shards could perhaps provide a means to extract the command, but not to run it directly.

[ysbaddaden]

I'm fine dropping support for running postinstall, and also for installing executables.

[kostya]

why you think only about executables, how to build dependency lib? would it be painful?
https://github.com/kostya/lexbor/blob/master/shard.yml#L8

[ysbaddaden]

@kostya The issue is identical: this is executing an arbitrary command. The src/ext/build_ext.cr file can be compromised to become {% run "rm -rf $HOME" %}.

Your usage setup would be identical to your development setup: use the convenient builder to build it... but it could allow it to be installed from packages (it's in alpine linux, freebsd ports, ...), or be installed manually.

[ysbaddaden]

BTW: I do have a few shards that build an external C dependency in a postinstall script, and the very reason I implemented the feature in the first place. I shouldn't have 🙈

[kostya]

I not understand, you can do the same unsafe things from gem install, or popular install way curl some.site | sh -, it always was unsafe.

[robmullr]

Rather than postinstall, I'd rather have good documentation and instructions on how to do it myself.

[yxhuvud]

It seems rather inconvenient to have to pass that explicitly to every shards install or shards update command.

Whitelisting what postinstalls to run in shards.yml seems sortof fine. Heck, even having to manually add the rebuild command to shards.yml (of app using it) seems fine.

Rather than postinstall, I'd rather have good documentation and instructions on how to do it myself.

The problem is that unless the user is reminded about this every time the dependency is updated, it may break builds as there is no guarantee that built dependency libs havn't been recompiled.

One solution is to (finally) bite the bullet and allow crystal builds to trigger noncrystal builds, similar to cargo or zig. This would be fine as we have no way to protect against compilations running arbitrary code anyhow (it is part of the macro system. Heck - I suppose compilation could be triggered by a macro too 🤔 ). (EDIT: Or allowing defining inline C shims directly in the code, like Go does)

Other things to strongly consider includes following the lead of Go and creating a central registry and force both signing and disallowing of changing what a tag points to. That wouldn't stop a worm from creating new releases, but it would stop it altering existing releases.

[ysbaddaden]

you can do the same unsafe things from gem install

That doesn't mean we must do the same.

@yxhuvud Indeed, the macro run is also a security liability 😮‍💨

A full fledged build tool, tailored for Crystal and external dependencies, would be very nice to have, and a very welcome replacement for the half-baked run command. That doesn't mean Shards itself shall become that tool, though.

Aside: I'd love to have "inlined C/CPP" in Crystal. We already require a C compiler, so the Crystal compiler could merely put the C/CPP source into a file, compile it to an object into the cache, then merely add it to the list of objects to link... the problems are --cross-compile and --single-module. Let's continue that discussion elsewhere.

[ysbaddaden]

Even without a registry, signing and verifying release tags would be 👍

[yxhuvud]

Even without a registry, signing and verifying release tags would be 👍

Very true, but less powerful for new installs. Also registries allow blacklisting bad actors that go after misspellings, so there are other nice security features there. Though perhaps a separate discussion..