Bug 1851889 - Undo temporary diagnostics code for prespawn CIG. r=bobowen

This patch removes the temporary diagnostics code that was added
to the chromium sandbox in bug 1869805 to investigate bug 1851889.

Differential Revision: https://phabricator.services.mozilla.com/D197422

security/sandbox/chromium/sandbox/win/src/signed_dispatcher.cc

@@ -6,7 +6,6 @@
 
 #include <stdint.h>
 
-#include <cstdio>
 #include <string>
 
 #include "base/strings/string_util.h"
@@ -44,26 +43,13 @@ bool SignedDispatcher::CreateSection(IPCInfo* ipc, HANDLE file_handle) {
   if (!::DuplicateHandle((*ipc->client_info).process, file_handle,
                          ::GetCurrentProcess(), &local_file_handle,
                          FILE_MAP_EXECUTE, false, 0)) {
-#if defined(DEBUG)
-    fwprintf(stderr,
-             L"Prespawn CIG: SignedDispatcher::CreateSection did not complete "
-             L"(!::DuplicateHandle) (pid=%u)\n",
-             (*ipc->client_info).process_id);
-#endif  // DEBUG
     return false;
   }
 
   base::win::ScopedHandle local_handle(local_file_handle);
   std::wstring path;
-  if (!GetPathFromHandle(local_handle.Get(), &path)) {
-#if defined(DEBUG)
-    fwprintf(stderr,
-             L"Prespawn CIG: SignedDispatcher::CreateSection did not complete "
-             L"(!::GetPathFromHandle) (pid=%u)\n",
-             (*ipc->client_info).process_id);
-#endif  // DEBUG
+  if (!GetPathFromHandle(local_handle.Get(), &path))
     return false;
-  }
   const wchar_t* module_name = path.c_str();
   CountedParameterSet<NameBased> params;
   params[NameBased::NAME] = ParamPickerMake(module_name);
@@ -76,14 +62,6 @@ bool SignedDispatcher::CreateSection(IPCInfo* ipc, HANDLE file_handle) {
   ipc->return_info.nt_status = SignedPolicy::CreateSectionAction(
       result, *ipc->client_info, local_handle, &section_handle);
   ipc->return_info.handle = section_handle;
-
-#if defined(DEBUG)
-  fwprintf(stderr,
-           L"Prespawn CIG: SignedDispatcher::CreateSection completed (pid=%u, "
-           L"path=%s, result=0x%x, nt_status=0x%x)\n",
-           (*ipc->client_info).process_id, module_name, result,
-           ipc->return_info.nt_status);
-#endif  // DEBUG
   return true;
 }
 

security/sandbox/chromium/sandbox/win/src/signed_interception.cc

@@ -14,27 +14,8 @@
 #include "sandbox/win/src/sandbox_nt_util.h"
 #include "sandbox/win/src/sharedmem_ipc_client.h"
 #include "sandbox/win/src/target_services.h"
-#include "mozilla/Assertions.h"
 #include "mozilla/sandboxing/sandboxLogging.h"
 
-#if defined(DEBUG)
-enum class BreakReason {
-  None = 0,
-  DesiredAccess = 1,
-  ObjectAttributes = 2,
-  MaximumSize = 3,
-  SectionPageProtection = 3,
-  AllocationAttributes = 4,
-  Memory = 5,
-  GetPath = 6,
-  QueryBroker = 7,
-  ValidParameter = 8,
-  ResultCode = 9,
-  NtStatus = 10,
-  Exception = 11,
-};
-#endif  // DEBUG
-
 namespace sandbox {
 
 NTSTATUS WINAPI
@@ -46,104 +27,45 @@ TargetNtCreateSection(NtCreateSectionFunction orig_CreateSection,
                       ULONG section_page_protection,
                       ULONG allocation_attributes,
                       HANDLE file_handle) {
-#if defined(DEBUG)
-  BreakReason break_reason = BreakReason::None;
-  union {
-    NTSTATUS nt_status;
-    ResultCode result_code;
-    DWORD exc_code;
-  } faulty_value;
-  wchar_t dll_path_local_copy[MAX_PATH]{};
-  faulty_value.exc_code = 0;
-#endif  // DEBUG
   do {
     // The section only needs to have SECTION_MAP_EXECUTE, but the permissions
     // vary depending on the OS. Windows 1903 and higher requests (SECTION_QUERY
     // | SECTION_MAP_READ | SECTION_MAP_EXECUTE) while previous OS versions also
     // request SECTION_MAP_WRITE. Just check for EXECUTE.
-    if (!(desired_access & SECTION_MAP_EXECUTE)) {
-#if defined(DEBUG)
-      break_reason = BreakReason::DesiredAccess;
-#endif  // DEBUG
+    if (!(desired_access & SECTION_MAP_EXECUTE))
       break;
-    }
-    if (object_attributes) {
-#if defined(DEBUG)
-      break_reason = BreakReason::ObjectAttributes;
-#endif  // DEBUG
+    if (object_attributes)
       break;
-    }
-    if (maximum_size) {
-#if defined(DEBUG)
-      break_reason = BreakReason::MaximumSize;
-#endif  // DEBUG
+    if (maximum_size)
       break;
-    }
-    if (section_page_protection != PAGE_EXECUTE) {
-#if defined(DEBUG)
-      break_reason = BreakReason::SectionPageProtection;
-#endif  // DEBUG
+    if (section_page_protection != PAGE_EXECUTE)
       break;
-    }
-    if (allocation_attributes != SEC_IMAGE) {
-#if defined(DEBUG)
-      break_reason = BreakReason::AllocationAttributes;
-#endif  // DEBUG
+    if (allocation_attributes != SEC_IMAGE)
       break;
-    }
 
     mozilla::sandboxing::LogBlocked("NtCreateSection");
 
     // IPC must be fully started.
     void* memory = GetGlobalIPCMemory();
-    if (!memory) {
-#if defined(DEBUG)
-      break_reason = BreakReason::Memory;
-#endif  // DEBUG
+    if (!memory)
       break;
-    }
 
     std::unique_ptr<wchar_t, NtAllocDeleter> path;
 
-    if (!NtGetPathFromHandle(file_handle, &path)) {
-#if defined(DEBUG)
-      break_reason = BreakReason::GetPath;
-#endif  // DEBUG
+    if (!NtGetPathFromHandle(file_handle, &path))
       break;
-    }
 
     const wchar_t* const_name = path.get();
 
-#if defined(DEBUG)
-    // Do a suboptimal wide string copy. This function is too low-level to
-    // depend on anything that lives in a DLL.
-    for (int i = 0; i < MAX_PATH - 1; ++i) {
-      wchar_t c = const_name[i];
-      dll_path_local_copy[i] = c;
-      if (!c) {
-        break;
-      }
-    }
-    dll_path_local_copy[MAX_PATH - 1] = 0;
-#endif  // DEBUG
-
     CountedParameterSet<NameBased> params;
     params[NameBased::NAME] = ParamPickerMake(const_name);
 
     // Check if this will be sent to the broker.
-    if (!QueryBroker(IpcTag::NTCREATESECTION, params.GetBase())) {
-#if defined(DEBUG)
-      break_reason = BreakReason::QueryBroker;
-#endif  // DEBUG
+    if (!QueryBroker(IpcTag::NTCREATESECTION, params.GetBase()))
       break;
-    }
 
-    if (!ValidParameter(section_handle, sizeof(HANDLE), WRITE)) {
-#if defined(DEBUG)
-      break_reason = BreakReason::ValidParameter;
-#endif  // DEBUG
+    if (!ValidParameter(section_handle, sizeof(HANDLE), WRITE))
       break;
-    }
 
     CrossCallReturn answer = {0};
     answer.nt_status = STATUS_INVALID_IMAGE_HASH;
@@ -151,49 +73,25 @@ TargetNtCreateSection(NtCreateSectionFunction orig_CreateSection,
     ResultCode code =
         CrossCall(ipc, IpcTag::NTCREATESECTION, file_handle, &answer);
 
-    if (code != SBOX_ALL_OK) {
-#if defined(DEBUG)
-      break_reason = BreakReason::ResultCode;
-      faulty_value.result_code = code;
-#endif  // DEBUG
+    if (code != SBOX_ALL_OK)
       break;
-    }
 
-    if (!NT_SUCCESS(answer.nt_status)) {
-#if defined(DEBUG)
-      break_reason = BreakReason::NtStatus;
-      faulty_value.nt_status = answer.nt_status;
-#endif  // DEBUG
+    if (!NT_SUCCESS(answer.nt_status))
       break;
-    }
 
     __try {
       *section_handle = answer.handle;
       mozilla::sandboxing::LogAllowed("NtCreateSection");
       return answer.nt_status;
     } __except (EXCEPTION_EXECUTE_HANDLER) {
-#if defined(DEBUG)
-      break_reason = BreakReason::Exception;
-      faulty_value.exc_code = GetExceptionCode();
-#endif  // DEBUG
       break;
     }
   } while (false);
 
   // Fall back to the original API in all failure cases.
-  NTSTATUS local_nt_status = orig_CreateSection(
-      section_handle, desired_access, object_attributes, maximum_size,
-      section_page_protection, allocation_attributes, file_handle);
-#if defined(DEBUG)
-  if (local_nt_status == STATUS_INVALID_IMAGE_HASH) {
-    MOZ_CRASH_UNSAFE_PRINTF(
-        "Prespawn CIG: STATUS_INVALID_IMAGE_HASH for %S (break_reason: %d, "
-        "faulty_value: 0x%lx)\n",
-        dll_path_local_copy, static_cast<int>(break_reason),
-        faulty_value.exc_code);
-  }
-#endif  // DEBUG
-  return local_nt_status;
+  return orig_CreateSection(section_handle, desired_access, object_attributes,
+                            maximum_size, section_page_protection,
+                            allocation_attributes, file_handle);
 }
 
 }  // namespace sandbox

security/sandbox/chromium/sandbox/win/src/signed_policy.cc

@@ -6,7 +6,6 @@
 
 #include <stdint.h>
 
-#include <cstdio>
 #include <string>
 
 #include "sandbox/win/src/ipc_tags.h"
@@ -79,17 +78,8 @@ NTSTATUS SignedPolicy::CreateSectionAction(
 
   // The only action supported is ASK_BROKER which means create the requested
   // section as specified.
-  if (ASK_BROKER != eval_result) {
-#if defined(DEBUG)
-    fwprintf(stderr,
-             L"Prespawn CIG: SignedPolicy::CreateSectionAction failure "
-             L"(ASK_BROKER != eval_result) (pid=%u)\n",
-             client_info.process_id);
-#endif  // DEBUG
-    // (This should be an error NTSTATUS, not a boolean that converts to
-    // STATUS_SUCCESS, but let's keep the code as it is for now.)
+  if (ASK_BROKER != eval_result)
     return false;
-  }
 
   HANDLE local_section_handle = nullptr;
   NTSTATUS status = NtCreateSection(&local_section_handle,
@@ -97,26 +87,13 @@ NTSTATUS SignedPolicy::CreateSectionAction(
                                         SECTION_MAP_READ | SECTION_MAP_EXECUTE,
                                     nullptr, 0, PAGE_EXECUTE, SEC_IMAGE,
                                     local_file_handle.Get());
-  if (!local_section_handle) {
-#if defined(DEBUG)
-    fwprintf(stderr,
-             L"Prespawn CIG: SignedPolicy::CreateSectionAction failure "
-             L"(!local_section_handle) (pid=%u)\n",
-             client_info.process_id);
-#endif  // DEBUG
+  if (!local_section_handle)
     return status;
-  }
 
   // Duplicate section handle back to the target.
   if (!::DuplicateHandle(::GetCurrentProcess(), local_section_handle,
                          client_info.process, target_section_handle, 0, false,
                          DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
-#if defined(DEBUG)
-    fwprintf(stderr,
-             L"Prespawn CIG: SignedPolicy::CreateSectionAction failure "
-             L"(!DuplicateHandle) (pid=%u)\n",
-             client_info.process_id);
-#endif  // DEBUG
     return STATUS_ACCESS_DENIED;
   }
   return status;

security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp

@@ -10,7 +10,6 @@
 
 #include <aclapi.h>
 #include <shlobj.h>
-#include <cstdio>
 #include <string>
 
 #include "base/win/windows_version.h"
@@ -464,18 +463,11 @@ static const Maybe<Vector<const wchar_t*>>& GetPrespawnCigExceptionModules() {
     RefPtr<DllServices> dllSvc(DllServices::Get());
     auto sharedSection = dllSvc->GetSharedSection();
     if (!sharedSection) {
-#  if defined(DEBUG)
-      fwprintf(stderr, L"Prespawn CIG: disabled (!sharedSection).\n");
-#  endif  // DEBUG
       return Nothing();
     }
 
     Span<const wchar_t> dependentModules = sharedSection->GetDependentModules();
     if (dependentModules.IsEmpty()) {
-#  if defined(DEBUG)
-      fwprintf(stderr,
-               L"Prespawn CIG: disabled (dependentModules.IsEmpty()).\n");
-#  endif  // DEBUG
       return Nothing();
     }
 
@@ -492,18 +484,6 @@ static const Maybe<Vector<const wchar_t*>>& GetPrespawnCigExceptionModules() {
       ++p;
     }
 
-#  if defined(DEBUG)
-    fwprintf(stderr,
-             L"Prespawn CIG: enabled (%zd injected dependent modules).\n",
-             paths.length());
-    for (const auto& path : paths) {
-      fwprintf(
-          stderr,
-          L"Prespawn CIG: %s identified as an injected dependent module.\n",
-          path);
-    }
-#  endif  // DEBUG
-
     return Some(std::move(paths));
   }();