Enhanced Protected Mode support

src/shared/Communication.cpp

Index: src/shared/Communication.cpp
===================================================================
--- a/src/shared/Communication.cpp
+++ b/src/shared/Communication.cpp
@@ -1,9 +1,17 @@
 #include <Windows.h>
 #include <Lmcons.h>
 #include <Sddl.h>
+#include <aclapi.h>
 
 #include "Communication.h"
 #include "Utils.h"
+#include <strsafe.h>

[Wladimir Palant, 2013/09/16 13:45:07]

Please move that up, to the built-in include files.

+
+#ifndef SECURITY_APP_PACKAGE_AUTHORITY
+#define SECURITY_APP_PACKAGE_AUTHORITY              {0,0,0,0,0,15}
+#endif
+
+std::wstring Communication::browserSID;
 
 namespace
 {
@@ -25,6 +33,132 @@
       throw std::runtime_error(AppendErrorCode("Failed to get the current user's name"));
     return std::wstring(buffer.get(), length);
   }
+
+  // See http://msdn.microsoft.com/en-us/library/windows/desktop/hh448493(v=vs.85).aspx
+  bool GetLogonSid (HANDLE hToken, PSID *ppsid) 

[Felix Dahlke, 2013/09/16 16:30:12]

Nit: No space after GetLogonSid?

+  {
+    DWORD dwLength = 0;
+    PTOKEN_GROUPS ptg = NULL;
+
+    // Verify the parameter passed in is not NULL.

[Felix Dahlke, 2013/09/16 16:30:12]

I do think this is kinda obvious from the condition below.

+    if (NULL == ppsid)

[Felix Dahlke, 2013/09/16 16:30:12]

We normally don't use Yoda conditions :D But it's fine if you prefer it that
way.

+      return false;
+
+    // Get required buffer size and allocate the TOKEN_GROUPS buffer.
+    if (!GetTokenInformation(hToken, TokenLogonSid, (LPVOID) ptg, 0, &dwLength)) 

[Felix Dahlke, 2013/09/16 16:30:12]

This code looks very similar to what's in AdblockPlusClient.cpp, can it be
reused? Otherwise, the same comments apply here.

+    {
+      if (GetLastError() != ERROR_INSUFFICIENT_BUFFER) 
+        return false;
+
+      ptg = (PTOKEN_GROUPS)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength);
+
+      if (ptg == NULL)
+        return false;
+    }
+
+    // Get the token group information from the access token.
+    if (!GetTokenInformation(hToken, TokenLogonSid, (LPVOID) ptg, dwLength, &dwLength) || ptg->GroupCount != 1) 
+    {
+      HeapFree(GetProcessHeap(), 0, (LPVOID)ptg);
+      return false;
+    }
+
+    // Found the logon SID; make a copy of it.
+    dwLength = GetLengthSid(ptg->Groups[0].Sid);
+    *ppsid = (PSID) HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength);
+    if (*ppsid == NULL)
+    {
+      HeapFree(GetProcessHeap(), 0, (LPVOID)ptg);
+      return false;
+    }
+    if (!CopySid(dwLength, *ppsid, ptg->Groups[0].Sid)) 
+    {
+      HeapFree(GetProcessHeap(), 0, (LPVOID)ptg);
+      HeapFree(GetProcessHeap(), 0, (LPVOID)*ppsid);
+      return false;
+    }
+
+    HeapFree(GetProcessHeap(), 0, (LPVOID)ptg);
+    return true;
+  }
+
+  bool CreateObjectSecurityDescriptor(PSID pLogonSid, PSECURITY_DESCRIPTOR* ppSD)
+  {
+    BOOL bSuccess = FALSE;

[Felix Dahlke, 2013/09/16 16:30:12]

I'd rather have these declarations closer to their first use, and not in
hungarian :)

+    DWORD dwRes;
+    PSID pBrowserSID = NULL;
+    PACL pACL = NULL;
+    PSECURITY_DESCRIPTOR pSD = NULL;
+    EXPLICIT_ACCESS ea[2];
+    SID_IDENTIFIER_AUTHORITY ApplicationAuthority = SECURITY_APP_PACKAGE_AUTHORITY;
+
+    ConvertStringSidToSid(Communication::browserSID.c_str(), &pBrowserSID);
+
+    // Initialize an EXPLICIT_ACCESS structure for an ACE.
+    // The ACE will allow LogonSid generic all access
+    ZeroMemory(&ea, 2 * sizeof(EXPLICIT_ACCESS));
+    ea[0].grfAccessPermissions = STANDARD_RIGHTS_ALL | SPECIFIC_RIGHTS_ALL;
+    ea[0].grfAccessMode = SET_ACCESS;
+    ea[0].grfInheritance= NO_INHERITANCE;
+    ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
+    ea[0].Trustee.TrusteeType = TRUSTEE_IS_USER;
+    ea[0].Trustee.ptstrName  = (LPTSTR) pLogonSid;
+
+    // Initialize an EXPLICIT_ACCESS structure for an ACE.
+    // The ACE will give the browser SID all permissions
+    ea[1].grfAccessPermissions = STANDARD_RIGHTS_ALL | SPECIFIC_RIGHTS_ALL;
+    ea[1].grfAccessMode = SET_ACCESS;
+    ea[1].grfInheritance= NO_INHERITANCE;
+    ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID;
+    ea[1].Trustee.TrusteeType = TRUSTEE_IS_GROUP;
+    ea[1].Trustee.ptstrName  = (LPTSTR) pBrowserSID;
+
+    // Create a new ACL that contains the new ACEs.
+    dwRes = SetEntriesInAcl(2, ea, NULL, &pACL);
+    if (ERROR_SUCCESS != dwRes) 
+    {
+      FreeSid(pBrowserSID);
+      return false;
+    }
+
+    // Initialize a security descriptor.  
+    pSD = (PSECURITY_DESCRIPTOR) LocalAlloc(LPTR, SECURITY_DESCRIPTOR_MIN_LENGTH); 
+    if (NULL == pSD) 
+    { 
+      FreeSid(pBrowserSID);
+      LocalFree(pACL);
+      return false;; 
+    } 
+
+    if (!InitializeSecurityDescriptor(pSD, SECURITY_DESCRIPTOR_REVISION)) 
+    {  
+      FreeSid(pBrowserSID);
+      LocalFree(pACL);
+      LocalFree((LPVOID)pSD);
+      return false;
+    } 
+
+    // Add the ACL to the security descriptor. 
+    if (!SetSecurityDescriptorDacl(pSD, TRUE, pACL, FALSE))   // not a default DACL 
+    {  
+      FreeSid(pBrowserSID);
+      LocalFree(pACL);
+      LocalFree((LPVOID)pSD);
+      return false;
+    } 
+
+    *ppSD = pSD;
+    pSD = NULL;
+
+    if (pBrowserSID) 
+      FreeSid(pBrowserSID);
+    if (pACL) 
+      LocalFree(pACL);
+
+    return true;
+  }
+
+
 }
 
 const std::wstring Communication::pipeName = L"\\\\.\\pipe\\adblockplusengine_" + GetUserName();
