Add WebView-specific whitelist for crash keys.

components/crash/content/app/breakpad_linux.cc

 // Copyright 2013 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.
 
 // For linux_syscall_support.h. This makes it safe to call embedded system
 // calls when in seccomp mode.
 
 #include "components/crash/content/app/breakpad_linux.h"
 
 #include <fcntl.h>
 #include <poll.h>
 #include <signal.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/socket.h>
 #include <sys/time.h>
 #include <sys/types.h>
 #include <sys/uio.h>
 #include <sys/wait.h>
 #include <time.h>
 #include <unistd.h>
 
 #include <algorithm>
+#include <map>
 #include <string>
 
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/debug/crash_logging.h"
 #include "base/debug/dump_without_crashing.h"
 #include "base/files/file_path.h"
+#include "base/format_macros.h"
 #include "base/lazy_instance.h"
 #include "base/linux_util.h"
 #include "base/macros.h"
 #include "base/path_service.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/posix/global_descriptors.h"
 #include "base/process/memory.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/threading/thread_checker.h"
 #include "breakpad/src/client/linux/crash_generation/crash_generation_client.h"
 #include "breakpad/src/client/linux/handler/exception_handler.h"
 #include "breakpad/src/client/linux/minidump_writer/directory_reader.h"
 #include "breakpad/src/common/linux/linux_libc_support.h"
 #include "breakpad/src/common/memory.h"
 #include "build/build_config.h"
 #include "components/crash/content/app/breakpad_linux_impl.h"
 #include "components/crash/content/app/crash_reporter_client.h"
 #include "components/crash/core/common/crash_keys.h"
@@ skipping 1039 matching lines @@
       MinidumpDescriptor("/tmp"),  // Unused but needed or Breakpad will assert.
       nullptr,
       nullptr,
       nullptr,
       true,
       -1);
   g_breakpad->set_crash_generation_client(new NonBrowserCrashHandler());
 }
 #endif  // defined(OS_ANDROID)
 
+std::unordered_set<std::string> g_crash_keys_whitelist;

[gsennton, 2017/02/27 18:31:37]

I'm not sure what constraints we have on what data-structures to use here, any
thoughts?

[Robert Sesek, 2017/02/27 23:15:28]

Basically nothing that allocates, if possible. Creating an iterator here may, so
I wouldn't use this. A NULL-terminated C array is probably safest.

[gsennton, 2017/02/28 21:02:42]

This should be fine now :).

[Tobias Sargeant, 2017/02/28 22:18:55]

I think Robert's suggestion of a NULL terminated C array is safest (I expect
that std::vector won't allocate given the usage below, but I don't know for
sure, and I'm not sure that that guarantee is given in the STL spec).

However, we don't access this from a compromised context (Robert: I'm assuming
that keys are only set prior to a crash. Is that correct) so I think that the
unordered_set should be ok?

[Robert Sesek, 2017/03/02 15:00:38]

No, crash keys can be set in a compromised context.

[gsennton, 2017/03/02 16:18:01]

I changed the vector to a NULL terminated C array now :)

+
+// TODO copy-pasted from base/debug/crash_logging.cc, can we just BASE_EXPORT
+// that implementation instead?

[gsennton, 2017/02/27 18:31:38]

^^^ is BASE_EXPORTING NumChunksForLength OK?

[Robert Sesek, 2017/02/27 23:15:28]

Hm, it should be OK. What keys are going to be whitelisted? Are they going to
get chunked, or can this be avoided totally?

[gsennton, 2017/02/28 21:02:42]

Removed these - with a more hard-coded (ugh!) version these aren't needed.

+size_t NumChunksForLength(size_t length, size_t chunk_max_length) {
+  // Compute (length / g_chunk_max_length_), rounded up.
+  return (length + chunk_max_length - 1) / chunk_max_length;
+}
+
+// TODO also copy-pasted from base/debug/crash_logging.cc
+const char kChunkFormatString[] = "%s-%" PRIuS;

[gsennton, 2017/02/27 18:31:38]

^^ same question here, should we BASE_EXPORT this?

