Bug 1229829 - Part 1 - Apply chromium sandbox patches from upstream which improves alternate desktop support; r=bobowen
authorAlex Gaynor <agaynor@mozilla.com>
Wed, 16 Aug 2017 09:54:31 -0400
changeset 428406 d342ab7ef7c3565bf5bea01acd261cd1b3d2a456
parent 428405 643915efb742b9c561be7d46b6b599321d8679f5
child 428407 6d9980e17a8caf33d1c5c6a6f418b9a6b7522529
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbobowen
bugs1229829
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1229829 - Part 1 - Apply chromium sandbox patches from upstream which improves alternate desktop support; r=bobowen This is 0cb5dadc2b1f84fbbd9c6f75056e38d05a5b07d3 and db4c64b63d6098294ed255e962700fd2d465575e in the chromium repository. This allows a single process to create sandboxed children with alternate desktops on both an alternate winstation and the local winstation. MozReview-Commit-ID: 8sS7LjoveOk
security/sandbox/chromium/sandbox/win/src/policy_target_test.cc
security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.cc
security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.h
security/sandbox/chromium/sandbox/win/src/window.cc
security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt
--- a/security/sandbox/chromium/sandbox/win/src/policy_target_test.cc
+++ b/security/sandbox/chromium/sandbox/win/src/policy_target_test.cc
@@ -347,20 +347,51 @@ TEST(PolicyTargetTest, WinstaPolicy) {
   ::WaitForSingleObject(target.process_handle(), INFINITE);
 
   // Close the desktop handle.
   temp_policy = broker->CreatePolicy();
   temp_policy->DestroyAlternateDesktop();
   temp_policy->Release();
 }
 
