Fix named pipe security on Windows 7

src/shared/Communication.cpp

Index: src/shared/Communication.cpp
===================================================================
--- a/src/shared/Communication.cpp
+++ b/src/shared/Communication.cpp
@@ -73,8 +73,6 @@
       explicitAccess[0].Trustee.TrusteeType = TRUSTEE_IS_USER;
       explicitAccess[0].Trustee.ptstrName  = static_cast<LPWSTR>(logonSid);
 
-      std::tr1::shared_ptr<SID> sharedAllAppContainersSid;
-
       // 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) 
@@ -90,7 +88,7 @@
               SECURITY_BUILTIN_PACKAGE_ANY_PACKAGE,
               0, 0, 0, 0, 0, 0,
               &allAppContainersSid);
-      sharedAllAppContainersSid.reset(static_cast<SID*>(allAppContainersSid), FreeSid); // Just to simplify cleanup
+      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;
@@ -100,13 +98,13 @@
       explicitAccess[1].Trustee.ptstrName = static_cast<LPWSTR>(allAppContainersSid);
 
       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);
-      std::tr1::shared_ptr<ACL> sharedAcl(static_cast<ACL*>(acl), LocalFree); // Just to simplify cleanup
 
+      // 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.

       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
@@ -131,6 +129,18 @@
   }
 }
 
+  // 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)
@@ -182,7 +192,7 @@
     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);
     
@@ -191,12 +201,20 @@
       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));
     }
     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
   {
@@ -215,12 +233,12 @@
 
   DWORD pipeMode = PIPE_READMODE_MESSAGE | PIPE_WAIT;
   if (!SetNamedPipeHandleState(pipe, &pipeMode, 0, 0))
-    throw std::runtime_error("SetNamedPipeHandleState failed: error " + GetLastError());
+    throw std::runtime_error(AppendErrorCode("SetNamedPipeHandleState failed"));
 
   if (mode == MODE_CREATE && !ConnectNamedPipe(pipe, 0))
   {
     DWORD err = GetLastError();
-    throw std::runtime_error("Client failed to connect: error " + GetLastError());
+    throw std::runtime_error(AppendErrorCode("Client failed to connect"));
   }
 }