Neutralize dangerous subresource files during Save Page.

Neutralize dangerous subresource files during Save Page.

Downloading a complete page using "Save as..." can result in downloading
hundreds of subresources. The user often isn't interested in accessing
individual resources directly while they are on disk. In addition,
scanning hundreds of files during a single save page operation isn't
currently practical.

In order to mitigate the potential risk of leaving dangerous files
around on the users' filesystem, this CL renames known dangerous files
with an additional ".download" extension. I.e. A subresource named
foo.exe would be saved as foo.exe.download.

The code review includes lists of file types that are known to be
affected by this change. Notable file types include .js, .swf, and
.class. As a side-effect of the rename, they will not receive the
correct MIME type when loaded via a file:// URL. The saved page should
still function correctly even with the renamed resources.

R=nparker@chromium.org, jam@chromium.org
BUG=599224
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/953b8f294a892c364de0d9805156422ace1ce49c
Cr-Commit-Position: refs/heads/master@{#401729}

[asanka, 2016-06-13 16:37:52]

nparker: PTAL?

Looks like this will cause the following file types to be saved as
"foo.type.download" instead of "foo.type".


[
  "swf",
  "spl",
  "crx",
  "class",
  "jar",
  "jnlp",
  "pl",
  "py",
  "pyc",
  "pyw",
  "rb",
  "efi",
  "msi",
  "msp",
  "mst",
  "ade",
  "adp",
  "mad",
  "maf",
  "mag",
  "mam",
  "maq",
  "mar",
  "mas",
  "mat",
  "mav",
  "maw",
  "mda",
  "mdb",
  "mde",
  "mdt",
  "mdw",
  "mdz",
  "ocx",
  "ops",
  "paf",
  "pcd",
  "pif",
  "plg",
  "prf",
  "prg",
  "pst",
  "partial",
  "xrm-ms",
  "rels",
  "xml",
  "xsl",
  "ps1",
  "ps1xml",
  "ps2",
  "ps2xml",
  "psc1",
  "psc2",
  "url",
  "website",
  "js",
  "jse",
  "vb",
  "vbe",
  "vbs",
  "vbscript",
  "ws",
  "wsc",
  "wsf",
  "wsh",
  "msh",
  "msh1",
  "msh2",
  "mshxml",
  "msh1xml",
  "msh2xml",
  "ad",
  "app",
  "application",
  "appref-ms",
  "asp",
  "asx",
  "bas",
  "bat",
  "cfg",
  "chi",
  "chm",
  "cmd",
  "com",
  "cpl",
  "crt",
  "dll",
  "drv",
  "eml",
  "exe",
  "fon",
  "fxp",
  "gadget",
  "grp",
  "hlp",
  "hta",
  "htt",
  "inf",
  "ini",
  "ins",
  "inx",
  "isu",
  "isp",
  "job",
  "lnk",
  "local",
  "manifest",
  "mau",
  "mht",
  "mhtml",
  "mmc",
  "mof",
  "msc",
  "msg",
  "reg",
  "rgs",
  "scf",
  "scr",
  "sct",
  "search-ms",
  "shb",
  "shs",
  "sys",
  "u3p",
  "vsd",
  "vsmacros",
  "vss",
  "vst",
  "vsw",
  "xbap",
  "xnk"
]

Of these, the .js is the most likely to trip developers. But .js can also be
associated with cscript.exe which leads me to believe that the renaming should
be considered valid.

[Nathan Parker, 2016-06-13 21:15:01]

Cool!

https://codereview.chromium.org/2060923002/diff/20001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2060923002/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:442: if
(!file_type_policies)
This shouldn't be necessary.  The Singleton guarantees it.

https://codereview.chromium.org/2060923002/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:446:
safe_browsing::DownloadFileType::NOT_DANGEROUS)
So we're not actually checking with safe browsing here, we're just renaming any
file type that's potentially dangerous, correct?  That does mean that things
like .zips (NOT_DANGEROUS) won't be adjusted or scanned. I suspect that's ok,
since that'd be a lot of friction to overcome.... Except... if they followed
this with a .search-ms file to help the user find it.  I'm leaning towards
saying this is sufficient.

