Add some warnings to std::env::current_exe #33526
Conversation
|
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
|
lgtm |
| /// # Security | ||
| /// | ||
| /// This function should be used with care, as its incorrect usage can cause | ||
| /// security problems. Specifically, as with many operations invovling files and |
|
Thanks @steveklabnik! Perhaps we could emphasize that calling this function itself only has security implications if you assume some property about the return value which may have security implications? I don't find the example of overwriting something too compelling because if you can overwrite the executable then you can, well, overwrite the executable (as in, The real problem here is you can escalate privileges, which I found this example to be the most compelling. You can trivially cause For example I would consider the compiler's usage of this function "not insecure" because although you could cause rustc to link against a random libstd, it's already generating arbitrary code... The problem in that linked post was that the path would be used for executing an executable as root, which is clearly going to end badly if you can run arbitrary executables. |
|
@alexcrichton if you call the function, it is for the returned pathname (you don't call it just for a side effect): so you always assume one property: you will get the pathname of the running executable. One problem is the moving definition of the returned pathname against the platform.
So basically, if you got a pathname, you couldn't assume the pathname will be the right one. @steveklabnik I think your description is good enough. Maybe just add a note for saying that the accuracy of the returned pathname is platform dependant (and so should be assumed to be wrong) ? |
|
so, no documentation warning at all ? |
|
Whoops. I was looking at my "open PRs" and saw only the one I just sent. I then cleaned up a lot of my old branches. I guess this PR was still outstanding? Will re-send later today. |
|
@steveklabnik thoughts on updating with some of the discussion above? |
|
I will get on it today. |
|
Updated, to use the more real example, and link to the vulnerability. I do think that it's stronger as an example. Still not totally happy with the wording here, but it might be Good Enough. |
|
⌛ Testing commit c4730da with merge 5dd7e83... |
|
💔 Test failed - auto-win-msvc-64-opt |
|
@bors: retry On Tue, Jul 19, 2016 at 12:36 PM, bors [email protected] wrote:
|
|
⌛ Testing commit c4730da with merge 008e473... |
|
💔 Test failed - auto-linux-64-debug-opt |
|
@bors: retry On Tue, Jul 19, 2016 at 9:02 PM, bors [email protected] wrote:
|
|
⌛ Testing commit c4730da with merge 46d67fc... |
|
⛄ The build was interrupted to prioritize another pull request. |
|
⌛ Testing commit c4730da with merge cf04494... |
|
💔 Test failed - auto-win-msvc-64-opt-rustbuild |
|
@bors: retry |
|
⌛ Testing commit c4730da with merge b334e04... |
|
⛄ The build was interrupted to prioritize another pull request. |
|
⌛ Testing commit c4730da with merge 49fe9d0... |
|
💔 Test failed - auto-win-msvc-64-cargotest |
|
@bors: retry On Tue, Jul 19, 2016 at 11:04 PM, bors [email protected] wrote:
|
Add some warnings to std::env::current_exe /cc #21889 @rust-lang/libs @semarie I started writing this up. I'm not sure if we want to go into other things and in what depth; we don't currently have a lot of security-specific documentation to model after. Thoughts?
|
thanks |
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.
…ark-Simulacrum Remove hard links from `env::current_exe` security example 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 (rust-lang@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.
…acrum Remove hard links from `env::current_exe` security example 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/rust#33526. That PR actually has two commits, and the first (rust-lang/rust@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/rust#33526.
/cc #21889 @rust-lang/libs @semarie
I started writing this up. I'm not sure if we want to go into other things and in what depth; we don't currently have a lot of security-specific documentation to model after.
Thoughts?