NSS: don't advertise TLS 1.2-only ciphersuites in a TLS 1.1 ClientHello.

NSS: don't advertise TLS 1.2-only ciphersuites in a TLS 1.1 ClientHello.

Chrome is advertising SHA256 ciphersuites with a TLS 1.1 ClientHello. This
breaks PolarSSL servers because they also have a bug: they accept the SHA256
ciphersuite and then break because they negotiated TLS 1.1

This change causes NSS not to advertise cipher suites that aren't applicable to
the version being negotiated.

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

[agl, 2013-09-23 18:39:10]

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:12317: *size = count_cipher_suites(ss,
SSL_ALLOWED, PR_TRUE, ss->vrange.max);
I am somewhat unsure about this (and on line 12324, below). I'm using
ss->vrange.max because in sslcon.c:3139, we find:

    ss->clientHelloVersion = SSL3_ALL_VERSIONS_DISABLED(&ss->vrange) ?
	SSL_LIBRARY_VERSION_2 : ss->vrange.max;

We know that SSL3_ALL_VERSIONS_DISABLED() is false here because of line 12312.
Thus the ClientHello version should be ss->vrange.max.

[wtc, 2013-09-24 17:17:46]

Review comments on patch set 1:

Thank you for writing the CL. I am sorry I missed this issue
when adding TLS 1.2. In TLS 1.1, we only handled one variant
of this problem: an existing cipher suite becomes obsolete in
a new version. That only required checking on the server side.

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:808: * the given version. */

I think this is correct for the server side but wrong for the client side.

On the client side, we also need to allow a cipher suite that is applicable
to a version lower than the given version, because a ClientHello that contains
|version| actually advertises support for versions up to and including
|version|.

This affects the EXPORT cipher suites that are allowed in TLS 1.0 but disallowed
in TLS 1.1+.

So, one way to fix this is to add a variant of ssl3_CipherSuiteAllowedForVersion
that either takes a version range as input or (less preferrable) will consider
all versions lower than the given version. And then, we call this new function
manually in ssl3_SendClientHello and its SSL2 counterpart.

Perhaps just one function that takes a version range would be better: on the
server side we'll just set the min and max versions to the same value.

[wtc, 2013-09-24 17:24:12]

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/README.ch...
File net/third_party/nss/README.chromium (right):

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/README.ch...
net/third_party/nss/README.chromium:123: * Don't advertise TLS 1.2-only cipher
suites in a TLS 1.1 ClientHello.

Please add the upstream NSS bug URL
https://bugzilla.mozilla.org/show_bug.cgi?id=919677.

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:12317: *size = count_cipher_suites(ss,
SSL_ALLOWED, PR_TRUE, ss->vrange.max);

Your analysis is correct. At this point, some SSL 3.x version is enabled, so
it's
correct to pass ss->vrange.max. (As I noted before, we will need to consider the
entire version range here.)

[agl, 2013-09-24 18:58:22]

Please see diffs from patch set 1 to 2.

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/README.ch...
File net/third_party/nss/README.chromium (right):

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/README.ch...
net/third_party/nss/README.chromium:123: * Don't advertise TLS 1.2-only cipher
suites in a TLS 1.1 ClientHello.
On 2013/09/24 17:24:13, wtc wrote:
> 
> Please add the upstream NSS bug URL
> https://bugzilla.mozilla.org/show_bug.cgi?id=919677.

Done.

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23928007/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:808: * the given version. */
On 2013/09/24 17:17:46, wtc wrote:
> This affects the EXPORT cipher suites that are allowed in TLS 1.0 but
disallowed
> in TLS 1.1+.
> 
> So, one way to fix this is to add a variant of
ssl3_CipherSuiteAllowedForVersion
> that either takes a version range as input or (less preferrable) will consider
> all versions lower than the given version. And then, we call this new function
> manually in ssl3_SendClientHello and its SSL2 counterpart.
> 
> Perhaps just one function that takes a version range would be better: on the
> server side we'll just set the min and max versions to the same value.

That's a very good point, thanks for that.

Have changed ssl3_CipherSuiteAllowedForVersion to
ssl3_CipherSuiteAllowedForVersionRange.

[wtc, 2013-09-25 00:42:14]

Patch set 2 LGTM. Please note my comment about
SSL_ERROR_CIPHER_DISALLOWED_FOR_VERSION.

https://codereview.chromium.org/23928007/diff/27001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (left):

