Add a new group policy to disable safe browsing for files downloaded from trusted sources.

Add a new group policy to disable safe browsing for files downloaded from trusted sources.

TBR=jochen@chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
Review-Url: https://codereview.chromium.org/2943763002
Cr-Commit-Position: refs/heads/master@{#484149}
Committed: https://chromium.googlesource.com/chromium/src/+/669452f6e42579d09851122adecb6f4a5297c9ea

[MAD, 2017-06-16 19:59:07]

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

[commit-bot: I haz the power, 2017-06-16 19:59:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[MAD, 2017-06-16 20:00:16]

mad@chromium.org changed reviewers:
+ rogerta@chromium.org

[MAD, 2017-06-16 20:00:17]

Care to do the initial Code review before I ask OWNERs to validate?

Thanks!

BYE
MAD

[MAD, 2017-06-16 20:01:49]

Description was changed from

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

BUG=723658
==========

to

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=jochen@chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

[MAD, 2017-06-16 20:01:49]

mad@chromium.org changed reviewers:
+ jochen@chromium.org

[commit-bot: I haz the power, 2017-06-16 20:36:20]

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

[commit-bot: I haz the power, 2017-06-16 20:36:23]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[MAD, 2017-06-16 20:47:36]

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

[commit-bot: I haz the power, 2017-06-16 20:47:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 22:52:15]

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

[commit-bot: I haz the power, 2017-06-16 22:52:16]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2017-06-19 18:51:30]

lgtm, one nit below.

https://codereview.chromium.org/2943763002/diff/40001/chrome/browser/download...
File chrome/browser/download/trusted_sources_manager_win.cc (right):

https://codereview.chromium.org/2943763002/diff/40001/chrome/browser/download...
chrome/browser/download/trusted_sources_manager_win.cc:31:
base::win::ScopedComPtr<IInternetSecurityManager> security_manager_;
No trailing underscore.

[MAD, 2017-06-19 19:26:17]

Description was changed from

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=jochen@chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

to

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=
to be added later: jochen@chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

[MAD, 2017-06-19 19:26:36]

Description was changed from

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=
to be added later: jochen@chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

to

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

to be added later: jochen@chromium.org
TBR=
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

[MAD, 2017-06-19 19:27:07]

Description was changed from

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

to be added later: jochen@chromium.org
TBR=
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

to

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=to be added later: j o c h e n @chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

[MAD, 2017-06-19 19:29:43]

Description was changed from

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=to be added later: j o c h e n @chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

to

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=to be added later: jochen@chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

[MAD, 2017-06-19 19:29:43]

mad@chromium.org changed reviewers:
- jochen@chromium.org

[MAD, 2017-06-19 19:33:47]

mad@chromium.org changed reviewers:
+ dtrainor@chromium.org, pastarmovj@chromium.org

[MAD, 2017-06-19 19:33:48]

Thanks Roger, will apply the fix later (having remote desktop access issues from
home right now). 

pastarmovj@chromium.org: Please review changes in group policy related files
(and others if you feel like it)

dtrainor@chromium.org: Please review changes in download related files (and
others as you wish ;-).

Thanks!

BYE
MAD

https://codereview.chromium.org/2943763002/diff/40001/chrome/browser/download...
File chrome/browser/download/trusted_sources_manager_win.cc (right):

https://codereview.chromium.org/2943763002/diff/40001/chrome/browser/download...
chrome/browser/download/trusted_sources_manager_win.cc:31:
base::win::ScopedComPtr<IInternetSecurityManager> security_manager_;
On 2017/06/19 18:51:29, Roger Tawa wrote:
> No trailing underscore.

D'Ho! Of course... Copied this code too quickly... :-)

[MAD, 2017-06-19 19:34:22]

Description was changed from

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=to be added later: jochen@chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

to

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=jochen@chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

[MAD, 2017-06-19 19:34:22]

mad@chromium.org changed reviewers:
+ jochen@chromium.org

[MAD, 2017-06-19 22:26:44]

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

[commit-bot: I haz the power, 2017-06-19 22:27:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 00:48:21]

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

[commit-bot: I haz the power, 2017-06-20 00:48:22]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[jochen (gone - plz use gerrit), 2017-06-20 06:47:06]

profile_impl.cc lgtm

[MAD, 2017-06-20 19:48:03]

24h ping for DTrainor, PastarmovJ, OWNER review request.

Thanks!

BYE
MAD

[pastarmovj, 2017-06-22 23:08:45]

generally ok with that but one question.

If I understand the code right trusted sources either come from Windows' zone
mapping or from a command line flag. Does this mean that a user can arbitrarily
extend the trusted sources list and together with this policy download arbitrary
files.

https://codereview.chromium.org/2943763002/diff/80001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2943763002/diff/80001/components/policy/resou...
components/policy/resources/policy_templates.json:1816: When not set (or set to
True, which is the default value), downloaded files are sent to be analyzed by
Safe Browsing, even when it's from a trusted source.
nit: remove the "which is the default value" because it basically means not set.

[David Trainor- moved to gerrit, 2017-06-23 07:32:47]

downloads/ lgtm % nits.  Sorry for the delay!

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate_unittest.cc
(right):

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate_unittest.cc:890: GURL
download_url("http://untrusted.com/best-download-ever.exe");
Haha :)

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
File chrome/browser/download/trusted_sources_manager.cc (right):

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
chrome/browser/download/trusted_sources_manager.cc:17:
TrustedSourcesManager::~TrustedSourcesManager() {}
= default

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
File chrome/browser/download/trusted_sources_manager_win.cc (right):

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
chrome/browser/download/trusted_sources_manager_win.cc:15: class
TrustedSourcesManagerWin : public TrustedSourcesManager {
Put this and all methods in an anonymous namespace?

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
chrome/browser/download/trusted_sources_manager_win.cc:24:
TrustedSourcesManagerWin::TrustedSourcesManagerWin() {}
= default on constructor/destructor

[MAD, 2017-06-29 14:44:53]

Sorry about the delay, got caught up in something else.

Thanks for the comments, all addressed.

To answer Julian's question, I followed the pattern used by the
net/http/url_security_manager.h, where non-Windows platform use command line
switches to add trusted servers. In the case of the security manager they
decided to override the Windows settings if a command line is specified whereas
I decided to make it additive instead, which makes more sense in our case.

Note that this policy is restricted to domain joined machined on Windows, so
shouldn't be exploitable with command line values unless the machine is already
compromised with hack to fake domain joined.

Clear?

If we think it's better to not support the command line on Windows, I don't mind
removing it.

Thanks again!

BYE
MAD

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate_unittest.cc
(right):

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate_unittest.cc:890: GURL
download_url("http://untrusted.com/best-download-ever.exe");
On 2017/06/23 07:32:47, David Trainor-ping after 12h wrote:
> Haha :)

