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 79 matching lines @@
               &allAppContainersSid);
       std::tr1::shared_ptr<SID> sharedAllAppContainersSid(static_cast<SID*>(allAppContainersSid), FreeSid); // Just to simplify cleanup
 
       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);
 
+      // Will be released later
       PACL acl = 0;
-      // acl has to be released after this

[Felix Dahlke, 2014/07/03 13:38:07]

I don't really understand, it has to be released after SetEntries? Well it's
not.

[Oleksandr, 2014/07/03 13:51:49]

1. This allocates a new ACL, so it has to be released. 
2. That new ACL should not be released before we actually use it in
CreateNamedPipe. 
3. We release it after CreateNamedPipe by ReleaseDacl. 


On 2014/07/03 13:38:07, Felix H. Dahlke wrote:

[Felix Dahlke, 2014/07/03 13:56:33]

The comment is on top of "SetEntries", so I presumed it's commenting on that
line, confused me a bit. How about a comment above the |PACL acl = 0;| line
like: "Will be released later"?

       if (SetEntriesInAcl(2, explicitAccess, 0, &acl) != ERROR_SUCCESS)
         return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
 
-      // This only references the acl, not copies it

[Felix Dahlke, 2014/07/03 13:38:07]

I think this is not necessary, the official docs say: "The DACL is referenced
by, not copied into, the security descriptor."

[Oleksandr, 2014/07/03 13:51:49]

I don't really understand your point here. Could you rephrase, please?

On 2014/07/03 13:38:07, Felix H. Dahlke wrote:

[Felix Dahlke, 2014/07/03 13:56:33]

My point is that the Microsoft documentation says exactly this, if it's clearly
documented I don't see why we need a comment. You know I'm wary of comments :)
They tend to get out of sync with the code, so we generally try to minimise
comments.

[Oleksandr, 2014/07/15 14:16:55]

This comment is quite important, IMHO. It's pretty non intuitive how ACL is used
here, and one can make the same mistake again. Intuitively you would expect
SetSecurityDescriptorDacl makes a copy of ACL for itself. 

On 2014/07/03 13:56:33, Felix H. Dahlke wrote:

[Felix Dahlke, 2014/07/16 14:40:03]

Alright.

+      // NOTE: This only references the acl, not copies it. 
+      // DO NOT release the ACL before it's actually used
       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
+    // Create a dummy security descriptor with low integrirty preset and reference its SACL in 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);
-    if (saclPresent)
-    {
-      if (!SetSecurityDescriptorSacl(securityDescriptor.get(), TRUE, sacl, FALSE))
-      {
-        DWORD err = GetLastError();
+
+    DWORD sdSize(0), saclSize(0), daclSize(0), ownerSize(0), primaryGroupSize(0);
+    MakeAbsoluteSD(dummySecurityDescriptorLow, 0, &sdSize, 0, &daclSize, 0, &saclSize, 0, &ownerSize, 0, &primaryGroupSize);
+    if (saclSize == 0 || sdSize == 0)
+    {
         return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
-      }
+    }
+    // Will be released later
+    PACL sacl = static_cast<PACL>(malloc(saclSize));
+    std::auto_ptr<SECURITY_DESCRIPTOR> absoluteDummySecurityDescriptorLow(static_cast<SECURITY_DESCRIPTOR*>(malloc(sdSize)));
+    daclSize = 0;
+    ownerSize = 0;
+    primaryGroupSize = 0;
+    BOOL res = MakeAbsoluteSD(dummySecurityDescriptorLow, absoluteDummySecurityDescriptorLow.get(), &sdSize, 0, &daclSize, sacl, &saclSize, 0, &ownerSize, 0, &primaryGroupSize);
+    if (res == 0 || absoluteDummySecurityDescriptorLow.get() == 0 || saclSize == 0)
+    {
+        return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
+    }
+
+    // NOTE: This only references the acl, not copies it. 
+    // DO NOT release the ACL before it's actually used
+    if (!SetSecurityDescriptorSacl(securityDescriptor.get(), TRUE, sacl, FALSE))
+    {
+      return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
     }
 
     return securityDescriptor;
   }
 }
 
-  // Releases the DACL structure in the provided security descriptor

[Felix Dahlke, 2014/07/03 13:38:07]

I think the function name communicates this quite well :)

