[SafeBrowsing] Conditionalize unverified download blocking on whitelist.

[SafeBrowsing] Conditionalize unverified download blocking on whitelist.

Update the policy for handling unverified downloads to allow downloads
that are requested by a document that's whitelisted. The whitelist being
used is the SafeBrowsing download whitelist and likely represents
legitimate downloads.

The requesting document is determined based on the originator of the
file chooser request. This originator is currently furnished by the
renderer and, as such, considered potentially tainted in the event of a
renderer compromise.

BUG=533579
Committed: https://crrev.com/1cbf498112576e057284126d6888e6811c7516b3
Cr-Commit-Position: refs/heads/master@{#357568}

[asanka, 2015-10-23 19:29:07]

asanka@chromium.org changed reviewers:
+ mattm@chromium.org, sky@chromium.org

[asanka, 2015-10-23 19:31:24]

asanka@chromium.org changed reviewers:
+ davidben@chromium.org, wfh@chromium.org

[asanka, 2015-10-26 21:05:31]

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

[asanka, 2015-10-26 21:28:14]

asanka@chromium.org changed reviewers:
+ pdr@chromium.org
- davidben@chromium.org

[asanka, 2015-10-26 21:30:36]

asanka@chromium.org changed reviewers:
+ tkent@chromium.org
- pdr@chromium.org

[asanka, 2015-10-26 21:31:50]

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

[asanka, 2015-10-26 21:32:49]

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

[Will Harris, 2015-10-26 23:11:42]

looks like security_exploit_browsertest.cc tests this IPC - can you add
something to verify that your new "requester" field is validated correctly?

[asanka, 2015-10-27 20:52:17]

Patchset #1 (id:1) has been deleted

[asanka, 2015-10-27 20:52:24]

Patchset #1 (id:20001) has been deleted

[asanka, 2015-10-27 20:52:29]

Patchset #1 (id:40001) has been deleted

[asanka, 2015-10-27 20:52:36]

Patchset #1 (id:60001) has been deleted

[asanka, 2015-10-27 20:52:41]

Patchset #1 (id:80001) has been deleted

[asanka, 2015-10-27 21:26:08]

This is an addendum to https://codereview.chromium.org/1409003002. This CL
allows SafeBrowsing whitelists to override the FinchTrial controlled blocking of
dangerous downloads through Flash. Using the whitelist would mitigate collateral
damage if Chrome were to block Flash based dangerous downloads by allowing Flash
applets hosted on whitelisted URLs to download such files. The whitelist
consists of high-traffic download sites that are believed to be hosting
legitimate content.

This CL plumbs the PPAPI's document URL alongside the filechooser request (in
the |requestor| field from WebFileChooserParams through to
content::FileChooserParams) so that the browser process can factor it in when
deciding whether to allow the download.

Notably, the requestor URL crosses a trust boundary (originates in the renderer
and passed along to the browser process), and hence isn't as trustworthy as a
URL that's determined entirely browser-side. In the event of a sandbox
compromise, this would allow the sandbox to show a file chooser dialog and save
something which the Finch trial would otherwise block.

bbudge:
  chrome/test/ppapi/ppapi_filechooser_browsertest.cc
  content/renderer/pepper/pepper_file_chooser_host.cc 

isherman:
  tools/metrics/histograms/histograms.xml

sky:
  chrome/browser/file_select_helper.cc
  chrome/browser/file_select_helper.h
  chrome/test/ppapi/ppapi_filechooser_browsertest.cc (owner)
  third_party/WebKit/Source/web/ChromeClientImpl.cpp
  third_party/WebKit/public/web/WebFileChooserParams.h

  For completeness I touched ChromeClientImpl to populate the requestor field in
the <input type=file> code path. However, currently nobody looks at the
requestor for 'Open' type file choosers. Let me know if you'd like me to not set
the requestor in ChromeClientImpl.

mattm:
  chrome/browser/safe_browsing/unverified_download_policy.cc
  chrome/browser/safe_browsing/unverified_download_policy.h
  chrome/browser/safe_browsing/unverified_download_policy_unittest.cc

davidben:
  content/public/common/file_chooser_params.h
  content/renderer/pepper/pepper_file_chooser_host.cc (owner)
  content/renderer/render_view_impl.cc

wfh:
  content/common/view_messages.h

  I originally looked at security_exploit_browsertest and decided not to put
tests there since a compromised renderer could in principal modify the
|requestor| field in ways that can't be detected through validation. E.g.: a
likely approach would be to set the |requestor| field to a trusted URL. The
ParamTraits<GURL> implementation already takes care of dealing with malformed
URLs. Let me know if I've misunderstood your comment.

[bbudge, 2015-10-28 00:13:20]

https://codereview.chromium.org/1410423003/diff/100001/chrome/test/ppapi/ppap...
File chrome/test/ppapi/ppapi_filechooser_browsertest.cc (right):

https://codereview.chromium.org/1410423003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_filechooser_browsertest.cc:256: namespace {
Can these classes be moved to the first anonymous namespace above?

