From 36bf0a491e900f09b894b54dbe037262acec73e5 Mon Sep 17 00:00:00 2001 From: Nathan Breakwell Date: Tue, 3 Mar 2020 12:20:13 +0100 Subject: [PATCH] Make named pipe security configurable by security descriptor string Client: cpp Patch: Nathan Breakwell This closes #2083 --- lib/cpp/src/thrift/transport/TPipeServer.cpp | 120 +++++++++---------- lib/cpp/src/thrift/transport/TPipeServer.h | 11 ++ 2 files changed, 70 insertions(+), 61 deletions(-) diff --git a/lib/cpp/src/thrift/transport/TPipeServer.cpp b/lib/cpp/src/thrift/transport/TPipeServer.cpp index 0855e2cba..e1cee80ef 100644 --- a/lib/cpp/src/thrift/transport/TPipeServer.cpp +++ b/lib/cpp/src/thrift/transport/TPipeServer.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #endif //_WIN32 namespace apache { @@ -95,9 +96,15 @@ private: class TNamedPipeServer : public TPipeServerImpl { public: - TNamedPipeServer(const std::string& pipename, uint32_t bufsize, uint32_t maxconnections) - : stopping_(false), pipename_(pipename), bufsize_(bufsize), maxconns_(maxconnections) - { + TNamedPipeServer(const std::string& pipename, + uint32_t bufsize, + uint32_t maxconnections, + const std::string& securityDescriptor) + : stopping_(false), + pipename_(pipename), + bufsize_(bufsize), + maxconns_(maxconnections), + securityDescriptor_(securityDescriptor) { connectOverlap_.action = TOverlappedWorkItem::CONNECT; cancelOverlap_.action = TOverlappedWorkItem::CANCELIO; TAutoCrit lock(pipe_protect_); @@ -134,6 +141,7 @@ private: bool stopping_; std::string pipename_; + std::string securityDescriptor_; uint32_t bufsize_; uint32_t maxconns_; TManualResetEvent listen_event_; @@ -155,17 +163,30 @@ TPipeServer::TPipeServer(const std::string& pipename, uint32_t bufsize) : bufsize_(bufsize), isAnonymous_(false) { setMaxConnections(TPIPE_SERVER_MAX_CONNS_DEFAULT); setPipename(pipename); + setSecurityDescriptor(DEFAULT_PIPE_SECURITY); } TPipeServer::TPipeServer(const std::string& pipename, uint32_t bufsize, uint32_t maxconnections) : bufsize_(bufsize), isAnonymous_(false) { setMaxConnections(maxconnections); setPipename(pipename); + setSecurityDescriptor(DEFAULT_PIPE_SECURITY); +} + +TPipeServer::TPipeServer(const std::string& pipename, + uint32_t bufsize, + uint32_t maxconnections, + const std::string& securityDescriptor) + : bufsize_(bufsize), isAnonymous_(false) { + setMaxConnections(maxconnections); + setPipename(pipename); + setSecurityDescriptor(securityDescriptor); } TPipeServer::TPipeServer(const std::string& pipename) : bufsize_(1024), isAnonymous_(false) { setMaxConnections(TPIPE_SERVER_MAX_CONNS_DEFAULT); setPipename(pipename); + setSecurityDescriptor(DEFAULT_PIPE_SECURITY); } TPipeServer::TPipeServer(int bufsize) : bufsize_(bufsize), isAnonymous_(true) { @@ -191,7 +212,7 @@ bool TPipeServer::isOpen() const { void TPipeServer::listen() { if (isAnonymous_) return; - impl_.reset(new TNamedPipeServer(pipename_, bufsize_, maxconns_)); + impl_.reset(new TNamedPipeServer(pipename_, bufsize_, maxconns_, securityDescriptor_)); } shared_ptr TPipeServer::acceptImpl() { @@ -327,73 +348,47 @@ void TPipeServer::close() { impl_.reset(); } -bool TNamedPipeServer::createNamedPipe(const TAutoCrit & /*lockProof*/) { +bool TNamedPipeServer::createNamedPipe(const TAutoCrit& /*lockProof*/) { - // Windows - set security to allow non-elevated apps - // to access pipes created by elevated apps. - SID_IDENTIFIER_AUTHORITY SIDAuthWorld = SECURITY_WORLD_SID_AUTHORITY; - PSID everyone_sid = NULL; - AllocateAndInitializeSid( - &SIDAuthWorld, 1, SECURITY_WORLD_RID, 0, 0, 0, 0, 0, 0, 0, &everyone_sid); + PSECURITY_DESCRIPTOR psd = NULL; + ULONG size = 0; - EXPLICIT_ACCESS ea; - ZeroMemory(&ea, sizeof(EXPLICIT_ACCESS)); - ea.grfAccessPermissions = SPECIFIC_RIGHTS_ALL | STANDARD_RIGHTS_ALL; - ea.grfAccessMode = SET_ACCESS; - ea.grfInheritance = NO_INHERITANCE; - ea.Trustee.TrusteeForm = TRUSTEE_IS_SID; - ea.Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; - ea.Trustee.ptstrName = static_cast(everyone_sid); - - PACL acl = NULL; - SetEntriesInAcl(1, &ea, NULL, &acl); - - PSECURITY_DESCRIPTOR sd = (PSECURITY_DESCRIPTOR)LocalAlloc(LPTR, SECURITY_DESCRIPTOR_MIN_LENGTH); - if (!InitializeSecurityDescriptor(sd, SECURITY_DESCRIPTOR_REVISION)) { - auto lastError = GetLastError(); - LocalFree(sd); - LocalFree(acl); - GlobalOutput.perror("TPipeServer::InitializeSecurityDescriptor() GLE=", lastError); - throw TTransportException(TTransportException::NOT_OPEN, "InitializeSecurityDescriptor() failed", - lastError); - } - if (!SetSecurityDescriptorDacl(sd, TRUE, acl, FALSE)) { - auto lastError = GetLastError(); - LocalFree(sd); - LocalFree(acl); - GlobalOutput.perror("TPipeServer::SetSecurityDescriptorDacl() GLE=", lastError); - throw TTransportException(TTransportException::NOT_OPEN, - "SetSecurityDescriptorDacl() failed", lastError); + if (!ConvertStringSecurityDescriptorToSecurityDescriptorA(securityDescriptor_.c_str(), + SDDL_REVISION_1, &psd, &size)) { + DWORD lastError = GetLastError(); + GlobalOutput.perror("TPipeServer::ConvertStringSecurityDescriptorToSecurityDescriptorA() GLE=", + lastError); + throw TTransportException( + TTransportException::NOT_OPEN, + "TPipeServer::ConvertStringSecurityDescriptorToSecurityDescriptorA() failed", lastError); } SECURITY_ATTRIBUTES sa; sa.nLength = sizeof(SECURITY_ATTRIBUTES); - sa.lpSecurityDescriptor = sd; + sa.lpSecurityDescriptor = psd; sa.bInheritHandle = FALSE; // Create an instance of the named pipe - TAutoHandle hPipe(CreateNamedPipeA(pipename_.c_str(), // pipe name - PIPE_ACCESS_DUPLEX | // read/write access - FILE_FLAG_OVERLAPPED, // async mode - PIPE_TYPE_BYTE | // byte type pipe - PIPE_READMODE_BYTE, // byte read mode - maxconns_, // max. instances - bufsize_, // output buffer size - bufsize_, // input buffer size - 0, // client time-out - &sa)); // security attributes + TAutoHandle hPipe(CreateNamedPipeA(pipename_.c_str(), // pipe name + PIPE_ACCESS_DUPLEX | // read/write access + FILE_FLAG_OVERLAPPED, // async mode + PIPE_TYPE_BYTE | // byte type pipe + PIPE_READMODE_BYTE, // byte read mode + maxconns_, // max. instances + bufsize_, // output buffer size + bufsize_, // input buffer size + 0, // client time-out + &sa)); // security attributes auto lastError = GetLastError(); - LocalFree(sd); - LocalFree(acl); - FreeSid(everyone_sid); + if (psd) + LocalFree(psd); if (hPipe.h == INVALID_HANDLE_VALUE) { Pipe_.reset(); GlobalOutput.perror("TPipeServer::TCreateNamedPipe() GLE=", lastError); - throw TTransportException(TTransportException::NOT_OPEN, - "TCreateNamedPipe() failed", - lastError); + throw TTransportException(TTransportException::NOT_OPEN, "TCreateNamedPipe() failed", + lastError); } Pipe_.reset(hPipe.release()); @@ -404,13 +399,12 @@ bool TAnonPipeServer::createAnonPipe() { SECURITY_ATTRIBUTES sa; SECURITY_DESCRIPTOR sd; // security information for pipes - if (!InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) - { - GlobalOutput.perror("TPipeServer InitializeSecurityDescriptor (anon) failed, GLE=", GetLastError()); + if (!InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) { + GlobalOutput.perror("TPipeServer InitializeSecurityDescriptor (anon) failed, GLE=", + GetLastError()); return false; } - if (!SetSecurityDescriptorDacl(&sd, true, NULL, false)) - { + if (!SetSecurityDescriptorDacl(&sd, true, NULL, false)) { GlobalOutput.perror("TPipeServer SetSecurityDescriptorDacl (anon) failed, GLE=", GetLastError()); return false; @@ -482,6 +476,10 @@ void TPipeServer::setAnonymous(bool anon) { isAnonymous_ = anon; } +void TPipeServer::setSecurityDescriptor(const std::string& securityDescriptor) { + securityDescriptor_ = securityDescriptor; +} + void TPipeServer::setMaxConnections(uint32_t maxconnections) { if (maxconnections == 0) maxconns_ = 1; diff --git a/lib/cpp/src/thrift/transport/TPipeServer.h b/lib/cpp/src/thrift/transport/TPipeServer.h index d4255cb99..67c5d51a7 100644 --- a/lib/cpp/src/thrift/transport/TPipeServer.h +++ b/lib/cpp/src/thrift/transport/TPipeServer.h @@ -31,6 +31,11 @@ #define TPIPE_SERVER_MAX_CONNS_DEFAULT PIPE_UNLIMITED_INSTANCES +// Windows - set security to allow non-elevated apps +// to access pipes created by elevated apps. +// Full access to everyone +const std::string DEFAULT_PIPE_SECURITY{"D:(A;;FA;;;WD)"}; + namespace apache { namespace thrift { namespace transport { @@ -51,6 +56,10 @@ public: // Named Pipe - TPipeServer(const std::string& pipename, uint32_t bufsize); TPipeServer(const std::string& pipename, uint32_t bufsize, uint32_t maxconnections); + TPipeServer(const std::string& pipename, + uint32_t bufsize, + uint32_t maxconnections, + const std::string& securityDescriptor); TPipeServer(const std::string& pipename); // Anonymous pipe - TPipeServer(int bufsize); @@ -78,6 +87,7 @@ public: bool getAnonymous(); void setAnonymous(bool anon); void setMaxConnections(uint32_t maxconnections); + void setSecurityDescriptor(const std::string& securityDescriptor); // this function is intended to be used in generic / template situations, // so its name needs to be the same as TPipe's @@ -90,6 +100,7 @@ private: std::shared_ptr impl_; std::string pipename_; + std::string securityDescriptor_; uint32_t bufsize_; uint32_t maxconns_; bool isAnonymous_;