Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: src/shared/Communication.cpp

Issue 6308231068516352: Fix issues with security tokens (Enhanced Protected Mode, Protected Mode etc) (Closed)
Patch Set: Created Nov. 15, 2013, 7:22 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: src/shared/Communication.cpp
===================================================================
--- a/src/shared/Communication.cpp
+++ b/src/shared/Communication.cpp
@@ -8,11 +8,25 @@
#include "Communication.h"
#include "Utils.h"
-#ifndef SECURITY_APP_PACKAGE_AUTHORITY
+
+//
+// Application Package Authority.
+//
+
#define SECURITY_APP_PACKAGE_AUTHORITY {0,0,0,0,0,15}
-#endif
-
-std::wstring Communication::browserSID;
+
+#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
{
@@ -56,12 +70,11 @@
return sid;
}
- std::auto_ptr<SECURITY_DESCRIPTOR> CreateObjectSecurityDescriptor(PSID logonSid)
+ // 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
{
- PSID browserSid = 0;
- std::tr1::shared_ptr<SID> sharedBrowserSid(reinterpret_cast<SID*>(browserSid), FreeSid); // Just to simplify cleanup
- ConvertStringSidToSid(Communication::browserSID.c_str(), &browserSid);
-
EXPLICIT_ACCESSW explicitAccess[2] = {};
explicitAccess[0].grfAccessPermissions = STANDARD_RIGHTS_ALL | SPECIFIC_RIGHTS_ALL;
@@ -71,25 +84,66 @@
explicitAccess[0].Trustee.TrusteeType = TRUSTEE_IS_USER;
explicitAccess[0].Trustee.ptstrName = static_cast<LPWSTR>(logonSid);
- 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>(browserSid);
+ std::tr1::shared_ptr<SID> sharedAllAppContainersSid;
+ // TODO: Would be better to detect if AppContainers are supported instead of checking the Windows version
+ bool isWindows8 = IsWindows8();
Felix Dahlke 2013/12/10 16:46:36 Agreed, would be better to check for app container
+ 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.
+ // 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 fr
+ // 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,
+ 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;
- std::tr1::shared_ptr<ACL> sharedAcl(acl, FreeSid); // Just to simplify cleanup
- if (SetEntriesInAcl(2, explicitAccess, 0, &acl) != ERROR_SUCCESS)
+ if (SetEntriesInAcl(isWindows8 ? 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(new SECURITY_DESCRIPTOR[SECURITY_DESCRIPTOR_MIN_LENGTH]);
+ 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
+ BOOL saclPresent = FALSE;
+ BOOL saclDefaulted = FALSE;
+ PACL sacl;
+ GetSecurityDescriptorSacl(pDummySecurityDescriptorLow, &saclPresent, &sacl, &saclDefaulted);
+ std::tr1::shared_ptr<ACL> sharedSacl(static_cast<ACL*>(sacl), LocalFree); // Just to simplify cleanup
+ if (saclPresent)
+ {
+ if (!SetSecurityDescriptorSacl(securityDescriptor.get(), TRUE, sacl, FALSE))
+ {
+ DWORD err = GetLastError();
+ return std::auto_ptr<SECURITY_DESCRIPTOR>(0);
+ }
+ }
+
return securityDescriptor;
}
}
@@ -146,27 +200,18 @@
std::tr1::shared_ptr<SECURITY_DESCRIPTOR> sharedSecurityDescriptor; // Just to simplify cleanup
- const bool inAppContainer = !browserSID.empty();
- if (inAppContainer)
- {
- AutoHandle token;
- OpenProcessToken(GetCurrentProcess(), TOKEN_READ, token);
- std::auto_ptr<SID> logonSid = GetLogonSid(token);
- std::auto_ptr<SECURITY_DESCRIPTOR> securityDescriptor = CreateObjectSecurityDescriptor(logonSid.get());
- securityAttributes.lpSecurityDescriptor = securityDescriptor.release();
- sharedSecurityDescriptor.reset(static_cast<SECURITY_DESCRIPTOR*>(securityAttributes.lpSecurityDescriptor));
- }
- else 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,
- &securityAttributes.lpSecurityDescriptor, 0);
- sharedSecurityDescriptor.reset(static_cast<SECURITY_DESCRIPTOR*>(securityAttributes.lpSecurityDescriptor), LocalFree);
- }
+ 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());
+ 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
{

Powered by Google App Engine
This is Rietveld