https://codereview.chromium.org/2060923002/diff/20001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/2060923002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:453: file_path =
file_path.ReplaceExtension(kDefaultHtmlExtension);
Does this remove the existing extension?  Might the original one be useful to
the user?  I'm not sure.

[asanka, 2016-06-14 21:24:07]

https://codereview.chromium.org/2060923002/diff/20001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2060923002/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:446:
safe_browsing::DownloadFileType::NOT_DANGEROUS)
On 2016/06/13 at 21:15:00, Nathan Parker wrote:
> So we're not actually checking with safe browsing here, we're just renaming
any file type that's potentially dangerous, correct?  That does mean that things
like .zips (NOT_DANGEROUS) won't be adjusted or scanned. I suspect that's ok,
since that'd be a lot of friction to overcome.... Except... if they followed
this with a .search-ms file to help the user find it.  I'm leaning towards
saying this is sufficient.

Yeah. I was a bit on the fence about what to do with supported-but-not-dangerous
file types.

These are:

[
  "bin",
  "pdf",
  "rtf",
  "001",
  "7z",
  "ace",
  "arc",
  "arj",
  "b64",
  "balz",
  "bhx",
  "bz",
  "bz2",
  "bzip2",
  "cab",
  "cpio",
  "fat",
  "gz",
  "gzip",
  "hfs",
  "hqx",
  "iso",
  "lha",
  "lpaq1",
  "lpaq5",
  "lpaq8",
  "lzh",
  "lzma",
  "mim",
  "ntfs",
  "paq8f",
  "paq8jd",
  "paq8l",
  "paq8o",
  "pea",
  "quad",
  "r00",
  "r01",
  "r02",
  "r03",
  "r04",
  "r05",
  "r06",
  "r07",
  "r08",
  "r09",
  "r10",
  "r11",
  "r12",
  "r13",
  "r14",
  "r15",
  "r16",
  "r17",
  "r18",
  "r19",
  "r20",
  "r21",
  "r22",
  "r23",
  "r24",
  "r25",
  "r26",
  "r27",
  "r28",
  "r29",
  "rar",
  "squashfs",
  "swm",
  "tar",
  "taz",
  "tbz",
  "tbz2",
  "tgz",
  "tpz",
  "txz",
  "tz",
  "udf",
  "uu",
  "uue",
  "vhd",
  "vhdx",
  "vmdk",
  "wim",
  "wrc",
  "xar",
  "xxe",
  "xz",
  "z",
  "zip",
  "zipx",
  "zpaq",
  "torrent",
  "svg",
  "apk"
]

By the way, why aren't the archive file types considered dangerous? If safe
browsing is enabled, then we won't warn since we wait to act on the SB ping. If
safe browsing is disabled shouldn't Chrome prompt for these file types?

https://codereview.chromium.org/2060923002/diff/20001/content/browser/downloa...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/2060923002/diff/20001/content/browser/downloa...
content/browser/download/save_package.cc:453: file_path =
file_path.ReplaceExtension(kDefaultHtmlExtension);
On 2016/06/13 at 21:15:00, Nathan Parker wrote:
> Does this remove the existing extension?  Might the original one be useful to
the user?  I'm not sure.

Indeed. Now using .AddExtension() instead.

[Nathan Parker, 2016-06-15 00:01:21]

lgtm

