Support the new TLS 1.2 HMAC-SHA256 cipher suites specified in

Support the new TLS 1.2 HMAC-SHA256 cipher suites specified in
RFC 5246 and RFC 5289.

To avoid making ClientHello too big, the new DH_DSS, DH_RSA,
DHE_DSS, DH_anon, ECDH_ECDSA, and ECDH_RSA are not added.

Do not generate client_write_IV and server_write_IV in TLS 1.1+
for CBC block ciphers because 1) they aren't used, and 2) a
buffer in the NSS softoken is not big enough if the HMAC key
is 32 bytes (for HMAC-SHA256) and client_write_IV and
server_write_IV are still generated.

Do not downgrade to TLS 1.1 silently when SSL_BYPASS_PKCS11
mode is requested because we won't be able to test the new
TLS 1.2 only cipher suites in PKCS #11 bypass mode. Instead,
silently turn off PKCS #11 bypass if TLS 1.2 is enabled.

R=agl@chromium.org
BUG=90392
TEST=none (done in NSS upstream)

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

[wtc, 2013-06-05 00:30:40]

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:1363: mac += 2;

This mac += 2 is used to change ssl_mac_md5 to ssl_hmac_md5 and
ssl_mac_sha to ssl_hmac_sha, since MD5 and SHA-1 are used in both
SSL 3.0 MAC and TLS HMAC.  ssl3_hmac_sha256 is only used in TLS (1.2),
so this mac += 2 should not apply.

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:3553: }

I found that our TLS 1.1+ code was still asking the NSS softoken
to generate the client_write_IV and server_write_IV. This was OK
until a new TLS 1.2 HMAC-SHA256 cipher suite would ask for so much
key material to exceed the size of a static buffer ('key_block') in
nss/lib/softoken/pkcs11c.c.

IMPORTANT: This change must be reviewed carefully. Here I zero the
client.write_iv and server.write_iv, which will be used to create
the block cipher CBC mode PK11Contexts. However, in
ssl3_CompressMACEncryptRecord (lines 2356-2383) we will use the
PK11Contexts to encrypt a block of random bytes to generate the
explicit IVs in TLS 1.1+, so the bytes in client.write_iv and
server.write_iv don't matter.

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslsock.c (right):

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslsock.c:801: ss->opt.bypassPKCS11   = PR_FALSE;

This change is necessary to make the upstream NSS SSL tests pass.

If we silently downgrade to TLS 1.1 here, the new HMAC-SHA256 cipher suites
will be disabled, and those upstream NSS SSL tests exercising these cipher
suites will fail in "bypass" mode.  (NSS test programs call SSL_VersionRangeSet
first, and then enable the SSL_BYPASS_PKCS11 option.)

So I instead turn off ss->opt.bypassPKCS11 here if TLS 1.2 is enabled.
This means an application interested in PKCS #11 bypass can enable at
most TLS 1.1.

[agl, 2013-06-05 14:53:47]

lgtm

LGTM

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:99: { TLS_DHE_RSA_WITH_AES_256_CBC_SHA256,   
SSL_NOT_ALLOWED, PR_TRUE,PR_FALSE},
This is prioritizing HMAC-SHA256 over HMAC-SHA1, right?

I'm not sure whether we want to do that. It's certainly stronger, but HMAC-SHA1
is still secure and, given that a TLS connection is a bomb oracle, it seems
quite sufficient. SHA256 is quite a bit slower and takes another 12 bytes of
overhead per record. I think HMAC-SHA1 is a the better choice for the majority
of uses.

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:337:
{TLS_RSA_WITH_AES_128_CBC_SHA256,	cipher_aes_128, hmac_sha256, kea_rsa},
(Here, HMAC-SHA1 is coming first.)

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:3550: key_material_params.ulIVSizeInBits = 0;
perhaps:

// Block ciphers in >= TLS 1.1 use a per-record, explicit IV.

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslsock.c (right):

https://codereview.chromium.org/16394004/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslsock.c:801: ss->opt.bypassPKCS11   = PR_FALSE;
On 2013/06/05 00:30:40, wtc wrote:
> 
> This change is necessary to make the upstream NSS SSL tests pass.
> 
> If we silently downgrade to TLS 1.1 here, the new HMAC-SHA256 cipher suites
> will be disabled, and those upstream NSS SSL tests exercising these cipher
> suites will fail in "bypass" mode.  (NSS test programs call
SSL_VersionRangeSet
> first, and then enable the SSL_BYPASS_PKCS11 option.)
> 
> So I instead turn off ss->opt.bypassPKCS11 here if TLS 1.2 is enabled.
> This means an application interested in PKCS #11 bypass can enable at
> most TLS 1.1.

(nit: extra space before equals.)

This seems like slightly unfortunate behaviour - that the bypass request is
silently ignored. However, I don't have a better idea for making the tests pass
and users of bypass are already mostly on their own I guess.

[wtc, 2013-06-05 18:51:20]

Thanks for the review. I made the changes you suggested.
Please check the diffs between patch sets 2 and 4.

https://chromiumcodereview.appspot.com/16394004/diff/6001/net/third_party/nss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/16394004/diff/6001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:99: { TLS_DHE_RSA_WITH_AES_256_CBC_SHA256,   
SSL_NOT_ALLOWED, PR_TRUE,PR_FALSE},

On 2013/06/05 14:53:47, agl wrote:
> This is prioritizing HMAC-SHA256 over HMAC-SHA1, right?

Yes. This list is sorted first in the order of security, then in the
order of performance. The order I used was based on the assumption
that HMAC-SHA256 is more secure than HMAC-SHA1. But you are right
that for the way TLS uses HMAC, they are equally secure. I've changed
to list HMAC-SHA1 before HMAC-SHA256 based on performance.

https://chromiumcodereview.appspot.com/16394004/diff/6001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:337:
{TLS_RSA_WITH_AES_128_CBC_SHA256,	cipher_aes_128, hmac_sha256, kea_rsa},

On 2013/06/05 14:53:47, agl wrote:
> (Here, HMAC-SHA1 is coming first.)

The order of cipher suites in this array does not imply any precedence.
This array is used as a look-up table. Right now it is searched linearly.
(See ssl_LookupCipherSuiteDef.) We could sort this array by the cipher
suite number, which would allow us to use a binary search.

https://chromiumcodereview.appspot.com/16394004/diff/6001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:3550: key_material_params.ulIVSizeInBits = 0;

On 2013/06/05 14:53:47, agl wrote:
> perhaps:
> 
> // Block ciphers in >= TLS 1.1 use a per-record, explicit IV.

Done.

https://chromiumcodereview.appspot.com/16394004/diff/6001/net/third_party/nss...
File net/third_party/nss/ssl/sslsock.c (right):

https://chromiumcodereview.appspot.com/16394004/diff/6001/net/third_party/nss...
net/third_party/nss/ssl/sslsock.c:801: ss->opt.bypassPKCS11   = PR_FALSE;

On 2013/06/05 14:53:47, agl wrote:
>
> (nit: extra space before equals.)

Done.

> This seems like slightly unfortunate behaviour - that the bypass request is
> silently ignored. However, I don't have a better idea for making the tests
pass
> and users of bypass are already mostly on their own I guess.

Another solution is to modify the upstream NSS SSL test script to skip
the tests that use these TLS 1.2-only cipher suites (or even skip all
TLS 1.2 tests) in bypass mode.

I will revisit this later.

[commit-bot: I haz the power, 2013-06-06 00:11:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/16394004/37002

[commit-bot: I haz the power, 2013-06-06 09:48:35]

Change committed as 204467