Validate that zip entry filenames are UTF8.

Validate that zip entry filenames are UTF8.

BUG=462584
Committed: https://crrev.com/105d38dd7029364e9b164606cb2220f0d6ba11a4
Cr-Commit-Position: refs/heads/master@{#322085}

[grt (UTC plus 2), 2015-03-23 20:09:09]

grt@chromium.org changed reviewers:
+ mattm@chromium.org

[grt (UTC plus 2), 2015-03-23 20:11:35]

grt@chromium.org changed reviewers:
+ mpearson@chromium.org

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

mpearson: histograms.xml
mattm: everything else

Thanks.

https://codereview.chromium.org/1028243002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/zip_analyzer.cc (right):

https://codereview.chromium.org/1028243002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/zip_analyzer.cc:108: const base::FilePath& file =
reader.current_entry_info()->file_path();
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]

https://codereview.chromium.org/1028243002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/zip_analyzer.cc (right):

https://codereview.chromium.org/1028243002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/zip_analyzer.cc:108: const base::FilePath& file =
reader.current_entry_info()->file_path();
On 2015/03/23 20:11:55, grt wrote:
> 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.

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.

[Mark P, 2015-03-24 18:13:34]

https://codereview.chromium.org/1028243002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1028243002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:32662: +<histogram
name="SBClientDownload.ZipFileHasUnsafeFile" enum="BooleanHit">
Do you feel that BooleanHit is somehow more appropriate than Boolean here?

https://codereview.chromium.org/1028243002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:32666: +    file with an unsafe name.
For clarity, perhaps add something like (if true):
Emitted each time a zip file is successfully downloaded and analyzed.

[grt (UTC plus 2), 2015-03-24 18:53:30]

grt@chromium.org changed reviewers:
- mpearson@chromium.org

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

Thanks, PTAL.

https://codereview.chromium.org/1028243002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/zip_analyzer.cc (right):

https://codereview.chromium.org/1028243002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/zip_analyzer.cc:108: const base::FilePath& file =
reader.current_entry_info()->file_path();
On 2015/03/23 23:04:13, mattm wrote:
> On 2015/03/23 20:11:55, grt wrote:
> > 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.
> 
> 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.

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.

[mattm, 2015-03-24 21:28:27]

lgtm

[grt (UTC plus 2), 2015-03-24 21:43:42]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2015-03-24 21:43:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1028243002/20001

[commit-bot: I haz the power, 2015-03-24 23:02:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-24 23:03:33]

Patchset 2 (id:??) landed as
https://crrev.com/105d38dd7029364e9b164606cb2220f0d6ba11a4
Cr-Commit-Position: refs/heads/master@{#322085}