Implement TLS 1.2.

Implement TLS 1.2.

Patch by Adam Langley.

R=agl@chromium.org
BUG=90392
TEST=net_unittests

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

[wtc, 2013-05-28 17:50:24]

agl: please review patch set 9.

Patch set 1 is your original NSS patch. I suggest you review
this CL as follows:

1. The files in net/third_party/nss/ssl: review the diffs
against patch set 1. These are the changes I made to your
NSS patch. Hopefully most of my changes are self-explanatory.
I annotated some of the changes.

2. The files in net/http and net/ssl: review patch set 9
directly.

Thanks.

https://codereview.chromium.org/14772023/diff/1/net/third_party/nss/ssl/sslim...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/14772023/diff/1/net/third_party/nss/ssl/sslim...
net/third_party/nss/ssl/sslimpl.h:1676: sslSocket *ss,
SSL3SignatureAndHashAlgorithm *out);

These two functions are only used in ssl3con.c, so I made
them static functions.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/S...
File net/third_party/nss/ssl/SSLerrs.h (right):

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/S...
net/third_party/nss/ssl/SSLerrs.h:417: "Unsupported hash algorithm used by TLS
peer.")

I changed HASH_FUNCTION to HASH_ALGORITHM because this is
referring to the HashAlgorithm enum type in the TLS 1.2 spec.

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

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:877: rv = SGN_Digest(key, hash->hashAlg, buf,
&hashItem);

I figured out how to use the NSS high-level signature
functions SGN_Digest and VFY_VerifyDigestDirect to generate
and verify the signatures used in TLS 1.2. This allows us to
delete the ssl3_GetPKCS1v15ASN1Data function.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:983: if (hashAlg == SEC_OID_UNKNOWN ||
key->keyType == dsaKey) {

Please let me know if you find this boolean expression a
little hard to understand. I can do more work to clarify
when we need to use PK11_Verify rather than VFY_VerifyDigestDirect.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:4103: default:

clang warns if this switch statement doesn't handle all the
KeyType enum values, so I added a default case.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:4125: SECKEY_DestroyPublicKey(key);

This fixes a leak of the public key.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:5738: isTLS12 =
(PRBool)(ss->ssl3.pwSpec->version >= SSL_LIBRARY_VERSION_TLS_1_2);

In several places I changed ss->version to either
ss->ssl3.pwSpec->version or ss->ssl3.cwSpec->version, to
match how we define |isTLS|.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8046: rv = ssl3_InitTLS12HandshakeHash(ss);

I moved this ssl3_InitTLS12HandshakeHash call here, right
after the ssl3_NegotiateVersion call, to match the location
of the other two ssl3_NegotiateVersion calls. Please doublecheck
this is correct.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8317: tls_hash_sha224,

I added tls_hash_sha224 to this array. (NSS supports SHA-224.) Did you omit
tls_hash_sha224 in this array intentionally?

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8370: out->hashAlg = sh->hashAlg;

This was:
    *out = *sh;

Since we already assigned out->sigAlg on line 8348, we just
need to assign out->hashAlg here.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8473: rv = ssl3_SendECDHServerKeyExchange(ss,
&sigAndHash);

By passing &sigAndHash to ssl3_SendECDHServerKeyExchange, I
can avoid a duplicate call to ssl3_PickSignatureHashAlgorithm.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8623: goto alert_loser;

I think we should send a decrypt_error alert to the peer
in this case, as if ssl3_VerifySignedHashes failed.

I am less certain about the case where
ssl3_CheckSignatureAndHashAlgorithmConsistency
fails (lines 8610-8616).

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:9730: const   SSL3Hashes   *  hashes,

In the original code, SSL3Finished is typedef'ed to be a
synonym of SSL3Hashes. I think we should use SSL3Finished
only for the Finished struct in the SSL 3.0 protocol
(see http://tools.ietf.org/html/rfc6101#section-5.6.9),
so I changed it to SSL3Hashes here.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:9762: unsigned int retLen;

This fixes MSVC compilation errors (variables cannot be
declared in the middle of a block).

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:9983: sizeof(CHANNEL_ID_MAGIC) + hashes.len);

This updates the TLS Channel ID code for the new SSL3Hashes
struct.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:10068: SSL3Hashes      hashes;

This is another case where it is more correct semantically
to use SSL3Hashes instead of SSL3Finished.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:10107: rv = ssl3_AppendHandshakeHeader(ss,
finished, sizeof hashes.u.s);

I reverted your change to how |sFinished| is defined.

We still have an option of whether we use hashes.len or
sizeof hashes.u.s here. Please let me know which form
you prefer.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:2115: /* XXX Should we fail the handshake in
this case? */

agl: these two XXX comments are also questions for you.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:2133: tls_hash_sha384, tls_sig_rsa,

Should we add tls_hash_sha512 entries to this array? Or
do you want to keep ClientHello small?

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3prot.h (right):

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3prot.h:234: } TLSSignatureAlgorithm;

