Fix named pipe security on Windows 7

src/shared/Communication.cpp

 #include <Windows.h>
 #include <Lmcons.h>
 #include <Sddl.h>
 #include <aclapi.h>
 #include <strsafe.h>
 
 #include "AutoHandle.h"
 #include "Communication.h"
 #include "Utils.h"
 
@@ skipping 37 matching lines @@
     DWORD sidLength = GetLengthSid(tokenGroups->Groups[0].Sid);
     std::auto_ptr<SID> sid(new SID[sidLength]);
     if (!CopySid(sidLength, sid.get(), tokenGroups->Groups[0].Sid)) 
       throw std::runtime_error("CopySid failed");
     return sid;
   }
 
   // Creates a security descriptor: 
   // Allows ALL access to Logon SID and to all app containers in DACL.
   // Sets Low Integrity in SACL.
   std::auto_ptr<SECURITY_DESCRIPTOR> CreateSecurityDescriptor(PSID logonSid)

[Eric, 2014/06/25 16:13:00]

We can use unique_ptr here. VS2012 supports it.

It's admittedly a more extensive modification, since we'd need to change the
callers also, but as long as we're touching code with auto_ptr, we can take the
opportunity to eliminate an occurrence of this de-facto-deprecated class.

[Felix Dahlke, 2014/06/25 16:17:13]