https://codereview.chromium.org/2060923002/diff/20001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2060923002/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:446:
safe_browsing::DownloadFileType::NOT_DANGEROUS)
On 2016/06/14 21:24:07, asanka wrote:
> On 2016/06/13 at 21:15:00, Nathan Parker wrote:
> > So we're not actually checking with safe browsing here, we're just renaming
> any file type that's potentially dangerous, correct?  That does mean that
things
> like .zips (NOT_DANGEROUS) won't be adjusted or scanned. I suspect that's ok,
> since that'd be a lot of friction to overcome.... Except... if they followed
> this with a .search-ms file to help the user find it.  I'm leaning towards
> saying this is sufficient.
> 
> Yeah. I was a bit on the fence about what to do with
supported-but-not-dangerous
> file types.
> 
> These are:
> 
> [
>   "bin",
>   "pdf",
>   "rtf",
>   "001",
>   "7z",
>   "ace",
>   "arc",
>   "arj",
>   "b64",
>   "balz",
>   "bhx",
>   "bz",
>   "bz2",
>   "bzip2",
>   "cab",
>   "cpio",
>   "fat",
>   "gz",
>   "gzip",
>   "hfs",
>   "hqx",
>   "iso",
>   "lha",
>   "lpaq1",
>   "lpaq5",
>   "lpaq8",
>   "lzh",
>   "lzma",
>   "mim",
>   "ntfs",
>   "paq8f",
>   "paq8jd",
>   "paq8l",
>   "paq8o",
>   "pea",
>   "quad",
>   "r00",
>   "r01",
>   "r02",
>   "r03",
>   "r04",
>   "r05",
>   "r06",
>   "r07",
>   "r08",
>   "r09",
>   "r10",
>   "r11",
>   "r12",
>   "r13",
>   "r14",
>   "r15",
>   "r16",
>   "r17",
>   "r18",
>   "r19",
>   "r20",
>   "r21",
>   "r22",
>   "r23",
>   "r24",
>   "r25",
>   "r26",
>   "r27",
>   "r28",
>   "r29",
>   "rar",
>   "squashfs",
>   "swm",
>   "tar",
>   "taz",
>   "tbz",
>   "tbz2",
>   "tgz",
>   "tpz",
>   "txz",
>   "tz",
>   "udf",
>   "uu",
>   "uue",
>   "vhd",
>   "vhdx",
>   "vmdk",
>   "wim",
>   "wrc",
>   "xar",
>   "xxe",
>   "xz",
>   "z",
>   "zip",
>   "zipx",
>   "zpaq",
>   "torrent",
>   "svg",
>   "apk"
> ]
> 
> By the way, why aren't the archive file types considered dangerous? If safe
> browsing is enabled, then we won't warn since we wait to act on the SB ping.
If
> safe browsing is disabled shouldn't Chrome prompt for these file types?


While it's true that an archive can contain an executable, I think it would be
confusing to warn the user if we didn't KNOW it had an executable. I suspect
we'd lose credibility if we said "this .zip file can harm your computer" when
it's just full of text files.  Ya?

[asanka, 2016-06-16 01:53:08]

Description was changed from

==========
Neutralize dangerous subresource files during Save Page.

R=nparker@chromium.org
BUG=599224
==========

to

==========
Neutralize dangerous subresource files during Save Page.

Downloading a complete page using "Save as..." can result in downloading
hundreds of subresources. The user often isn't interested in accessing
individual resources directly while they are on disk. In addition,
scanning hundreds of files during a single save page operation isn't
currently practical.

In order to mitigate the potential risk of leaving dangerous files
around on the users' filesystem, this CL renames known dangerous files
with an additional ".download" extension. I.e. A subresource named
foo.exe would be saved as foo.exe.download.

The code review includes lists of file types that are known to be
affected by this change. The one notable file type is .js. The saved
page should still function correctly even with the renamed resources.

R=nparker@chromium.org, jam@chromium.org
BUG=599224
==========

[asanka, 2016-06-16 01:56:16]

Description was changed from

==========
Neutralize dangerous subresource files during Save Page.

Downloading a complete page using "Save as..." can result in downloading
hundreds of subresources. The user often isn't interested in accessing
individual resources directly while they are on disk. In addition,
scanning hundreds of files during a single save page operation isn't
currently practical.

In order to mitigate the potential risk of leaving dangerous files
around on the users' filesystem, this CL renames known dangerous files
with an additional ".download" extension. I.e. A subresource named
foo.exe would be saved as foo.exe.download.

The code review includes lists of file types that are known to be
affected by this change. The one notable file type is .js. The saved
page should still function correctly even with the renamed resources.

R=nparker@chromium.org, jam@chromium.org
BUG=599224
==========

to

==========
Neutralize dangerous subresource files during Save Page.

Downloading a complete page using "Save as..." can result in downloading
hundreds of subresources. The user often isn't interested in accessing
individual resources directly while they are on disk. In addition,
scanning hundreds of files during a single save page operation isn't
currently practical.

In order to mitigate the potential risk of leaving dangerous files
around on the users' filesystem, this CL renames known dangerous files
with an additional ".download" extension. I.e. A subresource named
foo.exe would be saved as foo.exe.download.

