Remove logging::SetLogMessageHandler calls from RPH_WriteableFileTest

This removes a dependency on being able to cancel a LOG(FATAL) message
using a log-message handler. Removing these test dependencies are a
prerequisite for making LOG(FATAL) properly [[noreturn]].

This CL instead adds a testing callback for the handle checks, and uses
that in the test to verify the behavior.

BUG=1418101

Change-Id: Ica8ad45135263d0f5c080552f0ab3becf8acc889
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4292117
Commit-Queue: Will Harris <wfh@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/main@{#1110742}

content/browser/renderer_host/render_process_host_browsertest.cc

@@ -72,9 +72,10 @@
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
-#include "base/no_destructor.h"
+#include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
+#include "mojo/public/cpp/platform/platform_handle_security_util_win.h"
 #include "sandbox/policy/switches.h"
 #endif
 
@@ -1937,7 +1938,7 @@ IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, ZeroExecutionTimes) {
   process->Cleanup();
 }
 
-class RenderProcessHostWriteableFileDeathTest
+class RenderProcessHostWriteableFileTest
     : public RenderProcessHostTest,
       public ::testing::WithParamInterface<
           std::tuple</*enforcement_enabled=*/bool,
@@ -1958,7 +1959,12 @@ class RenderProcessHostWriteableFileDeathTest
   base::test::ScopedFeatureList enforcement_feature_;
 };
 
