libstd: support creation of anonymous pipe on WinXP/2K3 #37677
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Isn't this opening a security hole on these platforms, as the flag is presumably there for literally preventing remote connections to the pipes created? The correct behavior seems doable on those legacy systems though, albeit only with some ACL tricks, and I don't know if we really should go this far to support Tier-3 platforms like that. |
|
Also, the
so IMO this may require some degree of rethink. At least the version check could be unnecessary if we simply go ahead creating, checking |
|
@xen0n you're right, I missassumed the possible performance implication of I can even try to hack something in the spirit of the linked SO answer later today, but I'm by no means a winapi expert @alexcrichton sorry, didn't notice - rustfmt stopped working for me a while back and I haven't attempted to resurrect it yet, will fix in the next commit |
|
@xen0n's solution also sounds fine by me (try the current strategy and fall back if that fails). I'd be fine punting on ACL business and whatnot, XP is only Tier 3 after all |
|
@alexcrichton Sure, XP is Tier 3 and ACL is a lot of unneeded complexity, but IMO the security implication should at least be documented. People are bound to leverage the compatibility if it's there, and (incorrectly) expect the security level to be the same as the Tier-1 Windows versions. Although XP users' problems are much deeper than some under-protected pipes could bring about, I certainly don't think Rust should take the blame in case the introduced inconsistent behavior actually leads to exploits. |
|
Windows XP hasn't received security updates for over 2 years. |
|
@sfackler Yeah, sure. XP's overall security won't get any better even if we implemented the correct behavior, and since it's unsupported not documenting the inconsistency is perfectly acceptable as well. I'd then suggest putting a comment about the decision and forget about it. 😂 |
alexcrichton
added
the
T-libs-api
| } | ||
|
|
||
| let sid_world_auth = c::SECURITY_WORLD_SID_AUTHORITY; | ||
| let mut world_sid: *mut c_void = ptr::null_mut(); |
There was a problem hiding this comment.
TODO: when to free the memory pointed to by world_sid, network_sid and psd?
|
Alright, I made a rough draft of how it might look like... !!!Currently the code leaks memory!!! so it's not ready for prime time, but I'd like to ask for help from someone more experienced with this winapi stuff. I believe the code is not very idiomatic rust, so all comments are very welcome. |
|
@jsen- I think @sfackler's got a good point that guaranteeing the same semantics here is probably the last of one's "security concerns" on XP. In that sense what do you think of the strategy of attempting to pass the flag, and then trying to not pass the flag if that fails with a particular error? (for XP) |
|
@alexcrichton fully agree, I think this PR got a little overkill for what the issue deserves. Let me shake of some of it's weight |
|
Here's the original simple version minus windows version check. Relying on |
|
Thanks! Looks good to me, but I think we can remove the note for XP users. We typically don't have this sort of platform-specific notes in the docs just yet, and this is a bit prominent for something that's relevant to very few people. |
|
Putting the note (preferably along with justification of security reduction) in code comment should be enough IMO. |
|
Removed the note from documentation and added code comment with implications and justification. |
|
@jsen- Don't worry and just use |
|
Ah yeah code looks good to me, and with a squash I'd r+. Let me know if you need help squashing though |
|
okay I think it's good now, thanks for patience :) |
|
@bors: r+ |
|
📌 Commit fc5a361 has been approved by |
libstd: support creation of anonymous pipe on WinXP/2K3 `PIPE_REJECT_REMOTE_CLIENTS` flag is not supported on Windows < VISTA, and every invocation of `anon_pipe` including attempts to pipe `std::process::Child`'s stdio fails. This PR should work around this issue by performing a runtime check of windows version and conditionally omitting this flag on "XP and friends". Getting the version should be probably moved out of the function `anon_pipe` itself (the OS version does not often change during runtime :) ), but: - I didn't find any precedent for this and assuming there's not much overhead (I hope windows does not perform any heuristics to find out it's own version, just fills couple of fields in the struct). - the code path is not especially performance sensitive anyway.
PIPE_REJECT_REMOTE_CLIENTSflag is not supported on Windows < VISTA, and every invocation ofanon_pipeincluding attempts to pipestd::process::Child's stdio fails.This PR should work around this issue by performing a runtime check of windows version and conditionally omitting this flag on "XP and friends".
Getting the version should be probably moved out of the function
anon_pipeitself (the OS version does not often change during runtime :) ), but: