Fix issues with security tokens (Enhanced Protected Mode, Protected Mode etc)

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"
 
 
-//
-// Application Package Authority.
-//
-
-#define SECURITY_APP_PACKAGE_AUTHORITY              {0,0,0,0,0,15}
-
-#define SECURITY_APP_PACKAGE_BASE_RID               (0x00000002L)
-#define SECURITY_BUILTIN_APP_PACKAGE_RID_COUNT      (2L)
-#define SECURITY_APP_PACKAGE_RID_COUNT              (8L)
-#define SECURITY_CAPABILITY_BASE_RID                (0x00000003L)
-#define SECURITY_BUILTIN_CAPABILITY_RID_COUNT       (2L)
-#define SECURITY_CAPABILITY_RID_COUNT               (5L)
-
-//
-// Built-in Packages.
-//
-
-#define SECURITY_BUILTIN_PACKAGE_ANY_PACKAGE        (0x00000001L)
 
 namespace
 {
   const int bufferSize = 1024;
 
   std::string AppendErrorCode(const std::string& message)
   {
     std::stringstream stream;
     stream << message << " (Error code: " << GetLastError() << ")";
     return stream.str();
@@ skipping 26 matching lines @@
     DWORD sidLength = GetLengthSid(tokenGroups->Groups[0].Sid);
     std::auto_ptr<SID> sid(new SID[sidLength]);
     if (!CopySid(sidLength, sid.get(), tokenGroups->Groups[0].Sid)) 
       throw std::runtime_error("CopySid failed");
     return sid;
   }
 
   // Creates a security descriptor: 
   // Allows ALL access to Logon SID and to all app containers in DACL.
   // Sets Low Integrity in SACL.
-  std::auto_ptr<SECURITY_DESCRIPTOR> CreateSecurityDescriptorLowAndAppContainers(PSID logonSid)

[Felix Dahlke, 2013/12/10 16:46:36]

I don't think we need to put the specifics of the descriptor we create into the
function name :)

