Add fuzzer test for preg_parser

components/policy/core/common/preg_parser.cc

Index: components/policy/core/common/preg_parser.cc
diff --git a/components/policy/core/common/preg_parser.cc b/components/policy/core/common/preg_parser.cc
index 81261996f933bf0a77ca95dc5dfd2c6fcc3b677f..1aa58cdc1526e14a38a485f14b638daaaf380c02 100644
--- a/components/policy/core/common/preg_parser.cc
+++ b/components/policy/core/common/preg_parser.cc
@@ -24,10 +24,10 @@
 #include "base/strings/string16.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_byteorder.h"
 #include "base/values.h"
-#include "components/policy/core/common/policy_load_status.h"
 #include "components/policy/core/common/registry_dict.h"
 
 #if defined(OS_WIN)
@@ -74,6 +74,12 @@ const char kActionTriggerDeleteKeys[] = "deletekeys";
 const char kActionTriggerSecureKey[] = "securekey";
 const char kActionTriggerSoft[] = "soft";
 
+// Same as base::EqualsCaseInsensitiveASCII, but with base::string16 args.
+bool EqualsCaseInsensitiveASCII(const base::string16& a,
+                                const base::string16& b) {
+  return base::EqualsCaseInsensitiveASCII(a, b);
+}
+
 // Returns the character at |cursor| and increments it, unless the end is here
 // in which case -1 is returned. The calling code must guarantee that
 // end - *cursor does not overflow ptrdiff_t.
@@ -178,11 +184,17 @@ bool DecodePRegValue(uint32_t type,
   return false;
 }
 