Acknowledged.

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
File chrome/browser/download/trusted_sources_manager.cc (right):

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
chrome/browser/download/trusted_sources_manager.cc:17:
TrustedSourcesManager::~TrustedSourcesManager() {}
On 2017/06/23 07:32:47, David Trainor-ping after 12h wrote:
> = default

Done.

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
File chrome/browser/download/trusted_sources_manager_win.cc (right):

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
chrome/browser/download/trusted_sources_manager_win.cc:15: class
TrustedSourcesManagerWin : public TrustedSourcesManager {
On 2017/06/23 07:32:47, David Trainor-ping after 12h wrote:
> Put this and all methods in an anonymous namespace?

Done.

https://codereview.chromium.org/2943763002/diff/80001/chrome/browser/download...
chrome/browser/download/trusted_sources_manager_win.cc:24:
TrustedSourcesManagerWin::TrustedSourcesManagerWin() {}
On 2017/06/23 07:32:47, David Trainor-ping after 12h wrote:
> = default on constructor/destructor

Done.

https://codereview.chromium.org/2943763002/diff/80001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2943763002/diff/80001/components/policy/resou...
components/policy/resources/policy_templates.json:1816: When not set (or set to
True, which is the default value), downloaded files are sent to be analyzed by
Safe Browsing, even when it's from a trusted source.
On 2017/06/22 23:08:45, pastarmovj wrote:
> nit: remove the "which is the default value" because it basically means not
set.
>  

Done.

[MAD, 2017-06-29 14:45:09]

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

[commit-bot: I haz the power, 2017-06-29 14:45:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 16:03:34]

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

[commit-bot: I haz the power, 2017-06-29 16:03:36]

Dry run: This issue passed the CQ dry run.

[MAD, 2017-07-04 13:37:10]

Friendly ping for pastarmovj@ :-)

Thanks!

BYE
MAD

[MAD, 2017-07-04 15:00:09]

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

[commit-bot: I haz the power, 2017-07-04 15:00:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-04 16:23:06]

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

[commit-bot: I haz the power, 2017-07-04 16:23:12]

Dry run: This issue passed the CQ dry run.

[pastarmovj, 2017-07-04 16:38:34]

On 2017/07/04 13:37:10, MAD wrote:
> Friendly ping for pastarmovj@ :-)
> 
> Thanks!
> 
> BYE
> MAD

What I wanted to verify with my question is whether you have thought about the
following scenario - the admin enables this policy because he wants the user to
be able to easily download intranet resources which is safe but they still
should not be able to simply download whatever other url they stumble upon.
However they can easily achieve that by adding the url to the command line
switch and the restriction is lifted. I consider this especially problematic in
the presence of the download restriction policy too. If I get this right you can
have both to enable easy intranet downloads but keep random downloads at bey.
I am sorry I am delaying its landing that way but this question still troubles
me a bit. I know it is mostly perceived security instead of real one but still
I'd rather avoid the surprises for admins if possible.

[MAD, 2017-07-04 16:54:11]

OK, let's remove the command line support on Windows for now, and let's make the
policy windows only. Then we can add another policy to specify a list of trusted
source for other platforms.

[MAD, 2017-07-04 16:59:57]

Like this?

Thanks!

BYE
MAD

[MAD, 2017-07-04 17:02:23]

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

[commit-bot: I haz the power, 2017-07-04 17:02:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[MAD, 2017-07-04 17:03:05]

Oups, forgot to update unit test, another patch is on the way...

[pastarmovj, 2017-07-04 17:06:10]

LGTM with a few nits.

https://codereview.chromium.org/2943763002/diff/160001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2943763002/diff/160001/components/policy/reso...
components/policy/resources/policy_templates.json:1802: 'supported_on':
['chrome.win:61-'],
nit: Haven't we branched 61 by now? I think this has to be bumped up.

https://codereview.chromium.org/2943763002/diff/160001/components/policy/reso...
components/policy/resources/policy_templates.json:1820: But these restrictions
do not apply to the save / download of the currently displayed page, nor does it
apply to saving as PDF from the printing options.
nit. Remove the "But" and merge with the previous paragraph.

https://codereview.chromium.org/2943763002/diff/160001/components/policy/reso...
components/policy/resources/policy_templates.json:1823: to a Microsoft® Active
Directory® domain.''',
nit: Maybe put the "MS AD" in a ph tag to prevent the translation of this term.

[MAD, 2017-07-04 17:10:27]

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

[commit-bot: I haz the power, 2017-07-04 17:10:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[MAD, 2017-07-04 17:18:28]

Thanks! 

All addressed/answered.

CQ'ing... 
Will interrupt if you disagree with the last fixes.

BYE
MAD

https://codereview.chromium.org/2943763002/diff/160001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2943763002/diff/160001/components/policy/reso...
components/policy/resources/policy_templates.json:1802: 'supported_on':
['chrome.win:61-'],
On 2017/07/04 17:06:10, pastarmovj wrote:
> nit: Haven't we branched 61 by now? I think this has to be bumped up.

Chrome 61 Branch Point: Thu Jul 20

https://codereview.chromium.org/2943763002/diff/160001/components/policy/reso...
components/policy/resources/policy_templates.json:1820: But these restrictions
do not apply to the save / download of the currently displayed page, nor does it
apply to saving as PDF from the printing options.
On 2017/07/04 17:06:10, pastarmovj wrote:
> nit. Remove the "But" and merge with the previous paragraph.

Done.

https://codereview.chromium.org/2943763002/diff/160001/components/policy/reso...
components/policy/resources/policy_templates.json:1823: to a Microsoft® Active
Directory® domain.''',
On 2017/07/04 17:06:10, pastarmovj wrote:
> nit: Maybe put the "MS AD" in a ph tag to prevent the translation of this
term.

Done.

[MAD, 2017-07-04 17:18:34]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-07-04 17:18:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, jochen@chromium.org, dtrainor@chromium.org,
pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2943763002/#ps200001
(title: "Final nits")

[commit-bot: I haz the power, 2017-07-04 17:18:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-04 17:28:35]

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

[commit-bot: I haz the power, 2017-07-04 17:28:36]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[MAD, 2017-07-04 17:39:43]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-07-04 17:39:43]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
dtrainor@chromium.org, pastarmovj@chromium.org, rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/2943763002/#ps220001
(title: "Fix an XML goof")

[commit-bot: I haz the power, 2017-07-04 17:39:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-04 19:39:32]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1499189983245370,
"parent_rev": "dc7ec85c15ac598a3b0040c71712ffb862074208", "commit_rev":
"669452f6e42579d09851122adecb6f4a5297c9ea"}

[commit-bot: I haz the power, 2017-07-04 19:39:48]

Description was changed from

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=jochen@chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658
==========

to

==========
Add a new group policy to disable safe browsing for files downloaded from
trusted sources.

TBR=jochen@chromium.org
for trivial change in:
chrome/browser/profiles/profile_impl.cc

BUG=723658

Review-Url: https://codereview.chromium.org/2943763002
Cr-Commit-Position: refs/heads/master@{#484149}
Committed:
https://chromium.googlesource.com/chromium/src/+/669452f6e42579d09851122adecb...
==========

[commit-bot: I haz the power, 2017-07-04 19:39:50]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/669452f6e42579d09851122adecb...