Implement TLS 1.3 anti-downgrade measure

Implement TLS 1.3 anti-downgrade measure

[ekr-rietveld, 2016-01-24 23:39:01]

This patch builds on my draft-11 patch and implements the anti-downgrade measure
in 6.3.1.1.

[mt, 2016-01-25 01:41:58]

I find it strange that you permit the possibility of configuring a value greater
than the maximum supported version.   At that point, you are only setting
yourself up for failure.  A client enabled for TLS 1.2 talking to a TLS 1.3
server will look like it's version intolerant.

Same for a 1.1 client talking to a 1.2 or 1.3 server.

A simple on/off option would be cleaner than the new function.  I think that
enabling it by default is a fine choice, since the risk of random failures is
miniscule [1].

If you decide to keep the API, please also test the API and its interaction with
SSL_SetVersionRange.

[1] ... which is also why I'm ok with having a TLS 1.2 client reject the 1.2
random prefix.

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:80: PRUint16 version_;
uint16_t in these tests.

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:781:
(SSL_LIBRARY_VERSION_TLS_1_0));
Line wrapping is a bit disgusting here.  Maybe a new temporary is needed.

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:798: ConnectExpectFail();
I wouldn't have thought that this would be an error-worthy scenario.  The client
is configured for 1.1 and it got 1.1.  That the server decided to signal a
potential downgrade isn't of much interest to the client.  That you then decide
to abort is problematic, I think.

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:453: void
TlsAgent::SetDowngradeCheckVersion(PRUint16 version) {
uint16_t

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.h:98: void SetDowngradeCheckVersion(PRUint16
version);
uint16_t

https://codereview.appspot.com/289900043/diff/1/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/SSLerrs.h#newcode469
lib/ssl/SSLerrs.h:469: "Invalid version set for downgrade check.")
I think that SEC_ERROR_INVALID_ARGS is plenty good for this.

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl.h
File lib/ssl/ssl.h (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl.h#newcode461
lib/ssl/ssl.h:461: * range. This behavior is enabled by setting |version| to
zero. */
Might want to clarify which range.

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c#newcode6577
lib/ssl/ssl3con.c:6577: * abort the handshake with a fatal "illegal_parameter"
alert.
trailing space

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c#newcode6580
lib/ssl/ssl3con.c:6580: ss->ssl3.downgradeCheckVersion);
I think that this is an abuse of PR_MAX.  You require that the
downgradeCheckVersion is >= vrange.max, so that is the relevant value to use.

downgradeCheckVersion = ss->ssl3.downgradeCheckVersion;
if (!downgradeCheckVersion) {
  downgradeCheckVersion = ss->vrange.max;
}

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c#newcode8147
lib/ssl/ssl3con.c:8147: * we ship the final version of TLS 1.3.
Acknowledged, but you can remove this note because we all don't care about
SSLv2.  Anyone sending a v2-compatible hello gets what they deserve :)

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c#newcode8154
lib/ssl/ssl3con.c:8154: sizeof tls13_downgrade_random);
parens on sizeof

https://codereview.appspot.com/289900043/diff/1/lib/ssl/sslerr.h
File lib/ssl/sslerr.h (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/sslerr.h#newcode221
lib/ssl/sslerr.h:221: SSL_ERROR_INVALID_DOWNGRADE_VERSION = (SSL_ERROR_BASE +
146),
See previous note about SEC_ERROR_INVALID_ARGS.

https://codereview.appspot.com/289900043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/sslimpl.h#newcode1038
lib/ssl/sslimpl.h:1038: * Used only on the client. */
The comment in ssl.h implies that the value defaults to the maximum configured
version.

[ekr-rietveld, 2016-01-25 03:35:31]

Martin,

Consider the following two cases:

1. We have a client which simply is configured for TLS 1.2
2. We have a client which tries TLS 1.3 and then falls back to TLS 1.2 upon RST.

These look identical to NSS: as a client configured to the range (1.1, 1.2), and
a true TLS 1.3 server responds identically: with a tainted random value.
However, in the first case, we need a successful handshake and in the second
case we need it to fail.

WRT to the rest of your note: it's actually not possible to set the downgrade
version to be > than the *supported* version; I check for that. It's simply
possible to configure it *between* the maximum supported version and the maximum
configured version (inclusive) to support the case above.

[ekr-rietveld, 2016-01-25 03:42:35]

Martin, see comments above. I may be short on sleep here, but I don't think a
single "check" bit works properly here, since I think we need to compare the
version we *want* against the version we *got*.

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:798: ConnectExpectFail();
On 2016/01/25 01:41:59, mt wrote:
> I wouldn't have thought that this would be an error-worthy scenario.  The
client
> is configured for 1.1 and it got 1.1.  That the server decided to signal a
> potential downgrade isn't of much interest to the client.  That you then
decide
> to abort is problematic, I think.

The reason it's aborting is that this API (SSL_SetDowngradeCheckVersion() tells
NSS that we have fallen back insecurely from version X).

