Modify DACL to allow initial token to open resource.

This CL modifies the DACL for the primary LowBox token to allow the
initial privileged impersonation token to open a handle to it. This
is necessary as the Windows kernel implements a feature which prevents
Low IL tokens accessing any securable resource which has a package SID
as part of the DACL. Without the change opening the primary process
token fails during warmup which causes numerous unexpected behaviors
such as the process crashing.

Bug: 1000447
Change-Id: I76ac6eecae08d8de6f2130e5841b50a17b3739e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1783728Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: James Forshaw <forshaw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697354}

sandbox/win/src/acl.cc

@@ -151,4 +151,21 @@ bool AddKnownSidToObject(HANDLE object,
   return true;
 }
 
+bool ReplacePackageSidInDacl(HANDLE object,
+                             SE_OBJECT_TYPE object_type,
+                             const Sid& package_sid,
+                             ACCESS_MASK access) {
+  if (!AddKnownSidToObject(object, object_type, package_sid, REVOKE_ACCESS,
+                           0)) {
+    return false;
+  }
+
+  Sid any_package_sid(::WinBuiltinAnyPackageSid);
+  if (!AddKnownSidToObject(object, object_type, any_package_sid, GRANT_ACCESS,
+                           access)) {
+    return false;
+  }
+  return true;
+}
+
 }  // namespace sandbox

sandbox/win/src/acl.h

@@ -51,6 +51,14 @@ bool AddKnownSidToObject(HANDLE object,
                          ACCESS_MODE access_mode,
                          ACCESS_MASK access);
 
+// Replace package SID in DACL to the "any package" SID. It allows Low-IL
+// tokens to open the object which is important for warm up when using renderer
+// AppContainer.
+bool ReplacePackageSidInDacl(HANDLE object,
+                             SE_OBJECT_TYPE object_type,
+                             const Sid& package_sid,
+                             ACCESS_MASK access);
+
 }  // namespace sandbox
 
 #endif  // SANDBOX_SRC_ACL_H_

sandbox/win/src/restricted_token_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/win/atl.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/windows_version.h"
+#include "sandbox/win/src/acl.h"
 #include "sandbox/win/src/security_capabilities.h"
 #include "sandbox/win/src/sid.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -84,6 +85,46 @@ bool GetVariableTokenInformation(const base::win::ScopedHandle& token,
                                      information);
 }
 
+void CheckDaclForPackageSid(const base::win::ScopedHandle& token,
+                            PSECURITY_CAPABILITIES security_capabilities,
+                            bool package_sid_required) {
+  DWORD length_needed = 0;
+  ::GetKernelObjectSecurity(token.Get(), DACL_SECURITY_INFORMATION, nullptr, 0,
+                            &length_needed);
+  ASSERT_EQ(::GetLastError(), DWORD{ERROR_INSUFFICIENT_BUFFER});
+
+  std::vector<char> security_desc_buffer(length_needed);
+  SECURITY_DESCRIPTOR* security_desc =
+      reinterpret_cast<SECURITY_DESCRIPTOR*>(security_desc_buffer.data());
+
+  ASSERT_TRUE(::GetKernelObjectSecurity(token.Get(), DACL_SECURITY_INFORMATION,
+                                        security_desc, length_needed,
+                                        &length_needed));
+
+  ATL::CSecurityDesc token_sd(*security_desc);
+  ATL::CDacl dacl;
+  ASSERT_TRUE(token_sd.GetDacl(&dacl));
+
+  ATL::CSid package_sid(
+      static_cast<SID*>(security_capabilities->AppContainerSid));
+  ATL::CSid all_package_sid(
+      static_cast<SID*>(sandbox::Sid(::WinBuiltinAnyPackageSid).GetPSID()));
+
+  unsigned int ace_count = dacl.GetAceCount();
+  for (unsigned int i = 0; i < ace_count; ++i) {
+    ATL::CSid sid;
+    ACCESS_MASK mask = 0;
+    BYTE type = 0;
+    dacl.GetAclEntry(i, &sid, &mask, &type);
+    if (mask != TOKEN_ALL_ACCESS || type != ACCESS_ALLOWED_ACE_TYPE)
+      continue;
+    if (sid == package_sid)
+      EXPECT_TRUE(package_sid_required);
+    else if (sid == all_package_sid)
+      EXPECT_FALSE(package_sid_required);
+  }
+}
+
 void CheckLowBoxToken(const base::win::ScopedHandle& token,
                       TOKEN_TYPE token_type,
                       PSECURITY_CAPABILITIES security_capabilities) {
@@ -126,38 +167,7 @@ void CheckLowBoxToken(const base::win::ScopedHandle& token,
                            security_capabilities->Capabilities[index].Sid));
   }
 
