net: disable SSLv3 fallback.

net: disable SSLv3 fallback.

This change adds a "minimum fallback version" (TLS 1.0). When doing fallback, versions below this minimum will still be tried but the resulting connection, if successful won't actually be used. Rather Chrome will show a different error message if SSLv3 would have worked.

SSLv3 fallback can still be enabled via a new command line flag for those who need it but it'll be interesting to see how this does on dev and beta.

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

Committed: https://crrev.com/32352ad08ee673a4d43e8593ce988b224f6482d3
Cr-Commit-Position: refs/heads/master@{#299567}

[agl, 2014-09-30 03:13:06]

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

[agl, 2014-09-30 03:13:31]

(Note limited visibility at this time.)

[Ryan Sleevi, 2014-09-30 19:19:17]

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

[Ryan Sleevi, 2014-09-30 19:19:17]

Adding felt on this, for her expertise in UI strings that don't confuse people
:)

[Ryan Sleevi, 2014-09-30 19:34:19]

https://codereview.chromium.org/619463002/diff/20001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/619463002/diff/20001/net/base/net_error_list....
net/base/net_error_list.h:340: // minimum fallback version.
This reads a little weird. Perhaps something like

// The SSL server requires falling back to a version weaker than the configured
minimum fallback version, and thus fallback failed.

(s/weaker/older/ or /less than/ for taste)

https://codereview.chromium.org/619463002/diff/20001/net/base/net_error_list....
net/base/net_error_list.h:341: NET_ERROR(SSL_NEEDS_MORE_FALLBACK, -165)
NEEDS_MORE_FALLBACK is... a little less descriptive than I'd hope.

SSL_CANT_FALLBACK_BEYOND_MINIMUM_VERSION ?

That is, I'd hope that the error code tries to describe what's going on without
the error message.

I'm pretty flexible on this if you have alternatives, the concrete concern I
have with "needs more fallback" is that it feels like it combines two
comparators (more = greater than, fallback = less than). That is, you need more
of less security. Which is weird.

Something that can avoid words like "more" is ideal.

[felt, 2014-10-01 02:22:19]

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9210: +        Chrome was unable to make an
acceptably secure connection to the server. Chrome has workarounds to deal with
old and buggy HTTPS servers and the most extreme of these workarounds does
appear to have been able to make a connection to this server, which is why
Chrome may have conected to it previously. However, new research has found that
this workaround causes significant security problems for all Chrome users and
thus it has been disabled. It is possible, for the moment, to reenable this
workaround by adding the command line flag --ssl-version-fallback-min=ssl3.
However, you should fix the server promptly because this may not be supported
forever.
These strings have the product name in them, so they need to be put in the
appropriate product string files (chromium_strings.grd and
google_chrome_strings.grd).

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9211: +      </message>
That's a pretty long paragraph, and I'm not sure the details are really going to
matter. What about something simpler, like:

<ph>foo.com</ph>'s server is outdated and buggy. As a result, Chrome couldn't
set up a secure communication to <ph>foo.com</ph>. If you don't care about a
secure connection, you can add the command line flag
--ssl-version-fallback-min=ssl3 to connect to the website insecurely.

(Although I question why we're promoting this command line flag in the error, do
we really want people to use it?)

https://codereview.chromium.org/619463002/diff/20001/chrome/common/localized_...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/619463002/diff/20001/chrome/common/localized_...
chrome/common/localized_error.cc:41:
"http://sites.google.com/a/chromium.org/dev/"
^ while you're here, can you make that https?

[mmenke, 2014-10-01 02:23:49]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2014-10-01 02:23:50]

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9211: +      </message>
On 2014/10/01 02:22:18, felt wrote:
> That's a pretty long paragraph, and I'm not sure the details are really going
to
> matter. What about something simpler, like:
> 
> <ph>foo.com</ph>'s server is outdated and buggy. As a result, Chrome couldn't
> set up a secure communication to <ph>foo.com</ph>. If you don't care about a
> secure connection, you can add the command line flag
> --ssl-version-fallback-min=ssl3 to connect to the website insecurely.
> 
> (Although I question why we're promoting this command line flag in the error,
do
> we really want people to use it?)

