Skip to content

Commit

Permalink
GWP-ASan: Add sanitization support
Browse files Browse the repository at this point in the history
Android WebView has a stricter privacy model because it runs in
embedders memory spaces. To ensure crash dumps don't include information
from the embedding process, additional sanitization is performed on the
crash dump.

To make GWP-ASan work under sanitization, white list GWP-ASan memory
regions that are required by the crash handler. Furthermore, add support
for testing under sanitization on Linux/Android (the only platforms that
currently support sanitization.)

CQ-DEPEND=chromium:1756705

Bug: 973167
Change-Id: I6a48b17bd128f44ffd62fe0f180db79c175a561b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1757160
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Vitaly Buka <vitalybuka@chromium.org>
Commit-Queue: Vlad Tsyrklevich <vtsyrklevich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688159}
  • Loading branch information
vlad902 authored and Commit Bot committed Aug 19, 2019
1 parent cf20cf4 commit 9d57abf
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 17 deletions.
4 changes: 4 additions & 0 deletions components/gwp_asan/client/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ component("client") {
"//components/crash/core/common:crash_key",
"//components/gwp_asan/common",
]

if (is_android) {
deps += [ "//components/crash/content/app" ]
}
}

source_set("unit_tests") {
Expand Down
1 change: 1 addition & 0 deletions components/gwp_asan/client/DEPS
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
include_rules = [
"+components/crash/core/common/crash_key.h",
"+components/crash/content/app/crashpad.h",
]
25 changes: 25 additions & 0 deletions components/gwp_asan/client/guarded_page_allocator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
#include "components/gwp_asan/common/crash_key_name.h"
#include "components/gwp_asan/common/pack_stack_trace.h"

#if defined(OS_ANDROID)
#include "components/crash/content/app/crashpad.h" // nogncheck
#endif

#if defined(OS_MACOSX)
#include <pthread.h>
#endif
Expand Down Expand Up @@ -166,6 +170,27 @@ void GuardedPageAllocator::Init(size_t max_alloced_pages,
metadata_ =
std::make_unique<AllocatorState::SlotMetadata[]>(state_.num_metadata);
state_.metadata_addr = reinterpret_cast<uintptr_t>(metadata_.get());

#if defined(OS_ANDROID)
// Explicitly whitelist memory ranges the crash_handler needs to reads. This
// is required for WebView because it has a stricter set of privacy
// constraints on what it reads from the crashing process.
for (auto& region : GetInternalMemoryRegions())
crash_reporter::WhitelistMemoryRange(region.first, region.second);
#endif
}

std::vector<std::pair<void*, size_t>>
GuardedPageAllocator::GetInternalMemoryRegions() {
std::vector<std::pair<void*, size_t>> regions;
regions.push_back(std::make_pair(&state_, sizeof(state_)));
regions.push_back(std::make_pair(
metadata_.get(),
sizeof(AllocatorState::SlotMetadata) * state_.num_metadata));
regions.push_back(
std::make_pair(slot_to_metadata_idx_.data(),
sizeof(AllocatorState::MetadataIdx) * state_.total_pages));
return regions;
}

GuardedPageAllocator::~GuardedPageAllocator() {
Expand Down
4 changes: 4 additions & 0 deletions components/gwp_asan/client/guarded_page_allocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ class GWP_ASAN_EXPORT GuardedPageAllocator {
// crash handler.
std::string GetCrashKey() const;

// Returns internal memory used by the allocator (required for sanitization
// on supported platforms.)
std::vector<std::pair<void*, size_t>> GetInternalMemoryRegions();

// Returns true if ptr points to memory managed by this class.
inline bool PointerIsMine(const void* ptr) const {
return state_.PointerIsMine(reinterpret_cast<uintptr_t>(ptr));
Expand Down
89 changes: 72 additions & 17 deletions components/gwp_asan/crash_handler/crash_handler_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@
#include "third_party/crashpad/crashpad/snapshot/minidump/process_snapshot_minidump.h"
#include "third_party/crashpad/crashpad/tools/tool_support.h"

#if defined(OS_LINUX) || defined(OS_ANDROID)
#include "third_party/crashpad/crashpad/snapshot/sanitized/sanitization_information.h"
#endif

namespace gwp_asan {
namespace internal {

Expand Down Expand Up @@ -118,18 +122,43 @@ MULTIPROCESS_TEST_MAIN(CrashingProcess) {
std::map<std::string, std::string> annotations;
std::vector<std::string> arguments;

crashpad::CrashpadClient* client = new crashpad::CrashpadClient();
#if defined(OS_LINUX) || defined(OS_ANDROID)
static crashpad::SanitizationInformation sanitization_info = {};
static crashpad::SanitizationMemoryRangeWhitelist memory_whitelist;
if (cmd_line->HasSwitch("sanitize")) {
auto memory_ranges = gpa->GetInternalMemoryRegions();
auto* range_array =
new crashpad::SanitizationMemoryRangeWhitelist::Range[memory_ranges
.size()];
for (size_t i = 0; i < memory_ranges.size(); i++) {
range_array[i].base =
reinterpret_cast<crashpad::VMAddress>(memory_ranges[i].first);
range_array[i].length = memory_ranges[i].second;
}
memory_whitelist.size = memory_ranges.size();
memory_whitelist.entries =
reinterpret_cast<crashpad::VMAddress>(range_array);
sanitization_info.memory_range_whitelist_address =
reinterpret_cast<crashpad::VMAddress>(&memory_whitelist);
arguments.push_back(base::StringPrintf("--sanitization-information=%p",
&sanitization_info));
}
#endif

#if !defined(OS_ANDROID)
arguments.push_back("--test-child-process=CrashpadHandler");
bool handler = client->StartHandler(/* handler */ cmd_line->GetProgram(),
/* database */ directory,
/* metrics_dir */ metrics_dir,
/* url */ "",
/* annotations */ annotations,
/* arguments */ arguments,
/* restartable */ false,
/* asynchronous_start */ false);
#else
#endif

crashpad::CrashpadClient* client = new crashpad::CrashpadClient();
#if defined(OS_LINUX)
bool handler =
client->StartHandlerAtCrash(/* handler */ cmd_line->GetProgram(),
/* database */ directory,
/* metrics_dir */ metrics_dir,
/* url */ "",
/* annotations */ annotations,
/* arguments */ arguments);
#elif defined(OS_ANDROID)
// TODO: Once the minSdkVersion is >= Q define a CrashpadHandlerMain() and
// use the /system/bin/linker approach instead of using
// libchrome_crashpad_handler.so
Expand All @@ -149,7 +178,17 @@ MULTIPROCESS_TEST_MAIN(CrashingProcess) {

bool handler = client->StartHandlerAtCrash(
executable_path, directory, metrics_dir, "", annotations, arguments);
#else
bool handler = client->StartHandler(/* handler */ cmd_line->GetProgram(),
/* database */ directory,
/* metrics_dir */ metrics_dir,
/* url */ "",
/* annotations */ annotations,
/* arguments */ arguments,
/* restartable */ false,
/* asynchronous_start */ false);
#endif

if (!handler) {
LOG(ERROR) << "Crash handler failed to launch";
return kSuccess;
Expand Down Expand Up @@ -204,10 +243,18 @@ MULTIPROCESS_TEST_MAIN(CrashingProcess) {
return kSuccess;
}

struct TestParams {
TestParams(const char* allocator, bool sanitize)
: allocator(allocator), sanitize(sanitize) {}

const char* allocator;
bool sanitize;
};

class CrashHandlerTest : public base::MultiProcessTest,
public testing::WithParamInterface<const char*> {
public testing::WithParamInterface<TestParams> {
protected:
CrashHandlerTest() : allocator_(GetParam()) {}
CrashHandlerTest() : params_(GetParam()) {}

// Launch a child process and wait for it to crash. Set |gwp_asan_found_| if a
// GWP-ASan data was found and if so, read it into |proto_|.
Expand Down Expand Up @@ -239,7 +286,10 @@ class CrashHandlerTest : public base::MultiProcessTest,
base::GetMultiProcessTestChildBaseCommandLine();
cmd_line.AppendSwitchPath("directory", database_dir);
cmd_line.AppendSwitchASCII("test-name", test_name);
cmd_line.AppendSwitchASCII("allocator", allocator_);
cmd_line.AppendSwitchASCII("allocator", params_.allocator);

if (params_.sanitize)
cmd_line.AppendSwitch("sanitize");

base::LaunchOptions options;
#if defined(OS_WIN)
Expand Down Expand Up @@ -339,16 +389,16 @@ class CrashHandlerTest : public base::MultiProcessTest,
EXPECT_FALSE(proto_.missing_metadata());

EXPECT_TRUE(proto_.has_allocator());
if (allocator_ == "malloc")
if (!strcmp(params_.allocator, "malloc"))
EXPECT_EQ(proto_.allocator(), Crash_Allocator_MALLOC);
else if (allocator_ == "partitionalloc")
else if (!strcmp(params_.allocator, "partitionalloc"))
EXPECT_EQ(proto_.allocator(), Crash_Allocator_PARTITIONALLOC);
else
ASSERT_TRUE(false) << "Unknown allocator name";
}

gwp_asan::Crash proto_;
std::string allocator_;
TestParams params_;
bool gwp_asan_found_;
};

Expand Down Expand Up @@ -407,7 +457,12 @@ TEST_P(CrashHandlerTest, MAYBE_DISABLED(UnrelatedException)) {

INSTANTIATE_TEST_SUITE_P(VaryAllocator,
CrashHandlerTest,
testing::Values("malloc", "partitionalloc"));
testing::Values(
#if defined(OS_LINUX) || defined(OS_ANDROID)
TestParams("malloc", true),
#endif
TestParams("malloc", false),
TestParams("partitionalloc", false)));

} // namespace

Expand Down

0 comments on commit 9d57abf

Please sign in to comment.