Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand documentation of PathBuf, discussing lack of sanitization #125060

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

ChrisJefferson
Copy link
Contributor

Various methods in PathBuf, in particular set_file_name and set_extension accept strings which include path seperators (like ../../etc). These methods just glue together strings, so you can end up with strange strings.

This isn't reasonable to change/fix at this point, and might not even be fixable, but I think should be documented. In particular, you probably shouldn't blindly build paths using strings given by possibly malicious users.

@rustbot
Copy link
Collaborator

rustbot commented May 13, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 13, 2024
tbu- added a commit to tbu-/rust that referenced this pull request May 13, 2024
This is likely never intended and potentially a security vulnerability
if it happens.

I'd guess that it's mostly literal strings that are passed to this
function in practice, so I'm guessing this doesn't break anyone.

CC rust-lang#125060
@tbu-
Copy link
Contributor

tbu- commented May 13, 2024

I think it'd be better if we fixed this at least for PathBuf::set_extension, where I can't come up with a legitimate use case: #125070.

tbu- added a commit to tbu-/rust that referenced this pull request May 13, 2024
This is likely never intended and potentially a security vulnerability
if it happens.

I'd guess that it's mostly literal strings that are passed to this
function in practice, so I'm guessing this doesn't break anyone.

CC rust-lang#125060
@workingjubilee
Copy link
Member

workingjubilee commented May 13, 2024

I think it's working-as-intended for the set_file_name case.

@workingjubilee workingjubilee added I-libs-nominated Nominated for discussion during a libs team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 13, 2024
@workingjubilee
Copy link
Member

I think this should be discussed before I make a decision to accept or reject this, given that this implies some pretty hefty security concerns or usability questions, either way.

@ChrisJefferson
Copy link
Contributor Author

Just to say, I did send a message about this issue to security@rust-lang, before posting it, just in case it was deemed as a security issue, but after discussion I was told it wasn't considered a security issue.

I assumed possibly breaking existing code by adding panics would be considered a no-no, even when that code is doing some very silly things, but I have no idea what is acceptable.

@workingjubilee
Copy link
Member

workingjubilee commented May 14, 2024

I assumed possibly breaking existing code by adding panics would be considered a no-no, even when that code is doing some very silly things, but I have no idea what is acceptable.

We inject new panics all the time. We promise not to break builds, and mostly by preserving type signatures, and that's very different.

It's true we don't necessarily want to panic frivolously, but it's hard to argue this is frivolous.

@workingjubilee
Copy link
Member

Just to say, I did send a message about this issue to security@rust-lang, before posting it, just in case it was deemed as a security issue, but after discussion I was told it wasn't considered a security issue.

Regarding this, "A security issue in the sense that the rust-lang security response team has to handle it" is somewhat different than "a security issue for consideration by Rust programmers". One is "the stdlib has a CVE in it", basically, the other is "if you mishandle this, you have a CVE in your program". Some of this is a semi-arbitrary decision as to who gets to handle the blame, but when it's not possible for a programmer to avoid, or it involves memory soundness in purely safe code, that falls squarely on us.

@workingjubilee workingjubilee added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 14, 2024
@Amanieu
Copy link
Member

Amanieu commented May 15, 2024

We discussed this in the library team meeting today. We decided to apply the change in #125070 which makes set_extension panic if the extension contains invalid characters. This PR should be changed to only apply to set_filename.

@Amanieu Amanieu removed I-libs-nominated Nominated for discussion during a libs team meeting. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 15, 2024
@tbu-
Copy link
Contributor

tbu- commented May 15, 2024

Perhaps we (I?) could do a quick survey to see whether this is a used feature.

@Amanieu
Copy link
Member

Amanieu commented May 15, 2024

One concern we had was that making this function panic would allow arbitrary user input to cause a program to panic. This can be a problem e.g. in a network service, especially if there is code to validate the path after it has been formed with set_filename.

@tbu-
Copy link
Contributor

tbu- commented May 15, 2024

One concern we had was that making this function panic would allow arbitrary user input to cause a program to panic. This can be a problem e.g. in a network service,

I think a network service will likely contain panics to the connection level anyway, otherwise it'll likely be vulnerable to DoS.

@workingjubilee
Copy link
Member

workingjubilee commented May 15, 2024

Based on the comments when initiating the FCP on the other function, here: #125070 (comment)

