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

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

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@pitdicker
Copy link
Contributor

pitdicker commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 6, 2019

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

@pitdicker

This comment has been minimized.

Copy link
Contributor Author

pitdicker commented Feb 6, 2019

@DemiMarie

This comment has been minimized.

Copy link
Contributor

DemiMarie commented Feb 6, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

pitdicker commented Feb 6, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

pitdicker commented Feb 7, 2019

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

This comment has been minimized.

pitdicker added some commits Feb 6, 2019

Fix SECURITY_SQOS_PRESENT missing
if security_qos_flags(SECURITY_ANONYMOUS) is set

@pitdicker pitdicker force-pushed the pitdicker:sqos_flags branch from 685efd0 to 9295f49 Feb 15, 2019

@pitdicker

This comment has been minimized.

Copy link
Contributor Author

pitdicker commented Feb 15, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment