Provide enterprise policy option to set the minimum SSL version.

Provide enterprise policy option to set the minimum SSL version.

In order to address the POODLE attack, some large deployments of Chrome may
wish to disable SSLv3 before we make it a default. Equally, they may need to
preserve SSLv3 when we disable it by default.

BUG=419870
Committed: https://crrev.com/e83b4651dd1c9fc4e0f6f21d74627a52cf4a8782
Cr-Commit-Position: refs/heads/master@{#299755}

[agl, 2014-10-13 22:43:14]

agl@chromium.org changed reviewers:
+ atwilson@chromium.org

[agl, 2014-10-13 22:43:14]

(Please note that this change relates to an unpublished security issue. Please
do not discuss it in the open yet.

This change will not be landed until the security issue is public.)

[Andrew T Wilson (Slow), 2014-10-14 08:32:53]

atwilson@chromium.org changed reviewers:
+ joaodasilva@chromium.org

[Andrew T Wilson (Slow), 2014-10-14 08:32:54]

Any chance I could have access to the bug itself?

+joao since he owns policy_templates.json

I'll send an email to discuss enterprise impact.

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6795: 'name': 'SSLVersionMin',
I wonder if instead of a blanket policy, if instead admins should be allowed to
provide a trusted whitelist of sites that are allowed to use sslv3?

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6828: 'supported_on':
['chrome.*:39-', 'chrome_os:39-', 'android:39-'],
We use our own net stack on ios - is this policy supported on that platform?

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6836: 'desc': '''If this
policy is not configured then <ph name="PRODUCT_NAME">$1<ex>Google
Chrome</ex></ph> will use a default minimum version, which is SSLv3 in Chrome 39
but may be TLS 1.0 in Chrome 40.
The idea is that ssl3 < tls1 for the purposes of this check - I'm wondering if
we need to make this fact clearer in the documentation?

[Joao da Silva, 2014-10-14 08:40:10]

Looks good. Can you clarify if this is meant to be merged to 39?

https://codereview.chromium.org/649373002/diff/1/chrome/browser/policy/policy...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/649373002/diff/1/chrome/browser/policy/policy...
chrome/browser/policy/policy_browsertest.cc:2212: static bool
IsMinSSLVersionTLS12(Profile *profile) {
nit: Profile* profile

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6828: 'supported_on':
['chrome.*:39-', 'chrome_os:39-', 'android:39-'],
Is this going to be merged back to 39? Current trunk is 40.

[agl, 2014-10-14 17:39:51]

https://codereview.chromium.org/649373002/diff/1/chrome/browser/policy/policy...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/649373002/diff/1/chrome/browser/policy/policy...
chrome/browser/policy/policy_browsertest.cc:2212: static bool
IsMinSSLVersionTLS12(Profile *profile) {
On 2014/10/14 08:40:10, Joao da Silva wrote:
> nit: Profile* profile

Done.

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6795: 'name': 'SSLVersionMin',
On 2014/10/14 08:32:53, Andrew T Wilson wrote:
> I wonder if instead of a blanket policy, if instead admins should be allowed
to
> provide a trusted whitelist of sites that are allowed to use sslv3?

The message is very much that we're moving on from SSLv3. Adding infrastructure
to support of a set of hostnames would make the patch more difficult and
dangerous to merge. Users who are using this to reenable SSLv3 /should/ feel
that it's a questionable choice!

(Also, some might want to use it to *disable* SSLv3 before we do by default. I
rather hope that's the more common usage.)

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6828: 'supported_on':
['chrome.*:39-', 'chrome_os:39-', 'android:39-'],
On 2014/10/14 08:32:53, Andrew T Wilson wrote:
> We use our own net stack on ios - is this policy supported on that platform?

Thanks -- I just omitted ios. Done.

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6828: 'supported_on':
['chrome.*:39-', 'chrome_os:39-', 'android:39-'],
On 2014/10/14 08:40:10, Joao da Silva wrote:
> Is this going to be merged back to 39? Current trunk is 40.

We are going to watch and listen to requests once the issue is public but I do
expect that this will be merged to 39 at least.

https://codereview.chromium.org/649373002/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:6836: 'desc': '''If this
policy is not configured then <ph name="PRODUCT_NAME">$1<ex>Google
Chrome</ex></ph> will use a default minimum version, which is SSLv3 in Chrome 39
but may be TLS 1.0 in Chrome 40.
On 2014/10/14 08:32:53, Andrew T Wilson wrote:
> The idea is that ssl3 < tls1 for the purposes of this check - I'm wondering if
> we need to make this fact clearer in the documentation?

That's good point. It is something that people have misunderstood in the past.
I've added a note to that effect.

[Joao da Silva, 2014-10-14 18:52:37]

lgtm

[agl, 2014-10-15 17:02:41]

The CQ bit was checked by agl@chromium.org

[commit-bot: I haz the power, 2014-10-15 17:03:52]

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

[commit-bot: I haz the power, 2014-10-15 17:13:00]

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

[commit-bot: I haz the power, 2014-10-15 17:13:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[agl, 2014-10-15 17:36:47]

agl@chromium.org changed reviewers:
+ asvitkine@chromium.org

[agl, 2014-10-15 17:36:47]

+asvitkine for histograms.xml change.

[Alexei Svitkine (slow), 2014-10-15 17:38:59]

histograms.xml lgtm

[Joao da Silva, 2014-10-15 18:38:20]

Adam, see inline re the failing test.

https://codereview.chromium.org/649373002/diff/90001/chrome/browser/policy/po...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/649373002/diff/90001/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:2221: PrefService* prefs =
browser()->profile()->GetPrefs();
This pref is registered for Local State, so this should probably be
browser()->local_state() instead.

The remaining profile checks should still work, because profile_impl.cc creates
a SSLConfigServiceManager for the profile with prefs from local_state.

[agl, 2014-10-15 18:48:02]

https://codereview.chromium.org/649373002/diff/90001/chrome/browser/policy/po...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/649373002/diff/90001/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:2221: PrefService* prefs =
browser()->profile()->GetPrefs();
On 2014/10/15 18:38:19, Joao da Silva wrote:
> This pref is registered for Local State, so this should probably be
> browser()->local_state() instead.
> 
> The remaining profile checks should still work, because profile_impl.cc
creates
> a SSLConfigServiceManager for the profile with prefs from local_state.

Thanks!

browser()->local_state() didn't type check, but the rest of the file uses
g_browser_process->local_state() and that works for me locally. I'll see whether
the bots like it this time.

[agl, 2014-10-15 18:48:25]

The CQ bit was checked by agl@chromium.org

[commit-bot: I haz the power, 2014-10-15 18:49:28]

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

[commit-bot: I haz the power, 2014-10-15 20:28:31]

Committed patchset #3 (id:110001)

[commit-bot: I haz the power, 2014-10-15 20:30:19]

Patchset 3 (id:??) landed as
https://crrev.com/e83b4651dd1c9fc4e0f6f21d74627a52cf4a8782
Cr-Commit-Position: refs/heads/master@{#299755}