+1 to *not* suggesting the command line flag.  That just seems like a raelly bad
idea.  Also, remember these messages are used on ChromeOS, Android, and iOS....

[agl, 2014-10-01 21:10:54]

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9210: +        Chrome was unable to make an
acceptably secure connection to the server. Chrome has workarounds to deal with
old and buggy HTTPS servers and the most extreme of these workarounds does
appear to have been able to make a connection to this server, which is why
Chrome may have conected to it previously. However, new research has found that
this workaround causes significant security problems for all Chrome users and
thus it has been disabled. It is possible, for the moment, to reenable this
workaround by adding the command line flag --ssl-version-fallback-min=ssl3.
However, you should fix the server promptly because this may not be supported
forever.
On 2014/10/01 02:22:18, felt wrote:
> These strings have the product name in them, so they need to be put in the
> appropriate product string files (chromium_strings.grd and
> google_chrome_strings.grd).

There seem to be lots of uses of "Chrome" and placeholders for it in this file.
However, localized_error.cc doesn't allow a placeholder for Chrome to be
included so I've reworked the message to avoid using the product name.

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9211: +      </message>
On 2014/10/01 02:22:18, felt wrote:
> That's a pretty long paragraph, and I'm not sure the details are really going
to
> matter. What about something simpler, like:
> 
> <ph>foo.com</ph>'s server is outdated and buggy. As a result, Chrome couldn't
> set up a secure communication to <ph>foo.com</ph>. If you don't care about a
> secure connection, you can add the command line flag
> --ssl-version-fallback-min=ssl3 to connect to the website insecurely.
> 
> (Although I question why we're promoting this command line flag in the error,
do
> we really want people to use it?)

Have removed mention of the command line flag in the message. The Learn More
link still directs people to the crbug.com link where we can hopefully help them
out.

It's unclear whether we'll actually be able to make this change stick so, for
now, it's tuned towards people on dev and beta channels and getting useful
feedback.

https://codereview.chromium.org/619463002/diff/20001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/619463002/diff/20001/net/base/net_error_list....
net/base/net_error_list.h:340: // minimum fallback version.
On 2014/09/30 19:34:19, Ryan Sleevi wrote:
> This reads a little weird. Perhaps something like
> 
> // The SSL server requires falling back to a version weaker than the
configured
> minimum fallback version, and thus fallback failed.
> 
> (s/weaker/older/ or /less than/ for taste)

Done.

https://codereview.chromium.org/619463002/diff/20001/net/base/net_error_list....
net/base/net_error_list.h:341: NET_ERROR(SSL_NEEDS_MORE_FALLBACK, -165)
On 2014/09/30 19:34:19, Ryan Sleevi wrote:
> NEEDS_MORE_FALLBACK is... a little less descriptive than I'd hope.
> 
> SSL_CANT_FALLBACK_BEYOND_MINIMUM_VERSION ?
> 
> That is, I'd hope that the error code tries to describe what's going on
without
> the error message.
> 
> I'm pretty flexible on this if you have alternatives, the concrete concern I
> have with "needs more fallback" is that it feels like it combines two
> comparators (more = greater than, fallback = less than). That is, you need
more
> of less security. Which is weird.
> 
> Something that can avoid words like "more" is ideal.

Went with SSL_FALLBACK_BEYOND_MINIMUM_VERSION.

[Ryan Sleevi, 2014-10-01 21:25:05]

//net and //chrome/browser/net LGTM, and I'm going to defer strings to felt@

Question: Should this be exposed as an enterprise policy (perhaps in a
follow-up?) Depends on how long you want to support this.

https://codereview.chromium.org/619463002/diff/40001/chrome/browser/net/ssl_c...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