-IN_PROC_BROWSER_TEST_P(RenderProcessHostWriteableFileDeathTest,
+// This test verifies that the renderer process is wired up correctly with the
+// mojo invitation flag that indicates that it's untrusted. The other half of
+// this test that verifies that a security violation actually causes a DCHECK
+// lives in mojo/core, and can't live here as death tests are not supported for
+// browser tests.
+IN_PROC_BROWSER_TEST_P(RenderProcessHostWriteableFileTest,
                        PassUnsafeWriteableExecutableFile) {
   // This test only works if DCHECKs are enabled.
 #if !DCHECK_IS_ON()
@@ -1992,41 +1998,29 @@ IN_PROC_BROWSER_TEST_P(RenderProcessHostWriteableFileDeathTest,
   base::File temp_file_writeable(file_path, flags);
   ASSERT_TRUE(temp_file_writeable.IsValid());
 
-  static base::NoDestructor<std::string> fatal_log_string;
-  // Note: logging::ScopedLogAssertHandler can't be used here as it does not
-  // capture CHECK.
-  auto old_handler = logging::GetLogMessageHandler();
-  logging::SetLogMessageHandler([](int severity, const char* file, int line,
-                                   size_t message_start,
-                                   const std::string& str) -> bool {
-    if (severity != logging::LOGGING_FATAL) {
-      return false;
-    }
-    *fatal_log_string = str;
-    return true;
-  });
+  bool error_was_called = false;
+  mojo::SetUnsafeFileHandleCallbackForTesting(
+      base::BindLambdaForTesting([&error_was_called]() -> bool {
+        error_was_called = true;
+        return true;
+      }));
 
   base::RunLoop run_loop;
   test_service->PassWriteableFile(std::move(temp_file_writeable),
                                   run_loop.QuitClosure());
   run_loop.Run();
-  logging::SetLogMessageHandler(old_handler);
-
-  // This test should only CHECK if enforcement is enabled and the file has not
-  // been marked no-execute correctly.
-  if (IsEnforcementEnabled() && !ShouldMarkNoExecute()) {
-    EXPECT_TRUE(fatal_log_string->find(
-                    "Transfer of writable handle to executable file to an "
-                    "untrusted process") != fatal_log_string->npos);
-  } else {
-    EXPECT_TRUE(fatal_log_string->empty());
-  }
+
+  // This test should only detect a violation if enforcement is enabled and the
+  // file has not been marked no-execute correctly.
+  bool should_violation_occur =
+      IsEnforcementEnabled() && !ShouldMarkNoExecute();
+  EXPECT_EQ(should_violation_occur, error_was_called);
 #endif  // DCHECK_IS_ON()
 }
 
 INSTANTIATE_TEST_SUITE_P(
     All,
-    RenderProcessHostWriteableFileDeathTest,
+    RenderProcessHostWriteableFileTest,
     testing::Combine(/*enforcement_enabled=*/testing::Bool(),
                      /*add_no_execute_flags=*/testing::Bool()));
 

mojo/public/cpp/platform/platform_handle_security_util_win.cc

@@ -12,7 +12,9 @@
 #include "base/debug/stack_trace.h"
 #include "base/feature_list.h"
 #include "base/features.h"
+#include "base/functional/callback_helpers.h"
 #include "base/logging.h"
+#include "base/no_destructor.h"
 #include "base/strings/string_util.h"
 #include "base/win/nt_status.h"
 #include "base/win/scoped_handle.h"
@@ -22,6 +24,11 @@ namespace mojo {
 
 namespace {
 
+FileHandleSecurityErrorCallback& GetErrorCallback() {
+  static base::NoDestructor<FileHandleSecurityErrorCallback> callback;
+  return *callback;
+}
+
 #if DCHECK_IS_ON()
 
 std::wstring GetPathFromHandle(HANDLE handle) {
@@ -99,13 +106,31 @@ void DcheckIfFileHandleIsUnsafe(HANDLE handle) {
       ::CreateFileW(path.c_str(), FILE_READ_DATA | FILE_EXECUTE,
                     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
                     nullptr, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr));
-  CHECK(!file_handle.is_valid())
-      << "Transfer of writable handle to executable "
-         "file to an untrusted process: "
-      << path
-      << ". Please use AddFlagsForPassingToUntrustedProcess or call "
-         "PreventExecuteMapping on the FilePath.";
+
+  // If the file cannot be opened with FILE_EXECUTE it means that it is safe.
+  if (!file_handle.is_valid()) {
+    return;
+  }
+
+  auto& error_callback = GetErrorCallback();
+  if (error_callback) {
+    bool handled = error_callback.Run();
+    if (handled) {
+      return;
+    }
+  }
+
+  DLOG(FATAL) << "Transfer of writable handle to executable file to an "
+                 "untrusted process: "
+              << path
+              << ". Please use AddFlagsForPassingToUntrustedProcess or call "
+                 "PreventExecuteMapping on the FilePath.";
 #endif  // DCHECK_IS_ON();
 }
 
+void SetUnsafeFileHandleCallbackForTesting(
+    FileHandleSecurityErrorCallback callback) {
+  GetErrorCallback() = std::move(callback);
+}
+
 }  // namespace mojo

mojo/public/cpp/platform/platform_handle_security_util_win.h

@@ -6,16 +6,28 @@
 #define MOJO_PUBLIC_CPP_PLATFORM_PLATFORM_HANDLE_SECURITY_UTIL_WIN_H_
 
 #include "base/component_export.h"
+#include "base/functional/callback.h"
 #include "base/win/windows_types.h"
 
 namespace mojo {
 
+using FileHandleSecurityErrorCallback = base::RepeatingCallback<bool()>;
+
 // This function DCHECKs if `handle` is to a writeable file that can be mapped
 // executable. If so, this is a security risk. Does nothing in non-DCHECK
 // builds.
 COMPONENT_EXPORT(MOJO_CPP_PLATFORM)
 void DcheckIfFileHandleIsUnsafe(HANDLE handle);
 
+// Sets a callback for testing that will be called before DCHECKing inside
+// DcheckIfFileHandleIsUnsafe because of an insecure handle. If the callback has
+// been set, and returns true, then the error has been successfully handled and
+// a DCHECK will not happen, otherwise DcheckIfFileHandleIsUnsafe will DCHECK as
+// normal.
+COMPONENT_EXPORT(MOJO_CPP_PLATFORM)
+void SetUnsafeFileHandleCallbackForTesting(
+    FileHandleSecurityErrorCallback callback);
+
 }  // namespace mojo
 
 #endif  // MOJO_PUBLIC_CPP_PLATFORM_PLATFORM_HANDLE_SECURITY_UTIL_WIN_H_