Validate that zip entry filenames are UTF8.

chrome/common/safe_browsing/zip_analyzer.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.
 
 #include "chrome/common/safe_browsing/zip_analyzer.h"
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "chrome/common/safe_browsing/binary_feature_extractor.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
@@ skipping 82 matching lines @@
   bool advanced = true;
   for (; reader.HasMore(); advanced = reader.AdvanceToNextEntry()) {
     if (!advanced) {
       DVLOG(1) << "Could not advance to next entry, aborting zip scan.";
       return;
     }
     if (!reader.OpenCurrentEntryInZip()) {
       DVLOG(1) << "Failed to open current entry in zip file";
       continue;
     }
+    if (reader.current_entry_info()->is_unsafe()) {
+      DVLOG(1) << "Found unsafe entry in zip file.";
+      results->has_unsafe_file = true;
+      continue;
+    }
     const base::FilePath& file = reader.current_entry_info()->file_path();

[grt (UTC plus 2), 2015/03/23 20:11:55]

It seems to me that this file_path() member should not be used if the entry
is_unsafe() since it may not be UTF8 and who knows what the UTF16 conversion
would result in.

[mattm, 2015/03/23 23:04:13]

Hm, not sure this is necessary. I assume invalid chars would just get changed to
unicode replacement char? And the filename isn't used when extracting/analyzing
the file anyway, so I'm not seeing the risk. If IsBinaryFile or IsArchiveFile
get confused, they might think it's not binary when it actually was, but that's
not worse than just always skipping it.  Also is_unsafe includes more conditions
than just bad encoding.

May still be worth having the histogram, but I'm not sure that skipping the rest
of the processing is good.

[grt (UTC plus 2), 2015/03/24 18:55:23]

The motivation for this is that the file_basename member of the protobuf message
must be UTF8. I just re-noticed that we have a UTF8 validator, so I've switched
to that. It appears that the entry's file_path() could be invalid UTF8 on some
POSIX platforms where no validation is done in FilePath::FromUTF8Unsafe.

     if (download_protection_util::IsBinaryFile(file)) {
       // Don't consider an archived archive to be executable, but record
       // a histogram.
       if (download_protection_util::IsArchiveFile(file)) {
         results->has_archive = true;
       } else {
         DVLOG(2) << "Downloaded a zipped executable: " << file.value();
         results->has_executable = true;
         AnalyzeContainedFile(binary_feature_extractor, file, &reader,
                              &temp_file, results->archived_binary.Add());
       }
     } else {
       DVLOG(3) << "Ignoring non-binary file: " << file.value();
     }
   }
   results->success = true;
 }
 
 }  // namespace zip_analyzer
 }  // namespace safe_browsing