Though if we were confident that were not the case we'd love to do that too.

And the fact that, well, I think @tbu- has a good point, here, basically...

If @rust-lang/libs-api is willing, then I believe we should document the non-sanitizing behavior with an explicit note that set_file_name may panic in the future if you use it with something that is not, well, a file name.

Other ideas that aren't necessarily critical to address in this PR: We should also suggest a pattern that clearly denotes the "yes, I know I am joining arbitrary paths together, possibly with 'hilarious' results". We might suggest, for instance, path_buf.extend(path) or path.join(other_path) for "yes, I am clearly adding a multi-part path" cases.

I believe this is the sort of commentary in the documentation that will eventually provoke feedback and inform whether we should zig... er, not the programming language... or zag.

@ChrisJefferson What do you think?

@BurntSushi
Copy link
Member

@workingjubilee I think that's a good idea. It might also be worth adding fallible variants of these routines. I imagine in some cases, the inputs to these routines will come directly from end users. And in those cases, using a fallible routine is probably what you want.

@workingjubilee
Copy link
Member

workingjubilee commented May 16, 2024

Neat.

Also @ChrisJefferson I realize that feedback is slightly open-ended so all it really needs is the note about "set_file_name may panic in the future", if you wanna punt on the rest of it feel free to open an issue and I'll look at getting that sorted Eventually™.

fmease added a commit to fmease/rust that referenced this pull request May 26, 2024
…ratt

Panic if `PathBuf::set_extension` would add a path separator

This is likely never intended and potentially a security vulnerability if it happens.

I'd guess that it's mostly literal strings that are passed to this function in practice, so I'm guessing this doesn't break anyone.

CC rust-lang#125060
bors added a commit to rust-lang-ci/rust that referenced this pull request May 26, 2024
Panic if `PathBuf::set_extension` would add a path separator

This is likely never intended and potentially a security vulnerability if it happens.

I'd guess that it's mostly literal strings that are passed to this function in practice, so I'm guessing this doesn't break anyone.

CC rust-lang#125060
bors added a commit to rust-lang-ci/rust that referenced this pull request May 26, 2024
Panic if `PathBuf::set_extension` would add a path separator

This is likely never intended and potentially a security vulnerability if it happens.

I'd guess that it's mostly literal strings that are passed to this function in practice, so I'm guessing this doesn't break anyone.

CC rust-lang#125060
library/std/src/path.rs Outdated Show resolved Hide resolved
library/std/src/path.rs Outdated Show resolved Hide resolved
library/std/src/path.rs Outdated Show resolved Hide resolved
@ChrisJefferson
Copy link
Contributor Author

I think I've made the required tweaks, but if it's not right, I'm happy for someone to just chuck this / write their own, which I realise might be quicker than tweaking my PR via me!

@rust-log-analyzer

This comment has been minimized.

ChrisJefferson and others added 2 commits September 11, 2024 22:33
Mention that some methods do not sanitize their input fully
@workingjubilee
Copy link
Member

Just needed a rebase and a typo fix.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 12, 2024

📌 Commit 45c471b has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#125060 (Expand documentation of PathBuf, discussing lack of sanitization)
 - rust-lang#129367 (Fix default/minimum deployment target for Aarch64 simulator targets)
 - rust-lang#130156 (Add test for S_OBJNAME & update test for LF_BUILDINFO cl and cmd)
 - rust-lang#130160 (Fix `slice::first_mut` docs)
 - rust-lang#130235 (Simplify some nested `if` statements)
 - rust-lang#130250 (Fix `clippy::useless_conversion`)
 - rust-lang#130252 (Properly report error on `const gen fn`)
 - rust-lang#130256 (Re-run coverage tests if `coverage-dump` was modified)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8e037cc into rust-lang:master Sep 12, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 12, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024
Rollup merge of rust-lang#125060 - ChrisJefferson:pathbuf-doc, r=workingjubilee

Expand documentation of PathBuf, discussing lack of sanitization

Various methods in `PathBuf`, in particular `set_file_name` and `set_extension` accept strings which include path seperators (like `../../etc`). These methods just glue together strings, so you can end up with strange strings.

This isn't reasonable to change/fix at this point, and might not even be fixable, but I think should be documented. In particular, you probably shouldn't blindly build paths using strings given by possibly malicious users.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants