Avoid pseudo handles in sbox startup info helper

The std handles passed to child processes should be real
handles and not pseudo handles, so add checks and use
nullptr for uninitialized values.

Std handles are only inherited by sandboxed children in
developer builds so this should not affect production Chrome.

Bug: 406023316
Change-Id: Ida3e3effcc11a7547baa047cb587d46bd03a4d37
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6432673
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1443782}

sandbox/win/src/policy_target_test.cc

@@ -594,4 +594,10 @@ TEST(PolicyTargetTest, FilterEnvironment) {
   }
 }
 
+TEST(PolicyTargetDeathTest, SharePseudoHandle) {
+  TestRunner runner;
+  auto* policy = runner.GetPolicy();
+  EXPECT_DEATH(policy->AddHandleToShare(::GetCurrentThread()), "");
+}
+
 }  // namespace sandbox

sandbox/win/src/sandbox_policy_base.cc

@@ -13,10 +13,10 @@
 #include <stdint.h>
 
 #include <memory>
+#include <optional>
 #include <utility>
 #include <vector>
 
-#include <optional>
 #include "base/containers/span.h"
 #include "base/functional/callback.h"
 #include "base/logging.h"
@@ -24,6 +24,7 @@
 #include "base/win/access_token.h"
 #include "base/win/sid.h"
 #include "base/win/win_util.h"
+#include "base/win/windows_handle_util.h"
 #include "base/win/windows_version.h"
 #include "sandbox/features.h"
 #include "sandbox/win/src/acl.h"
@@ -73,10 +74,12 @@ sandbox::PolicyGlobal* MakeBrokerPolicyMemory() {
 }
 
 bool IsInheritableHandle(HANDLE handle) {
-  if (!handle)
+  if (!handle) {
     return false;
-  if (handle == INVALID_HANDLE_VALUE)
+  }
+  if (base::win::IsPseudoHandle(handle)) {
     return false;
+  }
   // File handles (FILE_TYPE_DISK) and pipe handles are known to be
   // inheritable.  Console handles (FILE_TYPE_CHAR) are not
   // inheritable via PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
@@ -469,8 +472,8 @@ PolicyBase::PolicyBase(std::string_view tag)
     : tag_(tag),
       config_(),
       config_ptr_(nullptr),
-      stdout_handle_(INVALID_HANDLE_VALUE),
-      stderr_handle_(INVALID_HANDLE_VALUE),
+      stdout_handle_(nullptr),
+      stderr_handle_(nullptr),
       delegate_data_(nullptr),
       dispatcher_(nullptr),
       job_() {}
@@ -528,7 +531,7 @@ ResultCode PolicyBase::SetStderrHandle(HANDLE handle) {
 
 void PolicyBase::AddHandleToShare(HANDLE handle) {
   CHECK(handle);
-  CHECK_NE(handle, INVALID_HANDLE_VALUE);
+  CHECK(!base::win::IsPseudoHandle(handle));
 
   // Ensure the handle can be inherited.
   bool result =

sandbox/win/src/startup_information_helper.cc

@@ -17,6 +17,7 @@
 #include "base/check.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/win/startup_information.h"
+#include "base/win/windows_handle_util.h"
 #include "base/win/windows_version.h"
 #include "sandbox/win/src/app_container.h"
 #include "sandbox/win/src/nt_internals.h"
@@ -64,7 +65,11 @@ void StartupInformationHelper::SetStdHandles(HANDLE stdout_handle,
 }
 
 void StartupInformationHelper::AddInheritedHandle(HANDLE handle) {
-  if (handle != INVALID_HANDLE_VALUE) {
+  // https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute
+  // "These handles must be created as inheritable handles and must not include
+  // pseudo handles such as those returned by the GetCurrentProcess or
+  // GetCurrentThread function."
+  if (handle && !base::win::IsPseudoHandle(handle)) {
     auto it = std::ranges::find(inherited_handle_list_, handle);
     if (it == inherited_handle_list_.end())
       inherited_handle_list_.push_back(handle);
@@ -161,7 +166,7 @@ bool StartupInformationHelper::BuildStartupInformation() {
       return false;
     }
     startup_info_.startup_info()->dwFlags |= STARTF_USESTDHANDLES;
-    startup_info_.startup_info()->hStdInput = INVALID_HANDLE_VALUE;
+    startup_info_.startup_info()->hStdInput = nullptr;
     startup_info_.startup_info()->hStdOutput = stdout_handle_;
     startup_info_.startup_info()->hStdError = stderr_handle_;
     // Allowing inheritance of handles is only secure now that we

sandbox/win/src/startup_information_helper.h

@@ -78,8 +78,8 @@ class StartupInformationHelper {
   // This can only be true if security_capabilities_ is also initialized.
   bool enable_low_privilege_app_container_ = false;
   bool restrict_child_process_creation_ = false;
-  HANDLE stdout_handle_ = INVALID_HANDLE_VALUE;
-  HANDLE stderr_handle_ = INVALID_HANDLE_VALUE;
+  HANDLE stdout_handle_ = nullptr;
+  HANDLE stderr_handle_ = nullptr;
   bool inherit_handles_ = false;
   bool filter_environment_ = false;
   size_t mitigations_size_ = 0;