Cargo and Rustup safe file discovery. #3279

ehuss · 2022-06-08T20:10:37Z

This RFC is a proposal to fix a security issue with how Cargo and Rustup discover their files.

In consultation with the Security Response Working Group, we decided to disembargo this and solicit feedback and testing before this change is made. We feel like the relative security risk of this is low compared to the other risks involved with using these tools. Additionally, we think that the risk is high that this will be a very disruptive change. We would like to get feedback from the community and let you have a chance to test and prepare your environments ahead of time instead of pushing a new stable release immediately.

However, we are planning to move this change through quickly so that the protection is available to users as soon as possible. Our current thinking is to aim for stabilizing it in July for the 1.64 release which will be released in September. The implementation will be available on nightly soon, and a pre-release version of Rustup will also be made available as soon as possible.

Rendered

fbstj · 2022-06-08T20:39:55Z

text/0000-cargo-rustup-discovery.md

+Since the settings file is private to Rustup,
+the following CLI options can be used to manage this setting:
+
+* `rustup set safe-directories add PATH`


does rustup set safe-directories add . expand to the absolute path of the current directory? do other relative paths expand?

I would expect to process such paths through std::fs::canonicalize() if that's what you mean.

Thanks for raising this point!

The current PR does not attempt to guess what the user intended, and adds it literally. However, I see that is not a good approach as I imagine something like rustup set safe-directories add . would not be unreasonable.

I started down the path of figuring out the best way to deal with this, but I've run out of time/energy. Using fs::canonicalize is difficult and fraught with problems, particularly on Windows. I want to avoid putting verbatim paths in the config (as they get exposed to the user). Unfortunately fs::absolute isn't stable (I would very much prefer to use GetFullPathNameW over anything else). I considered using Cargo's hacky normalizer, but that has its own problems. And I'm finding it difficult to consider the security considerations of these approaches.

I'll continue to work on it when I can.

nagisa · 2022-06-08T20:44:24Z

text/0000-cargo-rustup-discovery.md

+This could significantly ease the burden of dealing with this issue,
+as it will avoid needing to set safe directories in almost all use cases where they would be needed.
+
+However, I feel like this approach is too risky.


OTOH this makes use-cases that use group-wide permissions (i.e. rwxrwx---) and a umask setup in a certain way work mostly out of the box.

I'm somewhat invested in this, since I tried to utilize that sort of approach, though not with Rust...

nagisa · 2022-06-08T20:47:03Z

text/0000-cargo-rustup-discovery.md

+(unless they have elevated permissions like root).
+I believe this is the case for all major operating systems and filesystems.
+
+One (major) known exception is if a network-mounted filesystem is configured to map all files to the local user, which is not uncommon.


Another major exception is UID reuse – where the attacker creates an user, poisons the filesystem and then later a genuine user gets created at that specific UID.

systemd comes to mind as an example of a tool that a) creates users left and right; and b) has mitigations for this problem.

Thanks! I have added that as another concern. I personally don't feel like the risk here is very high, as there are likely other avenues than Rustup/Cargo for escalating once an attacker has control of a temporary new user. However, it definitely is a concern.

ChrisDenton · 2022-06-09T01:05:04Z

Should mention be made of TOCTOU concerns? Or is that not considered a threat?

teor2345 · 2022-06-16T08:54:48Z

text/0000-cargo-rustup-discovery.md

+If you have a directory `Foo`, and you type `cd foo`,
+then the operating system will report the current directory is `foo`.
+
+I am not aware of an easy way to detect if a filesystem is case-sensitive.


A typical case-sensitivity check creates two files with names that only differ in case. If the second create fails, or listing the directory only returns one file, then the filesystem is case-insensitive.

But it's faster to get the metadata of an existing file, via a path with a different case:

List the directory containing the file a

If the directory contains two entries for the file a that only differ by case, the filesystem is case-sensitive

Get the metadata of the file A, which has a name that only differs by case

If the query is successful, the filesystem is case-insensitive

Otherwise, if it returns a file not found error, the filesystem is case-sensitive

Unfortunately there's no easy API for this, because Windows can change case-sensitivity by directory, macOS by process (rarely), and most OSes can change it by filesystem.

RalfJung · 2022-06-24T17:48:46Z

I don't entirely get the attacker model here. Let's say we are in the specific situation where there is a Cargo.toml in my current directory. If I do cargo build, that will run build scripts, which can do arbitrary things. What is the additional risk posed by Cargo.toml specifically that makes it so that we need a special defense against evil Cargo.toml but nothing similar against evil build scripts?