The code review includes lists of file types that are known to be
affected by this change. Notable file types include .js, .swf, and
.class. As a side-effect of the rename, they will not receive the
correct MIME type when loaded via a file:// URL. The saved page should
still function correctly even with the renamed resources.

R=nparker@chromium.org, jam@chromium.org
BUG=599224
==========

[asanka, 2016-06-16 18:35:16]

asanka@chromium.org changed reviewers:
+ jam@chromium.org

[asanka, 2016-06-16 18:35:17]

Thanks!

+jam: for content/public/browser/download_manager_delegate.h

   I had to add a DownloadManagerDelegate method for overriding subresource
names. Chrome uses it to neutralize dangerous file extensions based on the local
SafeBrowsing file type policies.

https://codereview.chromium.org/2060923002/diff/20001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2060923002/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:446:
safe_browsing::DownloadFileType::NOT_DANGEROUS)
On 2016/06/15 at 00:01:21, Nathan Parker wrote:
[...]
> 
> While it's true that an archive can contain an executable, I think it would be
confusing to warn the user if we didn't KNOW it had an executable. I suspect
we'd lose credibility if we said "this .zip file can harm your computer" when
it's just full of text files.  Ya?

True. The danger classifications for archives predate Chrome's ability to look
inside them. So at the time the only option was to warn the user.

Also, as far as the issue of archive formats as subresources; there's sufficient
friction here that I don't feel special handling is necessary.

[Nathan Parker, 2016-06-16 18:38:07]

Can you also update chrome/browser/resources/safe_browsing/README.md to describe
this added effect of the danger setting?

[asanka, 2016-06-16 18:50:36]

Description was changed from

==========
Neutralize dangerous subresource files during Save Page.

Downloading a complete page using "Save as..." can result in downloading
hundreds of subresources. The user often isn't interested in accessing
individual resources directly while they are on disk. In addition,
scanning hundreds of files during a single save page operation isn't
currently practical.

In order to mitigate the potential risk of leaving dangerous files
around on the users' filesystem, this CL renames known dangerous files
with an additional ".download" extension. I.e. A subresource named
foo.exe would be saved as foo.exe.download.

The code review includes lists of file types that are known to be
affected by this change. Notable file types include .js, .swf, and
.class. As a side-effect of the rename, they will not receive the
correct MIME type when loaded via a file:// URL. The saved page should
still function correctly even with the renamed resources.

R=nparker@chromium.org, jam@chromium.org
BUG=599224
==========

to

==========
Neutralize dangerous subresource files during Save Page.

Downloading a complete page using "Save as..." can result in downloading
hundreds of subresources. The user often isn't interested in accessing
individual resources directly while they are on disk. In addition,
scanning hundreds of files during a single save page operation isn't
currently practical.

In order to mitigate the potential risk of leaving dangerous files
around on the users' filesystem, this CL renames known dangerous files
with an additional ".download" extension. I.e. A subresource named
foo.exe would be saved as foo.exe.download.

The code review includes lists of file types that are known to be
affected by this change. Notable file types include .js, .swf, and
.class. As a side-effect of the rename, they will not receive the
correct MIME type when loaded via a file:// URL. The saved page should
still function correctly even with the renamed resources.

R=nparker@chromium.org, jam@chromium.org
BUG=599224
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[asanka, 2016-06-16 18:51:17]

https://codereview.chromium.org/2060923002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/safe_browsing/README.md (right):

https://codereview.chromium.org/2060923002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/safe_browsing/README.md:78: 
+nparker: PTAL?

[Nathan Parker, 2016-06-16 20:34:03]

lgtm

https://codereview.chromium.org/2060923002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/safe_browsing/README.md (right):

https://codereview.chromium.org/2060923002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/safe_browsing/README.md:78: 
On 2016/06/16 18:51:17, asanka wrote:
> +nparker: PTAL?

LGTM.  Thanks.

[jam, 2016-06-16 23:13:06]

https://codereview.chromium.org/2060923002/diff/160001/content/public/browser...
File content/public/browser/download_manager_delegate.h (right):

https://codereview.chromium.org/2060923002/diff/160001/content/public/browser...
content/public/browser/download_manager_delegate.h:122: // Sanitize a filename
that's going to be used for a subresource of a
should SavePackage be in the method name to make it clear?

