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.

Sign up for GitHub

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

Jump to bottom

Set secure flags when opening a named pipe on Windows #58216

Merged
merged 2 commits into from
Mar 2, 2019
Merged

Set secure flags when opening a named pipe on Windows #58216

merged 2 commits into from
Mar 2, 2019

Conversation

pitdicker
Copy link
Contributor

Fixes #42036, see also the previous attempt in #44556.

Whether this is correct depends on if it is somehow possible to create a symlink to a named pipe, outside the named pipe filesystem (NPFS). But as far as I can tell that should be impossible.

Also fixes that security_qos_flags(SECURITY_ANONYMOUS) does not set the SECURITY_SQOS_PRESENT flag, and the incorrect documentation about the default value of security_qos_flags.

@rust-highfive
Copy link
Collaborator

r? @Kimundi

(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 Feb 6, 2019
@pitdicker
Copy link
Contributor Author

cc @DemiMarie @retep998?

@DemiMarie
Copy link
Contributor

@pitdicker There are other potential paths through which a named pipe could be found. Specifically, I know that \??\;LanManRedirector\server\pipe is (mostly) an alias to \??\server\pipe and is similarly vulnerable, as are paths beginning with \??\.

I strongly believe that we should pass these flags unconditionally. They are only unwanted in rare cases, and purely path-based blacklisting is likely insufficient, due to symbolic links and other cases.

@pitdicker
Copy link
Contributor Author

I half remember CreateFileW can't open files in the NT namespace (starting with \??\). Something to double-check though.

I understand your concern. If a path-based solution is not watertight it is of not much use. But if it works, it is the nicest solution because it doesn't make flags in OpenOptions behave really different from CreateFileW.

Microsoft made it difficult here by reusing the same bits for different things. I think setting the flags unconditionally is also not great, because we would then need to offer some way to un-set them. And what happens if another duplicate flag is added in the future?

@retep998
Copy link
Member

retep998 commented Feb 6, 2019

Paths that start with \??\ or \\?\ are usually sent as is to NT with only the minimal transformation of turning \\?\ into \??\. Those paths can refer to files or pipes or a variety of other devices. And yes CreateFileW does actually work with such paths, so good luck handling those paths!

@pitdicker
Copy link
Contributor Author

Just tested it just to make sure, and you are both right about CreateFileW being able to open NT namespace paths. That is a hole I don't want to know about. Definitely a path-based solution is not going to work.

Now I am not sure what is the best way to proceed. Shall I just include the fix for SECURITY_IDENTIFICATION which is not always being set and forget about #42036?

Or fix #42036 by setting the flags unconditionally (although I don't feel completely confident about that). @DemiMarie do you want to defend that choice if I change the commit?

@samlh
Copy link

samlh commented Feb 14, 2019

Useful reference: https://googleprojectzero.blogspot.com/2016/02/the-definitive-guide-on-win32-to-nt.html?m=1

@pitdicker
Fix SECURITY_SQOS_PRESENT missing
503e74e 
if security_qos_flags(SECURITY_ANONYMOUS) is set
@pitdicker
Copy link
Contributor Author

Removed the commit that attempted to set secure defaults. I think it is best to let further discussion for that take place in the #42036.

This PR is now a basic bug and documentation fix.

@pitdicker
Copy link
Contributor Author

ping @Kimundi, or does someone else want to review?

@retep998
Copy link
Member

cc @alexcrichton

@retep998 retep998 added the O-windows Operating system: Windows label Feb 22, 2019
@alexcrichton
Copy link
Member

@bors: r+

Sorry for the delay, thanks @pitdicker!

@bors
Copy link
Contributor

bors commented Feb 22, 2019

📌 Commit 9295f49 has been approved by alexcrichton

@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 Feb 22, 2019
Centril added a commit to Centril/rust that referenced this pull request Feb 23, 2019
@Centril
Rollup merge of rust-lang#58216 - pitdicker:sqos_flags, r=alexcrichton
8c66221 
 Set secure flags when opening a named pipe on Windows

Fixes rust-lang#42036, see also the previous attempt in rust-lang#44556.

Whether this is correct depends on if it is somehow possible to create a symlink to a named pipe, outside the named pipe filesystem (NPFS). But as far as I can tell that should be impossible.

Also fixes that `security_qos_flags(SECURITY_ANONYMOUS)` does not set the `SECURITY_SQOS_PRESENT` flag, and the incorrect documentation about the default value of `security_qos_flags`.
@Centril Centril mentioned this pull request Feb 23, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Feb 23, 2019
@Centril
Rollup merge of rust-lang#58216 - pitdicker:sqos_flags, r=alexcrichton
e018763 
 Set secure flags when opening a named pipe on Windows

Fixes rust-lang#42036, see also the previous attempt in rust-lang#44556.

Whether this is correct depends on if it is somehow possible to create a symlink to a named pipe, outside the named pipe filesystem (NPFS). But as far as I can tell that should be impossible.

Also fixes that `security_qos_flags(SECURITY_ANONYMOUS)` does not set the `SECURITY_SQOS_PRESENT` flag, and the incorrect documentation about the default value of `security_qos_flags`.
@Centril Centril mentioned this pull request Feb 23, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Feb 23, 2019
@Centril
Rollup merge of rust-lang#58216 - pitdicker:sqos_flags, r=alexcrichton
249e103 
 Set secure flags when opening a named pipe on Windows

Fixes rust-lang#42036, see also the previous attempt in rust-lang#44556.

Whether this is correct depends on if it is somehow possible to create a symlink to a named pipe, outside the named pipe filesystem (NPFS). But as far as I can tell that should be impossible.

Also fixes that `security_qos_flags(SECURITY_ANONYMOUS)` does not set the `SECURITY_SQOS_PRESENT` flag, and the incorrect documentation about the default value of `security_qos_flags`.
@Centril Centril mentioned this pull request Feb 23, 2019
Closed
@Centril
Copy link
Contributor

Centril commented Feb 23, 2019

Failed in #58666 (comment), @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 23, 2019
retep998
retep998 reviewed Feb 23, 2019
View reviewed changes
src/libstd/sys/windows/ext/fs.rs Outdated
///
/// .open("foo.txt");
/// .open("\\.\pipe\MyPipe");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a raw string for this.

@pitdicker
Copy link
Contributor Author

@alexcrichton can you please r+ again?

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 1, 2019

📌 Commit 089524c has been approved by alexcrichton

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2019
@kennytm kennytm mentioned this pull request Mar 2, 2019
Merged
@bors
Copy link
Contributor

bors commented Mar 2, 2019

⌛ Testing commit 089524c with merge fab272e...

bors added a commit that referenced this pull request Mar 2, 2019
@bors
Auto merge of #58216 - pitdicker:sqos_flags, r=alexcrichton
fab272e 
 Set secure flags when opening a named pipe on Windows

Fixes #42036, see also the previous attempt in #44556.

Whether this is correct depends on if it is somehow possible to create a symlink to a named pipe, outside the named pipe filesystem (NPFS). But as far as I can tell that should be impossible.

Also fixes that `security_qos_flags(SECURITY_ANONYMOUS)` does not set the `SECURITY_SQOS_PRESENT` flag, and the incorrect documentation about the default value of `security_qos_flags`.
@bors
Copy link
Contributor

bors commented Mar 2, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing fab272e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 2, 2019
@bors bors merged commit 089524c into rust-lang:master Mar 2, 2019
@pitdicker pitdicker deleted the sqos_flags branch March 4, 2019 06:33
@udoprog udoprog mentioned this pull request May 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@retep998 retep998 retep998 left review comments

Assignees

@Kimundi Kimundi

Labels
merged-by-bors This PR was explicitly merged by bors. O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Always pass SECURITY_SQOS_PRESENT|SECURITY_IDENTIFICATION when opening a named pipe
9 participants
@pitdicker @rust-highfive @DemiMarie @retep998 @samlh @alexcrichton @bors @Centril @Kimundi