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

Remove hard links from env::current_exe security example #96671

Merged
merged 1 commit into from
May 8, 2022

Conversation

mgeisler
Copy link
Contributor

@mgeisler mgeisler commented May 3, 2022

The security example shows that env::current_exe will return the path used when the program was started. This is not really surprising considering how hard links work: after ln foo bar, the two files are equivalent. It is not the case that bar is a “link” to foo, nor is foo a link to bar. They are simply two names for the same underlying data.

The security vulnerability linked to seems to be different: there an attacker would start a SUID binary from a directory under the control of the attacker. The binary would respawn itself by executing the program found at /proc/self/exe (which the attacker can control). This is a real problem. In my opinion, the example given here doesn’t really show the same problem, it just shows a misunderstanding of what hard links are.

I looked through the history a bit and found that the example was introduced in #33526. That PR actually has two commits, and the first (8478d48) explains the race condition at the root of the linked security vulnerability. The second commit proceeds to replace the explanation with the example we have today.

This commit reverts most of the second commit from #33526.

The security example shows that `env::current_exe` will return the
path used when the program was started. This is not really surprising
considering how hard links work: after `ln foo bar`, the two files are
_equivalent_. It is _not_ the case that `bar` is a “link” to `foo`,
nor is `foo` a link to `bar`. They are simply two names for the same
underlying data.

The security vulnerability linked to seems to be different: there an
attacker would start a SUID binary from a directory under the control
of the attacker. The binary would respawn itself by executing the
program found at `/proc/self/exe` (which the attacker can control).
This is a real problem. In my opinion, the example given here doesn’t
really show the same problem, it just shows a misunderstanding of what
hard links are.

I looked through the history a bit and found that the example was
introduced in rust-lang#33526. That PR actually has two commits, and the
first (8478d48) explains the race
condition at the root of the linked security vulnerability. The second
commit proceeds to replace the explanation with the example we have
today.

This commit reverts most of the second commit from rust-lang#33526.
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 3, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2022
@Mark-Simulacrum
Copy link
Member

The security vulnerability linked to seems to be different: there an attacker would start a SUID binary from a directory under the control of the attacker. The binary would respawn itself by executing the program found at /proc/self/exe (which the attacker can control). This is a real problem. In my opinion, the example given here doesn’t really show the same problem, it just shows a misunderstanding of what hard links are.

I think the hard link example sort of illustrates (at least IMO) that the directory being under control isn't necessary -- you can start the binary in an arbitrary place. This means that for example assuming current_exe().with_filename(config) to discover configuration at runtime has security implications.

But I think the specificity of the example hurt more than helped -- and the new text does a slightly better job at explaining the problem.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 7, 2022

📌 Commit 9a1dc2a has been approved by Mark-Simulacrum

@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 May 7, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#96336 (Link to correct `as_mut` in docs for `pointer::as_ref`)
 - rust-lang#96586 (Add aliases for std::fs::canonicalize)
 - rust-lang#96667 (Add regression test)
 - rust-lang#96671 (Remove hard links from `env::current_exe` security example)
 - rust-lang#96726 (Add regression and bug tests)
 - rust-lang#96756 (Enable compiler-docs by default for `compiler`, `codegen`, and `tools` profiles)
 - rust-lang#96757 (Don't constantly rebuild clippy on `x test src/tools/clippy`.)
 - rust-lang#96769 (Remove `adx_target_feature` feature from active features list)
 - rust-lang#96777 (Make the test `check-pass` not to produce a JSON file)
 - rust-lang#96822 (Enforce quote rule for JS source code)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 416d600 into rust-lang:master May 8, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 8, 2022
@mgeisler
Copy link
Contributor Author

mgeisler commented May 9, 2022

The security vulnerability linked to seems to be different: there an attacker would start a SUID binary from a directory under the control of the attacker. The binary would respawn itself by executing the program found at /proc/self/exe (which the attacker can control). This is a real problem. In my opinion, the example given here doesn’t really show the same problem, it just shows a misunderstanding of what hard links are.

I think the hard link example sort of illustrates (at least IMO) that the directory being under control isn't necessary -- you can start the binary in an arbitrary place. This means that for example assuming current_exe().with_filename(config) to discover configuration at runtime has security implications.

Good point and yeah, it does demonstrate it in an indirect way.

But I think the specificity of the example hurt more than helped -- and the new text does a slightly better job at explaining the problem.

Thanks, I think that's a good way to put it.

@mgeisler mgeisler deleted the current-exe-docstring branch March 23, 2023 08:06
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.

5 participants