Skip to content

Commit

Permalink
Tracing to diagnose ContentScriptTracker-related bad message reports.
Browse files Browse the repository at this point in the history
This CL adds new TRACE_EVENT directives that should help with diagnosing
ContentScriptTracker-related bad message reports tracked in
https://crbug.com/1212918.

To allow correlating events associated with the same extension, the
traced events contain pseudonymized extension id.  This is calculated
using simple //content/public/common/pseudonymization_util.h introduced
by this CL.

For more details, see the document below (Google-internal because parts
of the Chrometto pipeline and privacy-review process are not public):
https://docs.google.com/document/d/10SCwawcuURNo_D45lltM26hoCJ2V5f5cZwBMXKSBjPk/edit?usp=sharing

This Chromium CL is related to the internal cl/389753453 which
replicates the new pseudonymized_extension_id field into internal
protos.  Chrometto config is contained in the internal cl/387445308.

Bug: 1212918
Change-Id: I4eb2340eba6f7a8b3dd978b616010c1d534ddddd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3057922
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: ssid <ssid@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/main@{#913192}
  • Loading branch information
anforowicz authored and Chromium LUCI CQ committed Aug 18, 2021
1 parent 44b1cef commit 2b8b862
Show file tree
Hide file tree
Showing 33 changed files with 530 additions and 28 deletions.
14 changes: 13 additions & 1 deletion base/tracing/protos/chrome_track_event.proto
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,19 @@ message ChildProcessLauncherPriority {
optional Importance importance = 3;
}

// Information that identifies a Chrome Extension.
message ChromeExtensionId {
// Unique id that identifies a Chrome Extension.
optional string extension_id = 1;

// Pseudonymized `extension_id` field (see also
// content::PseudonymizationUtil::PseudonymizeString method).
optional uint32 pseudonymized_extension_id = 2;
}

message ChromeTrackEvent {
// Extension range for Chrome: 1000-1999
// Next ID: 1018
// Next ID: 1019
extend TrackEvent {
optional ChromeAppState chrome_app_state = 1000;

Expand Down Expand Up @@ -259,5 +269,7 @@ message ChromeTrackEvent {
optional ResourceBundle resource_bundle = 1016;

optional ChromeWebAppBadNavigate chrome_web_app_bad_navigate = 1017;

optional ChromeExtensionId chrome_extension_id = 1018;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class MockChildProcess : public mojom::ChildProcess {
MOCK_METHOD1(BindReceiver, void(mojo::GenericPendingReceiver receiver));
MOCK_METHOD1(EnableSystemTracingService,
void(mojo::PendingRemote<tracing::mojom::SystemTracingService>));
MOCK_METHOD1(SetPseudonymizationSalt, void(uint32_t salt));
};

class ChildProcessTaskPortProviderTest : public testing::Test,
Expand Down
19 changes: 17 additions & 2 deletions content/browser/renderer_host/render_process_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@
#include "content/common/content_constants_internal.h"
#include "content/common/content_switches_internal.h"
#include "content/common/in_process_child_thread_params.h"
#include "content/common/pseudonymization_salt.h"
#include "content/common/service_worker/service_worker_utils.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_or_resource_context.h"
Expand Down Expand Up @@ -2415,6 +2416,9 @@ void RenderProcessHostImpl::DumpProfilingData(base::OnceClosure callback) {
#endif

void RenderProcessHostImpl::WriteIntoTrace(perfetto::TracedValue context) {
// TODO(https://crbug.com/1116471): Add support for writing typed events into
// untyped event contexts and merge the two overloaded methods (the one taking
// perfetto::TracedValue` and the one taking `perfetto::TracedProto<...>`).
auto dict = std::move(context).WriteDictionary();
dict.Add("id", GetID());
dict.Add("process_lock", ChildProcessSecurityPolicyImpl::GetInstance()
Expand All @@ -2436,8 +2440,11 @@ void RenderProcessHostImpl::WriteIntoTrace(
}

// TODO(ssid): Consider moving this to ChildProcessLauncher proto field.
if (child_process_launcher_)
proto->set_child_process_id(child_process_launcher_->GetProcess().Pid());
if (child_process_launcher_) {
const base::Process& process = child_process_launcher_->GetProcess();
if (process.IsValid())
proto->set_child_process_id(process.Pid());
}
}

void RenderProcessHostImpl::RegisterMojoInterfaces() {
Expand Down Expand Up @@ -3795,6 +3802,14 @@ void RenderProcessHostImpl::OnChannelConnected(int32_t peer_pid) {
#if BUILDFLAG(CLANG_PROFILING_INSIDE_SANDBOX)
child_process_->SetProfilingFile(OpenProfilingFile());
#endif

// Propagate the pseudonymization salt to all the child processes.
//
// TODO(dullweber, lukasza): Figure out if it is possible to reset the salt
// at a regular interval (on the order of hours?). The browser would need to
// be responsible for 1) deciding when the refresh happens and 2) pushing the
// updated salt to all the child processes.
child_process_->SetPseudonymizationSalt(GetPseudonymizationSalt());
}

void RenderProcessHostImpl::OnChannelError() {
Expand Down
5 changes: 3 additions & 2 deletions content/browser/renderer_host/render_process_host_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,9 @@ class CONTENT_EXPORT RenderProcessHostImpl
void CleanupNetworkServicePluginExceptionsUponDestruction() override;
std::string GetInfoForBrowserContextDestructionCrashReporting() override;
void WriteIntoTrace(perfetto::TracedValue context) override;
void WriteIntoTrace(
perfetto::TracedProto<perfetto::protos::pbzero::RenderProcessHost> proto)
override;
#if BUILDFLAG(CLANG_PROFILING_INSIDE_SANDBOX)
void DumpProfilingData(base::OnceClosure callback) override;
#endif
Expand All @@ -316,8 +319,6 @@ class CONTENT_EXPORT RenderProcessHostImpl
child_process_activity_time_ = base::TimeTicks::Now();
}

void WriteIntoTrace(
perfetto::TracedProto<perfetto::protos::pbzero::RenderProcessHost> proto);

// Return the set of previously stored frame tokens for a |new_routing_id|.
// The frame tokens were stored on the IO thread via the
Expand Down
5 changes: 5 additions & 0 deletions content/child/child_thread_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
#include "content/common/field_trial_recorder.mojom.h"
#include "content/common/in_process_child_thread_params.h"
#include "content/common/mojo_core_library_support.h"
#include "content/common/pseudonymization_salt.h"
#include "content/public/common/content_client.h"
#include "content/public/common/content_features.h"
#include "content/public/common/content_switches.h"
Expand Down Expand Up @@ -396,6 +397,10 @@ class ChildThreadImpl::IOThreadState
}
#endif

void SetPseudonymizationSalt(uint32_t salt) override {
content::SetPseudonymizationSalt(salt);
}

const scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner_;
const base::WeakPtr<ChildThreadImpl> weak_main_thread_;
const base::RepeatingClosure quit_closure_;
Expand Down
6 changes: 4 additions & 2 deletions content/common/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ source_set("common") {
"process_type.cc",
"process_visibility_tracker.cc",
"process_visibility_tracker.h",
"pseudonymization_salt.cc",
"pseudonymization_salt.h",
"service_worker/service_worker_utils.cc",
"service_worker/service_worker_utils.h",
"set_process_title.cc",
Expand Down Expand Up @@ -530,14 +532,14 @@ mojom("mojo_bindings") {
"//content/common/input/synthetic_smooth_scroll_gesture_params.h",
"//content/common/input/synthetic_tap_gesture_params.h",
"//content/common/navigation_gesture.h",
"//third_party/blink/public/common/widget/visual_properties.h",
"//third_party/blink/public/common/web_preferences/web_preferences.h",
"//net/base/network_change_notifier.h",
"//third_party/blink/public/common/input/web_coalesced_input_event_mojom_traits.h",
"//third_party/blink/public/common/input/web_input_event.h",
"//third_party/blink/public/common/input/web_mouse_wheel_event.h",
"//third_party/blink/public/common/input/web_pointer_properties.h",
"//third_party/blink/public/common/input/web_touch_point.h",
"//third_party/blink/public/common/web_preferences/web_preferences.h",
"//third_party/blink/public/common/widget/visual_properties.h",
"//ui/events/blink/did_overscroll_params.h",
"//ui/events/blink/web_input_event_traits.h",
"//ui/latency/ipc/latency_info_param_traits.h",
Expand Down
7 changes: 7 additions & 0 deletions content/common/child_process.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,11 @@ interface ChildProcess {
// The callback is invoked once the profile has been flushed to disk.
[EnableIf=clang_profiling_inside_sandbox]
WriteClangProfilingProfile() => ();

// Sets the pseudonymization salt in a child process.
//
// PRIVACY NOTE: It is important that the `salt` is never persisted anywhere
// or sent to a server. Whoever has access to the salt can de-anonymize
// results of the content::PseudonymizationUtil::PseudonymizeString method.
SetPseudonymizationSalt(uint32 salt);
};
9 changes: 9 additions & 0 deletions content/common/child_process_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "base/threading/thread_task_runner_handle.h"
#include "build/build_config.h"
#include "content/common/content_constants_internal.h"
#include "content/common/pseudonymization_salt.h"
#include "content/public/common/child_process_host_delegate.h"
#include "content/public/common/content_client.h"
#include "content/public/common/content_paths.h"
Expand Down Expand Up @@ -346,6 +347,14 @@ bool ChildProcessHostImpl::OnMessageReceived(const IPC::Message& msg) {
}

void ChildProcessHostImpl::OnChannelConnected(int32_t peer_pid) {
// Propagate the pseudonymization salt to all the child processes.
//
// TODO(dullweber, lukasza): Figure out if it is possible to reset the salt
// at a regular interval (on the order of hours?). The browser would need
// to be responsible for 1) deciding when the refresh happens and 2) pushing
// the updated salt to all the child processes.
child_process_->SetPseudonymizationSalt(GetPseudonymizationSalt());

// We ignore the `peer_pid` argument, which ultimately comes over IPC from the
// remote process, in favor of the PID already known by the browser after
// launching the process. This is partly because IPC Channel is being phased
Expand Down
67 changes: 67 additions & 0 deletions content/common/pseudonymization_salt.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright 2021 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "content/common/pseudonymization_salt.h"

#include <atomic>

#include "base/check_op.h"
#include "base/dcheck_is_on.h"
#include "base/rand_util.h"

#if DCHECK_IS_ON()
#include "sandbox/policy/sandbox.h"
#endif

namespace content {

namespace {

std::atomic<uint32_t> g_salt(0);

uint32_t InitializeSalt() {
uint32_t salt;
do {
salt = base::RandUint64();
} while (salt == 0);

// If `g_salt` is still uninitialized (has a value of 0), then put `salt` into
// `g_salt`. Otherwise, use the current `value` of `g_salt`.
uint32_t value = 0;
if (!g_salt.compare_exchange_strong(value, salt))
salt = value;

return salt;
}

} // namespace

uint32_t GetPseudonymizationSalt() {
uint32_t salt = g_salt.load();

if (salt == 0) {
#if DCHECK_IS_ON()
// Only the Browser process needs to initialize the `salt` on demand.
// Other processes (identified via the IsProcessSandboxed heuristic) should
// receive the salt from their parent processes.
DCHECK(!sandbox::policy::Sandbox::IsProcessSandboxed());
#endif
salt = InitializeSalt();
}

return salt;
}

void SetPseudonymizationSalt(uint32_t salt) {
DCHECK_NE(0u, salt);

// TODO(lukasza): Ideally we would DCHECK that `g_salt` is not set twice (e.g.
// that DCHECK_EQ(0u, g_salt.load(std::memory_order_acquire))), but this is
// made rather difficult by tests that run in single-process-mode, or
// construct ChildProcessHostImpl directly (e.g. RenderThreadImplBrowserTest).

g_salt.store(salt);
}

} // namespace content
37 changes: 37 additions & 0 deletions content/common/pseudonymization_salt.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright 2021 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CONTENT_COMMON_PSEUDONYMIZATION_SALT_H_
#define CONTENT_COMMON_PSEUDONYMIZATION_SALT_H_

#include <stdint.h>

namespace content {

// Gets the pseudonymization salt.
//
// Note that this function returns the same salt in all Chromium processes (e.g.
// in the Browser process, the Renderer processes and other child processes),
// because the propagation taking place via callers of SetPseudonymizationSalt
// below. This behavior ensures that the
// content::PseudonymizationUtil::PseudonymizeString method produces the same
// results across all processes.
//
// This function is thread-safe - it can be called on any thread.
//
// PRIVACY NOTE: It is important that the returned value is never persisted
// anywhere or sent to a server. Whoever has access to the salt can
// de-anonymize results of the content::PseudonymizationUtil::PseudonymizeString
// method.
uint32_t GetPseudonymizationSalt();

// Called in child processes, for setting the pseudonymization `salt` received
// in an IPC from a parent process.
//
// This function is thread-safe - it can be called on any thread.
void SetPseudonymizationSalt(uint32_t salt);

} // namespace content

#endif // CONTENT_COMMON_PSEUDONYMIZATION_SALT_H_
5 changes: 5 additions & 0 deletions content/public/browser/render_process_host.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "base/process/kill.h"
#include "base/process/process.h"
#include "base/supports_user_data.h"
#include "base/tracing/protos/chrome_track_event.pbzero.h"
#include "build/build_config.h"
#include "content/common/content_export.h"
#include "ipc/ipc_channel_proxy.h"
Expand Down Expand Up @@ -46,6 +47,7 @@
#include "third_party/blink/public/mojom/permissions/permission.mojom-forward.h"
#include "third_party/blink/public/mojom/quota/quota_manager_host.mojom-forward.h"
#include "third_party/blink/public/mojom/websockets/websocket_connector.mojom-forward.h"
#include "third_party/perfetto/include/perfetto/tracing/traced_proto.h"
#include "third_party/perfetto/include/perfetto/tracing/traced_value_forward.h"
#include "ui/gfx/native_widget_types.h"

Expand Down Expand Up @@ -561,6 +563,9 @@ class CONTENT_EXPORT RenderProcessHost : public IPC::Sender,

// Write a representation of this object into a trace.
virtual void WriteIntoTrace(perfetto::TracedValue context) = 0;
virtual void WriteIntoTrace(
perfetto::TracedProto<perfetto::protos::pbzero::RenderProcessHost>
proto) = 0;

#if BUILDFLAG(CLANG_PROFILING_INSIDE_SANDBOX)
// Ask the renderer process to dump its profiling data to disk. Invokes
Expand Down
2 changes: 2 additions & 0 deletions content/public/common/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ source_set("common_sources") {
"process_type.h",
"profiling.cc",
"profiling.h",
"pseudonymization_util.cc",
"pseudonymization_util.h",
"referrer.cc",
"referrer.h",
"referrer_type_converters.cc",
Expand Down
3 changes: 3 additions & 0 deletions content/public/common/OWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,6 @@ per-file *_type_converter*.*=set noparent
per-file *_type_converter*.*=file://ipc/SECURITY_OWNERS
per-file sandbox*=set noparent
per-file sandbox*=file://sandbox/OWNERS

# New usage of pseudonymization_util requires a privacy review.
per-file pseudonymization_util*=file://docs/privacy/OWNERS
48 changes: 48 additions & 0 deletions content/public/common/pseudonymization_util.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright 2021 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "content/public/common/pseudonymization_util.h"

#include <string.h>

#include "base/hash/sha1.h"
#include "base/strings/string_piece.h"
#include "content/common/pseudonymization_salt.h"

namespace content {

// static
uint32_t PseudonymizationUtil::PseudonymizeString(base::StringPiece string) {
// Include `string` in the SHA1 hash.
base::SHA1Context sha1_context;
base::SHA1Init(sha1_context);
base::SHA1Update(string, sha1_context);

// When `string` comes from a small set of possible strings (or when it is
// possible to compare a hash with results of hashing the 100 most common
// input strings), then its hash can be deanonymized. To protect against this
// threat, we include a random `salt` in the SHA1 hash (the salt is never
// retained or sent anywhere).
uint32_t salt = GetPseudonymizationSalt();
base::SHA1Update(
base::StringPiece(reinterpret_cast<const char*>(&salt), sizeof(salt)),
sha1_context);

// Compute the SHA1 hash.
base::SHA1Digest sha1_hash_bytes;
base::SHA1Final(sha1_context, sha1_hash_bytes);

// Taking just the first 4 bytes is okay, because SHA1 should uniformly
// distribute all possible results over all of the `sha1_hash_bytes`.
uint32_t hash;
static_assert(
sizeof(hash) <
sizeof(base::SHA1Digest::value_type) * sha1_hash_bytes.size(),
"Is `memcpy` safely within the bounds of `hash` and `sha1_hash_bytes`?");
memcpy(&hash, sha1_hash_bytes.data(), sizeof(hash));

return hash;
}

} // namespace content
Loading

0 comments on commit 2b8b862

Please sign in to comment.