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 https://github.com/rust-lang/cargo/pull/9768#issuecomment-900583118 , 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 https://github.com/rust-lang/cargo/pull/10736 got merged.