-  DWORD length_needed = 0;
-  ::GetKernelObjectSecurity(token.Get(), DACL_SECURITY_INFORMATION, nullptr, 0,
-                            &length_needed);
-  ASSERT_EQ(::GetLastError(), DWORD{ERROR_INSUFFICIENT_BUFFER});
-
-  std::vector<char> security_desc_buffer(length_needed);
-  SECURITY_DESCRIPTOR* security_desc =
-      reinterpret_cast<SECURITY_DESCRIPTOR*>(security_desc_buffer.data());
-
-  ASSERT_TRUE(::GetKernelObjectSecurity(token.Get(), DACL_SECURITY_INFORMATION,
-                                        security_desc, length_needed,
-                                        &length_needed));
-
-  ATL::CSecurityDesc token_sd(*security_desc);
-  ATL::CSid check_sid(
-      static_cast<SID*>(security_capabilities->AppContainerSid));
-  bool package_sid_found = false;
-
-  ATL::CDacl dacl;
-  ASSERT_TRUE(token_sd.GetDacl(&dacl));
-  unsigned int ace_count = dacl.GetAceCount();
-  for (unsigned int i = 0; i < ace_count; ++i) {
-    ATL::CSid sid;
-    ACCESS_MASK mask = 0;
-    BYTE type = 0;
-    dacl.GetAclEntry(i, &sid, &mask, &type);
-    if (sid == check_sid && mask == TOKEN_ALL_ACCESS &&
-        type == ACCESS_ALLOWED_ACE_TYPE) {
-      package_sid_found = true;
-    }
-  }
-  ASSERT_TRUE(package_sid_found);
+  CheckDaclForPackageSid(token, security_capabilities, true);
 }
 
 // Checks if a sid is in the restricting list of the restricted token.
@@ -761,6 +771,11 @@ TEST(RestrictedTokenTest, LowBoxToken) {
   ASSERT_TRUE(token.IsValid());
   CheckLowBoxToken(token, ::TokenPrimary, &caps_no_capabilities);
 
+  ASSERT_TRUE(ReplacePackageSidInDacl(token.Get(), SE_KERNEL_OBJECT,
+                                      Sid(caps_no_capabilities.AppContainerSid),
+                                      TOKEN_ALL_ACCESS));
+  CheckDaclForPackageSid(token, &caps_no_capabilities, false);
+
   ASSERT_EQ(DWORD{ERROR_SUCCESS},
             CreateLowBoxToken(nullptr, IMPERSONATION, &caps_no_capabilities,
                               nullptr, 0, &token));

sandbox/win/src/sandbox_policy_base.cc

@@ -15,6 +15,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/win/win_util.h"
 #include "base/win/windows_version.h"
+#include "sandbox/win/src/acl.h"
 #include "sandbox/win/src/filesystem_policy.h"
 #include "sandbox/win/src/interception.h"
 #include "sandbox/win/src/job.h"
@@ -473,6 +474,11 @@ ResultCode PolicyBase::MakeTokens(base::win::ScopedHandle* initial,
                           saved_handles_count, lowbox) != ERROR_SUCCESS) {
       return SBOX_ERROR_GENERIC;
     }
+
+    if (!ReplacePackageSidInDacl(lowbox->Get(), SE_KERNEL_OBJECT, package_sid,
+                                 TOKEN_ALL_ACCESS)) {
+      return SBOX_ERROR_GENERIC;
+    }
   }
 
   // Create the 'better' token. We use this token as the one that the main