Implement TLS_FALLBACK_SCSV for SSLClientSocketOpenSSL.

Implement TLS_FALLBACK_SCSV for SSLClientSocketOpenSSL.

In doing so, fix a bug in tlslite's TLS_FALLBACK_SCSV support; the fallback
alert should be sent with the client's version. Otherwise OpenSSL reports
SSL_R_UNSUPPORTED_PROTOCOL and doesn't report the alert. This behavior is
probably not wrong as, if the server responds with a TLS version higher than
what is supported, we can't really be sure of the parse.

BUG=388425
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285764

[agl, 2014-07-24 19:27:14]

I'm concerned about the OpenSSL client behaviour here.

What version is tlslite returning that's causing OpenSSL to return a different
error? If it's reasonable behaviour then either we need to adjust OpenSSL, or we
need to change the fallback IETF draft to nail down the alert record's version.

(Technically it's not a security issue because the server has already killed the
connection, but it might cause a bunch of confusion.)

[davidben, 2014-07-24 20:00:58]

On 2014/07/24 19:27:14, agl wrote:
> I'm concerned about the OpenSSL client behaviour here.
> 
> What version is tlslite returning that's causing OpenSSL to return a different
> error? If it's reasonable behaviour then either we need to adjust OpenSSL, or
we
> need to change the fallback IETF draft to nail down the alert record's
version.
> 
> (Technically it's not a security issue because the server has already killed
the
> connection, but it might cause a bunch of confusion.)

tlslite is returning TLS 1.2 because that's the highest version it's configured
to accept. OpenSSL then rejects it because at this point it's been configured to
disable 1.0 through 1.2 and so it behaves as if it doesn't know what TLS 1.2 is
and rejects. This is the order of the block at line 603 and the block at line
633.

https://boringssl.googlesource.com/boringssl/+/master/ssl/s23_clnt.c#603

OpenSSL's behavior here seems defensible. While it would be perfectly fine for
it to accept the alert in this case, say the server implements a TLS 1.3 that
wasn't in the client yet. It seems a little off to accept a record whose
record-layer version number is higher than any version you know exists. Makes
more sense if it ended up being a ServerHello, but perhaps that should be parsed
differently in TLS 1.3.

Then again, NSS seems to be perfectly happy here and even doesn't complain if I
set self.version to (3, 4). So that ship has sailed anyway and we're not
actually allowed to change the format for null cipher alerts sent in response to
a ClientHello, even if you pick a higher version number. In which case,
reordering those two blocks seems pretty safe and we may as well report the more
specific alert value.

I think the patch for tlslite does match how a server should behave though. It
knows the client speaks client_version, so alerting at that level seems fine.
(It actually even knows the client speaks client_version + 1.) But it doesn't
know the client speaks TLS 1.2 and should avoid responding in that version. And
it'll be the version the current server implementation uses because
FALLBACK_SCSV happens after the normal version negotiation, though I should
double-check this against our servers.

[agl, 2014-07-24 22:09:42]

lgtm

[davidben, 2014-07-25 05:17:11]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-07-25 05:19:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/418553003/1

[commit-bot: I haz the power, 2014-07-25 05:29:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[commit-bot: I haz the power, 2014-07-25 05:31:26]

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

[commit-bot: I haz the power, 2014-07-25 05:31:27]

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

[davidben, 2014-07-25 22:08:38]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-07-25 22:09:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/418553003/20001

[commit-bot: I haz the power, 2014-07-26 18:05:17]

Change committed as 285764