https://codereview.chromium.org/619463002/diff/40001/chrome/browser/net/ssl_c...
chrome/browser/net/ssl_config_service_manager_pref.cc:309: }
nit: no braces (consistent with the rest of the file for one liners)

https://codereview.chromium.org/619463002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/619463002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:7095: }
nit: no braces

[felt, 2014-10-01 21:28:10]

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9210: +        Chrome was unable to make an
acceptably secure connection to the server. Chrome has workarounds to deal with
old and buggy HTTPS servers and the most extreme of these workarounds does
appear to have been able to make a connection to this server, which is why
Chrome may have conected to it previously. However, new research has found that
this workaround causes significant security problems for all Chrome users and
thus it has been disabled. It is possible, for the moment, to reenable this
workaround by adding the command line flag --ssl-version-fallback-min=ssl3.
However, you should fix the server promptly because this may not be supported
forever.
On 2014/10/01 21:10:53, agl wrote:
> On 2014/10/01 02:22:18, felt wrote:
> > These strings have the product name in them, so they need to be put in the
> > appropriate product string files (chromium_strings.grd and
> > google_chrome_strings.grd).
> 
> There seem to be lots of uses of "Chrome" and placeholders for it in this
file.
> However, localized_error.cc doesn't allow a placeholder for Chrome to be
> included so I've reworked the message to avoid using the product name.

The placeholder method is the deprecated way. Instead, don't put any string in
generated_resources.grd. Instead put the strings in the two other files I
mentioned: one that says "Chrome" and the other that says "Chromium." It will
automatically select the right string for you.

[felt, 2014-10-01 21:29:08]

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9210: +        Chrome was unable to make an
acceptably secure connection to the server. Chrome has workarounds to deal with
old and buggy HTTPS servers and the most extreme of these workarounds does
appear to have been able to make a connection to this server, which is why
Chrome may have conected to it previously. However, new research has found that
this workaround causes significant security problems for all Chrome users and
thus it has been disabled. It is possible, for the moment, to reenable this
workaround by adding the command line flag --ssl-version-fallback-min=ssl3.
However, you should fix the server promptly because this may not be supported
forever.
On 2014/10/01 21:28:10, felt wrote:
> On 2014/10/01 21:10:53, agl wrote:
> > On 2014/10/01 02:22:18, felt wrote:
> > > These strings have the product name in them, so they need to be put in the
> > > appropriate product string files (chromium_strings.grd and
> > > google_chrome_strings.grd).
> > 
> > There seem to be lots of uses of "Chrome" and placeholders for it in this
> file.
> > However, localized_error.cc doesn't allow a placeholder for Chrome to be
> > included so I've reworked the message to avoid using the product name.
> 
> The placeholder method is the deprecated way. Instead, don't put any string in
> generated_resources.grd. Instead put the strings in the two other files I
> mentioned: one that says "Chrome" and the other that says "Chromium." It will
> automatically select the right string for you.

For example take a look at how IDS_SSL_CLOCK_ERROR was defined.

[felt, 2014-10-01 21:40:32]

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9211: +      </message>
On 2014/10/01 21:10:53, agl wrote:
> On 2014/10/01 02:22:18, felt wrote:
> > That's a pretty long paragraph, and I'm not sure the details are really
going
> to
> > matter. What about something simpler, like:
> > 
> > <ph>foo.com</ph>'s server is outdated and buggy. As a result, Chrome
couldn't
> > set up a secure communication to <ph>foo.com</ph>. If you don't care about a
> > secure connection, you can add the command line flag
> > --ssl-version-fallback-min=ssl3 to connect to the website insecurely.
> > 
> > (Although I question why we're promoting this command line flag in the
error,
> do
> > we really want people to use it?)
> 
> Have removed mention of the command line flag in the message. The Learn More
> link still directs people to the http://crbug.com link where we can hopefully
help them
> out.
> 
> It's unclear whether we'll actually be able to make this change stick so, for
> now, it's tuned towards people on dev and beta channels and getting useful
> feedback.