We really shouldn't mix switching from TR1 shared pointers to C++11 shared
pointers in here, it'll make this change impossible to understand.

   {
-    EXPLICIT_ACCESSW explicitAccess[2] = {};
-
-    explicitAccess[0].grfAccessPermissions = STANDARD_RIGHTS_ALL | SPECIFIC_RIGHTS_ALL;
-    explicitAccess[0].grfAccessMode = SET_ACCESS;
-    explicitAccess[0].grfInheritance= NO_INHERITANCE;
-    explicitAccess[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
-    explicitAccess[0].Trustee.TrusteeType = TRUSTEE_IS_USER;
-    explicitAccess[0].Trustee.ptstrName  = static_cast<LPWSTR>(logonSid);
-
-    std::tr1::shared_ptr<SID> sharedAllAppContainersSid;
+    std::auto_ptr<SECURITY_DESCRIPTOR> securityDescriptor((SECURITY_DESCRIPTOR*)new char[SECURITY_DESCRIPTOR_MIN_LENGTH]);
+    if (!InitializeSecurityDescriptor(securityDescriptor.get(), SECURITY_DESCRIPTOR_REVISION)) 
+      return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
     // TODO: Would be better to detect if AppContainers are supported instead of checking the Windows version
-    bool isAppContainersSupported = IsAppContainersSupported();
+    bool isAppContainersSupported = IsWindows8OrLater();
     if (isAppContainersSupported)
     {
+      EXPLICIT_ACCESSW explicitAccess[2] = {};
+
+      explicitAccess[0].grfAccessPermissions = STANDARD_RIGHTS_ALL | SPECIFIC_RIGHTS_ALL;
+      explicitAccess[0].grfAccessMode = SET_ACCESS;
+      explicitAccess[0].grfInheritance= NO_INHERITANCE;
+      explicitAccess[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
+      explicitAccess[0].Trustee.TrusteeType = TRUSTEE_IS_USER;
+      explicitAccess[0].Trustee.ptstrName  = static_cast<LPWSTR>(logonSid);
+
+      std::tr1::shared_ptr<SID> sharedAllAppContainersSid;

[Eric, 2014/06/25 16:13:00]

This declaration doesn't need to be separate, but can be combined with its
single use below.

+
       // Create a well-known SID for the all appcontainers group.
       // We need to allow access to all AppContainers, since, apparently,
       // giving access to specific AppContainer (for example AppContainer of IE) 
       // tricks Windows into thinking that token is IN AppContainer. 
       // Which blocks all the calls from outside, making it impossible to communicate
       // with the engine when IE is launched with different security settings.
       PSID allAppContainersSid = 0;
       SID_IDENTIFIER_AUTHORITY applicationAuthority = SECURITY_APP_PACKAGE_AUTHORITY;
 
       AllocateAndInitializeSid(&applicationAuthority, 
               SECURITY_BUILTIN_APP_PACKAGE_RID_COUNT,
               SECURITY_APP_PACKAGE_BASE_RID,
               SECURITY_BUILTIN_PACKAGE_ANY_PACKAGE,
               0, 0, 0, 0, 0, 0,
               &allAppContainersSid);
       sharedAllAppContainersSid.reset(static_cast<SID*>(allAppContainersSid), FreeSid); // Just to simplify cleanup

[Eric, 2014/06/25 16:13:00]

We can use a constructor here instead of reset(). We can also use unique_ptr,
which can specify a deleter as a constructor argument.

 
       explicitAccess[1].grfAccessPermissions = STANDARD_RIGHTS_ALL | SPECIFIC_RIGHTS_ALL;
       explicitAccess[1].grfAccessMode = SET_ACCESS;
       explicitAccess[1].grfInheritance= NO_INHERITANCE;
       explicitAccess[1].Trustee.TrusteeForm = TRUSTEE_IS_SID;
       explicitAccess[1].Trustee.TrusteeType = TRUSTEE_IS_GROUP;
       explicitAccess[1].Trustee.ptstrName = static_cast<LPWSTR>(allAppContainersSid);
+
+      PACL acl = 0;
+      if (SetEntriesInAcl(2, explicitAccess, 0, &acl) != ERROR_SUCCESS)
+        return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
+      std::tr1::shared_ptr<ACL> sharedAcl(static_cast<ACL*>(acl), LocalFree); // Just to simplify cleanup

[Wladimir Palant, 2014/05/15 07:36:18]

If this is just to simplify cleanup, why not use unique_ptr?
https://stackoverflow.com/a/9893418/785541

+
+      if (!SetSecurityDescriptorDacl(securityDescriptor.get(), TRUE, acl, FALSE))
+        return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
+
     }
-    PACL acl = 0;
-    if (SetEntriesInAcl(isAppContainersSupported ? 2 : 1, explicitAccess, 0, &acl) != ERROR_SUCCESS)
-      return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
-    std::tr1::shared_ptr<ACL> sharedAcl(static_cast<ACL*>(acl), LocalFree); // Just to simplify cleanup
-
-    std::auto_ptr<SECURITY_DESCRIPTOR> securityDescriptor((SECURITY_DESCRIPTOR*)new char[SECURITY_DESCRIPTOR_MIN_LENGTH]);
-    if (!InitializeSecurityDescriptor(securityDescriptor.get(), SECURITY_DESCRIPTOR_REVISION)) 
-      return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
-
-    if (!SetSecurityDescriptorDacl(securityDescriptor.get(), TRUE, acl, FALSE))
-      return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
 
     // Create a dummy security descriptor with low integrirty preset and copy its SACL into ours
     LPCWSTR accessControlEntry = L"S:(ML;;NW;;;LW)";
     PSECURITY_DESCRIPTOR dummySecurityDescriptorLow;
     ConvertStringSecurityDescriptorToSecurityDescriptorW(accessControlEntry, SDDL_REVISION_1, &dummySecurityDescriptorLow, 0);
     std::tr1::shared_ptr<SECURITY_DESCRIPTOR> sharedDummySecurityDescriptor(static_cast<SECURITY_DESCRIPTOR*>(dummySecurityDescriptorLow), LocalFree); // Just to simplify cleanup
     BOOL saclPresent = FALSE;
     BOOL saclDefaulted = FALSE;
     PACL sacl;
-    GetSecurityDescriptorSacl(dummySecurityDescriptorLow, &saclPresent, &sacl, &saclDefaulted); 
+    GetSecurityDescriptorSacl(dummySecurityDescriptorLow, &saclPresent, &sacl, &saclDefaulted);
     if (saclPresent)
     {
       if (!SetSecurityDescriptorSacl(securityDescriptor.get(), TRUE, sacl, FALSE))
       {
         DWORD err = GetLastError();
         return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
       }
     }
 
     return securityDescriptor;
@@ skipping 39 matching lines @@
 Communication::PipeDisconnectedError::PipeDisconnectedError()
   : std::runtime_error("Pipe disconnected")
 {
 }
 
 Communication::Pipe::Pipe(const std::wstring& pipeName, Communication::Pipe::Mode mode)
 {
   pipe = INVALID_HANDLE_VALUE;
   if (mode == MODE_CREATE)
   {
+

[Wladimir Palant, 2014/05/15 07:36:18]

Nit: don't add an empty line here.

     SECURITY_ATTRIBUTES securityAttributes = {};
     securityAttributes.nLength = sizeof(SECURITY_ATTRIBUTES);
     securityAttributes.bInheritHandle = TRUE;
 
     std::tr1::shared_ptr<SECURITY_DESCRIPTOR> sharedSecurityDescriptor; // Just to simplify cleanup
 
     AutoHandle token;
     OpenProcessToken(GetCurrentProcess(), TOKEN_READ, token);
     std::auto_ptr<SID> logonSid = GetLogonSid(token);
     // Create a SECURITY_DESCRIPTOR that has both Low Integrity and allows access to all AppContainers
@@ skipping 19 matching lines @@
   }
 
   if (pipe == INVALID_HANDLE_VALUE)
     throw PipeConnectionError();
 
   DWORD pipeMode = PIPE_READMODE_MESSAGE | PIPE_WAIT;
   if (!SetNamedPipeHandleState(pipe, &pipeMode, 0, 0))
     throw std::runtime_error("SetNamedPipeHandleState failed: error " + GetLastError());
 
   if (mode == MODE_CREATE && !ConnectNamedPipe(pipe, 0))
+  {
+    DWORD err = GetLastError();

[Wladimir Palant, 2014/05/15 07:36:18]

Why create a variable and not use it then?

     throw std::runtime_error("Client failed to connect: error " + GetLastError());

[Wladimir Palant, 2014/05/15 07:36:18]

Does this actually work? IMHO this adds an int to char* rather than doing string
concatenation, cast to str::string first?

+  }
 }
 
 Communication::Pipe::~Pipe()
 {
   CloseHandle(pipe);
 }
 
 Communication::InputBuffer Communication::Pipe::ReadMessage()
 {
   std::stringstream stream;
@@ skipping 24 matching lines @@
   return Communication::InputBuffer(stream.str());
 }
 
 void Communication::Pipe::WriteMessage(Communication::OutputBuffer& message)
 {
   DWORD bytesWritten;
   std::string data = message.Get();
   if (!WriteFile(pipe, data.c_str(), static_cast<DWORD>(data.length()), &bytesWritten, 0))
     throw std::runtime_error("Failed to write to pipe");
 }