Implement the AES GCM cipher suites for TLS.

Implement the AES GCM cipher suites for TLS.

The AES GCM cipher suites are disabled in DTLS. This will be fixed soon.

Disable the HMAC-SHA256 cipher suites so that our ClientHello doesn't
become too big.

Patch by Adam Langley.

R=agl@chromium.org,rsleevi@chromium.org
BUG=255241
TEST=none

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

[wtc, 2013-08-02 02:31:16]

rsleevi: you can ignore this CL for now.

agl: please take a quick look at the diffs between patch sets
1 and 3. Patch set 1 is based on your NSS patch. Patch set 3
handles older system NSS libraries on Linux that don't have
PK11_Encrypt and PK11_Decrypt, and has a few small cleanups.

I haven't finished the code review yet, so this is just a
preliminary request.

[agl, 2013-08-02 14:59:50]

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

https://codereview.chromium.org/21696002/diff/12005/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1801: ssl3_CheckGCMSupport(void)
This function is called CheckGCMSupport, but it doesn't return anything that
indicates whether GCM is supported.

ssl3_MaybeResolvePK11CryptFunctions?

https://codereview.chromium.org/21696002/diff/12005/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1834: const ssl3CipherSuite * suite;
It occurs that the cipher of the ciphersuites could be tested rather than
listing them. This would catch future additions of GCM ciphersuites
automatically.

https://codereview.chromium.org/21696002/diff/12005/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:5023: if (!pk11_encrypt) {
(Assuming that PK11_Encrypt and PK11_Decrypt appeared in the same version of
NSS.)

[wtc, 2013-08-03 01:02:14]

agl: thank you for your comments. Please take a look at
patch set 4.

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

https://codereview.chromium.org/21696002/diff/12005/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1801: ssl3_CheckGCMSupport(void)

On 2013/08/02 14:59:50, agl wrote:
> This function is called CheckGCMSupport, but it doesn't return anything that
> indicates whether GCM is supported.
> 
> ssl3_MaybeResolvePK11CryptFunctions?

Done. Renamed this function ssl3_ResolvePK11CryptFunctions. Also added an
ssl3_HasGCMSupport function.

https://codereview.chromium.org/21696002/diff/12005/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1834: const ssl3CipherSuite * suite;

On 2013/08/02 14:59:50, agl wrote:
> It occurs that the cipher of the ciphersuites could be tested rather than
> listing them. This would catch future additions of GCM ciphersuites
> automatically.

I re-implemented ssl3_DisableGCMSuites in patch set 4 using this approach.
Please take a look. It requires stepping through a much larger array
(cipher_suite_defs, 60 elements) than the gcmSuites array above, which has
only 3 elements. So it seems worthwhile to maintain a separate gcmSuites array.

I wonder if there is a better data structure that gives us the best of both
worlds.

https://codereview.chromium.org/21696002/diff/12005/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:5023: if (!pk11_encrypt) {

On 2013/08/02 14:59:50, agl wrote:
> (Assuming that PK11_Encrypt and PK11_Decrypt appeared in the same version of
> NSS.)

Yes, PK11_Encrypt and PK11_Decrypt were added in the same NSS checkin:
https://hg.mozilla.org/projects/nss/rev/75b566f18970

I added a comment to state this assumption.

[agl, 2013-08-05 16:53:59]

lgtm

[Ryan Sleevi, 2013-08-05 22:21:42]

wtc: Is this ready for review? I took a quick scan, and it looks fine, but I
wasn't sure if it was ready for a deep dive.

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

https://codereview.chromium.org/21696002/diff/41001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1803: #ifdef LINUX
Strictly speaking, this feels wrong if we're trying to match something
upstreamable.

#ifdef NSS_USE_STATIC_LIBS
pk11_encrypt = PK11_Encrypt;
pk11_decrypt = PK11_Decrypt;
#else
PR_Library *handle = PR_LoadLibrary(NULL, PR_LD_LAZY);
if (!handle) {
  ...
}
pk11_encrypt = (PK11CryptFcn)PR_FindSymbol(handle, "PK11_Encrypt");
pk11_decrypt = (PK11CryptFcn)PR_FindSymbol(handle, "PK11_Decrypt");
PR_UnloadLibrary(handle);
return PR_SUCCESS;
#endif