+  std::auto_ptr<SECURITY_DESCRIPTOR> CreateSecurityDescriptor(PSID logonSid)
   {
     EXPLICIT_ACCESSW explicitAccess[2] = {};
 
     explicitAccess[0].grfAccessPermissions = STANDARD_RIGHTS_ALL | SPECIFIC_RIGHTS_ALL;
     explicitAccess[0].grfAccessMode = SET_ACCESS;
     explicitAccess[0].grfInheritance= NO_INHERITANCE;
     explicitAccess[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
     explicitAccess[0].Trustee.TrusteeType = TRUSTEE_IS_USER;
     explicitAccess[0].Trustee.ptstrName  = static_cast<LPWSTR>(logonSid);
 
     std::tr1::shared_ptr<SID> sharedAllAppContainersSid;
     // TODO: Would be better to detect if AppContainers are supported instead of checking the Windows version

[Felix Dahlke, 2014/06/24 15:30:39]

You can also slay the TODO comment here now!

-    bool isWindows8 = IsWindows8();

[Felix Dahlke, 2013/12/10 16:46:36]

Agreed, would be better to check for app container support. I'd definitely
suggest to rename isWindows8 to isAppContainerSupported or something.

-    if (isWindows8)
-    {
-      PSID allAppContainersSid = 0;
-      SID_IDENTIFIER_AUTHORITY ApplicationAuthority = SECURITY_APP_PACKAGE_AUTHORITY;

[Felix Dahlke, 2013/12/10 16:46:36]

Variables should start with a lower case letter.

-
+    bool isAppContainersSupported = IsAppContainersSupported();
+    if (isAppContainersSupported)
+    {
       // Create a well-known SID for the all appcontainers group.

[Felix Dahlke, 2013/12/10 16:46:36]

I think it'd make sense to move this comment in front of the two declarations
above.

       // We need to allow access to all AppContainers, since, apparently,
       // giving access to specific AppContainer (for example AppContainer of IE) 
       // tricks Windows into thinking that token is IN AppContainer. 
       // Which blocks all the calls from outside, making it impossible to communicate
-      // with engine when IE is launched with different security settings.

[Felix Dahlke, 2013/12/10 16:46:36]

with "the" engine

-      AllocateAndInitializeSid(&ApplicationAuthority, 
+      // with the engine when IE is launched with different security settings.
+      PSID allAppContainersSid = 0;
+      SID_IDENTIFIER_AUTHORITY applicationAuthority = SECURITY_APP_PACKAGE_AUTHORITY;
+
+      AllocateAndInitializeSid(&applicationAuthority, 
               SECURITY_BUILTIN_APP_PACKAGE_RID_COUNT,
               SECURITY_APP_PACKAGE_BASE_RID,
               SECURITY_BUILTIN_PACKAGE_ANY_PACKAGE,
               0, 0, 0, 0, 0, 0,
               &allAppContainersSid);
       sharedAllAppContainersSid.reset(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);
     }
     PACL acl = 0;
-    if (SetEntriesInAcl(isWindows8 ? 2 : 1, explicitAccess, 0, &acl) != ERROR_SUCCESS)
+    if (SetEntriesInAcl(isAppContainersSupported ? 2 : 1, 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
 
     std::auto_ptr<SECURITY_DESCRIPTOR> securityDescriptor((SECURITY_DESCRIPTOR*)new char[SECURITY_DESCRIPTOR_MIN_LENGTH]);
     if (!InitializeSecurityDescriptor(securityDescriptor.get(), SECURITY_DESCRIPTOR_REVISION)) 
       return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
 
     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
     LPCWSTR accessControlEntry = L"S:(ML;;NW;;;LW)";
-    PSECURITY_DESCRIPTOR pDummySecurityDescriptorLow;

[Felix Dahlke, 2013/12/10 16:46:36]

Also quite hungarian :)

-    ConvertStringSecurityDescriptorToSecurityDescriptorW(accessControlEntry, SDDL_REVISION_1, &pDummySecurityDescriptorLow, 0);
-    std::tr1::shared_ptr<SECURITY_DESCRIPTOR> sharedDummySecurityDescriptor(static_cast<SECURITY_DESCRIPTOR*>(pDummySecurityDescriptorLow), LocalFree); // Just to simplify cleanup
+    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;

[Felix Dahlke, 2014/06/24 15:30:39]

Hm, don't we leak sacl now that you removed sharedSacl?

[Eric, 2014/06/25 17:43:55]

Evidently not. The security descriptor structure has internal structure not
exposed in the SECURITY_DESCRIPTOR declaration. My reading of the MS docs (which
are notoriously incomplete about allocation issues) is that the function
GetSecurityDescriptorSacl() does not allocate, but rather returns a pointer to
something that's already present in its first argument.

SECURITY_DESCRIPTOR structure
http://msdn.microsoft.com/en-us/library/windows/desktop/aa379561%28v=vs.85%29...

GetSecurityDescriptorSacl function
http://msdn.microsoft.com/en-us/library/windows/desktop/aa446653%28v=vs.85%29...

[Felix Dahlke, 2014/06/27 14:35:56]

Yeah, I think you're right Eric. Seems like Oleksandr interpreted it the same
way too :)

-    GetSecurityDescriptorSacl(pDummySecurityDescriptorLow, &saclPresent, &sacl, &saclDefaulted); 
-    std::tr1::shared_ptr<ACL> sharedSacl(static_cast<ACL*>(sacl), LocalFree); // Just to simplify cleanup
+    GetSecurityDescriptorSacl(dummySecurityDescriptorLow, &saclPresent, &sacl, &saclDefaulted); 
     if (saclPresent)
     {
       if (!SetSecurityDescriptorSacl(securityDescriptor.get(), TRUE, sacl, FALSE))
       {
         DWORD err = GetLastError();
         return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
       }
     }
 
     return securityDescriptor;
@@ skipping 50 matching lines @@
     securityAttributes.nLength = sizeof(SECURITY_ATTRIBUTES);
     securityAttributes.bInheritHandle = TRUE;
 
     std::tr1::shared_ptr<SECURITY_DESCRIPTOR> sharedSecurityDescriptor; // Just to simplify cleanup
 
     AutoHandle token;
     OpenProcessToken(GetCurrentProcess(), TOKEN_READ, token);
     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)
-    std::auto_ptr<SECURITY_DESCRIPTOR> securityDescriptor = CreateSecurityDescriptorLowAndAppContainers(logonSid.get());
+    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);
 
   }
   else
   {
     pipe = CreateFileW(pipeName.c_str(), GENERIC_READ | GENERIC_WRITE, 0, 0, OPEN_EXISTING, 0, 0);
@@ skipping 52 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");
 }