Relative path overrides are still possible via /proc/self

[matklad]

Relative path overrides permit a freshly downloaded source tree to
execute arbitrary code on any rustup command that executes a binary from
the configured toolchain, and its a reasonable tradeoff for us to remove
this feature. Absolute path overrides are kept intact - these were added
to support users of large monorepo tool systems, and can be kept with
reasonable safety.

I think relative paths are a subset of absolute paths, so, security-wise, this is a no-op.

Namely, ./foo/bar relative path is equivalent to /proc/self/cwd/foo/bar absolute path.

Originally posted by @matklad in #3340 (comment)

[rbtcollins]

I can see three paths forward:

• remove path based toolchains entirely. This would affect some users disproportionately - e.g. AWS's internal build system @jonhoo
• give up on this security attempt, possibly even rollback and restore the relative path access, and then use the separate trusted directory functionality to gate all access to path based toolchain overrides.
• refine the security attempt by filtering absolute paths for known relative path proxies - which might be a never ending sequence of tweaks.

[matklad]

refine the security attempt by filtering absolute paths for known relative path proxies - which might be a never ending sequence of tweaks.

I guess, security wise there's also an angle that the attacker could just guess absolute paths? Like, it's pretty obvious, if one stalks me, that I clone stuff into either /home/matklad/p or /home/matklad/tmp, so, if someone delibirately targets me, they potentially could just hard-code those absolute paths.

So I think 3) is a fundamentally incomplete approach.

2. makes probably most sense --- that's how similar tools, like .direnv, work.

On the meta level, probably makes sense to ask some relevant security expert in the rust ecosystem for what's best practice here?

[djc]

Option (2) sounds good to me, too.

@walterhpearce / @LawnGnome, we'd like your input here!

[jonhoo]

cc @lovesegfault who currently maintains the build tooling at AWS given I've since left :)

[lovesegfault]

Thanks for the tag, Jon!

Regarding (2), I'm not sure what "the separate trusted directory functionality" is referring to, so it's hard to opine.

I can say that (1) would cause massive churn at AWS, and I'd have to drop just about everything to re-think how we use Rustup. It could lead to us maintaining a fork of Rustup, which I'd like to avoid at all costs.

(3) sounds like "drying ice", but sometimes that's all you can really do, I don't know if that's the case here.

[LawnGnome]

(@walterhpearce and I discussed this earlier, but I'm speaking for myself here. I suspect he'll be by with an opinion at some point too.)

I don't see a lot of value in trying to distinguish relative and absolute paths here, honestly — while it might make some non-targeted attacks slightly more difficult, it doesn't help with targeted attacks, and I suspect a creative attacker could still come up with some pretty damaging non-targeted attacks without much effort, even if we try to whack the moles of things like /proc/self with option 3.

It sounds like option 1 is also off the table given the monorepo use case.

So, given that, option 2 (including a rollback of the recent relative path change) feels right to me. Am I right in thinking that the "trusted directory" functionality hasn't yet been implemented or designed in any serious way? I think this is personally how I'd want the check to work:

1. rustup checks the canonical path of every binary it's going to execute against a prefix allowlist before executing it.
2. Without any configuration, the default allowlist rustup uses is only ~/.rustup/toolchains (allowing for platform specific differences) and maybe whatever is required for distro-shipped toolchains. (I'm ignorant of the details there. Could that be merged in from /etc/rustup/settings.toml?)
3. A new setting is added that allows other prefixes to be added to the allowlist, thereby addressing the monorepo use case.
4. Similarly, explicitly running rustup toolchain link would add that prefix to the allowlist by default.

Does that seem reasonable at first blush? I won't pretend to be an expert on all the complications that I'm sure exist with supporting rustup across the variety of environments that are out there, but it seems like that would handle the use cases I know of and that have been articulated here.

[lovesegfault]

@LawnGnome As stated, option (2) would be perfectly workable on our end, though the devil's always in the details.

I'm strongly in favor of rolling back the recent relative path work as well, regardless of the specifics of (2)'s design.

[rbtcollins]

 I guess, security wise there's also an angle that the attacker could just guess absolute paths? Like, it's pretty obvious, if one stalks me, that I clone stuff into either /home/matklad/p or /home/matklad/tmp, so, if someone delibirately targets me, they potentially could just hard-code those absolute paths.

We could also exclude the path to the dir, so the toolchain must always be provided in a separate tree ... and then you'll get git repos with
+

• • toolchain-attack-dir
    +- attractive-dir-to-be-lured-into

So yeah, I think 3 going to be fragile and unsustainable.

[rbtcollins]

So, given that, option 2 (including a rollback of the recent relative path change) feels right to me. Am I right in thinking that the "trusted directory" functionality hasn't yet been implemented or designed in any serious way? I think this is personally how I'd want the check to work:

Theres an RFC for it in cargo, that also speaks to rustup. A lot of design work, and last I recall I had raised some concerns/questions.

And yes, I'd consider any form of configuration a variant of (2).

On the question of rolling back relative-path support - let my hindbrain think about it please, but do feel free to articulate the value proposition for it.