Fix safe-browsing sub parsing for download whitelist.

chrome/browser/safe_browsing/protocol_parser.cc

 // Copyright (c) 2011 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.
 //
 // Parse the data returned from the SafeBrowsing v2.1 protocol response.
 
 #include <stdlib.h>
 
 #include "base/format_macros.h"
 #include "base/logging.h"
@@ skipping 328 matching lines @@
       list_name == safe_browsing_util::kDownloadWhiteList) {
     // These lists only contain prefixes, no HOSTKEY and COUNT.
     DCHECK_EQ(0, remaining % hash_len);
     prefix_count = remaining / hash_len;
     SBChunkHost chunk_host;
     chunk_host.host = 0;
     chunk_host.entry = SBEntry::Create(type, prefix_count);
     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);
       hosts->push_back(chunk_host);
       if (!ReadPrefixes(&chunk_data, &remaining, chunk_host.entry,
                         prefix_count))
         return false;
+      DCHECK_GE(remaining, 0);
     }
   }
   return remaining == 0;
 }
 
 bool SafeBrowsingProtocolParser::ParseSubChunk(const std::string& list_name,
                                                const char* data,
                                                int data_len,
                                                int hash_len,
                                                std::deque<SBChunkHost>* hosts) {
   int remaining = data_len;
   const char* chunk_data = data;
   int prefix_count;
   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;
     // kBinHashList only contains (add_chunk_number, prefix) pairs, no HOSTKEY
     // and COUNT. |add_chunk_number| is int32.
     prefix_count = remaining / (sizeof(int32) + hash_len);
     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);
   *remaining -= sizeof(SBPrefix);
 
   // Next 1 byte is the prefix count (could be zero, but never negative).
   *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(
     const char** data, int* remaining, SBEntry* entry, int count) {
   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;
 }
 
 bool SafeBrowsingProtocolParser::ParseNewKey(const char* chunk_data,
                                              int chunk_length,
                                              std::string* client_key,
                                              std::string* wrapped_key) {
   DCHECK(client_key && wrapped_key);
@@ skipping 26 matching lines @@
 
     data += line.size() + 1;
     remaining -= static_cast<int>(line.size()) + 1;
   }
 
   if (client_key->empty() || wrapped_key->empty())
     return false;
 
   return true;
 }