Miscellaneous cleanup of TLS 1.2 code.

Miscellaneous cleanup of TLS 1.2 code.

1. Support TLS 1.2 in PKCS #11 bypass mode.
2. Delay the creation of handshake hash contexts until we have established
   the handshake hash functions.
3. Buffer handshake messages until the handshake hash contexts have been
   created.
4. Fix the double ssl3_RestartHandshakeHashes calls, by removing the
   ssl3_RestartHandshakeHashes call in ssl3_NewHandshakeHashes.
5. Remove redundant ssl3_InitState calls.
6. Remove NSS_SURVIVE_DOUBLE_BYPASS_FAILURE support, so that
   ss->ssl3.hs.messages is used for only one purpose.

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

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

[wtc, 2013-06-15 19:40:55]

agl: I'm sorry the CL is not easy to review. I added some
annotations. Please let me know if you have any questions.
Thanks a lot.

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3con.c (left):

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:3462: #if
defined(NSS_SURVIVE_DOUBLE_BYPASS_FAILURE)

I don't think this code is used by any NSS user. I removed this code so that
ss->ssl3.hs.messages is used for only one purpose, which simplifies our logic.

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:3715: ssl3_NewHandshakeHashes(sslSocket *ss)

After I removed the NSS_SURVIVE_DOUBLE_BYPASS_FAILURE code, ssl3_InitState
becomes the only caller of ssl3_NewHandshakeHashes.

I moved the code in ssl3_NewHandshakeHashes that creates the handshake hash
contexts to ssl3_InitHandshakeHashes (formerly named
ssl3_InitTLS12HandshakeHash).
This left only three lines that initializes ss->ssl3.hs.messages (lines
3726-3728). I moved them to ssl3_InitState.

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:3741: if (SECSuccess ==
ssl3_RestartHandshakeHashes(ss)) {

Note that ssl3_NewHandshakeHashes calls ssl3_RestartHandshakeHashes,
which results in ssl3_RestartHandshakeHashes being called twice in some
places. So I removed this ssl3_RestartHandshakeHashes call. This requires
adding ssl3_RestartHandshakeHashes calls to some other places.

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:3930: (unsigned char*)ss->ssl3.hs.sha_cx,
SHA1_LENGTH));

These two PRINT_BUF statements are broken because the MD5 and SHA hashes
are not at the beginning of ss->ssl3.hs.md5_cx and ss->ssl3.hs.sha_cx.
I just deleted them rather than trying to fix them.

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:10566: (unsigned char*)ss->ssl3.hs.sha_cx,
SHA1_LENGTH));

These two PRINT_BUF statements are also broken and are removed.

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
File net/third_party/nss/ssl/sslimpl.h (right):

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
net/third_party/nss/ssl/sslimpl.h:795: HANDSHAKE_HASH_COMBO = 1,  /* The
MD5/SHA-1 combination */

Do you think this should be named HANDSHAKE_HASH_MD5SHA instead?

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
net/third_party/nss/ssl/sslimpl.h:823: void (*sha_clone)(void *dest, void *src);

The sha_obj and sha_clone members are not necessary now, but they will be
necessary when we support using SHA-384 in the TLS 1.2 PRF.

The function prototype of sha_obj->clone() matches the prototype of
PK11_CloneContext, but not those of freebl's SHA256_Clone, SHA384_Clone, etc.
So we need a dedicated function pointer for cloning freebl hash contexts.

[agl, 2013-06-17 14:01:13]

LGTM

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
net/third_party/nss/ssl/ssl3con.c:3686: goto loser;
Several error paths in this function return directly - presumably
ss->ssl3.hs.[md5|sha] will be deleted when the ss object is destroyed. However,
here we have an explicit cleanup. It seems that handling of errors should be
uniform in this function.

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
File net/third_party/nss/ssl/sslimpl.h (right):

https://chromiumcodereview.appspot.com/17109007/diff/19001/net/third_party/ns...
net/third_party/nss/ssl/sslimpl.h:795: HANDSHAKE_HASH_COMBO = 1,  /* The
MD5/SHA-1 combination */
On 2013/06/15 19:40:55, wtc wrote:
> 
> Do you think this should be named HANDSHAKE_HASH_MD5SHA instead?

I think this name is fine if you like it.

[wtc, 2013-06-17 20:05:15]

Thank you for the review. Please review the diffs between
patch sets 2 and 3.

I made the error handling in ssl3_InitHandshakeHashes uniform.

I changed the HANDSHAKE_HASH_xxx enumerators to lowercase
because that seems to be the prevalent style in libSSL.

I added a patch file.

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

https://codereview.chromium.org/17109007/diff/19001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:3686: goto loser;

On 2013/06/17 14:01:14, agl wrote:
> Several error paths in this function return directly - presumably
> ss->ssl3.hs.[md5|sha] will be deleted when the ss object is destroyed.
However,
> here we have an explicit cleanup. It seems that handling of errors should be
> uniform in this function.

You are right. I was also unhappy with how I handled errors in this function.
It was influenced by the original code in ssl3_NewHandshakeHashes.

I've now removed the |loser| label. Now the function always returns directly.
The only property I try to maintain is that either ss->ssl3.hs.[md5|sha] are
both NULL, or they are both created successfully. Strictly speaking, this
invariant is not necessary because ss->ssl3.hs.[md5|sha] will be deleted when
the ss object is destroyed. I just wanted the ss object to be less confusing
when it is not created successfully.

[agl, 2013-06-17 20:09:12]

lgtm

[commit-bot: I haz the power, 2013-06-17 20:10:20]

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

[commit-bot: I haz the power, 2013-06-17 23:14:00]

Change committed as 206827