net: disable SSLv3.

net: disable SSLv3.

BUG=419870, 427671
R=davidben@chromium.org, felt@chromium.org, rsleevi@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/ac6f24b46f8e67ac595530d1e7403bb2c0fad839

[agl, 2014-10-27 23:06:46]

agl@chromium.org changed reviewers:
+ felt@chromium.org, rsleevi@chromium.org

[agl, 2014-10-27 23:06:47]

felt: the wording here is (nearly) the same as the previously reviewed wording
for removing SSLv3 fallback.

[agl, 2014-10-28 21:33:58]

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

[agl, 2014-10-28 21:34:24]

+davidben, since this is getting towards the Chrome 40 deadline.

[davidben, 2014-10-29 00:32:05]

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
chrome/common/localized_error.cc:317: {net::ERR_SSL_VERSION_OR_CIPHER_MISMATCH,
Is it worth separating out the version and cipher suite error codes from each
other. On the OpenSSL side, it also triggers on
SSL_R_TLSV1_ALERT_INSUFFICIENT_SECURITY while we probably just want to show this
message on local errors.

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
chrome/common/localized_error.cc:322: SUGGEST_NONE,
Do we want to link this one to a bug as well?

https://codereview.chromium.org/649413004/diff/1/net/url_request/url_request_...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/649413004/diff/1/net/url_request/url_request_...
net/url_request/url_request_unittest.cc:7319:
context.set_fallback_min_version(SSL_PROTOCOL_VERSION_TLS1);
To make sure I'm not missing something, this is just to be explicit about it,
right? I.e. the test still passes with the current defaults.

[agl, 2014-10-29 01:01:18]

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
chrome/common/localized_error.cc:317: {net::ERR_SSL_VERSION_OR_CIPHER_MISMATCH,
On 2014/10/29 00:32:05, David Benjamin wrote:
> Is it worth separating out the version and cipher suite error codes from each
> other. On the OpenSSL side, it also triggers on
> SSL_R_TLSV1_ALERT_INSUFFICIENT_SECURITY while we probably just want to show
this
> message on local errors.

My feeling here is to keep this as is, although only weakly.

NSS actually returns SSL_ERROR_NO_CYPHER_OVERLAP for a bad server version, so we
would need to patch that too. Additionally, this message is reasonably generic
and will probably serve if/when we want to do things like disable RC4 etc.

If someone does want this split, do we narrow the existing error to just cipher
mismatches and create a second, or deprecate the existing one and create two new
ones? (These error values end up in histograms so there are external
dependencies.)

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
chrome/common/localized_error.cc:322: SUGGEST_NONE,
On 2014/10/29 00:32:05, David Benjamin wrote:
> Do we want to link this one to a bug as well?

I don't think so. The existing learn-more link is just for trunk -- it's not in
Chrome 39. I'll remove it before branching. If we want a learn-more link for
this on trunk, it should be a different CL, after the branch point.

https://codereview.chromium.org/649413004/diff/1/net/url_request/url_request_...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/649413004/diff/1/net/url_request/url_request_...
net/url_request/url_request_unittest.cc:7319:
context.set_fallback_min_version(SSL_PROTOCOL_VERSION_TLS1);
On 2014/10/29 00:32:05, David Benjamin wrote:
> To make sure I'm not missing something, this is just to be explicit about it,
> right? I.e. the test still passes with the current defaults.

This triggers a TestSSLConfigService which sets the minimum version to be SSLv3.

[felt, 2014-10-29 01:58:19]

Given how many users will see this, can you please send an e-mail with a
screenshot to ainslie@, egm@, and me? I'd like to have a designer go over the
strings & icon choice.

[felt, 2014-10-29 01:59:53]

On 2014/10/29 01:58:19, felt wrote:
> Given how many users will see this, can you please send an e-mail with a
> screenshot to ainslie@, egm@, and me? I'd like to have a designer go over the
> strings & icon choice.

P.S. I can do this in the next few hours; I'm on Sydney time, as are Elizabeth
and Alex.

[agl, 2014-10-29 02:12:51]

On 2014/10/29 01:59:53, felt wrote:
> P.S. I can do this in the next few hours; I'm on Sydney time, as are Elizabeth
> and Alex.

Sent.

[davidben, 2014-10-29 19:19:33]

net/ LGTM. I'll leave the particulars of the messaging and UI to felt.

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
chrome/common/localized_error.cc:317: {net::ERR_SSL_VERSION_OR_CIPHER_MISMATCH,
On 2014/10/29 01:01:18, agl wrote:
> On 2014/10/29 00:32:05, David Benjamin wrote:
> > Is it worth separating out the version and cipher suite error codes from
each
> > other. On the OpenSSL side, it also triggers on
> > SSL_R_TLSV1_ALERT_INSUFFICIENT_SECURITY while we probably just want to show
> this
> > message on local errors.
> 
> My feeling here is to keep this as is, although only weakly.
> 
> NSS actually returns SSL_ERROR_NO_CYPHER_OVERLAP for a bad server version, so
we
> would need to patch that too. Additionally, this message is reasonably generic
> and will probably serve if/when we want to do things like disable RC4 etc.
> 
> If someone does want this split, do we narrow the existing error to just
cipher
> mismatches and create a second, or deprecate the existing one and create two
new
> ones? (These error values end up in histograms so there are external
> dependencies.)

Alright. I'll defer to what felt and co come with re messaging. For something
like disabling RC4, it would be the server reporting it, not us, so splitting
there wouldn't let us reuse the message.

(Mostly I'm thinking about the case where the server has stricter requirements
than our ClientHello advertises. Though, realistically, it's probably always
going to be the inverse.)

If we were to split, I don't have particular opinions as to whether we'd reuse
the old one or deprecate it.

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
chrome/common/localized_error.cc:322: SUGGEST_NONE,
On 2014/10/29 01:01:18, agl wrote:
> On 2014/10/29 00:32:05, David Benjamin wrote:
> > Do we want to link this one to a bug as well?
> 
> I don't think so. The existing learn-more link is just for trunk -- it's not
in
> Chrome 39. I'll remove it before branching. If we want a learn-more link for
> this on trunk, it should be a different CL, after the branch point.

Acknowledged.

https://codereview.chromium.org/649413004/diff/1/net/url_request/url_request_...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/649413004/diff/1/net/url_request/url_request_...
net/url_request/url_request_unittest.cc:7319:
context.set_fallback_min_version(SSL_PROTOCOL_VERSION_TLS1);
On 2014/10/29 01:01:18, agl wrote:
> On 2014/10/29 00:32:05, David Benjamin wrote:
> > To make sure I'm not missing something, this is just to be explicit about
it,
> > right? I.e. the test still passes with the current defaults.
> 
> This triggers a TestSSLConfigService which sets the minimum version to be
SSLv3.

Acknowledged.

[felt, 2014-10-29 19:28:18]

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
chrome/common/localized_error.cc:317: {net::ERR_SSL_VERSION_OR_CIPHER_MISMATCH,
On 2014/10/29 19:19:33, David Benjamin wrote:
> On 2014/10/29 01:01:18, agl wrote:
> > On 2014/10/29 00:32:05, David Benjamin wrote:
> > > Is it worth separating out the version and cipher suite error codes from
> each
> > > other. On the OpenSSL side, it also triggers on
> > > SSL_R_TLSV1_ALERT_INSUFFICIENT_SECURITY while we probably just want to
show
> > this
> > > message on local errors.
> > 
> > My feeling here is to keep this as is, although only weakly.
> > 
> > NSS actually returns SSL_ERROR_NO_CYPHER_OVERLAP for a bad server version,
so
> we
> > would need to patch that too. Additionally, this message is reasonably
generic
> > and will probably serve if/when we want to do things like disable RC4 etc.
> > 
> > If someone does want this split, do we narrow the existing error to just
> cipher
> > mismatches and create a second, or deprecate the existing one and create two
> new
> > ones? (These error values end up in histograms so there are external
> > dependencies.)
> 
> Alright. I'll defer to what felt and co come with re messaging. For something
> like disabling RC4, it would be the server reporting it, not us, so splitting
> there wouldn't let us reuse the message.
> 
> (Mostly I'm thinking about the case where the server has stricter requirements
> than our ClientHello advertises. Though, realistically, it's probably always
> going to be the inverse.)
> 
> If we were to split, I don't have particular opinions as to whether we'd reuse
> the old one or deprecate it.

is it worth having & maintaining very slightly different neterror strings? seems
better to have a single neterror that is generic enough.

[davidben, 2014-10-29 19:34:32]

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/649413004/diff/1/chrome/common/localized_erro...
chrome/common/localized_error.cc:317: {net::ERR_SSL_VERSION_OR_CIPHER_MISMATCH,
On 2014/10/29 19:28:18, felt wrote:
> On 2014/10/29 19:19:33, David Benjamin wrote:
> > On 2014/10/29 01:01:18, agl wrote:
> > > On 2014/10/29 00:32:05, David Benjamin wrote:
> > > > Is it worth separating out the version and cipher suite error codes from
> > each
> > > > other. On the OpenSSL side, it also triggers on
> > > > SSL_R_TLSV1_ALERT_INSUFFICIENT_SECURITY while we probably just want to
> show
> > > this
> > > > message on local errors.
> > > 
> > > My feeling here is to keep this as is, although only weakly.
> > > 
> > > NSS actually returns SSL_ERROR_NO_CYPHER_OVERLAP for a bad server version,
> so
> > we
> > > would need to patch that too. Additionally, this message is reasonably
> generic
> > > and will probably serve if/when we want to do things like disable RC4 etc.
> > > 
> > > If someone does want this split, do we narrow the existing error to just
> > cipher
> > > mismatches and create a second, or deprecate the existing one and create
two
> > new
> > > ones? (These error values end up in histograms so there are external
> > > dependencies.)
> > 
> > Alright. I'll defer to what felt and co come with re messaging. For
something
> > like disabling RC4, it would be the server reporting it, not us, so
splitting
> > there wouldn't let us reuse the message.
> > 
> > (Mostly I'm thinking about the case where the server has stricter
requirements
> > than our ClientHello advertises. Though, realistically, it's probably always
> > going to be the inverse.)
> > 
> > If we were to split, I don't have particular opinions as to whether we'd
reuse
> > the old one or deprecate it.
> 
> is it worth having & maintaining very slightly different neterror strings?
seems
> better to have a single neterror that is generic enough.

Generic SGTM. I guess it depends on how specific the messaging ends up being. If
it's specifically talking about SSLv3 or dropping a version of SSL, we'll
probably want a dedicated error code. If it's a generic "we decided to drop
support for something", the single net error would be fine.

The case I was thinking about was if, say, the server rejected the ClientHello
because it only supports stronger ciphers. But that's not distinguishable from
it rejecting because it only supports weaker ciphers. And realistically, the
latter's much much more likely than the former, so it's probably not worth
worrying about.

[Ryan Sleevi, 2014-10-29 20:32:01]

lgtm

https://codereview.chromium.org/649413004/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/649413004/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:9278: +        The client and server don't
support a common SSL protocol version or cipher suite. This is usually caused
when the server needs SSLv3 support, which has been removed.
I think it's a little awkward to mention the SSL3 support, since we don't know
if it's 'usually' caused by it. But whatever felt is happy with I can live with.

[agl, 2014-10-29 20:41:27]

I've updated the two visible strings as suggested by ainslie.

https://codereview.chromium.org/649413004/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/649413004/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:9278: +        The client and server don't
support a common SSL protocol version or cipher suite. This is usually caused
when the server needs SSLv3 support, which has been removed.
On 2014/10/29 20:32:01, Ryan Sleevi wrote:
> I think it's a little awkward to mention the SSL3 support, since we don't know
> if it's 'usually' caused by it. But whatever felt is happy with I can live
with.

I don't think that this message is ever actually shown. (At least I don't know
how to see it.) So I've left this as is because I think it's moot.

[agl, 2014-10-29 20:44:47]

To be clear, the message is now "This webpage is not available" + "A secure
connection cannot be established because this site uses an unsupported
protocol.". So it doesn't mention SSLv3, although it does mention "protocol",
which is maybe a little inaccurate if it's about cipher suites.

But I'm not too worried, I think the timing of reports will tell us everything
that we need to know.

In any case, felt et al can continue to update this wording if they wish. I'm
going to make this Finchable next.

[felt, 2014-10-29 20:45:23]

lgtm

[agl, 2014-10-29 20:45:58]

The CQ bit was checked by agl@chromium.org

[commit-bot: I haz the power, 2014-10-29 20:47:35]

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

[commit-bot: I haz the power, 2014-10-29 20:59:37]

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

[commit-bot: I haz the power, 2014-10-29 20:59:38]

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

[commit-bot: I haz the power, 2014-10-31 20:25:14]

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

[agl, 2014-10-31 20:25:17]

Committed patchset #5 (id:80001) manually as
ac6f24b46f8e67ac595530d1e7403bb2c0fad839.