If it's meant for developers, I'd recommend making it more technically precise
(instead of vague things like "workarounds") and using Finch to explicitly gate
it to the canary and dev channels. If it's meant for users (beta+) I think we
need user-understandable strings assuming "regular" people will end up seeing
this at some point.

[agl, 2014-10-01 23:02:40]

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9210: +        Chrome was unable to make an
acceptably secure connection to the server. Chrome has workarounds to deal with
old and buggy HTTPS servers and the most extreme of these workarounds does
appear to have been able to make a connection to this server, which is why
Chrome may have conected to it previously. However, new research has found that
this workaround causes significant security problems for all Chrome users and
thus it has been disabled. It is possible, for the moment, to reenable this
workaround by adding the command line flag --ssl-version-fallback-min=ssl3.
However, you should fix the server promptly because this may not be supported
forever.
On 2014/10/01 21:28:10, felt wrote:
> On 2014/10/01 21:10:53, agl wrote:
> > On 2014/10/01 02:22:18, felt wrote:
> > > These strings have the product name in them, so they need to be put in the
> > > appropriate product string files (chromium_strings.grd and
> > > google_chrome_strings.grd).
> > 
> > There seem to be lots of uses of "Chrome" and placeholders for it in this
> file.
> > However, localized_error.cc doesn't allow a placeholder for Chrome to be
> > included so I've reworked the message to avoid using the product name.
> 
> The placeholder method is the deprecated way. Instead, don't put any string in
> generated_resources.grd. Instead put the strings in the two other files I
> mentioned: one that says "Chrome" and the other that says "Chromium." It will
> automatically select the right string for you.

None of the other error messages phrase it as "Chrome was unable...". They all
say "Unable to...". Since I've changed this error message to match, Chrome isn't
mentioned any longer, which makes it easy.

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9211: +      </message>
> If it's meant for developers, I'd recommend making it more technically precise
> (instead of vague things like "workarounds") and using Finch to explicitly
gate
> it to the canary and dev channels. If it's meant for users (beta+) I think we
> need user-understandable strings assuming "regular" people will end up seeing
> this at some point.

It's aimed at the later, although there's a significant risk that we'll find out
that we can't get away with this and need to revert during Beta.

I think the current wording is sufficiently friendly. I feel that your suggested
wording (above) doesn't cover question that I expect in users minds: "this
worked last week, why did you break it?". But, if you think you have a better
way to communicate that, please go ahead. (There's no rush on this change, we
can't make it public at the moment anyway.)

[felt, 2014-10-01 23:13:54]

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/619463002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9211: +      </message>
On 2014/10/01 23:02:39, agl wrote:
> > If it's meant for developers, I'd recommend making it more technically
precise
> > (instead of vague things like "workarounds") and using Finch to explicitly
> gate
> > it to the canary and dev channels. If it's meant for users (beta+) I think
we
> > need user-understandable strings assuming "regular" people will end up
seeing
> > this at some point.
> 
> It's aimed at the later, although there's a significant risk that we'll find
out
> that we can't get away with this and need to revert during Beta.
> 
> I think the current wording is sufficiently friendly. I feel that your
suggested
> wording (above) doesn't cover question that I expect in users minds: "this
> worked last week, why did you break it?". But, if you think you have a better
> way to communicate that, please go ahead. (There's no rush on this change, we
> can't make it public at the moment anyway.)
> 

My feedback is that the following things are confusing:

* "There exist workarounds to deal with old and buggy HTTPS servers" -- Is this
indirectly implying that foo.com's server is old and buggy? If so, it would be
more straightforward to just say that directly.
* What is a workaround supposed to mean here?
* What is an "acceptably secure" connection? Why not just a "secure connection"?
* What is research? Who did this research? 

Here's another suggestion (just to give an idea of what I mean, you don't have
to use this text):

"Unable to connect securely to the server. The server is outdated and buggy.
This website may have worked previously, but Chrome has stopped accepting
out-of-date server connections for your safety."

[agl, 2014-10-02 19:26:26]

On 2014/10/01 23:13:54, felt wrote:
> "Unable to connect securely to the server. The server is outdated and buggy.
> This website may have worked previously, but Chrome has stopped accepting
> out-of-date server connections for your safety."

Here's the current version:

"Unable to connect securely to the server. This website may have worked
previously, but connecting to such sites has now been shown to cause security
risks to all users and thus has been disabled your safety."

Please render judgement :)

