Plumb in support for ProcessMemorySanitized

A previous change added a ProcessMemorySanitized class, in this change
plumb support for ProcessMemorySanitized into ProcessSnapshotSanitized.
This involves reading whitelisted regions using the a new field in the
SanitizationInformation struct and returning an initialized
ProcessMemorySanitized object from ProcessSnapshotSanitized::Memory().

Bug: crashpad:263, chromium:973167
Change-Id: I121c5a584a1704ad043757c113099978a9ec2f4e
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/1754737
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Commit-Queue: Vlad Tsyrklevich <vtsyrklevich@chromium.org>

handler/linux/crash_report_exception_handler.cc

@@ -141,6 +141,7 @@ bool CrashReportExceptionHandler::HandleExceptionWithConnection(
     ProcessSnapshot* snapshot = nullptr;
     ProcessSnapshotSanitized sanitized;
     std::vector<std::string> annotations_whitelist;
+    std::vector<std::pair<VMAddress, VMAddress>> memory_range_whitelist;
     if (info.sanitization_information_address) {
       SanitizationInformation sanitization_info;
       ProcessMemoryRange range;
@@ -153,11 +154,14 @@ bool CrashReportExceptionHandler::HandleExceptionWithConnection(
         return false;
       }
 
-      if (sanitization_info.annotations_whitelist_address &&
+      if (!ReadAnnotationsWhitelist(
-          !ReadAnnotationsWhitelist(
               range,
               sanitization_info.annotations_whitelist_address,
-              &annotations_whitelist)) {
+              &annotations_whitelist) ||
+          !ReadMemoryRangeWhitelist(
+              range,
+              sanitization_info.memory_range_whitelist_address,
+              &memory_range_whitelist)) {
         Metrics::ExceptionCaptureResult(
             Metrics::CaptureResult::kSanitizationInitializationFailed);
         return false;
@@ -167,6 +171,7 @@ bool CrashReportExceptionHandler::HandleExceptionWithConnection(
                                 sanitization_info.annotations_whitelist_address
                                     ? &annotations_whitelist
                                     : nullptr,
+                                &memory_range_whitelist,
                                 sanitization_info.target_module_address,
                                 sanitization_info.sanitize_stacks)) {
         Metrics::ExceptionCaptureResult(

snapshot/sanitized/process_snapshot_sanitized.cc

@@ -85,6 +85,7 @@ ProcessSnapshotSanitized::~ProcessSnapshotSanitized() = default;
 bool ProcessSnapshotSanitized::Initialize(
     const ProcessSnapshot* snapshot,
     const std::vector<std::string>* annotations_whitelist,
+    const std::vector<std::pair<VMAddress, VMAddress>>* memory_range_whitelist,
     VMAddress target_module_address,
     bool sanitize_stacks) {
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
@@ -157,6 +158,8 @@ bool ProcessSnapshotSanitized::Initialize(
     }
   }
 
+  process_memory_.Initialize(snapshot_->Memory(), memory_range_whitelist);
+
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;
 }
@@ -264,8 +267,7 @@ std::vector<const MemorySnapshot*> ProcessSnapshotSanitized::ExtraMemory()
 
 const ProcessMemory* ProcessSnapshotSanitized::Memory() const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
-  NOTREACHED();  // https://crashpad.chromium.org/bug/263
+  return &process_memory_;
-  return nullptr;
 }
 
 }  // namespace crashpad

snapshot/sanitized/process_snapshot_sanitized.h

@@ -30,6 +30,7 @@
 #include "util/misc/initialization_state_dcheck.h"
 #include "util/misc/range_set.h"
 #include "util/process/process_id.h"
+#include "util/process/process_memory_sanitized.h"
 
 namespace crashpad {
 
@@ -62,6 +63,8 @@ class ProcessSnapshotSanitized final : public ProcessSnapshot {
   //!     should be filtered entirely. Otherwise `true`.
   bool Initialize(const ProcessSnapshot* snapshot,
                   const std::vector<std::string>* annotations_whitelist,
+                  const std::vector<std::pair<VMAddress, VMAddress>>*
+                      memory_range_whitelist,
                   VMAddress target_module_address,
                   bool sanitize_stacks);
 
@@ -95,6 +98,7 @@ class ProcessSnapshotSanitized final : public ProcessSnapshot {
 
   RangeSet address_ranges_;
   const ProcessSnapshot* snapshot_;
+  ProcessMemorySanitized process_memory_;
   const std::vector<std::string>* annotations_whitelist_;
   bool sanitize_stacks_;
   InitializationStateDcheck initialized_;

snapshot/sanitized/process_snapshot_sanitized_test.cc

@@ -145,6 +145,22 @@ void ExpectAnnotations(ProcessSnapshot* snapshot, bool sanitized) {
   }
 }
 
+void ExpectProcessMemory(ProcessSnapshot* snapshot,
+                         VMAddress whitelisted_byte,
+                         bool sanitized) {
+  auto memory = snapshot->Memory();
+
+  char out;
+  EXPECT_TRUE(memory->Read(whitelisted_byte, 1, &out));
+
+  bool unwhitelisted_read = memory->Read(whitelisted_byte + 1, 1, &out);
+  if (sanitized) {
+    EXPECT_FALSE(unwhitelisted_read);
+  } else {
+    EXPECT_TRUE(unwhitelisted_read);
+  }
+}
+
 class StackSanitizationChecker : public MemorySnapshot::Delegate {
  public:
   StackSanitizationChecker() = default;
@@ -251,20 +267,33 @@ class SanitizeTest : public MultiprocessExec {
 
     ExpectAnnotations(&snapshot, /* sanitized= */ false);
     ExpectStackData(&snapshot, addrs, /* sanitized= */ false);
+    ExpectProcessMemory(&snapshot,
+                        addrs.string_address,
+                        /* sanitized= */ false);
 
     std::vector<std::string> annotations_whitelist;
     annotations_whitelist.push_back(kWhitelistedAnnotationName);
 
+    std::vector<std::pair<VMAddress, VMAddress>> memory_ranges_whitelist;
+    memory_ranges_whitelist.push_back(
+        std::make_pair(addrs.string_address, addrs.string_address + 1));
+
     ProcessSnapshotSanitized sanitized;
-    ASSERT_TRUE(sanitized.Initialize(
+    ASSERT_TRUE(sanitized.Initialize(&snapshot,
-        &snapshot, &annotations_whitelist, addrs.module_address, true));
+                                     &annotations_whitelist,
+                                     &memory_ranges_whitelist,
+                                     addrs.module_address,
+                                     true));
 
     ExpectAnnotations(&sanitized, /* sanitized= */ true);
     ExpectStackData(&sanitized, addrs, /* sanitized= */ true);
+    ExpectProcessMemory(&sanitized,
+                        addrs.string_address,
+                        /* sanitized= */ true);
 
     ProcessSnapshotSanitized screened_snapshot;
     EXPECT_FALSE(screened_snapshot.Initialize(
-        &snapshot, nullptr, addrs.non_module_address, false));
+        &snapshot, nullptr, nullptr, addrs.non_module_address, false));
   }
 
   DISALLOW_COPY_AND_ASSIGN(SanitizeTest);

snapshot/sanitized/sanitization_information.cc

@@ -14,6 +14,8 @@
 
 #include "snapshot/sanitized/sanitization_information.h"
 
+#include <limits>
+
 #include "client/annotation.h"
 
 namespace crashpad {
@@ -59,4 +61,54 @@ bool ReadAnnotationsWhitelist(const ProcessMemoryRange& memory,
                                 memory, whitelist_address, whitelist);
 }
 
+bool ReadMemoryRangeWhitelist(
+    const ProcessMemoryRange& memory,
+    VMAddress whitelist_address,
+    std::vector<std::pair<VMAddress, VMAddress>>* whitelist) {
+  whitelist->clear();
+  if (!whitelist_address) {
+    return true;
+  }
+
+  SanitizationMemoryRangeWhitelist list;
+  if (!memory.Read(whitelist_address, sizeof(list), &list)) {
+    LOG(ERROR) << "Failed to read memory range whitelist.";
+    return false;
+  }
+
+  if (!list.size) {
+    return true;
+  }
+
+  // An upper bound of entries that we never expect to see more than.
+  constexpr size_t kMaxListSize = 256;
+  if (list.size > kMaxListSize) {
+    LOG(ERROR) << "Whitelist exceeded maximum, size=" << list.size;
+    return false;
+  }
+
+  SanitizationMemoryRangeWhitelist::Range ranges[list.size];
+  if (!memory.Read(list.entries, sizeof(ranges), &ranges)) {
+    LOG(ERROR) << "Failed to read memory range whitelist entries.";
+    return false;
+  }
+
+  const VMAddress vm_max = memory.Is64Bit()
+                               ? std::numeric_limits<uint64_t>::max()
+                               : std::numeric_limits<uint32_t>::max();
+  std::vector<std::pair<VMAddress, VMAddress>> local_whitelist;
+  for (size_t i = 0; i < list.size; i++) {
+    if (ranges[i].base > vm_max || ranges[i].length > vm_max - ranges[i].base) {
+      LOG(ERROR) << "Invalid memory range whitelist entry base="
+                 << ranges[i].base << " length=" << ranges[i].length;
+      return false;
+    }
+    local_whitelist.emplace_back(ranges[i].base,
+                                 ranges[i].base + ranges[i].length);
+  }
+
+  whitelist->swap(local_whitelist);
+  return true;
+}
+
 }  // namespace crashpad

snapshot/sanitized/sanitization_information.h

@@ -18,6 +18,7 @@
 #include <stdint.h>
 
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "util/misc/address_types.h"
@@ -45,10 +46,29 @@ struct SanitizationInformation {
   //!     if there is no target module.
   VMAddress target_module_address;
 
+  //! \brief The address in the client process' address space of a
+  //!     a \a SanitizationMemoryRangeWhitelist, a list of whitelisted address
+  //!     ranges allowed to be accessed by ProcessMemorySanitized. This value
+  //!     is 0 if no memory is allowed to be read using ProcessMemorySanitized.
+  VMAddress memory_range_whitelist_address;
+
   //! \brief Non-zero if stacks should be sanitized for possible PII.
   uint8_t sanitize_stacks;
 };
 
+//! \brief Describes a list of white listed memory ranges.
+struct SanitizationMemoryRangeWhitelist {
+  //! \brief Describes a range of memory.
+  struct Range {
+    VMAddress base;
+    VMSize length;
+  };
+
+  //! \brief Address of an array of |size| elements of type Range.
+  VMAddress entries;
+  VMSize size;
+};
+
 #pragma pack(pop)
 
 //! \brief Reads an annotations whitelist from another process.
@@ -63,6 +83,19 @@ bool ReadAnnotationsWhitelist(const ProcessMemoryRange& memory,
                               VMAddress whitelist_address,
                               std::vector<std::string>* whitelist);
 
+//! \brief Reads a memory range whitelist from another process.
+//!
+//! \param[in] memory A memory reader for the target process.
+//! \param[in] whitelist_address The address in the target process' address
+//!     space of a nullptr terminated array of NUL-terminated strings.
+//! \param[out] whitelist A list of whitelisted memory regions, valid only if
+//!     this function returns `true`.
+//! \return `true` on success, `false` on failure with a message logged.
+bool ReadMemoryRangeWhitelist(
+    const ProcessMemoryRange& memory,
+    VMAddress whitelist_address,
+    std::vector<std::pair<VMAddress, VMAddress>>* whitelist);
+
 }  // namespace crashpad
 
 #endif  // CRASHPAD_SNAPSHOT_SANITIZED_SANITIZATION_INFORMATION_H_