I agree that using files arbitrarily far up the folder hierarchy can be a problem, as described in the RFC. But I clearly trust the code in my current directory if I build+run it, so I don't see any harm coming from also trusting the Cargo.toml there.

BlackHoleFox · 2022-07-07T23:47:26Z

text/0000-cargo-rustup-discovery.md

+This may be particularly difficult for a user running Windows `cargo.exe` from within WSL.
+Paths in this environment look like `\\wsl$\Ubuntu-20.04\home\eric\foo`.
+I don't know how common this use case is, as I would expect someone doing things in WSL would install the native rustup/cargo binaries instead of using the ones from Windows.


That does sound like a tad pain. I even wrote a build definition that relies on being able to call the Windows installation of Cargo from WSL to make building easier.

I think that quite a few pieces of software have become unreasonably frustrating when trying to enforce permissions while running in WSL, fwiw.

Interesting! I was not expecting anyone to be doing that, though I guess I'm biased by mostly using mingw instead.

I think the error message should provide something that should work to copy-and-paste, so I hope that should be sufficient. This is also something that we may be able to iterate on without breaking things.

BlackHoleFox · 2022-07-08T05:27:24Z

I agree that using files arbitrarily far up the folder hierarchy can be a problem, as described in the RFC. But I clearly trust the code in my current directory if I build+run it, so I don't see any harm coming from also trusting the Cargo.toml there.

Yeah, this also makes sense to me. Opening a directory in your shell (or editor, but they already have solutions for this problem) is an explicit action taken by the user. Opening it means they're saying "I, my person, wants to do something here."

Hypothetically, if cargo allowed implicitly trusted everything in pwdir downwards, wouldn't that also get rid of a huge swathe of usability footguns listed in this RFC? sudo cargo would work (even if this is a certified bad idea 99% of the time), running Win32 cargo from WSL would work, and probably others too. That is, at least in common use cases though. Even with that, saying that you can't build a crate in a workspace from that crate's source directory from a different user account (or OS like WSL), but instead need to go from the crate workspace sounds more reasonable then blocking it in all cases.

ehuss · 2022-07-11T23:35:55Z

Should mention be made of TOCTOU concerns? Or is that not considered a threat?

The original contained a note about that, but I removed it since I didn't want to list all the things that won't go wrong. I added a small note back in. I do not consider it a threat since the attacker would need to have write access to the user's files, and that means they already have escalated privileges which I consider out of scope for this fix.

Did you have any particular threat scenario that you are concerned about?

ehuss · 2022-07-11T23:39:00Z

I agree that using files arbitrarily far up the folder hierarchy can be a problem, as described in the RFC. But I clearly trust the code in my current directory if I build+run it, so I don't see any harm coming from also trusting the Cargo.toml there.

This was a point we discussed quite a bit, and it is difficult for me to confidently make this exception. I have added some more discussion in the "Honor the user of the current directory" section.

I concede that the risk is small, but I am concerned about the possible inconsistency. If there is broad agreement that this exception is worth it, then I would consider it.

Opening it means they're saying "I, my person, wants to do something here."

This was one of the leading concerns that led git to implement their fix. The concern is that navigating to a directory may not necessarily be a signal that "I trust everything in this directory" (particularly for beginners). The example is a shared directory used by multiple students as a scratch space, where they may cd into that root space. I think cd to most users, including myself, is not a strong signal of that trust.

RalfJung · 2022-07-12T00:03:08Z

This was one of the leading concerns that led git to implement their fix. The concern is that navigating to a directory may not necessarily be a signal that "I trust everything in this directory" (particularly for beginners). The example is a shared directory used by multiple students as a scratch space, where they may cd into that root space. I think cd to most users, including myself, is not a strong signal of that trust.

No, cd is indeed not a signal of trust. Neither is git status, so I agree with the git decisions. But cargo build is to an extend (if people are familiar with build scripts and proc macros), and cargo {run,test} definitely are!

When I do ./run-thing, I also don't get a warning from my shell that it is running a thing. Such a warning would be rather disruptive and serve little purpose other than incur warning fatigue. This feels similar.

(To be clear, I would love to be able to do cargo build without having to trust the crate authors. But that is not the discussion we are having here.)

Or is the point that users are not aware that cargo build runs build scripts and proc macros and we need to teach them? The RFC is not phrased like that.

RalfJung · 2022-07-12T00:11:17Z

