Disable the NtMapViewOfSection hook when printing is initiated
Due to an issue with printing drivers possibly loading third-party DLLs
that mustn't be blocked, initiating a printing operation now disables
the hook in chrome_elf for the remainder of the process' lifetime.
This is a short term solution to allow us to ship third-party blocking.
Bug: 809738, 892294
Change-Id: I0b583197c2b619226e5a4a05836451b9f30eb133
Reviewed-on: https://chromium-review.googlesource.com/c/1312166
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610672}diff --git a/chrome/browser/conflicts/module_database_win.cc b/chrome/browser/conflicts/module_database_win.cc
index d00e1c5..145f969 100644
--- a/chrome/browser/conflicts/module_database_win.cc
+++ b/chrome/browser/conflicts/module_database_win.cc
@@ -11,6 +11,7 @@
#include "base/files/file_path.h"
#include "base/location.h"
#include "chrome/browser/conflicts/module_database_observer_win.h"
+#include "content/public/browser/browser_task_traits.h"
#if defined(GOOGLE_CHROME_BUILD)
#include "base/feature_list.h"
@@ -22,6 +23,7 @@
#include "chrome/browser/conflicts/third_party_conflicts_manager_win.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/pref_names.h"
+#include "chrome_elf/third_party_dlls/public_api.h"
#include "components/prefs/pref_change_registrar.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
@@ -263,6 +265,20 @@
return !third_party_blocking_enabled_pref->IsManaged() ||
third_party_blocking_enabled_pref->GetValue()->GetBool();
}
+
+// static
+void ModuleDatabase::DisableThirdPartyBlocking() {
+ // Immediately disable the hook. DisableHook() can be called concurrently.
+ DisableHook();
+
+ // Notify the ThirdPartyMetricsRecorder instance that the hook is disabled.
+ // Since this is meant for a heartbeat metric, the small latency introduced
+ // with the thread-hopping is perfectly acceptable.
+ base::PostTaskWithTraits(
+ FROM_HERE, {content::BrowserThread::UI}, base::BindOnce([]() {
+ ModuleDatabase::GetInstance()->third_party_metrics_.SetHookDisabled();
+ }));
+}
#endif // defined(GOOGLE_CHROME_BUILD)
// static
diff --git a/chrome/browser/conflicts/module_database_win.h b/chrome/browser/conflicts/module_database_win.h
index 70e4c37..94b87ee5 100644
--- a/chrome/browser/conflicts/module_database_win.h
+++ b/chrome/browser/conflicts/module_database_win.h
@@ -140,6 +140,15 @@
// administrative policy.
static bool IsThirdPartyBlockingPolicyEnabled();
+ // Disables the blocking of third-party modules in the browser process. It is
+ // safe to invoke this function from any thread.
+ // This function is meant to be used only as a workaround for the in-process
+ // printing code that may require that third-party DLLs be successfully
+ // loaded into the process to work correctly.
+ // TODO(pmonette): Remove this workaround when printing is moved to a utility
+ // process. See https://crbug.com/809738.
+ static void DisableThirdPartyBlocking();
+
// Accessor for the third party conflicts manager.
// Returns null if both the tracking of incompatible applications and the
// blocking of third-party modules are disabled.
diff --git a/chrome/browser/conflicts/module_load_attempt_log_listener_win.cc b/chrome/browser/conflicts/module_load_attempt_log_listener_win.cc
index 1fd45e5..8c1f504 100644
--- a/chrome/browser/conflicts/module_load_attempt_log_listener_win.cc
+++ b/chrome/browser/conflicts/module_load_attempt_log_listener_win.cc
@@ -18,8 +18,8 @@
#include "base/task/post_task.h"
#include "base/task_runner_util.h"
#include "chrome/browser/conflicts/module_blacklist_cache_util_win.h"
-#include "chrome_elf/third_party_dlls/logging_api.h"
#include "chrome_elf/third_party_dlls/packed_list_format.h"
+#include "chrome_elf/third_party_dlls/public_api.h"
namespace {
diff --git a/chrome/browser/conflicts/third_party_metrics_recorder_win.cc b/chrome/browser/conflicts/third_party_metrics_recorder_win.cc
index 5a67861..d4d1f48 100644
--- a/chrome/browser/conflicts/third_party_metrics_recorder_win.cc
+++ b/chrome/browser/conflicts/third_party_metrics_recorder_win.cc
@@ -18,11 +18,10 @@
#include "components/crash/core/common/crash_key.h"
#if defined(GOOGLE_CHROME_BUILD)
-#include "chrome_elf/third_party_dlls/logging_api.h"
+#include "chrome_elf/third_party_dlls/public_api.h"
#endif
namespace {
-
// Returns true if the module is signed by Google.
bool IsGoogleModule(base::StringPiece16 subject) {
static const wchar_t kGoogle[] = L"Google Inc";
@@ -157,15 +156,23 @@
"ThirdPartyModules.Heartbeat.UniqueBlockedModulesCount",
GetUniqueBlockedModulesCount());
- uint32_t blocked_modules_count = GetBlockedModulesCount();
- UMA_HISTOGRAM_COUNTS_1M("ThirdPartyModules.Heartbeat.BlockedModulesCount",
- blocked_modules_count);
+ if (record_blocked_modules_count_) {
+ uint32_t blocked_modules_count = GetBlockedModulesCount();
+ UMA_HISTOGRAM_COUNTS_1M("ThirdPartyModules.Heartbeat.BlockedModulesCount",
+ blocked_modules_count);
- // Stop recording when |blocked_modules_count| gets too high. This is to avoid
- // dealing with the possible integer overflow that would result in emitting
- // wrong values. The exact cutoff point is not important but it must be higher
- // than the max value for the histogram (1M in this case).
- if (blocked_modules_count > std::numeric_limits<uint32_t>::max() / 2)
- heartbeat_metrics_timer_.Reset();
+ // Stop recording when |blocked_modules_count| gets too high. This is to
+ // avoid dealing with the possible integer overflow that would result in
+ // emitting wrong values. The exact cutoff point is not important but it
+ // must be higher than the max value for the histogram (1M in this case).
+ // It's ok to continue logging the count of unique blocked modules because
+ // there's no expectation that this count can reach a high value.
+ if (blocked_modules_count > std::numeric_limits<uint32_t>::max() / 2)
+ record_blocked_modules_count_ = false;
+ }
+
+ UMA_HISTOGRAM_BOOLEAN(
+ "ThirdPartyModules.Heartbeat.PrintingWorkaround.BlockingEnabled",
+ hook_enabled_);
}
#endif
diff --git a/chrome/browser/conflicts/third_party_metrics_recorder_win.h b/chrome/browser/conflicts/third_party_metrics_recorder_win.h
index f367f39a..7507671 100644
--- a/chrome/browser/conflicts/third_party_metrics_recorder_win.h
+++ b/chrome/browser/conflicts/third_party_metrics_recorder_win.h
@@ -29,6 +29,10 @@
const ModuleInfoData& module_data) override;
void OnModuleDatabaseIdle() override;
+#if defined(GOOGLE_CHROME_BUILD)
+ void SetHookDisabled() { hook_enabled_ = false; }
+#endif
+
private:
// The size of the unsigned modules crash keys.
static constexpr size_t kCrashKeySize = 256;
@@ -47,6 +51,15 @@
// Timer that controls when heartbeat metrics are recorded.
base::RepeatingTimer heartbeat_metrics_timer_;
+
+ // Indicates if the ThirdPartyModules.Heartbeat.BlockedModulesCount heartbeat
+ // metric is being recorded.
+ bool record_blocked_modules_count_ = true;
+
+ // Indicates if the blocking of third-party DLLs is still enabled or if it
+ // was disabled because in-process printing was invoked.
+ // See ModuleDatabase::DisableThirdPartyBlocking().
+ bool hook_enabled_ = true;
#endif
// The index of the crash key that is currently being updated.
diff --git a/chrome/browser/printing/DEPS b/chrome/browser/printing/DEPS
index f5a9247..43cad55e 100644
--- a/chrome/browser/printing/DEPS
+++ b/chrome/browser/printing/DEPS
@@ -1,3 +1,4 @@
include_rules = [
+ "+chrome_elf/third_party_dlls/public_api.h",
"+pdf/pdf.h", # Test only.
]
diff --git a/chrome/browser/printing/print_preview_dialog_controller.cc b/chrome/browser/printing/print_preview_dialog_controller.cc
index 2b7cd45..2283ce8c 100644
--- a/chrome/browser/printing/print_preview_dialog_controller.cc
+++ b/chrome/browser/printing/print_preview_dialog_controller.cc
@@ -43,6 +43,10 @@
#include "ui/base/l10n/l10n_util.h"
#include "ui/web_dialogs/web_dialog_delegate.h"
+#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
+#include "chrome/browser/conflicts/module_database_win.h"
+#endif
+
using content::NavigationController;
using content::WebContents;
using content::WebUIMessageHandler;
@@ -166,6 +170,10 @@
// static
void PrintPreviewDialogController::PrintPreview(WebContents* initiator) {
+#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
+ ModuleDatabase::GetInstance()->DisableThirdPartyBlocking();
+#endif
+
if (initiator->ShowingInterstitialPage() || initiator->IsCrashed())
return;
diff --git a/chrome/browser/printing/printing_message_filter.cc b/chrome/browser/printing/printing_message_filter.cc
index b21042e..e415483 100644
--- a/chrome/browser/printing/printing_message_filter.cc
+++ b/chrome/browser/printing/printing_message_filter.cc
@@ -37,6 +37,10 @@
#include "chrome/browser/printing/print_view_manager_basic.h"
#endif
+#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
+#include "chrome/browser/conflicts/module_database_win.h"
+#endif
+
using content::BrowserThread;
namespace printing {
@@ -237,6 +241,10 @@
void PrintingMessageFilter::OnScriptedPrint(
const PrintHostMsg_ScriptedPrint_Params& params,
IPC::Message* reply_msg) {
+#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
+ ModuleDatabase::GetInstance()->DisableThirdPartyBlocking();
+#endif
+
scoped_refptr<PrinterQuery> printer_query =
queue_->PopPrinterQuery(params.cookie);
if (!printer_query.get()) {
diff --git a/chrome_elf/BUILD.gn b/chrome_elf/BUILD.gn
index 7f780b5..61efc526 100644
--- a/chrome_elf/BUILD.gn
+++ b/chrome_elf/BUILD.gn
@@ -256,10 +256,10 @@
source_set("third_party_shared_defines") {
sources = [
"sha1/sha1.h",
- "third_party_dlls/logging_api.cc",
- "third_party_dlls/logging_api.h",
"third_party_dlls/packed_list_format.cc",
"third_party_dlls/packed_list_format.h",
+ "third_party_dlls/public_api.cc",
+ "third_party_dlls/public_api.h",
"third_party_dlls/status_codes.cc",
"third_party_dlls/status_codes.h",
]
diff --git a/chrome_elf/chrome_elf_test_stubs.cc b/chrome_elf/chrome_elf_test_stubs.cc
index 23b1120..f163dab 100644
--- a/chrome_elf/chrome_elf_test_stubs.cc
+++ b/chrome_elf/chrome_elf_test_stubs.cc
@@ -10,7 +10,7 @@
#include "base/win/windows_types.h"
#include "chrome/common/chrome_switches.h"
#include "chrome_elf/chrome_elf_main.h"
-#include "chrome_elf/third_party_dlls/logging_api.h"
+#include "chrome_elf/third_party_dlls/public_api.h"
// This function is a temporary workaround for https://crbug.com/655788. We
// need to come up with a better way to initialize crash reporting that can
@@ -43,7 +43,7 @@
//------------------------------------------------------------------------------
// chrome_elf\third_party_dlls export test stubs.
// - For use by \\chrome\browser\conflicts\* testing.
-// - Stubs should shadow third_party_dlls\logging_api.h and logs_unittest.cc.
+// - Stubs should shadow third_party_dlls\public_api.h and logs_unittest.cc.
//------------------------------------------------------------------------------
struct TestLogEntry {
@@ -104,3 +104,5 @@
uint32_t GetUniqueBlockedModulesCount() {
return 0;
}
+
+void DisableHook() {}
diff --git a/chrome_elf/chrome_elf_x64.def b/chrome_elf/chrome_elf_x64.def
index 1ba7006..19c0eb2 100644
--- a/chrome_elf/chrome_elf_x64.def
+++ b/chrome_elf/chrome_elf_x64.def
@@ -42,3 +42,6 @@
RegisterLogNotification
GetBlockedModulesCount
GetUniqueBlockedModulesCount
+
+ ; From chrome_elf/third_party_dlls/hook.cc
+ DisableHook
diff --git a/chrome_elf/chrome_elf_x86.def b/chrome_elf/chrome_elf_x86.def
index 9145a482..d9c900b 100644
--- a/chrome_elf/chrome_elf_x86.def
+++ b/chrome_elf/chrome_elf_x86.def
@@ -38,3 +38,6 @@
RegisterLogNotification
GetBlockedModulesCount
GetUniqueBlockedModulesCount
+
+ ; From chrome_elf/third_party_dlls/hook.cc
+ DisableHook
diff --git a/chrome_elf/third_party_dlls/hook.cc b/chrome_elf/third_party_dlls/hook.cc
index 68db6ab2..5d3ac75 100644
--- a/chrome_elf/third_party_dlls/hook.cc
+++ b/chrome_elf/third_party_dlls/hook.cc
@@ -4,6 +4,7 @@
#include "chrome_elf/third_party_dlls/hook.h"
+#include <atomic>
#include <limits>
#include <memory>
@@ -18,6 +19,7 @@
#include "chrome_elf/third_party_dlls/main.h"
#include "chrome_elf/third_party_dlls/packed_list_file.h"
#include "chrome_elf/third_party_dlls/packed_list_format.h"
+#include "chrome_elf/third_party_dlls/public_api.h"
#include "sandbox/win/src/interception_internal.h"
#include "sandbox/win/src/internal_types.h"
#include "sandbox/win/src/nt_internals.h"
@@ -49,6 +51,9 @@
// Set if and when ApplyHook() has been successfully executed.
bool g_hook_active = false;
+// Indicates if the hook was disabled after the hook was activated.
+std::atomic<bool> g_hook_disabled(false);
+
// Set up NTDLL function pointers.
bool InitImports() {
HMODULE ntdll = ::GetModuleHandleW(sandbox::kNtdllName);
@@ -267,6 +272,9 @@
commit_size, offset, view_size, inherit,
allocation_type, protect);
+ if (g_hook_disabled.load(std::memory_order_relaxed))
+ return ret;
+
// If there was an OS-level failure, if the mapping target is NOT this
// process, or if the section is not a (valid) Portable Executable,
// we're not interested. Return the OS-level result code.
@@ -485,3 +493,9 @@
}
} // namespace third_party_dlls
+
+using namespace third_party_dlls;
+
+void DisableHook() {
+ g_hook_disabled.store(true, std::memory_order_relaxed);
+}
diff --git a/chrome_elf/third_party_dlls/logs.h b/chrome_elf/third_party_dlls/logs.h
index 48bb657..f6e2704 100644
--- a/chrome_elf/third_party_dlls/logs.h
+++ b/chrome_elf/third_party_dlls/logs.h
@@ -11,7 +11,7 @@
#include <string>
-#include "chrome_elf/third_party_dlls/logging_api.h"
+#include "chrome_elf/third_party_dlls/public_api.h"
#include "chrome_elf/third_party_dlls/status_codes.h"
namespace third_party_dlls {
diff --git a/chrome_elf/third_party_dlls/logs_unittest.cc b/chrome_elf/third_party_dlls/logs_unittest.cc
index 3aea400..23ddfd0 100644
--- a/chrome_elf/third_party_dlls/logs_unittest.cc
+++ b/chrome_elf/third_party_dlls/logs_unittest.cc
@@ -14,7 +14,7 @@
#include "base/synchronization/waitable_event.h"
#include "base/time/time.h"
#include "chrome_elf/sha1/sha1.h"
-#include "chrome_elf/third_party_dlls/logging_api.h"
+#include "chrome_elf/third_party_dlls/public_api.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace third_party_dlls {
diff --git a/chrome_elf/third_party_dlls/main_unittest_exe.cc b/chrome_elf/third_party_dlls/main_unittest_exe.cc
index f2acb80..e33ac26 100644
--- a/chrome_elf/third_party_dlls/main_unittest_exe.cc
+++ b/chrome_elf/third_party_dlls/main_unittest_exe.cc
@@ -17,9 +17,9 @@
#include "chrome/install_static/install_util.h"
#include "chrome/install_static/product_install_details.h"
#include "chrome_elf/nt_registry/nt_registry.h"
-#include "chrome_elf/third_party_dlls/logging_api.h"
#include "chrome_elf/third_party_dlls/main.h"
#include "chrome_elf/third_party_dlls/packed_list_file.h"
+#include "chrome_elf/third_party_dlls/public_api.h"
using namespace third_party_dlls::main_unittest_exe;
diff --git a/chrome_elf/third_party_dlls/logging_api.cc b/chrome_elf/third_party_dlls/public_api.cc
similarity index 91%
rename from chrome_elf/third_party_dlls/logging_api.cc
rename to chrome_elf/third_party_dlls/public_api.cc
index 107bca6a..8a912b4 100644
--- a/chrome_elf/third_party_dlls/logging_api.cc
+++ b/chrome_elf/third_party_dlls/public_api.cc
@@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include "chrome_elf/third_party_dlls/logging_api.h"
+#include "chrome_elf/third_party_dlls/public_api.h"
#include <stddef.h>
diff --git a/chrome_elf/third_party_dlls/logging_api.h b/chrome_elf/third_party_dlls/public_api.h
similarity index 88%
rename from chrome_elf/third_party_dlls/logging_api.h
rename to chrome_elf/third_party_dlls/public_api.h
index 3a1778b..a2b2e01b 100644
--- a/chrome_elf/third_party_dlls/logging_api.h
+++ b/chrome_elf/third_party_dlls/public_api.h
@@ -6,9 +6,10 @@
// 1. The packed-data format used to pass information from chrome.dll
// to chrome_elf.dll across restarts.
// 2. The APIs exported by chrome_elf.dll to share logs of module load attempts.
+// 3. The API used to disable the NtMapViewOfSection hook.
-#ifndef CHROME_ELF_THIRD_PARTY_DLLS_LOGGING_API_H_
-#define CHROME_ELF_THIRD_PARTY_DLLS_LOGGING_API_H_
+#ifndef CHROME_ELF_THIRD_PARTY_DLLS_PUBLIC_API_H_
+#define CHROME_ELF_THIRD_PARTY_DLLS_PUBLIC_API_H_
#include <windows.h>
@@ -89,4 +90,8 @@
// Returns the number of unique modules that have been blocked.
extern "C" uint32_t GetUniqueBlockedModulesCount();
-#endif // CHROME_ELF_THIRD_PARTY_DLLS_LOGGING_API_H_
+// Disables the hook for NtMapViewOfSection. This function does not remove the
+// hook but merely makes the hook forward the call to the original function.
+extern "C" void DisableHook();
+
+#endif // CHROME_ELF_THIRD_PARTY_DLLS_PUBLIC_API_H_
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index c6312e1..161ced4 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -113745,6 +113745,17 @@
</summary>
</histogram>
+<histogram
+ name="ThirdPartyModules.Heartbeat.PrintingWorkaround.BlockingEnabled"
+ enum="BooleanEnabled">
+ <owner>pmonette@chromium.org</owner>
+ <summary>
+ Records whether or not Chrome is still blocking third-party DLLs. This is a
+ bit that turns to false when the in-process printing is invoked. Recorded
+ every 5 minutes.
+ </summary>
+</histogram>
+
<histogram name="ThirdPartyModules.Heartbeat.UniqueBlockedModulesCount"
units="modules">
<owner>pmonette@chromium.org</owner>