[felt, 2014-10-02 19:30:26]

lgtm

[agl, 2014-10-07 20:53:22]

agl@chromium.org changed reviewers:
+ haavardm@opera.com

[davidben, 2014-10-07 22:06:29]

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

[davidben, 2014-10-07 22:06:30]

lgtm with some typos in the strings and a comment about session caching. I
believe it's fine on both backends, but it was worth pointing out.

https://codereview.chromium.org/619463002/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/619463002/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9213: +        Unable to connect securely to
the server. This website may have worked previously, but connecting to such
sites has now been shown to cause security risks to all users and thus has been
disabled your safety.
and thus has been disabled *for* your safety.

https://codereview.chromium.org/619463002/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9216: +        An SSLv3 fallback was able to
handshake with the server but we no longer accept SSLv3 fallbacks due to new
attacks against the protocol. The server needs to be updated to support a
minimum of TLS 1.0 and preferably TLS 1.2.
with the server*,* but we no longer

https://codereview.chromium.org/619463002/diff/60001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/619463002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3319: return
ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION;
I believe we're fine, but I want to point this out:

Is this going to avoid caching the session? Otherwise NSS is going to clamp
client_version to the session's version when it tries to resume. There's an
SSL_CacheSession call that doesn't get made until later, but probably worth
checking experimentally.

Or better yet: a unit test.

https://codereview.chromium.org/619463002/diff/60001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/619463002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:912: return
ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION;
Ditto for the NSS comment. We should avoid resuming the session. Especially if
this needs to get merged before M39 because that too will clamp the session.

I believe we're fine because MarkSSLSessionAsGood isn't called yet, but wanted
to point this out.

[agl, 2014-10-07 23:59:54]

https://codereview.chromium.org/619463002/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/619463002/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9213: +        Unable to connect securely to
the server. This website may have worked previously, but connecting to such
sites has now been shown to cause security risks to all users and thus has been
disabled your safety.
On 2014/10/07 22:06:30, David Benjamin wrote:
> and thus has been disabled *for* your safety.

Done.

https://codereview.chromium.org/619463002/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:9216: +        An SSLv3 fallback was able to
handshake with the server but we no longer accept SSLv3 fallbacks due to new
attacks against the protocol. The server needs to be updated to support a
minimum of TLS 1.0 and preferably TLS 1.2.
On 2014/10/07 22:06:30, David Benjamin wrote:
> with the server*,* but we no longer

Done.

https://codereview.chromium.org/619463002/diff/60001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/619463002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3319: return
ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION;
On 2014/10/07 22:06:30, David Benjamin wrote:
> Is this going to avoid caching the session? Otherwise NSS is going to clamp
> client_version to the session's version when it tries to resume. There's an
> SSL_CacheSession call that doesn't get made until later, but probably worth
> checking experimentally.
> 
> Or better yet: a unit test.

A very good point, thanks! Please see added unit test.

[davidben, 2014-10-08 02:54:37]

lgtm, thanks!

https://codereview.chromium.org/619463002/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/619463002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:7338: DEFAULT_PRIORITY,
Nit: indentation

[davidben, 2014-10-14 22:21:54]

lgtm

[agl, 2014-10-14 22:32:55]

Committed patchset #7 (id:120001) manually as
32352ad08ee673a4d43e8593ce988b224f6482d3.

[commit-bot: I haz the power, 2014-10-14 22:33:09]

Patchset 7 (id:??) landed as
https://crrev.com/32352ad08ee673a4d43e8593ce988b224f6482d3
Cr-Commit-Position: refs/heads/master@{#299567}