https://codereview.appspot.com/289900043/diff/1/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/SSLerrs.h#newcode469
lib/ssl/SSLerrs.h:469: "Invalid version set for downgrade check.")
On 2016/01/25 01:41:59, mt wrote:
> I think that SEC_ERROR_INVALID_ARGS is plenty good for this.

Yes, that's fine.

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c#newcode6580
lib/ssl/ssl3con.c:6580: ss->ssl3.downgradeCheckVersion);
On 2016/01/25 01:41:59, mt wrote:
> I think that this is an abuse of PR_MAX.  You require that the
> downgradeCheckVersion is >= vrange.max, so that is the relevant value to use.
> 
> downgradeCheckVersion = ss->ssl3.downgradeCheckVersion;
> if (!downgradeCheckVersion) {
>   downgradeCheckVersion = ss->vrange.max;
> }

Well, it's either 0 or >= ssl3.ssl3.downgradeCheckVersion (as reflected in your
proposed code). I'm happy to change it as you suggest, but it's not clear to me
why that's an improvement.

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c#newcode8147
lib/ssl/ssl3con.c:8147: * we ship the final version of TLS 1.3.
On 2016/01/25 01:41:59, mt wrote:
> Acknowledged, but you can remove this note because we all don't care about
> SSLv2.  Anyone sending a v2-compatible hello gets what they deserve :)

I was thinking to leave this in place until we actually removed V2, but I could
go either way.

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c#newcode8154
lib/ssl/ssl3con.c:8154: sizeof tls13_downgrade_random);
On 2016/01/25 01:41:59, mt wrote:
> parens on sizeof

Yeah, good point.

https://codereview.appspot.com/289900043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/sslimpl.h#newcode1038
lib/ssl/sslimpl.h:1038: * Used only on the client. */
On 2016/01/25 01:41:59, mt wrote:
> The comment in ssl.h implies that the value defaults to the maximum configured
> version.

Right, I'll fix this comment.

[mt, 2016-01-25 03:48:49]

On 2016/01/25 03:35:31, ekr-webrtc wrote:
> 1. We have a client which simply is configured for TLS 1.2
> 2. We have a client which tries TLS 1.3 and then falls back to TLS 1.2 upon
RST.

Ugh, I hadn't thought about the fallback thing.  That's gross, but I guess that
I can accept that.  Please update the comment to make that clearer.  "downgrade"
here might refer to the attack, but fallback also applies.

[ekr-rietveld, 2016-01-25 04:16:08]

Ugh indeed. LMK if you think of something better.

[ekr-rietveld, 2016-02-02 17:02:49]

MT, ready to land?

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:80: PRUint16 version_;
On 2016/01/25 01:41:59, mt wrote:
> uint16_t in these tests.

Done.

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:453: void
TlsAgent::SetDowngradeCheckVersion(PRUint16 version) {
On 2016/01/25 01:41:59, mt wrote:
> uint16_t

Done.

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/289900043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.h:98: void SetDowngradeCheckVersion(PRUint16
version);
On 2016/01/25 01:41:59, mt wrote:
> uint16_t

Done.

https://codereview.appspot.com/289900043/diff/1/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/SSLerrs.h#newcode469
lib/ssl/SSLerrs.h:469: "Invalid version set for downgrade check.")
On 2016/01/25 03:42:35, ekr-webrtc wrote:
> On 2016/01/25 01:41:59, mt wrote:
> > I think that SEC_ERROR_INVALID_ARGS is plenty good for this.
> 
> Yes, that's fine.

Done.

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl.h
File lib/ssl/ssl.h (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl.h#newcode461
lib/ssl/ssl.h:461: * range. This behavior is enabled by setting |version| to
zero. */
On 2016/01/25 01:41:59, mt wrote:
> Might want to clarify which range.

Done.

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c#newcode6580
lib/ssl/ssl3con.c:6580: ss->ssl3.downgradeCheckVersion);
On 2016/01/25 03:42:35, ekr-webrtc wrote:
> On 2016/01/25 01:41:59, mt wrote:
> > I think that this is an abuse of PR_MAX.  You require that the
> > downgradeCheckVersion is >= vrange.max, so that is the relevant value to
use.
> > 
> > downgradeCheckVersion = ss->ssl3.downgradeCheckVersion;
> > if (!downgradeCheckVersion) {
> >   downgradeCheckVersion = ss->vrange.max;
> > }
> 
> Well, it's either 0 or >= ssl3.ssl3.downgradeCheckVersion (as reflected in
your
> proposed code). I'm happy to change it as you suggest, but it's not clear to
me
> why that's an improvement.
> 

Replaced with ?:

https://codereview.appspot.com/289900043/diff/1/lib/ssl/ssl3con.c#newcode8154
lib/ssl/ssl3con.c:8154: sizeof tls13_downgrade_random);
On 2016/01/25 03:42:35, ekr-webrtc wrote:
> On 2016/01/25 01:41:59, mt wrote:
> > parens on sizeof
> 
> Yeah, good point.

Done.

[mt, 2016-02-02 22:58:46]

LGTM