Enhanced Protected Mode support

src/shared/Communication.cpp

 #include <Windows.h>

[Wladimir Palant, 2013/09/17 07:53:48]

From what I can tell, none of my comments in this file have been addressed.

 #include <Lmcons.h>
 #include <Sddl.h>
 #include <aclapi.h>
+#include <strsafe.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
 {
   const int bufferSize = 1024;
 
   std::string AppendErrorCode(const std::string& message)
   {
     std::stringstream stream;
     stream << message << " (Error code: " << GetLastError() << ")";
     return stream.str();
   }
 
   std::wstring GetUserName()
   {
     const DWORD maxLength = UNLEN + 1;
     std::auto_ptr<wchar_t> buffer(new wchar_t[maxLength]);
     DWORD length = maxLength;
     if (!::GetUserNameW(buffer.get(), &length))
       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?

   {
+    if (ppsid == NULL)
+      return false;
+
+    // Get required buffer size and allocate the TOKEN_GROUPS buffer.
     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 (!GetTokenInformation(hToken, TokenLogonSid, ptg, 0, &dwLength)) 
     {
       if (GetLastError() != ERROR_INSUFFICIENT_BUFFER) 
         return false;
 
-      ptg = (PTOKEN_GROUPS)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength);
+      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);
+    if (!GetTokenInformation(hToken, TokenLogonSid, ptg, dwLength, &dwLength) || ptg->GroupCount != 1) 
+    {
+      HeapFree(GetProcessHeap(), 0, 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);
+      HeapFree(GetProcessHeap(), 0, 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);
+      HeapFree(GetProcessHeap(), 0, ptg);
+      HeapFree(GetProcessHeap(), 0, *ppsid);
+      return false;
+    }
+
+    HeapFree(GetProcessHeap(), 0, 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.
@@ skipping 96 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")
 {
 }
 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.

[Wladimir Palant, 2013/09/17 07:53:48]

Not addressed: the empty line before this function should stay.

 {
   pipe = INVALID_HANDLE_VALUE;
   if (mode == MODE_CREATE)
   {
     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;
       }
 
       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);
     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 48 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");
 }