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

Environment variable to disable rust-toolchain.toml #2793

Open
Tracked by #3935
jonas-schievink opened this issue Jun 14, 2021 · 13 comments
Open
Tracked by #3935

Environment variable to disable rust-toolchain.toml #2793

jonas-schievink opened this issue Jun 14, 2021 · 13 comments

Comments

@jonas-schievink
Copy link

Describe the problem you are trying to solve

In order to support VS Code's "workspace trust" feature in rust-analyzer (rust-lang/rust-analyzer#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
Copy link
Member

matklad commented Jun 14, 2021

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.

@kinnison
Copy link
Contributor

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
Copy link
Member

bjorn3 commented Jun 14, 2021

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
Copy link
Member

matklad commented Jun 14, 2021

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
Copy link
Contributor

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
Copy link
Member

bjorn3 commented Jun 14, 2021

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.

👍 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.

💖

@lnicola
Copy link
Member

lnicola commented Jun 16, 2021

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
Copy link
Contributor

@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
Copy link
Member

lnicola commented Jun 16, 2021

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

@kinnison
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

rami3l commented May 10, 2024

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:

  • A passive defense provided by Cargo and Rustup safe file discovery. rfcs#3279, which should allow you to specify the directories that you trust but don't own.
  • An active defense provided by the CEILING_DIRECTORIES mechanism, which should allow you to override the ceiling of Rustup's filesystem walk. I can imagine it being set to a special value meaning we don't do the walk at all.

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

No branches or pull requests

7 participants