looking at the method name now, i wouldn't guess that.

https://codereview.chromium.org/2060923002/diff/160001/content/public/browser...
content/public/browser/download_manager_delegate.h:129: // |filename->DirName()|
must be the same both before and after the call.
why not just pass in a file name that doesn't include directory, and then if
it's changed update the original FilePath object? i.e. seems less error prone
for embedders.

[asanka, 2016-06-21 15:39:38]

jam: PTAL?

https://codereview.chromium.org/2060923002/diff/160001/content/public/browser...
File content/public/browser/download_manager_delegate.h (right):

https://codereview.chromium.org/2060923002/diff/160001/content/public/browser...
content/public/browser/download_manager_delegate.h:122: // Sanitize a filename
that's going to be used for a subresource of a
On 2016/06/16 at 23:13:06, jam wrote:
> should SavePackage be in the method name to make it clear?
> 
> looking at the method name now, i wouldn't guess that.

Done.

https://codereview.chromium.org/2060923002/diff/160001/content/public/browser...
content/public/browser/download_manager_delegate.h:129: // |filename->DirName()|
must be the same both before and after the call.
On 2016/06/16 at 23:13:06, jam wrote:
> why not just pass in a file name that doesn't include directory, and then if
it's changed update the original FilePath object? i.e. seems less error prone
for embedders.

Done. This was already the case, but I've commented it to be so and updated the
DCHECK to verify.

[jam, 2016-06-23 16:06:36]

lgtm

(sorry for delay I missed your reply, feel free to IM me if I take more than a
few hours)

[asanka, 2016-06-23 16:08:55]

On 2016/06/23 at 16:06:36, jam wrote:
> lgtm
> 
> (sorry for delay I missed your reply, feel free to IM me if I take more than a
few hours)

No worries. Thanks!

[asanka, 2016-06-23 20:31:06]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-06-23 20:31:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2060923002/#ps200001
(title: "Catch up with ToT")

[commit-bot: I haz the power, 2016-06-23 20:32:08]

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

[commit-bot: I haz the power, 2016-06-23 21:33:53]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-23 21:37:07]

Description was changed from

==========
Neutralize dangerous subresource files during Save Page.

Downloading a complete page using "Save as..." can result in downloading
hundreds of subresources. The user often isn't interested in accessing
individual resources directly while they are on disk. In addition,
scanning hundreds of files during a single save page operation isn't
currently practical.

In order to mitigate the potential risk of leaving dangerous files
around on the users' filesystem, this CL renames known dangerous files
with an additional ".download" extension. I.e. A subresource named
foo.exe would be saved as foo.exe.download.

The code review includes lists of file types that are known to be
affected by this change. Notable file types include .js, .swf, and
.class. As a side-effect of the rename, they will not receive the
correct MIME type when loaded via a file:// URL. The saved page should
still function correctly even with the renamed resources.

R=nparker@chromium.org, jam@chromium.org
BUG=599224
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Neutralize dangerous subresource files during Save Page.

Downloading a complete page using "Save as..." can result in downloading
hundreds of subresources. The user often isn't interested in accessing
individual resources directly while they are on disk. In addition,
scanning hundreds of files during a single save page operation isn't
currently practical.

In order to mitigate the potential risk of leaving dangerous files
around on the users' filesystem, this CL renames known dangerous files
with an additional ".download" extension. I.e. A subresource named
foo.exe would be saved as foo.exe.download.

The code review includes lists of file types that are known to be
affected by this change. Notable file types include .js, .swf, and
.class. As a side-effect of the rename, they will not receive the
correct MIME type when loaded via a file:// URL. The saved page should
still function correctly even with the renamed resources.

R=nparker@chromium.org, jam@chromium.org
BUG=599224
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/953b8f294a892c364de0d9805156422ace1ce49c
Cr-Commit-Position: refs/heads/master@{#401729}
==========

[commit-bot: I haz the power, 2016-06-23 21:37:09]

Patchset 11 (id:??) landed as
https://crrev.com/953b8f294a892c364de0d9805156422ace1ce49c
Cr-Commit-Position: refs/heads/master@{#401729}