New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 630165 link

Starred by 4 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 630147



Sign in to add a comment

Restore the version fallback for TLS 1.3

Project Member Reported by davidben@chromium.org, Jul 21 2016

Issue description

TLS 1.3 version intolerance is rampant and the TLSWG did not agree to change the versioning scheme, so we must repeat a fourth time the song and dance that didn't work well the first three times either. :-(

This time, we should try to make the fallback less damaging than before.

1. Communicate to BoringSSL that, although the maximum version is TLS 1.2 on fallback legs, the true maximum version is TLS 1.3. In particular, this means we will enforce the 1.3 downgrade indicator. This will serve as FALLBACK_SCSV (though we may as well do that too).

2. Implement the fallback in SSLConnectJob instead of HttpNetworkTransaction. The metrics will be per-connection rather than per-request, but we won't bifurcate the socket pools and cause mmenke@ sadness. The cipher metrics were already per-connection, so it seems to have been fine.

3. Be very selective on the fallback trigger. This will take some care and experimentation. One of the many many failures of the fallback of yore was that we masked new implementation bugs. The TLSWG's bad decision forces us continue this practice, but let us try to minimize the damage on the ecosystem.

Previously, I had hoped we could condition this on whether we failed at ClientHello or later, but we have found 1.2 servers which correctly accept a 1.3 ClientHello with a 1.2 ServerHello but fail much later on Finished. (They probably feed a 1.2 ClientHello into the transcript, but I have not confirmed.)

Thus we will need something weaker. If we got to the ServerHello AND the ServerHello reports TLS 1.3, do NOT fallback. Intermittent 1.2 bugs will still be masked, but autoreload probably masked them anyway. Hopefully there are no servers which respond to a 1.3 ClientHello with a 1.3 ServerHello while still speaking 1.2.

We may also try conditioning on the exact alerts. ERR_SSL_PROTOCOL_ERROR is a rather wide net.

4. Measure measure measure so we can get rid of this.
 
> Previously, I had hoped we could condition this on whether we failed at ClientHello or later, but we have found 1.2 servers which correctly accept a 1.3 ClientHello with a 1.2 ServerHello but fail much later on Finished. (They probably feed a 1.2 ClientHello into the transcript, but I have not confirmed.)

Can we test for this when gathering metrics for version intolerance? If most of the version intolerance occurs at the ClientHello (and only a small fraction fail later on Finished), I think it would be worth it to break those servers and have a cleaner fallback.
Agreed. I'm not optimistic (will update my crawling tool, so we get early numbers), but in the last iteration I added metrics to try to categorize the fallback. This is certainly one we should include.
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 22 2016

The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl.git/+/5e7e7cc696e00ff903a021c5028882590cab7168

commit 5e7e7cc696e00ff903a021c5028882590cab7168
Author: David Benjamin <davidben@google.com>
Date: Thu Jul 21 10:55:28 2016

Add SSL_set_fallback_version.

Alas, we will need a version fallback for TLS 1.3 again.

This deprecates SSL_MODE_SEND_FALLBACK_SCSV. Rather than supplying a
boolean, have BoringSSL be aware of the real maximum version so we can
change the TLS 1.3 anti-downgrade logic to kick in, even when
max_version is set to 1.2.

The fallback version replaces the maximum version when it is set for
almost all purposes, except for downgrade protection purposes.

BUG= chromium:630165 

Change-Id: I4c841dcbc6e55a282b223dfe169ac89c83c8a01f
Reviewed-on: https://boringssl-review.googlesource.com/8882
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

[modify] https://crrev.com/5e7e7cc696e00ff903a021c5028882590cab7168/include/openssl/ssl.h
[modify] https://crrev.com/5e7e7cc696e00ff903a021c5028882590cab7168/ssl/handshake_client.c
[modify] https://crrev.com/5e7e7cc696e00ff903a021c5028882590cab7168/ssl/internal.h
[modify] https://crrev.com/5e7e7cc696e00ff903a021c5028882590cab7168/ssl/ssl_lib.c
[modify] https://crrev.com/5e7e7cc696e00ff903a021c5028882590cab7168/ssl/test/bssl_shim.cc
[modify] https://crrev.com/5e7e7cc696e00ff903a021c5028882590cab7168/ssl/test/runner/runner.go
[modify] https://crrev.com/5e7e7cc696e00ff903a021c5028882590cab7168/ssl/test/test_config.cc
[modify] https://crrev.com/5e7e7cc696e00ff903a021c5028882590cab7168/ssl/test/test_config.h

Comment 4 Deleted

Status: WontFix (was: Available)
No need for this with the new versioning scheme.

Sign in to add a comment