Make named pipe security configurable by security descriptor string #2083
Conversation
|
you should probably make a Jira, this isn't a trivial change. |
| LocalFree(acl); | ||
| FreeSid(everyone_sid); | ||
| if (psd) | ||
| LocalFree(psd); |
There was a problem hiding this comment.
can we use an RAII wrapper instead?
There was a problem hiding this comment.
I feel it's a bit overkill to add a RAII considering we only free once.
| @@ -76,6 +85,7 @@ class TPipeServer : public TServerTransport { | |||
| bool getAnonymous(); | |||
| void setAnonymous(bool anon); | |||
| void setMaxConnections(uint32_t maxconnections); | |||
| void setSecurityDescriptor(std::string securityDescriptor); | |||
|
I would suggest backing out the extra style changes to keep the diff minimal and relevant. |
| TNamedPipeServer(const std::string& pipename, | ||
| uint32_t bufsize, | ||
| uint32_t maxconnections, | ||
| const std::string& securityDescriptor) |
There was a problem hiding this comment.
to protect against this in Thrift it should allow the security of a pipe to be configured
This forces the issue, and is a breaking change. It would be better to introduce this functionality while still allowing existing code to compile, and deprecate the old constructors, and then remove them in the following release if we really think it is necessary to force a securityDescriptor in all use cases (I'm not convinced yet).
There was a problem hiding this comment.
I agree that this shouldn't be a breaking a change and shouldn't enforce the use of a security descriptor. TNamedPipeServer is internal to TPipeServer. I have given TPipeServer an additional constructor that now takes a security descriptor as a parameter. Now when TPipeServer is constructed unless a security descriptor is provided it has the security descriptor set to "Everyone" to maintain the previous behavior . As TNamedPipeServer is only in the cpp I don't see how this is a breaking change.
| return false; | ||
| } | ||
| if (!SetSecurityDescriptorDacl(&sd, true, NULL, false)) | ||
| { | ||
| if (!SetSecurityDescriptorDacl(&sd, true, NULL, false)) { |
There was a problem hiding this comment.
nit: remove brace line change.
There was a problem hiding this comment.
This is now inline with the clang format file in the repo. As it's a step in the right direction and doesn't make the PR unclear I think it's fine to leave the formatting change as is.
| if (!InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) | ||
| { | ||
| GlobalOutput.perror("TPipeServer InitializeSecurityDescriptor (anon) failed, GLE=", GetLastError()); | ||
| if (!InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) { |
There was a problem hiding this comment.
nit: remove unneeded formatting changes.
| throw TTransportException(TTransportException::NOT_OPEN, | ||
| "TCreateNamedPipe() failed", | ||
| lastError); | ||
| throw TTransportException(TTransportException::NOT_OPEN, "TCreateNamedPipe() failed", |
There was a problem hiding this comment.
nit: remove unneeded formatting changes
|
Not quite sure .... what is tghe status of this PR? Are we done or are there any pending change requests? |
I have addressed the comments for clarity. I am happy with the changes unless anyone feels particularly strongly I think it's fine as is. |
Description
Named pipes on Windows are vulnerable to privilege escalation through named pipe impersonation, to protect against this in Thrift it should allow the security of a pipe to be configured.
Implementation
Changed setting the pipe permissions to "everyone" to now be configured by security string this will default to "everyone" but can also by set in the constructor of a TPipeServer.