+// Splits a registry key A\B\C into parts {A, B, C}.
+std::vector<base::string16> SplitKey(const base::string16& key_name) {
+  return base::SplitString(key_name, kRegistryPathSeparator,
+                           base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+}
+
 // Adds |value| and |data| to |dict| or an appropriate sub-dictionary indicated
-// by |key_name|. Creates sub-dictionaries if necessary. Also handles special
+// by |key_parts|. Creates sub-dictionaries if necessary. Also handles special
 // action triggers, see |kActionTrigger*|, that can, for instance, remove an
 // existing value.
-void HandleRecord(const base::string16& key_name,
+void HandleRecord(const std::vector<base::string16>& key_parts,
                   const base::string16& value,
                   uint32_t type,
                   const std::vector<uint8_t>& data,
@@ -190,9 +202,7 @@ void HandleRecord(const base::string16& key_name,
   // Locate/create the dictionary to place the value in.
   std::vector<base::string16> path;
 
-  for (const base::string16& entry :
-       base::SplitString(key_name, kRegistryPathSeparator,
-                         base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
+  for (const base::string16& entry : key_parts) {
     if (entry.empty())
       continue;
     const std::string name = base::UTF16ToUTF8(entry);
@@ -216,8 +226,8 @@ void HandleRecord(const base::string16& key_name,
     return;
   }
 
-  std::string action_trigger(base::ToLowerASCII(value_name.substr(
-      arraysize(kActionTriggerPrefix) - 1)));
+  std::string action_trigger(base::ToLowerASCII(
+      value_name.substr(arraysize(kActionTriggerPrefix) - 1)));
   if (action_trigger == kActionTriggerDeleteValues) {
     for (const std::string& value :
          base::SplitString(DecodePRegStringValue(data), ";",
@@ -231,9 +241,8 @@ void HandleRecord(const base::string16& key_name,
       dict->RemoveKey(key);
   } else if (base::StartsWith(action_trigger, kActionTriggerDel,
                               base::CompareCase::SENSITIVE)) {
-  dict->RemoveValue(
-        value_name.substr(arraysize(kActionTriggerPrefix) - 1 +
-                          arraysize(kActionTriggerDel) - 1));
+    dict->RemoveValue(value_name.substr(arraysize(kActionTriggerPrefix) - 1 +
+                                        arraysize(kActionTriggerDel) - 1));
   } else if (base::StartsWith(action_trigger, kActionTriggerDelVals,
                               base::CompareCase::SENSITIVE)) {
     // Delete all values.
@@ -253,41 +262,65 @@ void HandleRecord(const base::string16& key_name,
 namespace policy {
 namespace preg_parser {
 
-const char kPRegFileHeader[8] =
-    { 'P', 'R', 'e', 'g', '\x01', '\x00', '\x00', '\x00' };
+const char kPRegFileHeader[8] = {'P',    'R',    'e',    'g',
+                                 '\x01', '\x00', '\x00', '\x00'};
 
 bool ReadFile(const base::FilePath& file_path,
               const base::string16& root,
               RegistryDict* dict,
-              PolicyLoadStatusSample* status) {
+              PolicyLoadStatusSample* status_sample) {
   base::MemoryMappedFile mapped_file;
   if (!mapped_file.Initialize(file_path) || !mapped_file.IsValid()) {
     PLOG(ERROR) << "Failed to map " << file_path.value();
-    status->Add(POLICY_LOAD_STATUS_READ_ERROR);
+    status_sample->Add(POLICY_LOAD_STATUS_READ_ERROR);
     return false;
   }
 
-  if (mapped_file.length() > kMaxPRegFileSize) {
-    LOG(ERROR) << "PReg file " << file_path.value() << " too large: "
-               << mapped_file.length();
-    status->Add(POLICY_LOAD_STATUS_TOO_BIG);
+  PolicyLoadStatus status = POLICY_LOAD_STATUS_SIZE;
+  bool res =
+      ReadData(mapped_file.data(), mapped_file.length(), root, dict, &status,
+               base::StringPrintf("file '%s'", file_path.value().c_str()));
+  if (!res) {
+    DCHECK(status != POLICY_LOAD_STATUS_SIZE);
+    status_sample->Add(status);
+  }
+  return res;
+}
+
+POLICY_EXPORT bool ReadData(const uint8_t* data,
+                            size_t data_size,
+                            const base::string16& root,
+                            RegistryDict* dict,
+                            PolicyLoadStatus* status,
+                            const std::string& debug_name) {
+  DCHECK(status);
+
+  // Check data size.
+  if (data_size > kMaxPRegFileSize) {
+    LOG(ERROR) << "PReg " << debug_name << " too large: " << data_size;
+    *status = POLICY_LOAD_STATUS_TOO_BIG;
     return false;
   }
 
   // Check the header.
   const int kHeaderSize = arraysize(kPRegFileHeader);
-  if (mapped_file.length() < kHeaderSize ||
-      memcmp(kPRegFileHeader, mapped_file.data(), kHeaderSize) != 0) {
-    LOG(ERROR) << "Bad policy file " << file_path.value();
-    status->Add(POLICY_LOAD_STATUS_PARSE_ERROR);
+  if (!data || data_size < kHeaderSize ||
+      memcmp(kPRegFileHeader, data, kHeaderSize) != 0) {
+    LOG(ERROR) << "Bad PReg " << debug_name;
+    *status = POLICY_LOAD_STATUS_PARSE_ERROR;
     return false;
   }
 
-  // Parse file contents, which is UCS-2 and little-endian. The latter I
+  // Split |root| into parts. It is important that we don't do a simple
+  // StartsWith check for |root| in the |key_name| (see below) since then root =
+  // "someroot" would trigger in key_name = "somerooting\data".

[Mattias Nissler (ping if slow), 2017/04/07 08:13:26]

Alternatively you could enforce that the prefix we test against ends with a
backslash, or that the matching prefix is followed by a backslash, both of which
seem simpler?

[ljusten (tachyonic), 2017/04/10 10:22:51]

I thought about that, I was a bit scared by the number of cases (including root
having a backslash in the end or not), but it's probably still simpler. I'll
give it a shot.

+  const std::vector<base::string16> root_parts = SplitKey(root);
+
+  // Parse data, which is expected to be UCS-2 and little-endian. The latter I
   // couldn't find documentation on, but the example I saw were all
   // little-endian. It'd be interesting to check on big-endian hardware.
-  const uint8_t* cursor = mapped_file.data() + kHeaderSize;
-  const uint8_t* end = mapped_file.data() + mapped_file.length();
+  const uint8_t* cursor = data + kHeaderSize;
+  const uint8_t* end = data + data_size;
   while (true) {
     if (cursor == end)
       return true;
@@ -337,14 +370,18 @@ bool ReadFile(const base::FilePath& file_path,
       break;
 
     // Process the record if it is within the |root| subtree.
-    if (base::StartsWith(key_name, root, base::CompareCase::INSENSITIVE_ASCII))
-      HandleRecord(key_name.substr(root.size()), value, type, data, dict);
+    std::vector<base::string16> key_parts = SplitKey(key_name);
+    if (key_parts.size() >= root_parts.size() &&
+        std::equal(root_parts.begin(), root_parts.end(), key_parts.begin(),
+                   &EqualsCaseInsensitiveASCII)) {
+      key_parts.erase(key_parts.begin(), key_parts.begin() + root_parts.size());
+      HandleRecord(key_parts, value, type, data, dict);
+    }
   }
 
-  LOG(ERROR) << "Error parsing " << file_path.value() << " at offset "
-             << reinterpret_cast<const uint8_t*>(cursor - 1) -
-                    mapped_file.data();
-  status->Add(POLICY_LOAD_STATUS_PARSE_ERROR);
+  LOG(ERROR) << "Error parsing PReg " << debug_name << " at offset "
+             << (reinterpret_cast<const uint8_t*>(cursor - 1) - data);
+  *status = POLICY_LOAD_STATUS_PARSE_ERROR;
   return false;
 }