https://codereview.chromium.org/23928007/diff/27001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:6368: errCode =
SSL_ERROR_CIPHER_DISALLOWED_FOR_VERSION;

Hmm... this change causes us to lose the more informative error code
SSL_ERROR_CIPHER_DISALLOWED_FOR_VERSION (as opposed to
SSL_ERROR_NO_CYPHER_OVERLAP). I wonder if we can avoid this easily.

Note: this is the only place we use SSL_ERROR_CIPHER_DISALLOWED_FOR_VERSION.

One simple fix is to call ssl3_CipherSuiteAllowedForVersionRange() redundantly
if
config_match() fails, to refine the error code. A more complicated fix would be
to make ssl3_CipherSuiteAllowedForVersionRange() return or set an error code.

https://codereview.chromium.org/23928007/diff/27001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23928007/diff/27001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:636: SSLVersionRange vrange)

Nit: use const SSLVersionRange *vrange ?

I know SSLVersionRange is a simple struct and can be easily passed by value.
This is just to be consistent with the SSL_VersionRangeSet and
SSL_VersionRangeSetDefault functions.

This comment also applies to the config_match() function on line 820.

https://codereview.chromium.org/23928007/diff/27001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:834: * applicable for the given protocol
version. */

"the given protocol version" is wrong because this function doesn't take a
|version| nor a |vrange| argument.

I think this should say "the configured protocol version range".

[agl, 2013-09-25 16:21:52]

Thanks. Running try servers now.

https://codereview.chromium.org/23928007/diff/27001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (left):

https://codereview.chromium.org/23928007/diff/27001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:6368: errCode =
SSL_ERROR_CIPHER_DISALLOWED_FOR_VERSION;
On 2013/09/25 00:42:14, wtc wrote:
> One simple fix is to call ssl3_CipherSuiteAllowedForVersionRange() redundantly
> if config_match() fails, to refine the error code.

Done.

https://codereview.chromium.org/23928007/diff/27001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23928007/diff/27001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:636: SSLVersionRange vrange)
On 2013/09/25 00:42:14, wtc wrote:
> 
> Nit: use const SSLVersionRange *vrange ?
> 
> I know SSLVersionRange is a simple struct and can be easily passed by value.
> This is just to be consistent with the SSL_VersionRangeSet and
> SSL_VersionRangeSetDefault functions.
> 
> This comment also applies to the config_match() function on line 820.

Done.

https://codereview.chromium.org/23928007/diff/27001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:834: * applicable for the given protocol
version. */
On 2013/09/25 00:42:14, wtc wrote:
> 
> "the given protocol version" is wrong because this function doesn't take a
> |version| nor a |vrange| argument.
> 
> I think this should say "the configured protocol version range".

Done.

[wtc, 2013-09-25 17:54:42]

Patch set 3 LGTM. Thanks.

https://codereview.chromium.org/23928007/diff/36001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23928007/diff/36001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:6374: * in order to give a more precise error
message. */

Nit: error message => error code

(I can fix this when I check in this patch to the NSS upstream.)

[agl, 2013-09-25 18:05:13]

https://codereview.chromium.org/23928007/diff/36001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23928007/diff/36001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:6374: * in order to give a more precise error
message. */
On 2013/09/25 17:54:42, wtc wrote:
> 
> Nit: error message => error code

Done.

> (I can fix this when I check in this patch to the NSS upstream.)

Change hasn't yet been submitted to Chromium so the patch file will be correct
when it lands, although upstream has the policy stuff removed and I'm sure
that'll cause some conflicts with this patch.

[commit-bot: I haz the power, 2013-09-25 18:11:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/23928007/52001

[wtc, 2013-09-25 18:14:09]

Patch set 4 LGTM.

https://codereview.chromium.org/23928007/diff/36001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23928007/diff/36001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:6374: * in order to give a more precise error
message. */
On 2013/09/25 18:05:13, agl wrote:
>
> ..., although upstream has the policy stuff removed and I'm sure
> that'll cause some conflicts with this patch.

It turns out that I had to resurrect most of the policy stuff after talking
to Bob Relyea, so the only conflict with this patch is the CHACHA20 cipher
suites. We will need to be careful to not lose them when we update to a new
NSS release.

[commit-bot: I haz the power, 2013-09-25 21:32:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/23928007/52001

[commit-bot: I haz the power, 2013-09-26 05:30:02]

Change committed as 225345