@@ -67,7 +201,6 @@
   : std::runtime_error("Pipe disconnected")
 {
 }
-
 Communication::Pipe::Pipe(const std::wstring& pipeName, Communication::Pipe::Mode mode)

[Wladimir Palant, 2013/09/16 13:45:07]

Nit: Please keep an empty line before this one.

 {
   pipe = INVALID_HANDLE_VALUE;
@@ -76,25 +209,36 @@
     SECURITY_ATTRIBUTES sa;
     memset(&sa, 0, sizeof(SECURITY_ATTRIBUTES));
     sa.nLength = sizeof(SECURITY_ATTRIBUTES);
+
+    HANDLE hToken = NULL;
+    OpenProcessToken(GetCurrentProcess(), TOKEN_READ, &hToken); 
+
+    if (browserSID.empty())
+    {
+      if (IsWindowsVistaOrLater())
+      {
+        // Low mandatory label. See http://msdn.microsoft.com/en-us/library/bb625958.aspx
+        LPCWSTR accessControlEntry = L"S:(ML;;NW;;;LW)";
+        ConvertStringSecurityDescriptorToSecurityDescriptorW(accessControlEntry, SDDL_REVISION_1, &securitydescriptor, 0);
+        sa.lpSecurityDescriptor = securitydescriptor;
+      }
 
-    PSECURITY_DESCRIPTOR securitydescriptor;
-    if (IsWindowsVistaOrLater())
-    {
-      // Low mandatory label. See http://msdn.microsoft.com/en-us/library/bb625958.aspx
-      LPCWSTR accessControlEntry = L"S:(ML;;NW;;;LW)";
-      ConvertStringSecurityDescriptorToSecurityDescriptorW(accessControlEntry, SDDL_REVISION_1, &securitydescriptor, 0);
-      sa.lpSecurityDescriptor = securitydescriptor;
-    }
-
-    sa.bInheritHandle = TRUE;
-
-    pipe = CreateNamedPipeW (pipeName.c_str(), PIPE_ACCESS_DUPLEX, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
-                                  PIPE_UNLIMITED_INSTANCES, bufferSize, bufferSize, 0, &sa);
-    if (IsWindowsVistaOrLater() && securitydescriptor)
-    {
-        LocalFree(securitydescriptor);
-    }
-  }
+      sa.bInheritHandle = TRUE;
+    }
+    else  // IE is in AppContainer
+    {
+      PSID pLogonSid = NULL;
+      //Allowing LogonSid and IE's appcontainer. 
+      if (GetLogonSid(hToken, &pLogonSid) && CreateObjectSecurityDescriptor(pLogonSid, &securitydescriptor) )

[Wladimir Palant, 2013/09/16 13:45:07]

Why do we need to allow the logon sid? Is it for testing only or can the plugin
run both inside and outside the app container?

[Oleksandr, 2013/09/17 08:27:19]

Our plugin is unable to access the named pipe if we don't allow logon sid

+      {
+        sa.nLength = sizeof(SECURITY_ATTRIBUTES);
+        sa.bInheritHandle = TRUE;

[Wladimir Palant, 2013/09/16 13:45:07]

I think that we still want to set this in any case. In fact, it's probably a
good idea to alwaysset sa.nLength  as well.

+        sa.lpSecurityDescriptor = securitydescriptor;                            
+      }
+    }
+    pipe = CreateNamedPipe(pipeName.c_str(),  PIPE_ACCESS_DUPLEX, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,

[Wladimir Palant, 2013/09/16 13:45:07]

Please use CreateNamedPipeW explicitly, as we had it before.

+                              PIPE_UNLIMITED_INSTANCES, bufferSize, bufferSize, 0, &sa);
+   }

[Wladimir Palant, 2013/09/16 13:45:07]

Bad indentation?

   else
   {
     pipe = CreateFileW(pipeName.c_str(), GENERIC_READ | GENERIC_WRITE, 0, 0, OPEN_EXISTING, 0, 0);
@@ -121,6 +265,9 @@
 Communication::Pipe::~Pipe()
 {
   CloseHandle(pipe);
+  if (securitydescriptor)
+    LocalFree(securitydescriptor);
+
 }
 
 Communication::InputBuffer Communication::Pipe::ReadMessage()