text/0000-cargo-rustup-discovery.md

+
+```text
+error: `C:\Projects\MyProject\Cargo.toml` is owned by a different user
+    For safety reasons, Cargo does not allow opening manifests owned by


"For safety reasons" is pretty useless information. It's the kind of wording someone uses when they don't want to justify their actions towards the public or the press.

For users to be able to do an informed decision, this should explain what the associated safety risks are. Is this referring to build scripts and proc macros, or some other kind of safety risk?

I am thinking of something like this:

error: manifest file `C:\Projects\MyProject\Cargo.toml` is owned by a different user If you proceed, cargo will build and run code as instructed by that file. Even just 'cargo build' or 'cargo check' will run code controlled by the manifest. [...]

Thanks, I have tweaked the wording to briefly explain what the safety concern is.

ChrisDenton · 2022-07-13T20:22:52Z

Should mention be made of TOCTOU concerns? Or is that not considered a threat?

The original contained a note about that, but I removed it since I didn't want to list all the things that won't go wrong. I added a small note back in. I do not consider it a threat since the attacker would need to have write access to the user's files, and that means they already have escalated privileges which I consider out of scope for this fix.

Did you have any particular threat scenario that you are concerned about?

Ok, I'm still trying to get a handle on what the threat model is. So we take ownership to mean that if the file gives another user rename/delete or write permissions then that is something the owner presumably intended and is therefore "safe" under this model.

What about ancestor directories? The RFC says:

I don't see a particularly strong reason to check the ownership along the way.

But surely if there's a symlink in the path or an ancestor directory can be renamed by another user then that can be exploited? Or is that not considered a threat here?

comex · 2022-07-14T07:15:19Z

This fix assumes that a user can't in any way create a file owned by another user (unless they have elevated permissions like root). I believe this is the case for all major operating systems and filesystems.

On macOS, and on Linux with fs.protected_hardlinks=0 (most distros set it to 1 though), any user can create a hard link to a file owned by another user, putting it in an arbitrary directory they have write access to with an arbitrary name. The resulting link will have the same ownership and permissions as the original and is generally indistinguishable from any other file. (Well, you can detect whether a file has multiple links to it by checking st_nlink, but that doesn't help if the original file is deleted for any reason, in which case st_nlink goes back to 1.)

The hard link creator can't affect the contents of the file, so for this to be dangerous, there would have to be some existing file owned by the target user, at some other path in the same filesystem, whose content would be dangerous if interpreted as config.toml, rust-toolchain.toml, etc. But it's not hard to imagine ways this might occur.

This issue doesn't affect Git's directory ownership check… I think… because macOS and Linux don't support hard links to directories, at least by default. (macOS does or did support them for HFS+ mounts, but not the newer APFS.)

text/0000-cargo-rustup-discovery.md

weihanglo · 2022-07-20T23:30:48Z

text/0000-cargo-rustup-discovery.md

+causing it to open repositories from every directory the user enters.
+
+`git` has been changed so that it will not open a repository owned by a different user, and instead return an error.
+For most cases, it will check that the ownership of the directory *containing* the `.git` directory matches the current user.


Might worth mentioning that a fix for CVE-2022-29187 (missing part of CVE-2022-24765) was released two weeks ago: git/git@3b0bf27

Basically, git now takes into account the ownership of gitdir and gitfile as well. Unlike git, this RFC proposes to check files themselves, so I don't think we need a revise accordingly.

bjorn3 · 2022-10-25T13:46:57Z

text/0000-cargo-rustup-discovery.md

+
+## Rustup toolchain override discovery
+
+When searching for a `rust-toolchain`/`rust-toolchain.toml` file, Rustup will check that the ownership of the file matches the current user.


I think this only needs to apply if the toolchain is specified using a local path rather than an official toolchain.

As mentioned in Granular capabilities, we didn't want to go down the route of trying to do fine-grained checks of the contents of files or determining exactly which parts would be safe and which wouldn't. I don't have confidence that we would be able to do that securely now, and particularly as things are added in the future. It might be possible to have something similar to what gitoxide has by marking what is safe and not (possibly at the type level), but that would likely be a very intrusive change.

I'd also not be so confident that official toolchains are "safe". For example, an attacker could force you to use an older version of Rust which has some vulnerability that they could then exploit in some way.

This could possibly be done in the future, but I'm not sure there would be a large value if it was done in just rustup and not cargo, since the false-positive scenario would likely involve both.

text/0000-cargo-rustup-discovery.md

…ting to address.

ehuss · 2023-05-20T23:23:42Z

I'm sorry for letting this languish for so long. Certain issues came up last year, and then I intended to return to this in February, but other things interrupted again.

I have pushed several updates to the RFC incorporating some feedback, and updating for more recent changes.

Or is the point that users are not aware that cargo build runs build scripts and proc macros and we need to teach them? The RFC is not phrased like that.

I've tried to clarify the RFC on the threats that this is intended to address. This is not quite related to build scripts or proc-macros. It is related to cargo or rustup or rustc (or any proxy) being able to run arbitrary executables if a user has write access to a directory above them. Cargo and rustup provide a myriad of ways to execute programs via various config options, where build scripts and proc-macros are only a part of that.

Ok, I'm still trying to get a handle on what the threat model is. So we take ownership to mean that if the file gives another user rename/delete or write permissions then that is something the owner presumably intended and is therefore "safe" under this model.

I don't see a particularly strong reason to check the ownership along the way.

But surely if there's a symlink in the path or an ancestor directory can be renamed by another user then that can be exploited? Or is that not considered a threat here?

I'm not sure if "intended" is quite the right word, since it may or may not be intentional. But I think it is somewhat outside of the scope of this RFC to have cargo or rustup police the ACLs of the files it accesses to require that they not be changeable by other users.

I have added an extended TOCTOU section to try to address some of the concerns. I'll admit that this is one area where I feel the least confident in, so if anyone can provide counter-arguments with concrete examples, that would be extremely helpful.

ehuss · 2023-06-06T18:59:36Z

I pushed a change to add a temporary warning period to give users advance notice of this change. I'm hoping that should ease the disruption and make it easier for users to prepare for it.

ehuss · 2023-06-06T19:05:09Z

I'm moving to go ahead and move to merge this.

This will also require the approval of the rustup team. However, since they are not configured for rfcbot, and rfcbot does not handle multi-team approvals very well, I'm going to handle them manually. This will not be merged without similar approval from that team. (rustup team: feel free to check a box, or leave a comment, or any blocking concerns.)

@rfcbot fcp merge

rfcbot · 2023-06-06T19:05:11Z

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

rustup-approval (Cargo and Rustup safe file discovery. #3279 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

ehuss · 2023-06-06T19:05:35Z

rustup review tracking:

epage · 2023-06-06T19:28:14Z

I pushed a change to add a temporary warning period to give users advance notice of this change. I'm hoping that should ease the disruption and make it easier for users to prepare for it.

As collecting feedback is the hardest problem, having a warning period helps resolve that and we can pivot if we need to. I'm good with moving forward with this.

As a wild idea, we could possibly express this as a diagnostic that we change the level for (rust-lang/cargo#12235). The main issue being that we need to know the users desired level before we even parse to find the package/level. I mentioned in that Issue that this is a general problem and we might need some cargo-diagnostic control in the config file for these non-package, non-workspace use cases.

ChrisDenton · 2023-06-06T19:31:25Z

Should you perhaps add a blocking concern mentioning the rustup team so rfcbot won't proceed until that's manually resolved?

ehuss · 2023-06-06T19:50:46Z

@rfcbot concern rustup-approval

rbtcollins · 2023-06-06T19:55:18Z

@ChrisDenton that would be good - I'm not quite sure how to do that - I haven't interacted with the RFC automation so far. https://www.rust-lang.org/governance/teams/dev-tools#Rustup%20team is up to datish (I'm not sure if @kinnison wants to step down until he's more available or not), so any leadership bits that need to be set should be set already.

I will try the bot though, just in case.

@rfcbot concern rustup hasn't reviewed

Context for that: This started while @kinnison was active, but he's stepped away for a while, and I haven't had time to read or think about this thing end to end. I've given it a quick glance, and I think its not clear what the threat model we're actually tackling is (for instance, if its 'shared parent directories can permit injection of arbitrary code via toolchain files', then the next release of rustup will correct that. But a toolchain file could still pick a pre-safe-aware version of cargo, and then attack that. But neither of these address the things that vscode and I presume other IDEs aim at with their 'trusted directory' UI, which is downloading of arbitrary git repos from the internet.

Introducing this will be a breaking change (by definition - an opt in security feature rarely works), so I'd like us to at least consider all the cases that are clear and visible today, which I will prioritise amongst the time I have available to do rustup contributions.

I appreciate the RFC has been around for a while, but this is the nature of volunteer personal-time contributions.

kinnison · 2023-06-07T14:11:32Z

I'm not sure if @kinnison wants to step down until he's more available or not

I have submitted a PR to step down from the team for now, I'm not going to be able to offer help in the medium term at least.

cbeuw · 2023-07-13T12:52:20Z

and I think its not clear what the threat model we're actually tackling is

While security is the main intention of the RFC, I want to mention that the RFC is also functionally necessary on some trusted but weirdly configured shared systems: rust-lang/cargo#12080

In my case, this is ETH Zürich's HPC cluster Euler. The OS creates a file under the parent of the user's home directory whenever an open syscall is attempted on a non-existent file. This dummy file does not give anyone read permissions, so the open fails with permission denied instead of file doesn't exist.

To scale the cluster to the thousands of users that we have, and to enable seamless login for any ETH member, an automounter mounts home directories onto /cluster/home on demand. Each time someone accesses /cluster/home/X and /cluster/home/X does not exist, the automounter is run. Due to technical limitations of the automounter and the operating system, this takes a few seconds, during which the system is noticeably slower for some I/O. Since some software continuously tries to “go up” the filesystem tree to find file /cluster/home/X, the system can be put under a denial of service (DOS) if /cluster/home/X does not exist. We have therefore been forced to create dummy entries for failed file lookups: these are what you encounter.

IMO this is a very janky solution, but as a user I can't do anything about it. Nonetheless, most other software that does recursive lookups (git, Python with modules) handle this without issue, but Cargo just breaks when a Cargo.toml file exists but has no permission to read it.

rbtcollins · 2023-07-14T18:58:00Z

@cbeuw I can imagine Cargo breaking on existing but unreadable files even with this RFC implemented. I suggest you file a bug for that (and perhaps work on a PR).

rbtcollins · 2023-07-14T21:21:15Z

Ok, so full review of the RFC finally - appreciate the patience with volunteer timescales.

A nit: On Windows C:\ is not world writable by default. The default permissions are read-only + create folders for users, with only Administrators having full write access. Threat actors that can create C:\Cargo.toml can also directly take over ownership of the target folders and write hostile code to e.g. replace rustup proxies. Any user could create a .cargo at the root though, but I don't know Cargo well enough to know if that is exploitable. Of course, systems can be misconfigured too, including labs, network servers and so on.

The threat statement : The primary threat this RFC is aiming to address is the situation where cargo or rustc enables other users or malicious programs to elevate their access by running arbitrary programs under your account is good; when I read that, it by definition includes downloaded git repositories, because asking users to audit every single vector present in what they downloaded is unreasonable. (Concretely, every vector the RFC covers directly can also be present in a downloaded repository, plus more (such as build.rs scripts with malicious dependencies and probably other more creative ways to cause havoc)). IDEs like vscode have a similar trust feature.

I propose we consider expanding this RFC's scope a little:- explicitly consider these threats and align with IDE behaviour. For instance, refusing to build a downloaded directory that uses any settings that can be escalated (e.g. build.rs, rustc wrappers and so on). The impact should be minimal, and it avoids have two back to back contract breaks, rather than one.

I realise the RFC rules this out today:

This RFC is not aiming to address the general threats of arbitrary execution of code defined in dependencies (such as build scripts or proc-macros) intentionally added by the user. Those threats still require reviewing the code and trust relationship with those dependencies.

I think the existing initiatives like cargo vet are great for empowering developers writing build.rs scripts or using crates with proc-macros to make good informed choices about dependencies. The specific case I'm highlighting is 'user has downloaded source' vs 'existing developer is adding dependencies'. They are likely related, but are quite different, and 'freshly downloaded code' looks closer to 'building in a tree with untrusted content' than 'editing source and evolving things'.

Moving on, I agree that Rustup will need this. Even with relative path toolchains removed, a hostile local directory that has been prepared can reference a hostile absolute path based toolchain : refusing relative paths makes downloading repositories safe, not running from arbitrary local directories. To me it makes sense to honour CARGO_SAFE_DIRECTORIES and $CARGO_HOME/cargo.toml rather than having users have to set this based on whether they use rustup or not - while any one user would only have to set it in one place, it is still complexity, and one has to be rigorous to drive complexity down. It is better for us to decide on one place to set it rather than pushing that complexity off onto users.

I don't agree with this line:

Rustup sets the RUSTUP_SAFE_DIRECTORIES environment variable when launching a tool via the Rustup proxies. This allows the user to configure their safe directories in one place (with Rustup), and have tools like Cargo inherit those settings.

I don't think it makes sense for rustup to set that variable when a proxy is invoked: if it is set already, it will pass through to the child process. If it is not set already, there are no sensible values for rustup to add : the current working directory can't be assumed to be safe, and the toolchain directory should be irrelevant. Perhaps the intent was to say that rustup's config file can store a safe directories list too; in which case the guide explanation listing 3 places for the setting should actually list 4 :)

https://github.com/ehuss/rfcs/blob/cargo-rustup-discovery/text/0000-cargo-rustup-discovery.md#cargo-config-discovery doesn't specify about the behaviour when a mismatch occurs. I suspect the intent is to error.
I'd like to propose an additional behaviour, copied from the Git CVE blog post. Once a directory has different ownership, stop searching with no error. If a needed file (e.g. Cargo.toml hasn't been located that is still an error, but it should also mention that the search stopped because had different ownership - to support diagnosing the situation) This prevents the attack and should avoid needing to set the safe variable as often.

While I agree that

Protecting against an attacker that has the ability to create files owned by the victim, or to replace an ancestor directory is out of scope for this RFC.

Not being TOCTOU safe opens risks that do not require creating files owned by the victim. For instance, a shared lab with world writable common directory allows for a victim created .cargo/config.toml that is then replaced by the attacker. APIs for reading metadata from file handles exist for both unix and windows, the only wrinkle being that processing a symlink stepwise is poorly defined on Unix, and will necessarily introduce a race. Considering the ownership of the target rather than the link itself would eliminate the race. And I think we should do that, because an attacker that can rename a legitimate symlink and replace it with a bad one: a hard race to win but possible without being able to impersonate ownership, and likely possible in the motivating configurations. That or we should write a very clear doc about how to secure Cargo/Rustup in these situations.

On the drawbacks. Rustup does have system configuration, so the ability to clear the setting would be useful if we decide to use rustup's config store.
I think adding interpolation later would be a compatible change, particularly if we document that consideration now.

I agree that we shouldn't honor the user of the current directory: its not obvious at all that cd'ing to a directory means trusting its contents. For instance shell prompts that query Cargo metadata could be silent ways that arbitrary code would run just by cding.

On the Check-the-ownership-of-directories-while-traversing: the overhead should be extremely low but we could test easily enough. But a middle-ground would be to behave as though we had. Concretely when we find a mismatched config file, before erroring, see if the parent directory would have triggered stopping searching, and if so, just ignore the file and stop searching.

On WSL: I don't think we should do anything special here. Yes, Microsoft has muddied the waters with some transparent glue, but the expected behaviour is definitely installing host-native tools and rustup versions.

On checking for case sensitivity... this is probably impossible in all cases, but a decent approximation is to assume that Linux is always case sensitive (e.g. ignoring mounted FAT fs's), and on windows query the file system https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getvolumeinformationa . There may be a per-directory setting as well - there are windows CLI tools that can control that, but I haven't found the underlying API. @ChrisDenton probably knows :).

ChrisDenton · 2023-07-14T21:52:07Z

On checking for case sensitivity... this is probably impossible in all cases, but a decent approximation is to assume that Linux is always case sensitive (e.g. ignoring mounted FAT fs's), and on windows query the file system https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getvolumeinformationa . There may be a per-directory setting as well - there are windows CLI tools that can control that, but I haven't found the underlying API. @ChrisDenton probably knows :).

You can check per-directory case sensitivity using GetFileInformationEx and the FileCaseSensitiveInfo class but I would suggest you don't. You can either get the canonical case of a path or if you want to know if two paths lead to the same file (or directory) then open them both (and keep them open) then test if they're the same.

I think insisting that users use the canonical case in config files, etc is a fair compromise in an initial implementation. Though admittedly it will annoy some users and those users may strongly disagree with me on this point.

ehuss · 2023-07-27T22:37:27Z

Thanks @rbtcollins for the review!

A nit: On Windows C:\ is not world writable by default. The default permissions are read-only + create folders for users, with only Administrators having full write access.

Thanks for the clarification. I didn't want to get into the details of when the root is writeable since I believe it is a complicated question (depending on various factors). I tried to update the text to make it clearer that it only applies in some situations.

I propose we consider expanding this RFC's scope a little:- explicitly consider these threats and align with IDE behaviour. For instance, refusing to build a downloaded directory that uses any settings that can be escalated (e.g. build.rs, rustc wrappers and so on). The impact should be minimal, and it avoids have two back to back contract breaks, rather than one.

I may be misunderstanding this suggestion, but it sounds like a very large change with a large impact. The intent of this RFC is that the vast majority of users will not be affected and won't even know this is a thing. This sounds like it is suggesting that every directory needs to be approved before it can be used, which sounds like it would affect essentially everyone. Can you clarify what is meant here, or why it seems like it should be a minimal impact?

To me it makes sense to honour CARGO_SAFE_DIRECTORIES and $CARGO_HOME/cargo.toml rather than having users have to set this based on whether they use rustup or not - while any one user would only have to set it in one place, it is still complexity, and one has to be rigorous to drive complexity down. It is better for us to decide on one place to set it rather than pushing that complexity off onto users.

I'm not sure I understand this completely. Are you suggesting that rustup should check for CARGO_SAFE_DIRECTORIES and read $CARGO_HOME/config.toml (I assume that was a typo)? And the intent is so that there is one place to set this whether the user is using rustup or not?

I would have some pretty high concerns about rustup trying to parse and understand cargo's config files. Trying to coordinate changes across the tools I think will be challenging, and potentially confusing (users may use rustup without cargo, for example). I'd also be concerned about the performance impact, since toml parsing can be a little slow, and the proxy startup time is already a concern.

I don't think it makes sense for rustup to set that variable when a proxy is invoked: if it is set already, it will pass through to the child process. If it is not set already, there are no sensible values for rustup to add : the current working directory can't be assumed to be safe, and the toolchain directory should be irrelevant. Perhaps the intent was to say that rustup's config file can store a safe directories list too; in which case the guide explanation listing 3 places for the setting should actually list 4 :)

The intent is that rustup is setting the environment variable from rustup's config. The place that lists the three options is specifying what cargo will read. The next section is for rustup which specifies what rustup will read. I altered the text to try to make that clear.

Once a directory has different ownership, stop searching with no error.

Perhaps I'm not understanding this suggestion properly, but as I'm reading it, it seems to be suggesting that no errors are ever generated? That seems to be a pretty large change to this RFC, perhaps you can clarify it in more detail?

In general, I don't think this is an option because that could lead to fragmented or broken configurations that could be quite challenging to diagnose and fix. For example, Cargo's workspaces could be broken such that a workspace member would load, but the workspace configuration would not. Also, the intent of the error is to be an alarm to alert the user something suspicious is happening. Although we want to avoid false positives as much as possible (and almost all errors are going to be false positives, since exploiting this is probably going to be very unlikely), I don't think we want to avoid the main purpose of the check.

Not being TOCTOU safe opens risks that do not require creating files owned by the victim. For instance, a shared lab with world writable common directory allows for a victim created .cargo/config.toml that is then replaced by the attacker.

This is an interesting point, and I'd like to dive into understanding it in more depth in a followup.

Just FYI, I'm mostly basing this off of git's implementation, which also does not do handle-based checks, and thus has limited TOCTOU protections. Implementing something that combines traversal, checks, and opening of files will likely be intrusive, making it harder to incorporate this change.

ehuss · 2024-05-09T18:02:22Z

@rust-lang/rustup Just checking in again to see if this is something ya'll can look at for approval? Thanks!

rami3l · 2024-05-10T02:20:13Z

@ehuss I have no more questions regarding this RFC. Can you tick the box for me? Thanks!

Update: I've made rust-lang/rustup#3935 to track the implementation on Rustup's side.

djc · 2024-05-13T11:07:20Z

Ticked my box, LGTM.

rami3l · 2024-10-26T05:21:32Z

rustup review tracking:

@rbtcollins

@djc

@rami3l

This list needs to be updated since @ChrisDenton has currently joined the team :)

cc @ChrisDenton @rbtcollins

ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Jun 8, 2022

fbstj reviewed Jun 8, 2022

View reviewed changes

nagisa reviewed Jun 8, 2022

View reviewed changes

ehuss mentioned this pull request Jun 8, 2022

Add safe.directories config rust-lang/cargo#10736

Open

ehuss mentioned this pull request Jun 12, 2022

Add safe-directories. rust-lang/rustup#3005

Open

teor2345 reviewed Jun 16, 2022

View reviewed changes

BlackHoleFox reviewed Jul 7, 2022

View reviewed changes

RalfJung reviewed Jul 12, 2022

View reviewed changes

weihanglo reviewed Jul 20, 2022

View reviewed changes

weihanglo mentioned this pull request Aug 12, 2022

cargo new failed to detect vcs if parent of path does not exist rust-lang/cargo#10981

Open

jonhoo mentioned this pull request Aug 17, 2022

Bump git2 to 0.15 and libgit2-sys to 0.14 rust-lang/cargo#11004

Merged

bjorn3 reviewed Oct 25, 2022

View reviewed changes

text/0000-cargo-rustup-discovery.md Show resolved Hide resolved

weihanglo mentioned this pull request May 3, 2023

A Cargo.toml file in parent directory which you cannot delete prevents cargo from building anything rust-lang/cargo#12080

Closed

ehuss added 7 commits May 19, 2023 11:24

Cargo and Rustup safe file discovery.

0977aa8

Mention UID reuse.

5987f6d

Add note on TOCTOU

d4032ae

Discuss CWD more.

1822884

Some minor editorial fixes.

b7677ef

Add a section describing the kinds of security threats this is attemp…

a85f0d7

…ting to address.

Update the suggested error message to better explain what is happening.

4daaa0f

Introduce a temporary warning period.

0834734

rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Jun 6, 2023

Updates from review.

87233bc

epage mentioned this pull request Aug 15, 2023

Put Searching Parent Directories for Cargo.toml Behind Logical Gate rust-lang/cargo#7871

Open

epage mentioned this pull request Oct 27, 2023

Cargo confused about broken Cargo.toml in $HOME rust-lang/cargo#6706

Open

epage mentioned this pull request Mar 18, 2024

Exclude all non-members from workspace rust-lang/cargo#13600

Open

This was referenced May 10, 2024

Make the toolchain overriding config hierarchical rust-lang/rustup#3492

Draft

Environment variable to disable rust-toolchain.toml rust-lang/rustup#2793

Open

rami3l mentioned this pull request Jul 10, 2024

Tracking: Workspace trust & safe file discovery rust-lang/rustup#3935

Open

hanna-kruppe mentioned this pull request Nov 23, 2024

Aliases should not be able to shadow external subcommands rust-lang/cargo#10049

Open


		## Rustup toolchain override discovery

		When searching for a `rust-toolchain`/`rust-toolchain.toml` file, Rustup will check that the ownership of the file matches the current user.

Cargo and Rustup safe file discovery. #3279

Are you sure you want to change the base?

Cargo and Rustup safe file discovery. #3279

Conversation

ehuss commented Jun 8, 2022 • edited by epage Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisDenton commented Jun 9, 2022

Choose a reason for hiding this comment

RalfJung commented Jun 24, 2022

BlackHoleFox Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlackHoleFox commented Jul 8, 2022

ehuss commented Jul 11, 2022

ehuss commented Jul 11, 2022

RalfJung commented Jul 12, 2022 • edited Loading

RalfJung Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisDenton commented Jul 13, 2022 • edited Loading

comex commented Jul 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss May 20, 2023 • edited Loading

Choose a reason for hiding this comment

ehuss commented May 20, 2023

ehuss commented Jun 6, 2023

ehuss commented Jun 6, 2023

rfcbot commented Jun 6, 2023 • edited by Muscraft Loading

ehuss commented Jun 6, 2023 • edited by djc Loading

epage commented Jun 6, 2023

ChrisDenton commented Jun 6, 2023

ehuss commented Jun 6, 2023

rbtcollins commented Jun 6, 2023

kinnison commented Jun 7, 2023

cbeuw commented Jul 13, 2023 • edited Loading

rbtcollins commented Jul 14, 2023

rbtcollins commented Jul 14, 2023

ChrisDenton commented Jul 14, 2023

ehuss commented Jul 27, 2023

ehuss commented May 9, 2024

rami3l commented May 10, 2024 • edited Loading

djc commented May 13, 2024

rami3l commented Oct 26, 2024

ehuss commented Jun 8, 2022 •

edited by epage

Loading

nagisa Jun 8, 2022 •

edited

Loading

BlackHoleFox Jul 7, 2022 •

edited

Loading

RalfJung commented Jul 12, 2022 •

edited

Loading

RalfJung Jul 12, 2022 •

edited

Loading

ChrisDenton commented Jul 13, 2022 •

edited

Loading

ehuss May 20, 2023 •

edited

Loading

rfcbot commented Jun 6, 2023 •

edited by Muscraft

Loading

ehuss commented Jun 6, 2023 •

edited by djc

Loading

cbeuw commented Jul 13, 2023 •

edited

Loading

rami3l commented May 10, 2024 •

edited

Loading