https://codereview.chromium.org/1410423003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_filechooser_browsertest.cc:309:
IN_PROC_BROWSER_TEST_F(PPAPIFileChooserTestWithSBService,
Is there a reason this is in the anonymous namespace?

[mattm, 2015-10-28 01:31:46]

safe_browsing lgtm

[Will Harris, 2015-10-28 02:45:31]

content/common/view_messages.h lgtm

[asanka, 2015-10-28 14:10:04]

https://codereview.chromium.org/1410423003/diff/100001/chrome/test/ppapi/ppap...
File chrome/test/ppapi/ppapi_filechooser_browsertest.cc (right):

https://codereview.chromium.org/1410423003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_filechooser_browsertest.cc:256: namespace {
On 2015/10/28 at 00:13:20, bbudge wrote:
> Can these classes be moved to the first anonymous namespace above?

Done.

https://codereview.chromium.org/1410423003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_filechooser_browsertest.cc:309:
IN_PROC_BROWSER_TEST_F(PPAPIFileChooserTestWithSBService,
On 2015/10/28 at 00:13:20, bbudge wrote:
> Is there a reason this is in the anonymous namespace?

Nope. Accidental. Fixed!

[Ilya Sherman, 2015-10-28 18:21:50]

metrics lgtm

[bbudge, 2015-10-28 19:01:11]

Thanks LGTM

[asanka, 2015-10-29 20:59:40]

Thanks folks!

sky, tkent: ping?

[davidben, 2015-10-29 21:10:10]

content lgtm. Didn't look at the rest and am assuming other people did the main
review.

https://codereview.chromium.org/1410423003/diff/120001/content/public/common/...
File content/public/common/file_chooser_params.h (right):

https://codereview.chromium.org/1410423003/diff/120001/content/public/common/...
content/public/common/file_chooser_params.h:61: // untrustworthy since it is
specified by the sandbox and not validated.
[I see you mention this in comment #14 that we're doing things based on
something from the renderer. I'm assuming this is okay given security folks
stamped it. Dunno if it's worth a note in the CL description or a comment where
requestor is used?]

[sky, 2015-10-29 23:04:26]

What files do you need me to review?

[asanka, 2015-10-30 02:03:51]

On 2015/10/29 at 23:04:26, sky wrote:
> What files do you need me to review?

https://codereview.chromium.org/1410423003/#msg14 has more details about why and
a couple of caveats.

sky:
  chrome/browser/file_select_helper.cc
  chrome/browser/file_select_helper.h
  chrome/test/ppapi/ppapi_filechooser_browsertest.cc (owner)

  I changed the FileChooserParams parameter to a scoped_ptr<> so that we can
pass around a pointer instead of copying the struct for each callback.

tkent:
  third_party/WebKit/Source/web/ChromeClientImpl.cpp
  third_party/WebKit/public/web/WebFileChooserParams.h

  For completeness I touched ChromeClientImpl to populate the requestor field in
the <input type=file> code path. However, currently nobody looks at the
requestor for 'Open' type file choosers. Let me know if you'd like me to not set
the requestor in ChromeClientImpl.

[sky, 2015-10-30 15:13:55]

LGTM

[asanka, 2015-10-30 23:41:37]

On 2015/10/30 at 15:13:55, sky wrote:
> LGTM

Thanks!

tkent: ping!

[tkent, 2015-11-01 23:47:40]

third_part/WebKit lgtm

[asanka, 2015-11-03 03:44:18]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-03 03:45:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410423003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410423003/160001

[commit-bot: I haz the power, 2015-11-03 05:32:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-03 05:32:20]

Dry run: This issue passed the CQ dry run.

[asanka, 2015-11-03 14:23:15]

Thanks everyone!

https://codereview.chromium.org/1410423003/diff/120001/content/public/common/...
File content/public/common/file_chooser_params.h (right):

https://codereview.chromium.org/1410423003/diff/120001/content/public/common/...
content/public/common/file_chooser_params.h:61: // untrustworthy since it is
specified by the sandbox and not validated.
On 2015/10/29 at 21:10:10, davidben wrote:
> [I see you mention this in comment #14 that we're doing things based on
something from the renderer. I'm assuming this is okay given security folks
stamped it. Dunno if it's worth a note in the CL description or a comment where
requestor is used?]

Done. Added a comment at time of use. Also updated the CL description.

[asanka, 2015-11-03 14:24:50]

The CQ bit was checked by asanka@chromium.org

[asanka, 2015-11-03 14:24:51]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org,
wfh@chromium.org, bbudge@chromium.org, sky@chromium.org, tkent@chromium.org,
davidben@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1410423003/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-11-03 14:25:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410423003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410423003/160001

[commit-bot: I haz the power, 2015-11-03 18:28:24]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2015-11-03 18:30:51]

Patchset 4 (id:??) landed as
https://crrev.com/1cbf498112576e057284126d6888e6811c7516b3
Cr-Commit-Position: refs/heads/master@{#357568}