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 90 matching lines @@
       PACL acl = 0;
       if (SetEntriesInAcl(2, explicitAccess, 0, &acl) != ERROR_SUCCESS)
         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 (!SetSecurityDescriptorDacl(securityDescriptor.get(), TRUE, acl, FALSE))
         return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
     }
 
-    // Create a dummy security descriptor with low integrirty preset and refference its SACL in ours

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

s/refference/reference/

[Oleksandr, 2014/07/17 17:32:48]

Um. Refference is meant to be a verb here. 
On 2014/07/16 14:40:03, Felix H. Dahlke wrote:

[Felix Dahlke, 2014/07/18 12:40:57]

But it's still one f too many! :)

[Oleksandr, 2014/07/18 12:48:34]

Oh. Totally missread your correction. 
On 2014/07/18 12:40:57, Felix H. Dahlke wrote:

+    // 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
 
-    DWORD sdSize, saclSize, daclSize, ownerSize, primaryGroupSize;

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

Shouldn't we initialise all the sizes to 0 here? Should be garbage currently.

+    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))

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

Redundant parentheses around both comparisons.

-    {
-        DWORD err = GetLastError();

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

Unused variable.

+    if (saclSize == 0 || sdSize == 0)
+    {
         return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
     }
     // Will be released later
-    PACL sacl = (PACL) new byte[saclSize];

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

I'd actually find malloc more intuitive here. byte is not a standard type. Also,
C++-style cast please :)

Same below.

-    std::auto_ptr<SECURITY_DESCRIPTOR> absoluteDummySecurityDescriptorLow((SECURITY_DESCRIPTOR*) new byte[sdSize]);
+    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))

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

Redundant parentheses here as well.

Also, !res would be more intuitive here IMO.

-    {
-        DWORD err = GetLastError();

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

Also unused.

+    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))
     {
-      DWORD err = GetLastError();
       return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
     }
 
     return securityDescriptor;
   }
 }
 
 const std::wstring Communication::pipeName = L"\\\\.\\pipe\\adblockplusengine_" + GetUserName();
 
 void Communication::InputBuffer::CheckType(Communication::ValueType expectedType)
@@ skipping 39 matching lines @@
 {
   BOOL aclPresent = FALSE;
   BOOL aclDefaulted = FALSE;
   PACL acl = 0;
   GetSecurityDescriptorDacl(securityDescriptor, &aclPresent, &acl, &aclDefaulted);
   if (aclPresent)
   {
     LocalFree(acl);
   }
   aclPresent = FALSE;
-  aclDefaulted = false;

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

Should also be that Windows FALSE macro for consistency.

+  aclDefaulted = FALSE;
   acl = 0;
   GetSecurityDescriptorSacl(securityDescriptor, &aclPresent, &acl, &aclDefaulted);
   if (aclPresent)
   {
-    delete acl;

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

You need to use delete[], since it was allocated using new[]. Same below.

-  }
-  delete securityDescriptor;
+    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;
@@ skipping 76 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");
 }