This type was named TLS12SignatureAlgorithm. I changed the "TLS12" prefix
to "TLS".

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3prot.h:238: TLSSignatureAlgorithm sigAlg;

It was a little confusing at first why |hashAlg| is a SECOidTag
but |sigAlg| isn't.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3prot.h:255: PRUint8 raw[64];

The size of |raw| was 32 bytes. I increased it to 64 to be large enough
for SHA-512. Since ssl3_ComputeHandshakeHashes and ssl3_SendCertificateVerify
are hardcoded to use SHA-256 in TLS 1.2, a 32-byte |raw| may be large enough.
But our client also advertises support of sha384 in the signature_algorithms
extension, so it seems that |raw| may need to be large enough for SHA-384 at
least, for example in the signatures in ServerKeyExchange.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3prot.h:317: typedef SSL3HashesIndividually
SSL3Finished;

This change means SSL3Finished can only be used for the
Finished struct in SSL 3.0 (defined in
http://tools.ietf.org/html/rfc6101#section-5.6.9).  SSL3Finished
is no longer a synonym of SSL3Hashes.

[agl, 2013-05-28 20:21:13]

I reviewed patch set 1 to try and remember what I had written. Then I reviewed
each change between the patch sets.

LGTM, however I worry that it breaks SSLv3 because of the Finished changes.
Please see comments.

(And thanks for doing this!)

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

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:983: if (hashAlg == SEC_OID_UNKNOWN ||
key->keyType == dsaKey) {
On 2013/05/28 17:50:25, wtc wrote:
> Please let me know if you find this boolean expression a
> little hard to understand. I can do more work to clarify
> when we need to use PK11_Verify rather than VFY_VerifyDigestDirect.

Based on my understanding I would write:

// VFY_VerifyDigestDirect requires DSA signatures to be DER-encoded. DSA
signatures are DER encoded in TLS but not SSLv3 and the code above always
removes the DER encoding of DSA signatures when present. Thus DSA signatures are
always processed with PK11_Verify.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:4070: /* ssl3_OIDToTLSHashAlgorithm converts
an OID to a TLS hash algorithm identifier.
You appear to have wrapped lines at 80 chars, but this line is >80 after the
function->algorithm change.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8046: rv = ssl3_InitTLS12HandshakeHash(ss);
On 2013/05/28 17:50:25, wtc wrote:
> 
> I moved this ssl3_InitTLS12HandshakeHash call here, right
> after the ssl3_NegotiateVersion call, to match the location
> of the other two ssl3_NegotiateVersion calls. Please doublecheck
> this is correct.

I can't see any problems.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8317: tls_hash_sha224,
On 2013/05/28 17:50:25, wtc wrote:
> I added tls_hash_sha224 to this array. (NSS supports SHA-224.) Did you omit
> tls_hash_sha224 in this array intentionally?

Ah, no, just missed it. Thanks!

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8623: goto alert_loser;
On 2013/05/28 17:50:25, wtc wrote:
> 
> I think we should send a decrypt_error alert to the peer
> in this case, as if ssl3_VerifySignedHashes failed.
> 
> I am less certain about the case where
> ssl3_CheckSignatureAndHashAlgorithmConsistency
> fails (lines 8610-8616).

I think decrypt_error is a reasonable pick in both cases, although I'd expected
handshake_error I think.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:10107: rv = ssl3_AppendHandshakeHeader(ss,
finished, sizeof hashes.u.s);
On 2013/05/28 17:50:25, wtc wrote:
> 
> I reverted your change to how |sFinished| is defined.
> 
> We still have an option of whether we use hashes.len or
> sizeof hashes.u.s here. Please let me know which form
> you prefer.

Is this correct for SSLv3? http://tools.ietf.org/html/rfc6101#section-5.6.9 says
that the SSLv3 finished contains the full MD5/SHA-1 combo hash, but this appears
to include just the SHA-1 part.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:2106: /* XXX Should we check if we support
tls_sig? */
Hmm, probably yes although it's just an optimisation at this point.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:2115: /* XXX Should we fail the handshake in
this case? */
On 2013/05/28 17:50:25, wtc wrote:
> 
> agl: these two XXX comments are also questions for you.

The handshake may not need to sign anything, in which case we would be rejecting
handshakes that we could actually complete. Because of this I decided to let the
code carry on.

We could have the handshake abort in the case that we end up having to sign
something however.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:2133: tls_hash_sha384, tls_sig_rsa,
On 2013/05/28 17:50:25, wtc wrote:
> 
> Should we add tls_hash_sha512 entries to this array? Or
> do you want to keep ClientHello small?

I was just trying to cover Suite-B. I think SHA-512 with RSA doesn't make sense,
but it could make sense with P-521. I don't have a strong opinion either way.

[wtc, 2013-05-28 23:22:19]

agl: thank you for the review. I made the changes you suggested
in patch set 10. I have some questions about your comments.

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

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:983: if (hashAlg == SEC_OID_UNKNOWN ||
key->keyType == dsaKey) {

On 2013/05/28 20:21:13, agl wrote:
>
> Based on my understanding I would write:
> 
> // VFY_VerifyDigestDirect requires DSA signatures to be DER-encoded. DSA
> signatures are DER encoded in TLS but not SSLv3 and the code above always
> removes the DER encoding of DSA signatures when present. Thus DSA signatures
are
> always processed with PK11_Verify.

Done.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:4070: /* ssl3_OIDToTLSHashAlgorithm converts
an OID to a TLS hash algorithm identifier.

On 2013/05/28 20:21:13, agl wrote:
> You appear to have wrapped lines at 80 chars, but this line is >80 after the
> function->algorithm change.

Done.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8317: tls_hash_sha224,

On 2013/05/28 20:21:13, agl wrote:
>
> Ah, no, just missed it. Thanks!

Good. Then my next question is where you think sha224 should be in the
hash preference. I put it after sha512 to discourage the use of sha224,
but perhaps that doesn't make sense.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8623: goto alert_loser;

On 2013/05/28 20:21:13, agl wrote:
>
> I think decrypt_error is a reasonable pick in both cases, although I'd
expected
> handshake_error I think.

decrypt_error is the alert used in TLS by the existing code. See line 8637.
I also checked the meaning of decrypt_error and thought it seems appropriate for
the case when we cannot verify the signature in CertificateVerify.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:10107: rv = ssl3_AppendHandshakeHeader(ss,
finished, sizeof hashes.u.s);

On 2013/05/28 20:21:13, agl wrote:
>
> Is this correct for SSLv3?

Yes, this is correct for SSLv3. I also tested it with an SSLv3
server: https://www.sqlite.org/.  If you know of another SSLv3 server,
please let me know.

> http://tools.ietf.org/html/rfc6101#section-5.6.9 says
> that the SSLv3 finished contains the full MD5/SHA-1 combo hash, but this
appears
> to include just the SHA-1 part.

hashes.u.s is the SSL3HashesIndividually struct.  The SHA-1 part would be
hashes.u.s.sha.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:2106: /* XXX Should we check if we support
tls_sig? */

On 2013/05/28 20:21:13, agl wrote:
> Hmm, probably yes although it's just an optimisation at this point.

Because we will check tls_sig later in ssl3_PickSignatureHashAlgorithm?

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:2133: tls_hash_sha384, tls_sig_rsa,

On 2013/05/28 20:21:13, agl wrote:
>
> I was just trying to cover Suite-B. I think SHA-512 with RSA doesn't make
sense,
> but it could make sense with P-521. I don't have a strong opinion either way.

OK, I replaced tls_hash_sha384 with tls_hash_sha224 for DSA because FIPS 186-3
specifies only these four combinations of p and q sizes:

L = 1024, N = 160
L = 2048, N = 224
L = 2048, N = 256
L = 3072, N = 256

[agl, 2013-05-28 23:36:36]

LGTM

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

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:8317: tls_hash_sha224,
On 2013/05/28 23:22:19, wtc wrote:
> 
> On 2013/05/28 20:21:13, agl wrote:
> >
> > Ah, no, just missed it. Thanks!
> 
> Good. Then my next question is where you think sha224 should be in the
> hash preference. I put it after sha512 to discourage the use of sha224,
> but perhaps that doesn't make sense.

I think it's either just above or just below SHA-512. Just below is fine by me!

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:10107: rv = ssl3_AppendHandshakeHeader(ss,
finished, sizeof hashes.u.s);
On 2013/05/28 23:22:19, wtc wrote:
> hashes.u.s is the SSL3HashesIndividually struct.  The SHA-1 part would be
> hashes.u.s.sha.

Right, sorry. I'm forgetting my own code.

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/14772023/diff/89001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:2106: /* XXX Should we check if we support
tls_sig? */
On 2013/05/28 23:22:19, wtc wrote:
> 
> On 2013/05/28 20:21:13, agl wrote:
> > Hmm, probably yes although it's just an optimisation at this point.
> 
> Because we will check tls_sig later in ssl3_PickSignatureHashAlgorithm?

Right. This filtering is just removing the options that we'll never pick.

[wtc, 2013-05-29 22:09:45]

agl: you just need to review the diffs between patch sets
11 and 12. You already reviewed patch set 10, and patch set
11 is just a sync with the current tip of trunk.

While updating the new code in sslplatf.c, I noticed that
the Windows system crypto libraries do not support SHA-224.
Since we still use the Windows system crypto libraries to
verify certificates and sign CertificateVerify, I removed
SHA-224 support to be safe.

rsleevi: You just need to review the changes to your new
ssl3_CngPlatformSignHashes function in sslplatf.c.  They
are analogous to the changes to ssl3_CAPIPlatformSignHashes.
The SSL3Hashes type is defined in ssl3prot.h.

[agl, 2013-05-29 22:18:11]

11 -> 12 LGTM.

[Ryan Sleevi, 2013-05-29 22:21:49]

sslplatf changes look good. I haven't looked over the rest of the CL.

https://codereview.chromium.org/14772023/diff/124001/net/third_party/nss/ssl/...
File net/third_party/nss/ssl/sslplatf.c (right):

https://codereview.chromium.org/14772023/diff/124001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/sslplatf.c:527: PORT_Assert(sigAlg == CSSM_ALGID_RSA);
I'm curious why you put the assert here, rather than line 552, to match 558.

[wtc, 2013-05-29 23:26:34]

https://codereview.chromium.org/14772023/diff/124001/net/third_party/nss/ssl/...
File net/third_party/nss/ssl/sslplatf.c (right):

https://codereview.chromium.org/14772023/diff/124001/net/third_party/nss/ssl/...
net/third_party/nss/ssl/sslplatf.c:527: PORT_Assert(sigAlg == CSSM_ALGID_RSA);

On 2013/05/29 22:21:49, Ryan Sleevi wrote:
> I'm curious why you put the assert here, rather than line 552, to match 558.

This is because the other cases will change sigAlg. I can, however, put the
assert on line 525, before this switch statement.

Also, would you prefer that I move this switch statement to line 552, under
the rsaKey case in that switch statement?  I was trying to avoid nested
switch statements, but I already used nested switch statements in
ssl3_CngPlatformSignHashes.

[commit-bot: I haz the power, 2013-05-29 23:41:52]

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

[commit-bot: I haz the power, 2013-05-30 05:52:59]

Change committed as 203090

[bshe, 2013-06-21 17:57:05]

On 2013/05/30 05:52:59, I haz the power (commit-bot) wrote:
> Change committed as 203090

I am seeing this error when build lumpy image on TOT today:
../../../../../../../home/bshe/chrome_root/src/net/third_party/nss/ssl/ssl3con.c:4096:24:
error: 'SEC_OID_SHA224' undeclared here (not in a function)

It seems SEC_OID_SHA224 is introduced here. Could be related?

[wtc, 2013-06-21 18:01:24]

On 2013/06/21 17:57:05, bshe wrote:
> 
> I am seeing this error when build lumpy image on TOT today:
>
../../../../../../../home/bshe/chrome_root/src/net/third_party/nss/ssl/ssl3con.c:4096:24:
> error: 'SEC_OID_SHA224' undeclared here (not in a function)
> 
> It seems SEC_OID_SHA224 is introduced here. Could be related?

Yes. The libnss3-dev package on your build machine is too old.
Can you update the libnss3-dev package?