[wtc, 2013-08-05 22:39:18]

rsleevi: thanks for the comments. No, this is not yet ready
for a deep dive. I'd like to review it carefully this week
first.

[wtc, 2013-08-05 23:07:28]

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

https://codereview.chromium.org/21696002/diff/41001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1803: #ifdef LINUX
On 2013/08/05 22:21:42, Ryan Sleevi wrote:
> Strictly speaking, this feels wrong if we're trying to match something
> upstreamable.
> 
> #ifdef NSS_USE_STATIC_LIBS
> pk11_encrypt = PK11_Encrypt;
> pk11_decrypt = PK11_Decrypt;
> #else
> PR_Library *handle = PR_LoadLibrary(NULL, PR_LD_LAZY);
> if (!handle) {
>   ...
> }
> pk11_encrypt = (PK11CryptFcn)PR_FindSymbol(handle, "PK11_Encrypt");
> pk11_decrypt = (PK11CryptFcn)PR_FindSymbol(handle, "PK11_Decrypt");
> PR_UnloadLibrary(handle);
> return PR_SUCCESS;
> #endif

Thank you for the suggestion. This code won't be upstreamed. I looked
into the meaning of NSS_USE_STATIC_LIBS and found that I am using it
incorrectly in src/third_party/nss/nss.gyp. In current NSS source code,
NSS_USE_STATIC_LIBS is only used with libSSL and it means libSSL is
built as a static library:
http://mxr.mozilla.org/nss/search?string=NSS_USE_STATIC_LIBS

I will fix this incorrect usage of NSS_USE_STATIC_LIBS in a separate CL.

[wtc, 2013-08-10 01:22:59]

This CL is ready for a careful review.

Patch set 1 is agl's original NSS patch.

Patch set 4 is what agl last reviewed. It contains my change
to look up the PK11_Encrypt and PK11_Decrypt functions at
run time on Linux.

Patch set 6 contains more changes by me after a complete
review. I annotate some of the changes below.

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

https://codereview.chromium.org/21696002/diff/41001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2566: rv = PK11_GenerateRandom(wrBuf->buf +
headerLen, nonceLen);

I now let cwSpec->aead() be responsible for generating the
explicit nonce.

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

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:95: { TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,   
SSL_NOT_ALLOWED, PR_TRUE,PR_FALSE},

I added TLS_DHE_RSA_WITH_AES_128_GCM_SHA256.

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:265: {cipher_aes_128_gcm,  calg_aes_gcm,    
16,16, type_aead,   4, 0,16, 8},

I renamed calg_aes_128_gcm to calg_aes_gcm because the other calg_xxx enums are
independent of key size.

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1861: int inlen,

I removed the explicitNonce input.

For decrypt, the |in| buffer starts with the explicit nonce.

For encrypt, we use the sequence number as the explicit
nonce (allowed by RFC 5288) and write the explicit nonce
to the beginning of the |out| buffer.

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:11467: cText->version,

Note: we still need to pass a "PRBool isDTLS" argument to
crSpec->aead so that we can convert cText->version to the
wire format for AEAD additional data.

[agl, 2013-08-13 12:17:12]

LGTM with nits.

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

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1896: memcpy(nonce + 4, &seq_num,
explicitNonceLen);
This is coping directly out of the SSL3SequenceNumber struct, which gives me the
willies. An ABI could add padding in the struct causing nonce duplication. It's
also gives away the endianness of the machine.

Could these 8 bytes be read from the pseudo-header?

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2228: ** But, we use the record'v version
value in the computation.
This relocated comment contains a typo ("record'v")

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2578: const int nonceLen =
cipher_def->explicit_nonce_size;
The explicit nonce size in the cipher_def is now only used here. However,
cwSpec->aead should be doing the length check on 2581, no? In which case it
could be removed and so could a column of the cipher_def table.

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslenum.c (right):

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslenum.c:32: TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,
Should this be the most preferable cipher? Should it be below the ECDHE ones?

[Ryan Sleevi, 2013-08-13 21:05:45]

