Safebrowsing: Fix failure parsing a gethash result with bad listname and metadata.

chrome/browser/safe_browsing/protocol_parser.cc

 // Copyright (c) 2012 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.
 
 // TODOv3(shess): Review these changes carefully.
 
 #include <stdlib.h>
 
@@ skipping 113 matching lines @@
     return true;
   }
 
  private:
   const char* data_;
   size_t length_;
 
   DISALLOW_COPY_AND_ASSIGN(BufferReader);
 };
 
+bool ParseGetHashMetadata(size_t hash_count, BufferReader* reader) {
+  for (size_t i = 0; i < hash_count; ++i) {
+    base::StringPiece line;
+    if (!reader->GetLine(&line))
+      return false;
+
+    size_t meta_data_len;
+    if (!base::StringToSizeT(line, &meta_data_len))
+      return false;
+
+    const void* meta_data;
+    if (!reader->RefData(&meta_data, meta_data_len))
+      return false;
+  }
+  return true;
+}
+
 }  // namespace
 
 namespace safe_browsing {
 
 // BODY          = CACHELIFETIME LF HASHENTRY* EOF
 // CACHELIFETIME = DIGIT+
 // HASHENTRY     = LISTNAME ":" HASHSIZE ":" NUMRESPONSES [":m"] LF
 //                 HASHDATA (METADATALEN LF METADATA)*
 // HASHSIZE      = DIGIT+                  # Length of each full hash
 // NUMRESPONSES  = DIGIT+                  # Number of full hashes in HASHDATA
@@ skipping 55 matching lines @@
     size_t hash_count;
     if (!base::StringToSizeT(hash_count_string, &hash_count))
       return false;
 
     if (hash_len * hash_count > reader.length())
       return false;
 
     // Ignore hash results from lists we don't recognize.
     if (full_hash.list_id < 0) {
       reader.Advance(hash_len * hash_count);
+      if (has_metadata && !ParseGetHashMetadata(hash_count, &reader))

[noé, 2014/09/18 20:41:07]

nit: can you add a TODO here to send the metadata back to the client that
triggered the gethash?  This information should be used to know which malware
interstitial to show (distribution vs. landing).

[mattm, 2014/09/18 20:50:32]

Done. 

I need to pull that out of my old api 2.3 WIP cl and update it...

+        return false;
       continue;
     }
 
     for (size_t i = 0; i < hash_count; ++i) {
       if (!reader.GetData(&full_hash.hash, hash_len))
         return false;
       full_hashes->push_back(full_hash);
     }
 
     // Discard the metadata for now.
-    if (has_metadata) {
-      for (size_t i = 0; i < hash_count; ++i) {
-        base::StringPiece line;
-        if (!reader.GetLine(&line))
-          return false;
-
-        size_t meta_data_len;
-        if (!base::StringToSizeT(line, &meta_data_len))
-          return false;
-
-        const void* meta_data;
-        if (!reader.RefData(&meta_data, meta_data_len))
-          return false;
-      }
-    }
+    if (has_metadata && !ParseGetHashMetadata(hash_count, &reader))
+      return false;
   }
 
   return reader.empty();
 }
 
 // BODY       = HEADER LF PREFIXES EOF
 // HEADER     = PREFIXSIZE ":" LENGTH
 // PREFIXSIZE = DIGIT+         # Size of each prefix in bytes
 // LENGTH     = DIGIT+         # Size of PREFIXES in bytes
 std::string FormatGetHash(const std::vector<SBPrefix>& prefixes) {
@@ skipping 131 matching lines @@
   if (!list.adds.empty() && !list.subs.empty())
     formatted_results.append(":");
   if (!list.subs.empty())
     formatted_results.append("s:").append(list.subs);
   formatted_results.append("\n");
 
   return formatted_results;
 }
 
 }  // namespace safe_browsing