Aliases should not be able to shadow external subcommands

[5225225]

Problem

If Ihave a repo that has, in it's .cargo/config.toml,

[alias]
crev = "run --quiet -- delete system 32"

then running cargo crev in this crate will run repo-controlled code, which could be a security problem. Granted, if you don't trust the repo then you need to be careful to not build it (because of build scripts), but allowing crev to be overidden seems like a bad idea.

Proposed Solution

Treat external subcommands (binaries with the name cargo-<subcommand>) the same as known subcommands for purposes of aliasing.

You already get a warning: user-defined alias build is ignored, because it is shadowed by a built-in command warning when overriding a built-in, this should extend to all global subcommands that exist outside the repo.

Notes

No response

[joshtriplett]

In #9768 (comment) , we did agree that we want to do this.

This would be a transition, since people might have such aliases today. So if someone wants to implement this, it would need to start with a warning, then a transition period, and then become an error.

[MaulingMonkey]

since people might have such aliases today.

I'm those people - I've been abusing shadowing external subcommands a lot. I have two main use cases:

1. local subcommand development - e.g. in the development dir for cargo-mysubcommand I'll typically have:

   # .cargo/config.toml
   [alias]
   mysubcommand = "run --bin cargo-mysubcommand -- mysubcommand"

   so cargo subcommand won't accidentally run my stale, previously globally installed copy, instead of my development copy, during development. Causing an error when trying to use said subcommand is... marginally acceptable, requiring a cargo uninstall cargo-mysubcommand to resolve the ambiguity. Causing an error when merely parsing would be unworkable, but it seems the warning isn't generated when using other, nonconflicting aliases.

2. local subcommand overrides - when a subcommand provides baseline behavior for most projects, but the tool isn't configureable/flexible enough for my needs. E.g.: thindx/.cargo/config:7 overrides global cargo-vsc behavior for thindx-specific .vscode/*.json generation. I suppose I can make cargo vsc test for the existence of a cargo vsc-override or check for cargo-vsc.toml or something?

(TL;DR: I can adapt, and I don't have a better alternative to recommend, but I at least want to complain 🤣)

[5225225]

Maybe it should only apply to shadowing that exists in repos?

Basically, introduce a distinction between "trusted config" and "untrusted config".

Anything in $HOME/.cargo/config.toml is trusted. If you can write there, you can presumably write to .bashrc, and you're screwed if someone can do that.

But don't trust any file that was..... loaded from a git repo?

You'd also want a way to explicitly trust configuration / commands.

Maybe a cargo config trust command (cargo config is a builtin and can't be shadowed even today) which writes an entry like

[trusted-configs]
"/home/$USER/src/some_project/project/.cargo/config" = "f52fbd32b2b3b86ff88ef6c490628285f482af15ddcb29541f94bcf526a3f6c7"

(or maybe the hashing is too much of a source of complexity, and just say "if you trust it once, you trust everything that can be written into that path)

And then a config is trusted if it is listed in a trusted configuration (that is loaded), and then whatever hard coded paths for the config (.cargo/config, /etc/cargo/config maybe idk if that exists) is then whatever starts out as trusted.

This would be a big change, but it would perhaps be useful to gate some more dangerous functionality under? Though as I said above, you are running code, so there's not too much you can do that isn't instant ACE from a hostile codebase.

[weihanglo]

FWIW, the security concern might be mitigated a bit after #10736 got merged.

[hanna-kruppe]

I see that #13629 proposes to turn this into a hard error. I don't know why now (it doesn't seem edition-dependent), but in any case: please don't do this before giving users a practical way to turn off this warning/error! Having to remove either the alias or the globally installed command is not very practical.

I understand not trusting random checked out git repositories to shadow a command like cargo crev. But there are legitimate use cases for shadowing other commands, especially those that build/run arbitrary code and therefore shouldn't be used in untrusted source trees anyway. It may not break CI pipelines because those rarely rely on aliases and also install the same subcommand into $PATH. But it can still be very annoying for users who are relying on such aliases for their local work. @MaulingMonkey gave some examples earlier, let me add my own. I mostly encounter this warning with commands like cargo fuzz and cargo nextest that are used regularly during local development. Why do I have aliases for these?

1. In projects where I can make such decisions, I prefer to pin the tools to a specific version, both to ensure that the same version is used in every checkout of the project and to allow updating (or not updating) them on a project-by-project basis. This is common in other software ecosystems (npm, Python), not so common in the Rust world, but totally doable - by defining aliases in the project's .cargo/config.toml. I have also pointed others to this possibility and to cargo-run-bin when they brought up this topic in discussions.
2. However, these tools are also useful for many other Rust project whose source code I check out, even if they don't subscribe to the "pin the versions of the tools you use" approach. Thus, I generally have these tools installed globally as well, except when I don't need them for a while and the deprecation warning starts getting on my nerves. If it was a hard error, I'd have to uninstall them immediately whenever I switch back to a project in the first bucket.

Keeping in mind the problem for tools like cargo crev, I wouldn't suggest not having this warning/error at all. But I'd like to have some way to opt into shadowing of specific subcommands being allowed (and not even raising a warning), for example via a new Cargo config key. That wouldn't solve the problem that some config files shouldn't be trusted, but RFC 3279 faces and solves the same challenge for the "safe directories" setting. The same solution could be applied for a "safe aliases" setting: only accept it in $CARGO_HOME/config.toml and possibly from environment variables, not from any other sources.

[epage]

#2267 is our issue for running programs from your dependencies. That is blocked on the artifact-deps feature and needs design work.

#12235 is our issue for user controlled lints. However, this is dependent on [lints] and we have no equivalent for config files, unless a problem can be re-framed in terms of a workspace. However, I feel like if turned the warning into an error, any motivation for doing so doesn't make sense if we make it configurable.

As for Editions, they cannot control the behavior of config unless we again find a way to re-frame things in terms of a workspace.

We do allow changes outside of Editions under some circumstances. The justification for this was not given (unless I missed it) and I wasn't a part of the team at that time. However, we have been warning people about this for 3 years at this point (for context, this is longer than cargo nextest has been around and 1.58 is old enough to be in Debian stable iirc).