-  void ReleaseDacl(PSECURITY_DESCRIPTOR securityDescriptor)
-  {
-    BOOL aclPresent = FALSE;
-    BOOL aclDefaulted = FALSE;
-    PACL acl;
-    GetSecurityDescriptorDacl(securityDescriptor, &aclPresent, &acl, &aclDefaulted);
-    if (aclPresent)
-    {
-      LocalFree(acl);
-    }
-  }
 const std::wstring Communication::pipeName = L"\\\\.\\pipe\\adblockplusengine_" + GetUserName();
 
 void Communication::InputBuffer::CheckType(Communication::ValueType expectedType)
 {
   if (!hasType)
     ReadBinary(currentType);
 
   if (currentType != expectedType)
   {
     // Make sure we don't attempt to read the type again
@@ skipping 21 matching lines @@
 Communication::PipeBusyError::PipeBusyError()
   : std::runtime_error("Timeout while trying to connect to a named pipe, pipe is busy")
 {
 }
 
 Communication::PipeDisconnectedError::PipeDisconnectedError()
   : std::runtime_error("Pipe disconnected")
 {
 }
 
+void FreeAbsoluteSecurityDescriptor(SECURITY_DESCRIPTOR* securityDescriptor)
+{
+  BOOL aclPresent = FALSE;
+  BOOL aclDefaulted = FALSE;
+  PACL acl = 0;
+  GetSecurityDescriptorDacl(securityDescriptor, &aclPresent, &acl, &aclDefaulted);
+  if (aclPresent)
+  {
+    LocalFree(acl);
+  }
+  aclPresent = FALSE;
+  aclDefaulted = FALSE;
+  acl = 0;
+  GetSecurityDescriptorSacl(securityDescriptor, &aclPresent, &acl, &aclDefaulted);
+  if (aclPresent)
+  {
+    free(acl);
+  }
+  free(securityDescriptor);
+}
+
 Communication::Pipe::Pipe(const std::wstring& pipeName, Communication::Pipe::Mode mode)
 {
   pipe = INVALID_HANDLE_VALUE;
   if (mode == MODE_CREATE)
   {
     SECURITY_ATTRIBUTES securityAttributes = {};
     securityAttributes.nLength = sizeof(SECURITY_ATTRIBUTES);
     securityAttributes.bInheritHandle = TRUE;
 
     std::tr1::shared_ptr<SECURITY_DESCRIPTOR> sharedSecurityDescriptor; // Just to simplify cleanup
-  

[Felix Dahlke, 2014/07/03 13:38:07]

Whitespace change?

     AutoHandle token;
     OpenProcessToken(GetCurrentProcess(), TOKEN_READ, token);
     
     if (IsWindowsVistaOrLater())
     {
       std::auto_ptr<SID> logonSid = GetLogonSid(token);
       // Create a SECURITY_DESCRIPTOR that has both Low Integrity and allows access to all AppContainers
       // This is needed since IE likes to jump out of Enhanced Protected Mode for specific pages (bing.com)
-
-      // ATTENTION: DACL in the returned securityDescriptor has to be manually released by ReleaseDacl
       std::auto_ptr<SECURITY_DESCRIPTOR> securityDescriptor = CreateSecurityDescriptor(logonSid.get());
 
       securityAttributes.lpSecurityDescriptor = securityDescriptor.release();
-      sharedSecurityDescriptor.reset(static_cast<SECURITY_DESCRIPTOR*>(securityAttributes.lpSecurityDescriptor));
+      sharedSecurityDescriptor.reset(static_cast<SECURITY_DESCRIPTOR*>(securityAttributes.lpSecurityDescriptor), FreeAbsoluteSecurityDescriptor);
     }
     pipe = CreateNamedPipeW(pipeName.c_str(),  PIPE_ACCESS_DUPLEX, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
       PIPE_UNLIMITED_INSTANCES, bufferSize, bufferSize, 0, &securityAttributes);
-
-    if (IsWindowsVistaOrLater())
-    {
-      ReleaseDacl(securityAttributes.lpSecurityDescriptor);

[Felix Dahlke, 2014/07/03 13:38:07]

Wouldn't it'd be cleaner to put this in a custom free function for
sharedSecurityDescriptor? It's scope ends right about here anyway. And that way
we'd be exception safe. And we wouldn't need the "isWindowsVistaOrLater" check
either. Note that we'd still need to manually free the security descriptor in
that function.

-    }
   }
   else
   {
     pipe = CreateFileW(pipeName.c_str(), GENERIC_READ | GENERIC_WRITE, 0, 0, OPEN_EXISTING, 0, 0);
     if (pipe == INVALID_HANDLE_VALUE && GetLastError() == ERROR_PIPE_BUSY)
     {
       if (!WaitNamedPipeW(pipeName.c_str(), 10000))
         throw PipeBusyError();
 
       pipe = CreateFileW(pipeName.c_str(), GENERIC_READ | GENERIC_WRITE, 0, 0, OPEN_EXISTING, 0, 0);
@@ skipping 49 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");
 }