+// Creates multiple policies, with alternate desktops on both local and
+// alternate winstations.
+TEST(PolicyTargetTest, BothLocalAndAlternateWinstationDesktop) {
+  BrokerServices* broker = GetBroker();
+
+  scoped_refptr<TargetPolicy> policy1 = broker->CreatePolicy();
+  scoped_refptr<TargetPolicy> policy2 = broker->CreatePolicy();
+  scoped_refptr<TargetPolicy> policy3 = broker->CreatePolicy();
+
+  ResultCode result;
+  result = policy1->SetAlternateDesktop(false);
+  EXPECT_EQ(SBOX_ALL_OK, result);
+  result = policy2->SetAlternateDesktop(true);
+  EXPECT_EQ(SBOX_ALL_OK, result);
+  result = policy3->SetAlternateDesktop(false);
+  EXPECT_EQ(SBOX_ALL_OK, result);
+
+  base::string16 policy1_desktop_name = policy1->GetAlternateDesktop();
+  base::string16 policy2_desktop_name = policy2->GetAlternateDesktop();
+
+  // Extract only the "desktop name" portion of
+  // "{winstation name}\\{desktop name}"
+  EXPECT_NE(policy1_desktop_name.substr(
+                policy1_desktop_name.find_first_of(L'\\') + 1),
+            policy2_desktop_name.substr(
+                policy2_desktop_name.find_first_of(L'\\') + 1));
+
+  policy1->DestroyAlternateDesktop();
+  policy2->DestroyAlternateDesktop();
+  policy3->DestroyAlternateDesktop();
+}
+
 // Launches the app in the sandbox and share a handle with it. The app should
 // be able to use the handle.
 TEST(PolicyTargetTest, ShareHandleTest) {
-
   BrokerServices* broker = GetBroker();
   ASSERT_TRUE(broker != NULL);
 
   base::StringPiece contents = "Hello World";
   std::string name = "TestSharedMemory";
   base::SharedMemoryCreateOptions options;
   options.size = contents.size();
   options.share_read_only = true;
--- a/security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.cc
+++ b/security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.cc
@@ -104,21 +104,27 @@ HANDLE CreateLowBoxObjectDirectory(PSID 
 
 }  // namespace
 
 namespace sandbox {
 
 SANDBOX_INTERCEPT IntegrityLevel g_shared_delayed_integrity_level;
 SANDBOX_INTERCEPT MitigationFlags g_shared_delayed_mitigations;
 
-// Initializes static members.
-HWINSTA PolicyBase::alternate_winstation_handle_ = NULL;
-HDESK PolicyBase::alternate_desktop_handle_ = NULL;
+// Initializes static members. alternate_desktop_handle_ is a desktop on
+// alternate_winstation_handle_, alternate_desktop_local_winstation_handle_ is a
+// desktop on the same winstation as the parent process.
+HWINSTA PolicyBase::alternate_winstation_handle_ = nullptr;
+HDESK PolicyBase::alternate_desktop_handle_ = nullptr;
+HDESK PolicyBase::alternate_desktop_local_winstation_handle_ = nullptr;
 IntegrityLevel PolicyBase::alternate_desktop_integrity_level_label_ =
     INTEGRITY_LEVEL_SYSTEM;
+IntegrityLevel
+    PolicyBase::alternate_desktop_local_winstation_integrity_level_label_ =
+        INTEGRITY_LEVEL_SYSTEM;
 
 PolicyBase::PolicyBase()
     : ref_count(1),
       lockdown_level_(USER_LOCKDOWN),
       initial_level_(USER_LOCKDOWN),
       job_level_(JOB_LOCKDOWN),
       ui_exceptions_(0),
       memory_limit_(0),
@@ -215,37 +221,36 @@ ResultCode PolicyBase::SetAlternateDeskt
 }
 
 base::string16 PolicyBase::GetAlternateDesktop() const {
   // No alternate desktop or winstation. Return an empty string.
   if (!use_alternate_desktop_ && !use_alternate_winstation_) {
     return base::string16();
   }
 
-  // The desktop and winstation should have been created by now.
-  // If we hit this scenario, it means that the user ignored the failure
-  // during SetAlternateDesktop, so we ignore it here too.
-  if (use_alternate_desktop_ && !alternate_desktop_handle_) {
-    return base::string16();
+  if (use_alternate_winstation_) {
+    // The desktop and winstation should have been created by now.
+    // If we hit this scenario, it means that the user ignored the failure
+    // during SetAlternateDesktop, so we ignore it here too.
+    if (!alternate_desktop_handle_ || !alternate_winstation_handle_) {
+      return base::string16();
+    }
+    return GetFullDesktopName(alternate_winstation_handle_,
+                              alternate_desktop_handle_);
+  } else {
+    if (!alternate_desktop_local_winstation_handle_) {
+      return base::string16();
+    }
+    return GetFullDesktopName(nullptr,
+                              alternate_desktop_local_winstation_handle_);
   }
-  if (use_alternate_winstation_ && (!alternate_desktop_handle_ ||
-                                    !alternate_winstation_handle_)) {
-    return base::string16();
-  }
-
-  return GetFullDesktopName(alternate_winstation_handle_,
-                            alternate_desktop_handle_);
 }
 
 ResultCode PolicyBase::CreateAlternateDesktop(bool alternate_winstation) {
   if (alternate_winstation) {
-    // Previously called with alternate_winstation = false?
-    if (!alternate_winstation_handle_ && alternate_desktop_handle_)
-      return SBOX_ERROR_UNSUPPORTED;
-
     // Check if it's already created.
     if (alternate_winstation_handle_ && alternate_desktop_handle_)
       return SBOX_ALL_OK;
 
     DCHECK(!alternate_winstation_handle_);
     // Create the window station.
     ResultCode result = CreateAltWindowStation(&alternate_winstation_handle_);
     if (SBOX_ALL_OK != result)
@@ -262,47 +267,51 @@ ResultCode PolicyBase::CreateAlternateDe
     if (SBOX_ALL_OK != result)
       return result;
 
     // Verify that everything is fine.
     if (!alternate_desktop_handle_ ||
         GetWindowObjectName(alternate_desktop_handle_).empty())
       return SBOX_ERROR_CANNOT_CREATE_DESKTOP;
   } else {
-    // Previously called with alternate_winstation = true?
-    if (alternate_winstation_handle_)
-      return SBOX_ERROR_UNSUPPORTED;
-
     // Check if it already exists.
-    if (alternate_desktop_handle_)
+    if (alternate_desktop_local_winstation_handle_)
       return SBOX_ALL_OK;
 
     // Create the destkop.
-    ResultCode result = CreateAltDesktop(NULL, &alternate_desktop_handle_);
+    ResultCode result =
+        CreateAltDesktop(nullptr, &alternate_desktop_local_winstation_handle_);
     if (SBOX_ALL_OK != result)
       return result;
 
     // Verify that everything is fine.
-    if (!alternate_desktop_handle_ ||
-        GetWindowObjectName(alternate_desktop_handle_).empty())
+    if (!alternate_desktop_local_winstation_handle_ ||
+        GetWindowObjectName(alternate_desktop_local_winstation_handle_).empty())
       return SBOX_ERROR_CANNOT_CREATE_DESKTOP;
   }
 
   return SBOX_ALL_OK;
 }
 
 void PolicyBase::DestroyAlternateDesktop() {
-  if (alternate_desktop_handle_) {
-    ::CloseDesktop(alternate_desktop_handle_);
-    alternate_desktop_handle_ = NULL;
-  }
+  if (use_alternate_winstation_) {
+    if (alternate_desktop_handle_) {
+      ::CloseDesktop(alternate_desktop_handle_);
+      alternate_desktop_handle_ = nullptr;
+    }
 
-  if (alternate_winstation_handle_) {
-    ::CloseWindowStation(alternate_winstation_handle_);
-    alternate_winstation_handle_ = NULL;
+    if (alternate_winstation_handle_) {
+      ::CloseWindowStation(alternate_winstation_handle_);
+      alternate_winstation_handle_ = nullptr;
+    }
+  } else {
+    if (alternate_desktop_local_winstation_handle_) {
+      ::CloseDesktop(alternate_desktop_local_winstation_handle_);
+      alternate_desktop_local_winstation_handle_ = nullptr;
+    }
   }
 }
 
 ResultCode PolicyBase::SetIntegrityLevel(IntegrityLevel integrity_level) {
   integrity_level_ = integrity_level;
   return SBOX_ALL_OK;
 }
 
@@ -445,30 +454,45 @@ ResultCode PolicyBase::MakeTokens(base::
                             lockdown);
   if (ERROR_SUCCESS != result)
     return SBOX_ERROR_GENERIC;
 
   // If we're launching on the alternate desktop we need to make sure the
   // integrity label on the object is no higher than the sandboxed process's
   // integrity level. So, we lower the label on the desktop process if it's
   // not already low enough for our process.
-  if (alternate_desktop_handle_ && use_alternate_desktop_ &&
-      integrity_level_ != INTEGRITY_LEVEL_LAST &&
-      alternate_desktop_integrity_level_label_ < integrity_level_) {
+  if (use_alternate_desktop_ && integrity_level_ != INTEGRITY_LEVEL_LAST) {
     // Integrity label enum is reversed (higher level is a lower value).
     static_assert(INTEGRITY_LEVEL_SYSTEM < INTEGRITY_LEVEL_UNTRUSTED,
                   "Integrity level ordering reversed.");
-    result = SetObjectIntegrityLabel(alternate_desktop_handle_,
-                                     SE_WINDOW_OBJECT,
-                                     L"",
-                                     GetIntegrityLevelString(integrity_level_));
-    if (ERROR_SUCCESS != result)
-      return SBOX_ERROR_GENERIC;
+    HDESK desktop_handle = nullptr;
+    IntegrityLevel desktop_integrity_level_label;
+    if (use_alternate_winstation_) {
+      desktop_handle = alternate_desktop_handle_;
+      desktop_integrity_level_label = alternate_desktop_integrity_level_label_;
+    } else {
+      desktop_handle = alternate_desktop_local_winstation_handle_;
+      desktop_integrity_level_label =
+          alternate_desktop_local_winstation_integrity_level_label_;
+    }
+    // If the desktop_handle hasn't been created for any reason, skip this.
+    if (desktop_handle && desktop_integrity_level_label < integrity_level_) {
+      result =
+          SetObjectIntegrityLabel(desktop_handle, SE_WINDOW_OBJECT, L"",
+                                  GetIntegrityLevelString(integrity_level_));
+      if (ERROR_SUCCESS != result)
+        return SBOX_ERROR_GENERIC;
 
-    alternate_desktop_integrity_level_label_ = integrity_level_;
+      if (use_alternate_winstation_) {
+        alternate_desktop_integrity_level_label_ = integrity_level_;
+      } else {
+        alternate_desktop_local_winstation_integrity_level_label_ =
+            integrity_level_;
+      }
+    }
   }
 
   if (lowbox_sid_) {
     NtCreateLowBoxToken CreateLowBoxToken = NULL;
     ResolveNTFunctionPtr("NtCreateLowBoxToken", &CreateLowBoxToken);
     OBJECT_ATTRIBUTES obj_attr;
     InitializeObjectAttributes(&obj_attr, NULL, 0, NULL, NULL);
     HANDLE token_lowbox = NULL;
--- a/security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.h
+++ b/security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.h
@@ -157,17 +157,20 @@ class PolicyBase final : public TargetPo
   std::vector<base::string16> capabilities_;
   PSID lowbox_sid_;
   base::win::ScopedHandle lowbox_directory_;
   std::unique_ptr<Dispatcher> dispatcher_;
   bool lockdown_default_dacl_;
 
   static HDESK alternate_desktop_handle_;
   static HWINSTA alternate_winstation_handle_;
+  static HDESK alternate_desktop_local_winstation_handle_;
   static IntegrityLevel alternate_desktop_integrity_level_label_;
+  static IntegrityLevel
+      alternate_desktop_local_winstation_integrity_level_label_;
 
   // Contains the list of handles being shared with the target process.
   // This list contains handles other than the stderr/stdout handles which are
   // shared with the target at times.
   base::HandlesToInheritVector handles_to_share_;
   bool enable_opm_redirection_;
 
   DISALLOW_COPY_AND_ASSIGN(PolicyBase);
--- a/security/sandbox/chromium/sandbox/win/src/window.cc
+++ b/security/sandbox/chromium/sandbox/win/src/window.cc
@@ -26,17 +26,17 @@ bool GetSecurityAttributes(HANDLE handle
                                    DACL_SECURITY_INFORMATION, NULL, NULL, &dacl,
                                    NULL, &attributes->lpSecurityDescriptor);
   if (ERROR_SUCCESS == result)
     return true;
 
   return false;
 }
 
-}
+}  // namespace
 
 namespace sandbox {
 
 ResultCode CreateAltWindowStation(HWINSTA* winsta) {
   // Get the security attributes from the current window station; we will
   // use this as the base security attributes for the new window station.
   HWINSTA current_winsta = ::GetProcessWindowStation();
   if (!current_winsta)
@@ -60,16 +60,20 @@ ResultCode CreateAltWindowStation(HWINST
     return SBOX_ALL_OK;
 
   return SBOX_ERROR_CANNOT_CREATE_WINSTATION;
 }
 
 ResultCode CreateAltDesktop(HWINSTA winsta, HDESK* desktop) {
   base::string16 desktop_name = L"sbox_alternate_desktop_";
 
+  if (!winsta) {
+    desktop_name += L"local_winstation_";
+  }
+
   // Append the current PID to the desktop name.
   wchar_t buffer[16];
   _snwprintf_s(buffer, sizeof(buffer) / sizeof(wchar_t), L"0x%X",
                ::GetCurrentProcessId());
   desktop_name += buffer;
 
   HDESK current_desktop = GetThreadDesktop(GetCurrentThreadId());
 
--- a/security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt
+++ b/security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt
@@ -3,8 +3,10 @@ Also, please update any existing links t
 
 https://hg.mozilla.org/mozilla-central/rev/a05726163a79
 https://hg.mozilla.org/mozilla-central/rev/e834e810a3fa
 https://hg.mozilla.org/mozilla-central/rev/c70d06fa5302
 https://hg.mozilla.org/mozilla-central/rev/d24db55deb85
 https://hg.mozilla.org/mozilla-central/rev/0e6bf137521e
 https://hg.mozilla.org/mozilla-central/rev/1755a454e2de
 https://bugzilla.mozilla.org/show_bug.cgi?id=1385928 bug1385928.patch
+https://chromium.googlesource.com/chromium/src/+/0cb5dadc2b1f84fbbd9c6f75056e38d05a5b07d3
+https://chromium.googlesource.com/chromium/src/+/db4c64b63d6098294ed255e962700fd2d465575e