Fix safe-browsing sub parsing for download whitelist.

chrome/browser/safe_browsing/protocol_parser.cc

Index: chrome/browser/safe_browsing/protocol_parser.cc
diff --git a/chrome/browser/safe_browsing/protocol_parser.cc b/chrome/browser/safe_browsing/protocol_parser.cc
index f09a26fbebed18f29da5ea25e700d9c27cea9e57..00756dd302011be1494382952fb3d9aa5b4520dc 100644
--- a/chrome/browser/safe_browsing/protocol_parser.cc
+++ b/chrome/browser/safe_browsing/protocol_parser.cc
@@ -346,11 +346,16 @@ bool SafeBrowsingProtocolParser::ParseAddChunk(const std::string& list_name,
     hosts->push_back(chunk_host);
     if (!ReadPrefixes(&chunk_data, &remaining, chunk_host.entry, prefix_count))
       return false;
+    DCHECK_GE(remaining, 0);
   } else {
     SBPrefix host;
     const int min_size = sizeof(SBPrefix) + 1;
     while (remaining >= min_size) {
-      ReadHostAndPrefixCount(&chunk_data, &remaining, &host, &prefix_count);
+      if (!ReadHostAndPrefixCount(&chunk_data, &remaining,
+                                  &host, &prefix_count)) {
+        return false;
+      }
+      DCHECK_GE(remaining, 0);
       SBChunkHost chunk_host;
       chunk_host.host = host;
       chunk_host.entry = SBEntry::Create(type, prefix_count);
@@ -358,6 +363,7 @@ bool SafeBrowsingProtocolParser::ParseAddChunk(const std::string& list_name,
       if (!ReadPrefixes(&chunk_data, &remaining, chunk_host.entry,
                         prefix_count))
         return false;
+      DCHECK_GE(remaining, 0);
     }
   }
   return remaining == 0;
@@ -374,7 +380,8 @@ bool SafeBrowsingProtocolParser::ParseSubChunk(const std::string& list_name,
   SBEntry::Type type = hash_len == sizeof(SBPrefix) ?
       SBEntry::SUB_PREFIX : SBEntry::SUB_FULL_HASH;
 
-  if (list_name == safe_browsing_util::kBinHashList) {
+  if (list_name == safe_browsing_util::kBinHashList ||
+      list_name == safe_browsing_util::kDownloadWhiteList) {
     SBChunkHost chunk_host;
     // Set host to 0 and it won't be used for kBinHashList.
     chunk_host.host = 0;
@@ -384,31 +391,43 @@ bool SafeBrowsingProtocolParser::ParseSubChunk(const std::string& list_name,
     chunk_host.entry = SBEntry::Create(type, prefix_count);
     if (!ReadPrefixes(&chunk_data, &remaining, chunk_host.entry, prefix_count))
       return false;
+    DCHECK_GE(remaining, 0);
     hosts->push_back(chunk_host);
   } else {
     SBPrefix host;
     const int min_size = 2 * sizeof(SBPrefix) + 1;
     while (remaining >= min_size) {
-      ReadHostAndPrefixCount(&chunk_data, &remaining, &host, &prefix_count);
+      if (!ReadHostAndPrefixCount(&chunk_data, &remaining,
+                                  &host, &prefix_count)) {
+        return false;
+      }
+      DCHECK_GE(remaining, 0);
       SBChunkHost chunk_host;
       chunk_host.host = host;
       chunk_host.entry = SBEntry::Create(type, prefix_count);
       hosts->push_back(chunk_host);
       if (prefix_count == 0) {
         // There is only an add chunk number (no prefixes).
-        chunk_host.entry->set_chunk_id(ReadChunkId(&chunk_data, &remaining));
+        int chunk_id;
+        if (!ReadChunkId(&chunk_data, &remaining, &chunk_id))
+          return false;
+        DCHECK_GE(remaining, 0);
+        chunk_host.entry->set_chunk_id(chunk_id);
         continue;
       }
       if (!ReadPrefixes(&chunk_data, &remaining, chunk_host.entry,
                         prefix_count))
         return false;
+      DCHECK_GE(remaining, 0);
     }
   }
   return remaining == 0;
 }
 
-void SafeBrowsingProtocolParser::ReadHostAndPrefixCount(
+bool SafeBrowsingProtocolParser::ReadHostAndPrefixCount(
     const char** data, int* remaining, SBPrefix* host, int* count) {
+  if (static_cast<size_t>(*remaining) < sizeof(SBPrefix) + 1)
+    return false;
   // Next 4 bytes are the host prefix.
   memcpy(host, *data, sizeof(SBPrefix));
   *data += sizeof(SBPrefix);
@@ -418,15 +437,23 @@ void SafeBrowsingProtocolParser::ReadHostAndPrefixCount(
   *count = static_cast<unsigned char>(**data);
   *data += 1;
   *remaining -= 1;
+  DCHECK_GE(*remaining, 0);
+  return true;
 }
 
-int SafeBrowsingProtocolParser::ReadChunkId(
-    const char** data, int* remaining) {
-  int chunk_number;
-  memcpy(&chunk_number, *data, sizeof(chunk_number));
-  *data += sizeof(chunk_number);
-  *remaining -= sizeof(chunk_number);
-  return htonl(chunk_number);
+bool SafeBrowsingProtocolParser::ReadChunkId(
+    const char** data, int* remaining, int* chunk_id) {
+  // Protocol says four bytes, not sizeof(int).  Make sure those
+  // values are the same.
+  DCHECK_EQ(sizeof(*chunk_id), 4u);

[noelutz, 2011/12/08 03:26:54]

good call.

[Scott Hess - ex-Googler, 2011/12/08 04:12:44]

For all that I said "I don't have time to rewrite the parsing code" in the
associated bug, as I think about it it seems to me that cleaning it up might
only take a few hours.  Maybe a good investment of free time around the
holidays...

+  if (static_cast<size_t>(*remaining) < sizeof(*chunk_id))
+    return false;
+  memcpy(chunk_id, *data, sizeof(*chunk_id));
+  *data += sizeof(*chunk_id);
+  *remaining -= sizeof(*chunk_id);
+  *chunk_id = htonl(*chunk_id);
+  DCHECK_GE(*remaining, 0);
+  return true;
 }
 
 bool SafeBrowsingProtocolParser::ReadPrefixes(
@@ -434,20 +461,25 @@ bool SafeBrowsingProtocolParser::ReadPrefixes(
   int hash_len = entry->HashLen();
   for (int i = 0; i < count; ++i) {
     if (entry->IsSub()) {
-      entry->SetChunkIdAtPrefix(i, ReadChunkId(data, remaining));
-      if (*remaining <= 0)
+      int chunk_id;
+      if (!ReadChunkId(data, remaining, &chunk_id))
         return false;
+      DCHECK_GE(*remaining, 0);
+      entry->SetChunkIdAtPrefix(i, chunk_id);
     }
 
+    if (*remaining < hash_len)
+      return false;
     if (entry->IsPrefix()) {
+      DCHECK_EQ(hash_len, (int)sizeof(SBPrefix));
       entry->SetPrefixAt(i, *reinterpret_cast<const SBPrefix*>(*data));
     } else {
+      DCHECK_EQ(hash_len, (int)sizeof(SBFullHash));
       entry->SetFullHashAt(i, *reinterpret_cast<const SBFullHash*>(*data));
     }
     *data += hash_len;
     *remaining -= hash_len;
-    if (*remaining < 0)
-      return false;
+    DCHECK_GE(*remaining, 0);
   }
 
   return true;