Forgot to publish draft reviews, but I see Adam made the same remark, so
publishing this note ahead.

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

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:95: { TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,   
SSL_NOT_ALLOWED, PR_TRUE,PR_FALSE},
On 2013/08/10 01:22:59, wtc wrote:
> 
> I added TLS_DHE_RSA_WITH_AES_128_GCM_SHA256.

Do we really want DHE_RSA over the ECDHE case?

I would think the order should be:

#ifdef NSS_ENABLE_ECC
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
#endif

TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
TLS_RSA_WITH_AES_128_GCM_SHA256

#ifdef NSS_ENABLE_ECC
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
#endif

[Ryan Sleevi, 2013-08-13 21:34:46]

LGTM. Just adding a note as a reminder.

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

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2357: #define CKM_NSS_SSL3_MAC_CONSTANT_TIME
(CKM_NSS + 20)
We can remove this in a follow-up patch, now that NSS 3.14.3 is the minimum
compile-time version.

[wtc, 2013-08-14 01:57:02]

agl,rsleevi: thank you for the review. I responded to all of
your comments. Please review patch set 7. You can just look
at the diffs between patch sets 6 and 7.

I added PKCS #11 bypass mode support (ssl3_AESGCMBypass),
which makes my changes to ssl3_AESGCM more difficult to see
in the diffs.

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

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:95: { TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,   
SSL_NOT_ALLOWED, PR_TRUE,PR_FALSE},
On 2013/08/13 21:05:45, Ryan Sleevi wrote:
> On 2013/08/10 01:22:59, wtc wrote:
> > 
> > I added TLS_DHE_RSA_WITH_AES_128_GCM_SHA256.
> 
> Do we really want DHE_RSA over the ECDHE case?
> 
> I would think the order should be:
> 
> #ifdef NSS_ENABLE_ECC
> TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
> #endif
> 
> TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_RSA_WITH_AES_128_GCM_SHA256
> 
> #ifdef NSS_ENABLE_ECC
> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
> TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
> #endif

Done.

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:1896: memcpy(nonce + 4, &seq_num,
explicitNonceLen);
On 2013/08/13 12:17:12, agl wrote:
> This is coping directly out of the SSL3SequenceNumber struct, which gives me
the
> willies. An ABI could add padding in the struct causing nonce duplication.
It's
> also gives away the endianness of the machine.
> 
> Could these 8 bytes be read from the pseudo-header?

Done.

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2228: ** But, we use the record'v version
value in the computation.
On 2013/08/13 12:17:12, agl wrote:
> This relocated comment contains a typo ("record'v")

Done.

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2578: const int nonceLen =
cipher_def->explicit_nonce_size;
On 2013/08/13 12:17:12, agl wrote:
> The explicit nonce size in the cipher_def is now only used here. However,
> cwSpec->aead should be doing the length check on 2581, no? In which case it
> could be removed and so could a column of the cipher_def table.

cipher_def->explicit_nonce_size is also used in the length check on line 11382
in ssl3_HandleRecord. But you are right, both length checks could be done by
c{r/w}Spec->aead. I will think about this more.

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslenum.c (right):

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslenum.c:32: TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,
On 2013/08/13 12:17:12, agl wrote:
> Should this be the most preferable cipher? Should it be below the ECDHE ones?

Done.

[wtc, 2013-08-14 22:05:42]

rsleevi: I am going to commit patch set 10 now. It would be
nice if you could review the diffs between patch sets 6 and
10. I will address your comments in a follow-up CL. Thanks.

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

https://codereview.chromium.org/21696002/diff/65001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2357: #define CKM_NSS_SSL3_MAC_CONSTANT_TIME
(CKM_NSS + 20)
On 2013/08/13 21:34:46, Ryan Sleevi wrote:
> We can remove this in a follow-up patch, now that NSS 3.14.3 is the minimum
> compile-time version.

Done. Added a TODO to README.chromium to remove cbc.patch.

[commit-bot: I haz the power, 2013-08-14 22:06:56]

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

[Ryan Sleevi, 2013-08-14 22:26:48]

lgtm

[commit-bot: I haz the power, 2013-08-15 00:51:35]

Change committed as 217716