rust-lang / rustup

The Rust toolchain installer
https://rust-lang.github.io/rustup/
Apache License 2.0
6.1k stars 877 forks source link

Environment variable to disable `rust-toolchain.toml` #2793

Open jonas-schievink opened 3 years ago

jonas-schievink commented 3 years ago

Describe the problem you are trying to solve

In order to support VS Code's "workspace trust" feature in rust-analyzer (https://github.com/rust-analyzer/rust-analyzer/issues/9224), we'd like to disable rust-toolchain.toml support, since that allows arbitrary code execution.

Describe the solution you'd like

The easiest solution would be some environment variable that simply disables this toolchain file, but I'm open to alternatives.

matklad commented 3 years ago

I'd phrase this more general terms. We need an env variable that indicates that the contents of the file-system is "untrusted". So that, if we set UNTRUSTED=1, running rustc -v and such can't steal the secrets and such. I think this is more useful than just the narrow sense of "disabling toolchain file" because:

kinnison commented 3 years ago

I agree that a more general solution is better. There's a number of possible approaches here. The approaches outlined so far are good, an alternative is a more explicit approach. Here's a starter-for-ten on that kind of approach:

Any rust-toolchain or rust-toolchain.toml must be explicitly enabled by the user in order to work a'la direnv.

UX would be approximately:

  1. User clones a repository containing a toolchain file
  2. By some mechanism they are informed that the directory is not yet trusted (e.g. they run cargo build)
  3. They choose to trust that directory with rustup set trusted or they choose to override the toolchain with rustup override
  4. They proceed to do their work

The trusting/overrides would not be stored in the dir, but rather in $RUSTUP_HOME and as such should be considered trustable.

What do people feel about that as an approach?

bjorn3 commented 3 years ago

That will break CI as there is no user to approve the rust-toolchain and nobody has such approval as part of the CI config yet.

matklad commented 3 years ago

Yeah, that's much better, provided that we don't actually break real-world CIs. I guess that's ok, if the logic triggers only for path-based overrides. I think it's OK, and probably better, if rustup downloads official toolchains without questions.

From the tooling perspective, we probably need a special exit code (451 ?) to distinguish this specific failure, so that, eg, an IDE can show a dialog to for the user. cc @Undin, @vlad20012

See also https://github.com/jonas-schievink/mallory

kinnison commented 3 years ago

So CI could run rustup trust or whatever as part of the pathway, providing that there's trust in who can cause CI to run then that should be okay.

If we choose to only require it for path based toolchains then that might help a bit with the usability. Rustup will try and download official toolchains (and add components/targets) by default if it needs to.

A special exit code for this makes sense, frankly I'd like to overhaul our exit code strategy entirely at some point.

To ease UX in situations where there are lots of projects (e.g. someone's company uses a custom toolchain) the rustup trust could be about trusting a path to contain a toolchain, rather than trusting a particular directory's rust-toolchain.toml.

That way CI could rustup trust /path/to/custom/toolchain too.

Thanks for the mallory link.

Opinions? @bjorn3 would that satisfy you?

bjorn3 commented 3 years ago

If we choose to only require it for path based toolchains then that might help a bit with the usability. Rustup will try and download official toolchains (and add components/targets) by default if it needs to.

:+1: This pretty much mitigates the usability and CI impact of requiring a trust command.

To ease UX in situations where there are lots of projects (e.g. someone's company uses a custom toolchain) the rustup trust could be about trusting a path to contain a toolchain, rather than trusting a particular directory's rust-toolchain.toml.

:sparkling_heart:

lnicola commented 3 years ago

Hypothetical: if there is, say, an arbitrary code execution bug (or another security bug) in rustc or cargo, a malicious repository would still be able to ask for that version and exploit it.

kinnison commented 3 years ago

@lnicola True, though there's an argument that any such toolchain should probably have its channel file yanked from static.rust-lang.org no?

lnicola commented 3 years ago

That depends. If it's a long-standing bug (introduced in 1.10, found today), that would be impractical.

kinnison commented 3 years ago

In that case we probably need a toolchain status file on static.rlo which rustup can read to understand if there are toolchains with security problems and/or known major regressions.

Then rustup could refuse to install a flagged official toolchain unless it was also marked as trusted by the user.

rbtcollins commented 1 year ago

I'm sorry to come in late, but I don't see the attack vector here.

toolchains are local machine configuration - write access to CARGO_HOME is required, or the ability to set RUSTUP_HOME=new_value.

If one has write access to CARGO_HOME, arbitrary code execution is possible by replacing the cargo registry. If one has the ability to set RUSTUP_HOME to a new value, one can replace the contents of stable or any other arbitrary rust-lang published toolchain.

You could make a case for defense in depth, but I'd like to understand the threat model a bit better: what is untrusted here, and what defines the trust boundaries?

rbtcollins commented 1 year ago

Argh, sorry for the noise, my brain had forgotten the [path] option in toolchain files. I somewhat regret us adding that.

That said, I think we should write up the threat model as part of fixing this - to be crystal clear what expectations we're putting on future us.

I think an env variable is potentially risky in terms of chained attacks - if someone finds a env variable setting gadget (say for instance that some IDE blacklists known risky variables rather than whitelisting safe ones... and misses RUSTUP_*) then we've opened the door to numerous bypasses (set RUSTUP_TOOLCHAIN directly for instance).

rami3l commented 4 months ago

I'd phrase this more general terms. We need an env variable that indicates that the contents of the file-system is "untrusted". So that, if we set UNTRUSTED=1, running rustc -v and such can't steal the secrets and such. I think this is more useful than just the narrow sense of "disabling toolchain file" because:

  • it guarantees forward security: if rustup grows another way to opt-in into insecure (for untrusted code) behaviors, tools that set the env var will work correctly.
  • it covers other attack vectors. For example, it probably should forbid overriding RUSTUP_DIST_SERVER as well
  • it gives better usability: I think allowing toolchain.toml should be fine for "standard" toolchains, it's only path overrides which are problematic.

Looks like we now have a clear path towards this: