rfcs
rfcs copied to clipboard
rust-lang
Cargo and Rustup safe file discovery.
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.
Should mention be made of TOCTOU concerns? Or is that not considered a threat?
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.
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.
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?
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.tomlthere.
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.
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.
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?
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.)
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 buildruns 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.
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.
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
Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Eh2406
- [x] @Muscraft
- [x] @arlosi
- [x] @ehuss
- [x] @epage
- [x] @joshtriplett
- [x] @weihanglo
Concerns:
- rustup-approval (https://github.com/rust-lang/rfcs/pull/3279#issuecomment-1579355014)
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.
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.
Should you perhaps add a blocking concern mentioning the rustup team so rfcbot won't proceed until that's manually resolved?
@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.
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.
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: https://github.com/rust-lang/cargo/issues/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.
@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).
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
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 :).
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.
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.