+
+void SetupCrashKeysWhiteList(const std::vector<base::debug::CrashKey>& keys,
+                             const size_t chunk_max_length) {
+  LOG(ERROR) << "in SetupCrashKeysWhiteList";
+  // TODO do we care that we are storing lots of keys in our map?

[gsennton, 2017/02/27 18:31:38]

In our whitelist set we are storing an entry for each possible chunk, this can
be up to 16 entries per kLargeSize-key... is this OK or should I parse the
integer part of the key-value in IsInWhiteList instead? (do we have any utility
methods to parse integers from strings, without failing badly if there is no
number there?)

[Robert Sesek, 2017/02/27 23:15:28]

I think it's fine to just have up to 16 entries. How many large keys are you
going to store?

[gsennton, 2017/02/28 21:02:42]

+1, no large keys.

[gsennton, 2017/02/28 21:02:42]

Yeah, we only whitelist small/medium-sized keys, so this won't be an issue (for
now).

+  for (size_t key_index = 0; key_index < keys.size(); ++key_index) {
+    const size_t num_chunks =
+        NumChunksForLength(keys[key_index].max_length, chunk_max_length);
+    if (num_chunks == 1) {
+      g_crash_keys_whitelist.insert(keys[key_index].key_name);
+    } else {
+      for (size_t chunk_index = 0; chunk_index < num_chunks; ++chunk_index) {
+        g_crash_keys_whitelist.insert(base::StringPrintf(
+            kChunkFormatString, keys[key_index].key_name, chunk_index + 1));
+      }
+    }
+  }
+  LOG(ERROR) << "in SetupCrashKeysWhiteList, whitelist length: "
+             << g_crash_keys_whitelist.size();
+  for (auto it = g_crash_keys_whitelist.begin();
+       it != g_crash_keys_whitelist.end(); it++) {
+    LOG(ERROR) << "in SetupCrashKeysWhiteList, entry: " << *it;
+  }
+}
+
+bool IsInWhiteList(const base::StringPiece& key) {
+  // TODO does this need to be thread-safe?

[gsennton, 2017/02/27 18:31:38]

I'm guessing we allow SetCrashKeyValue to be called from different threads?

[Robert Sesek, 2017/02/27 23:15:28]

Yup. I'd ensure the whitelist is immutable after it's set.

[gsennton, 2017/02/28 21:02:42]

Done (well, I don't touch it again after the initial set-up).

+  return g_crash_keys_whitelist.find(key.as_string()) !=
+         g_crash_keys_whitelist.end();
+}
+
 void SetCrashKeyValue(const base::StringPiece& key,
                       const base::StringPiece& value) {
-  g_crash_keys->SetKeyValue(key.data(), value.data());
+  LOG(ERROR) << "in SetCrashKeyValue for key " << key.data();
+  // TODO if #DEFINED(USE_WHITELIST) --use-whitelist
+  if (IsInWhiteList(key)) {
+    LOG(ERROR) << "in SetCrashKeyValue for key " << key.data()
+               << " it is in the whitelist :D";
+    g_crash_keys->SetKeyValue(key.data(), value.data());
+  } else {
+    LOG(ERROR) << "in SetCrashKeyValue for key " << key.data()
+               << ", not in the whitelist";
+  }
 }
 
 void ClearCrashKey(const base::StringPiece& key) {
   g_crash_keys->RemoveKey(key.data());
 }
 
 // GetCrashReporterClient() cannot call any Set methods until after
 // InitCrashKeys().
 void InitCrashKeys() {
   g_crash_keys = new CrashKeyStorage;
   GetCrashReporterClient()->RegisterCrashKeys();
+  // TODO if #DEFINED(USE_WHITELIST) --use-whitelist
+  SetupCrashKeysWhiteList(GetCrashReporterClient()->GetWhiteListedCrashKeys(),
+                          crash_keys::kChunkMaxLength);
   base::debug::SetCrashKeyReportingFunctions(&SetCrashKeyValue, &ClearCrashKey);
 }
 
 // Miscellaneous initialization functions to call after Breakpad has been
 // enabled.
 void PostEnableBreakpadInitialization() {
   SetProcessStartTime();
   g_pid = getpid();
 
   base::debug::SetDumpWithoutCrashingFunction(&DumpProcess);
@@ skipping 934 matching lines @@
 void SuppressDumpGeneration() {
   g_dumps_suppressed = G_DUMPS_SUPPRESSED_MAGIC;
 }
 #endif  // OS_ANDROID
 
 bool IsCrashReporterEnabled() {
   return g_is_crash_reporter_enabled;
 }
 
 }  // namespace breakpad