923089 - Support TLS 1.2 PRF with SHA-384 as the hash function

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[BoerenkoolMetWorst]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258



Actual results:

https://bugzilla.mozilla.org/show_bug.cgi?id=880543#c9
"1. NSS can only support the AES_128_GCM cipher suites because NSS only supports
the TLS 1.2 PRF with SHA-256 as the hash function. The AES_256_GCM cipher suites
all use the TLS 1.2 PRF with SHA-384 as the hash function."


Expected results:

Please add support for TLS 1.2 PRF with SHA-384 as the hash function in order to support the superior AES_256_GCM cipher suites.

Comment 1

[David Filiatrault]

Will resolution of this issue result in support of TLS_RSA_WITH_AES_256_GCM_SHA384 (0x9d)?  

If so, good news as clients connecting to servers based on Barracuda WAF will then be able to avoid the known CBC issues like Lucky13.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Filiatrault from comment #1)
> Will resolution of this issue result in support of
> TLS_RSA_WITH_AES_256_GCM_SHA384 (0x9d)?  

This is a prerequisite for that. However, Firefox is unlikely to support that specific cipher suite. Instead, we intend to support only the TLS_ECDHE_* variants. However, Chrome may add support for that cipher suite.

> If so, good news as clients connecting to servers based on Barracuda WAF
> will then be able to avoid the known CBC issues like Lucky13.

Does Barracuda WAF not support TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256/TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256?

Comment 3

[David Filiatrault]

Barracuda WAF does not support any ECDHE_ or DHE_ ciphers yet.  When I inquired, they have support of ECDHE ciphers in their roadmap, but no release date announced yet.

Comment 4

[David Filiatrault]

As a compromise, could you make Firefox choose TLS_RSA_WITH_AES_128_GCM_SHA256 instead of selecting TLS_RSA_WITH_AES_128_CBC_SHA as it does now with sites which support both, like those with the Barracuda WAF?

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

David, it was probably my mistake to start this conversation in this bug. I will email you.

Comment 7

[Elio Maldonado]

Work for adding support for the SHA384 TLS cipher suites has been conducted by Red Hat as this is required for obtaining Common Criteria certification. 

Some background information is in order. The work started based on, and extends, previous work done for Chromium. See https://codereview.chromium.org/23510003. It implements mechanisms as defined by Oasis for PKCS #11 V 2.40, see http://docs.oasis-open.org/pkcs11/pkcs11-curr/v2.40/cs01/pkcs11-curr-v2.40-cs01.pdf.
Some of the patches I'll submit have an overlap with a patch that Wan-Teh had submiited for Bug 951455 see https://bugzilla.mozilla.org/show_bug.cgi?id=951455. I'll split the work unto a series of patches for util, softoken, libssl, test scripts and tstcnt for ease of review. The libssl part in particular needs a bit more work.

Comment 8

[Elio Maldonado]

Let's start with Bob Relyea as the initial reviewer and let him choose the "official" reviewers from folks not associated with Red Hat.

Comment 9

[Elio Maldonado]

Attachment: the util portion - patch 1 of 5

Comment 10

[Elio Maldonado]

Attachment: the softoken portion - patch 2 of 5

Comment 11

[Elio Maldonado]

Attachment: the libssl portion - patch 3 of 5

This patch has been modified from the original to account for changes that Martin has already committed.

Comment 12

[Elio Maldonado]

Attachment: ssl test script - as used downstream

Comment 13

[Elio Maldonado]

Attachment: tstclnt ssl2 off by default

As we tested on rhel-7.1 I had to make that change for the tests to pass. On rhel-7 we build with ssl2 and export cipher suites disabled which is not yet the case here.

Comment 14

[Hubert Kario]

"tested" - we have conducted interoperability tests with OpenSSL and GnuTLS in configurations with and without client certificates with ECDHE, DHE and RSA key exchange with ECDSA, DSS and RSA authentication.

The only issue is with TLS_DHE_DSS_WITH_AES_256_GCM_SHA384 when it's used with client certificates, as GnuTLS won't use or accept SHA384 hash in Certificate Verify. To fix this, we need to fix bug 1179338.

Comment 15

[Elio Maldonado]

Let me add that the downstream testing was conducted with softoken and freebl libraries that had the FIPS-140 patches applied, the ones that Bob will submit for Bug 1181814, which are not in included here.

Comment 16

[Elio Maldonado]

Attachment: util portion V2 - patch 1 of 5

Comment 17

[Elio Maldonado]

Attachment: softoken portin V2 - patch 2 of 5

Comment 18

[Elio Maldonado]

Attachment: libssl prtion V2 - patch 3 of 5

Comment 19

[Elio Maldonado]

Revised 3 patches on account of the recent https://hg.mozilla.org/projects/nss/rev/f9b7bdf6e68b commit.

Comment 20

[Elio Maldonado]

Attachment: adds tls12 sha384 prf support to softoken & ssl libraries

Comment 21

[Elio Maldonado]

Comment on attachment 8631856 [details] [diff] [review]
ssl test script - as used downstream

Along with adding new tests it removes SSL2 and export cipher suites. Will attach one that just adds tests case.

Comment 22

[Elio Maldonado]

Attachment: adds test cases for new supported cipher suites

Comment 23

[Robert Relyea]

Comment on attachment 8632759 [details] [diff] [review]
adds test cases for new supported cipher suites

Review of attachment 8632759 [details] [diff] [review]:
-----------------------------------------------------------------

depending on the answer to why we had to add -V ssl3: to the command line.

::: tests/ssl/ssl.sh
@@ +218,4 @@
>    if [ ${fileout} -eq 1 ]; then
>        ${PROFTOOL} ${BINDIR}/selfserv -D -p ${PORT} -d ${P_R_SERVERDIR} -n ${HOSTADDR} ${SERVER_OPTIONS} \
>                 ${ECC_OPTIONS} -S ${HOSTADDR}-dsa -w nss ${sparam} -i ${R_SERVERPID} $verbose -H 1 \
> +               -V ssl3:> ${SERVEROUTFILE} 2>&1 &

What the -V ssl3?

Comment 24

[Robert Relyea]

Comment on attachment 8631857 [details] [diff] [review]
tstclnt ssl2 off by default

Review of attachment 8631857 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like another reviewer to validate this. basically tstclnt no longer tests ssl2.

bob

Comment 25

[Robert Relyea]

Comment on attachment 8632467 [details] [diff] [review]
adds tls12 sha384 prf support to softoken & ssl libraries

Review of attachment 8632467 [details] [diff] [review]:
-----------------------------------------------------------------

1) Most of this code I wrote, so we will need a second reviewer.
2) This code is missing support to run if we don't have the TLS_12_MECHANISM (RHEL 6). I think we should have a separate patch for this.
3) I think we should probably drop all the client auth changes from this patch and put them in the client auth hash bug.
4) I think there's a bug in the patch in ssl3con.c.

The bug is why I r-'ed this. I think I want the client auth changes dropped, but maybe we should disable client auth for SHA384 ciphers temporarily in this patch until the full client auth patch is dropped in. Otherwise we will silently send and accept the wrong data for client auth for the SHA384 ciphers.

::: lib/softoken/pkcs11c.c
@@ +5867,1 @@
>      CK_MECHANISM_TYPE mechanism = pMechanism->mechanism;

General common on this file... I wrote the changes below, it probably needs another reviewer.
These changes are necessary because the key_block is larger for larger hash sizes. It turns out that SHA256 fits because we don't need as many keys, so we don't use the full number of MIXERS. SHA384 doesn't fit. The actual keyblock size here safely bigger than it needs to be for TLS.

::: lib/ssl/ssl3con.c
@@ +3696,2 @@
>  	}
>  

I'm pretty sure this change isn't right. You still need to set master_param_len appropriately. The only change here is the ssl3_GetPrfHashMechanism for CKM_SHA256.

@@ +4767,5 @@
> +            ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
> +            rv = SECFailure;
> +            goto tls12_loser;
> +        }
> +

I think this is part of trying to handle the client auth case, we should probably drop our hack of swapping between sha256 and sha384 and plan on implementing the fix we already have a patch for.

@@ +7041,4 @@
>  static void
>  ssl3_DestroyBackupHandshakeHashIfNotNeeded(sslSocket *ss,
>  					   const SECItem *algorithms)
>  {

The changes in this file are part of the client auth case, we should probably drop our hack of swapping between sha256 and sha384 and plan on implementing the fix we already have a patch for.

@@ +9322,5 @@
> +    } else {
> +        PORT_Assert(hashOid->offset == SEC_OID_SHA256 ||
> +                    hashOid->offset == SEC_OID_SHA384);
> +	return SECFailure; 		/* err set by AppendHandshake. */
> +    }

This block is client auth changes.

::: lib/ssl/sslimpl.h
@@ +304,2 @@
>  #else
> +#define ssl_V3_SUITES_IMPLEMENTED 43

We should compile with ECC disabled and verify the number. There's a runtime check in SSL, so we will blow up pretty quickly.

Comment 26

[Robert Relyea]

Comment on attachment 8631856 [details] [diff] [review]
ssl test script - as used downstream

Review of attachment 8631856 [details] [diff] [review]:
-----------------------------------------------------------------

How is this different than the other patch I reviewed?

r- since we should only attach one. If you want this one obsolete the other patch and rerequest the review.

Comment 27

[Robert Relyea]

OK, Now that I read the comments, I see the difference between the patches. I'm OK with either, I've r+ the one that removes ssl2. I'm open to feedback by others about the idea of dropping ssl2 in the test cases (currently Red Hat is the only one that 'uses' it... in that we have products that can enable it and still officially support it). I'm OK if someone else wants us to keep the ssl2 tests util we actually ax the ssl2 code. NOTE to Elio.. this means if someone breaks SSL2 by accident you won't know it until you integrate it into RHEL 5, are you sure you want that (of course it makes RHEL 7 easier since you don't need to turn SSL2 off in RHEL 7).

bob

Comment 28

[Robert Relyea]

This bug does not depend on bug 1181814. The softoken work to make this work is already in softoken.

Comment 29

[Elio Maldonado]

Attachment: adds tls12 sha384 prf support to softoken & ssl libraries

This is still a work in progress and not ready for review but I attach it for discussion and to clarify some requests that Bob made an previous review and are not yet entirely clear to me.

Per Bob's request this patch does not try to handle the client auth case, drops the code for swapping between sha256 and sha384. 

The functions in question are
ssl3_DestroyBackupHandshakeHashIfNotNeeded
ssl3_ComputeHandshakeHashes
ssl3_HandleServerHello
ssl3_SendCertificateRequest

moved the changes to a separte patch which I will attach next.

What about the changes in ssl3_InitHandshakeHashes, shall they remain?

Bob, you stated as part of Comment 25
"...., we should probably drop our hack of swapping between sha256 and sha384 and plan on implementing the fix we already have a patch for."

And what is that patch?

I know that Kai is going to be working o something related 
Bug 1179338 - Don't require that the signature method for certificate verify is identical with PRF, support alternatives

Is that the work you are referring to?

Comment 30

[Elio Maldonado]

Attachment: client authentication changes due to sha384 support

Moved here the changes to
  ssl3_DestroyBackupHandshakeHashIfNotNeeded
  ssl3_ComputeHandshakeHashes
  ssl3_HandleServerHello
  ssl3_SendCertificateRequest
as requested. 

Will this be starting point for Kai's work?

Comment 31

[Elio Maldonado]

Comment on attachment 8738190 [details] [diff] [review]
adds tls12 sha384 prf support to softoken & ssl libraries

Preliminary reviewer Bob, will request review from Martin later once my questions are clarified.

Comment 32

[Elio Maldonado]

Comment on attachment 8738191 [details] [diff] [review]
client authentication changes due to sha384 support

Preliminary reviewer Bob, will request review from Martin later once my doubts are clarified.

Comment 33

[Elio Maldonado]

I should add that not all test are passing yet and I'm on working that but needed some early look at the work done so far.

Comment 34

[Elio Maldonado]

Comment on attachment 8738190 [details] [diff] [review]
adds tls12 sha384 prf support to softoken & ssl libraries

Review of attachment 8738190 [details] [diff] [review]:
-----------------------------------------------------------------

r- on self-review, new patch coming soon.

::: lib/ssl/ssl3con.c
@@ +112,4 @@
>   { TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, SSL_ALLOWED, PR_TRUE, PR_FALSE},
>   { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, SSL_ALLOWED, PR_FALSE, PR_FALSE},
> + { TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,   SSL_ALLOWED, PR_FALSE, PR_FALSE},

Not inconsistent with the order in the table from sslenum.c

@@ +312,5 @@
>      {cipher_camellia_256, calg_camellia,    32,32, type_block, 16,16, 0, 0, SEC_OID_CAMELLIA_256_CBC},
>      {cipher_seed,         calg_seed,        16,16, type_block, 16,16, 0, 0, SEC_OID_SEED_CBC},
>      {cipher_aes_128_gcm,  calg_aes_gcm,     16,16, type_aead,   4, 0,16, 8, SEC_OID_AES_128_GCM},
>      {cipher_chacha20,     calg_chacha20,    32,32, type_aead,  12, 0,16, 0, SEC_OID_CHACHA20_POLY1305},
> +    {cipher_aes_256_gcm,  calg_aes_gcm,     32,32, type_aead,   4, 0,16, 8},

Move it up one to be aftre cipher_aes_a128_gcm and before cipher_chacha20.

@@ +3883,5 @@
> +    SSL3PRF prf_alg = ss->ssl3.hs.suite_def->prf_alg;
> +
> +   if (prf_alg == 0)
> +        return CKM_SHA256;
> +    

NIT: whitespace

@@ +6971,5 @@
> +    if (rv != SECSuccess) {
> +        desc = internal_error;
> +        errCode = PORT_GetError();
> +        goto alert_loser;
> +    }

Not visible here but near the beginning of ssl3_InitHandshakeHashes(sslSocket *ss)
it okay to bring these changes:
-            ss->ssl3.hs.sha_obj = HASH_GetRawHashObject(HASH_AlgSHA256);
+            HASH_HashType ht;
+            CK_MECHANISM_TYPE hm;
+            SECOidTag ot;
+            SECOidData *hashOid;
+
+            hm = ssl3_GetPrfHashMechanism(ss);
+            hashOid = SECOID_FindOIDByMechanism(hm);
+
+            if (hashOid == NULL) {
+                ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
+                return SECFailure;
+            }
+            ot = hashOid->offset;
+            ht = HASH_GetHashTypeByOidTag(ot);
+            ss->ssl3.hs.sha_obj = HASH_GetRawHashObject(ht);

::: lib/ssl/sslimpl.h
@@ +319,2 @@
>  #else
>  #define ssl_V3_SUITES_IMPLEMENTED 41

Change
-#define ssl_V3_SUITES_IMPLEMENTED 41
+#define ssl_V3_SUITES_IMPLEMENTED 44

We are adding 7 ciphers of which 4 are ECC and 3 aren't.
While testing an assertion caught this.

::: tests/doc/nssqa.txt
@@ +86,5 @@
>  	header:exit (global)
>  		remove temporary files
>  		kill remaining selfservers
>  		send email to the list
>  		

This changes sneaked by mistake because the way I test. Remove them in the next round.

::: tests/doc/platform_specific_problems
@@ +13,2 @@
>  /local/llennox/NSS-PSM/mozilla/tests_results/security/conrail.20/server -n
>  conrail.cs.columbia.edu -w nss   -i /tmp/tests_pid.5505  & strsclnt -p 8443 -d . -w nss -c 1000 -C A  conrail.cs.columbia.edu 

These changes sneaked by mistake because the way I test. Remove them in the next round.

::: tests/doc/qa_wrapper.html
@@ +105,5 @@
>  no-lockfile)
>  <br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; set HOST and DOMSUF variables
>  if running interavtively
>  <br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; set flag to kill remaining
> +selfserv_9460 processes during cleanup

These changes sneaked by mistake because the way I test and didn't cleanup afterwards. Remove them in the next round.

::: tests/header
@@ +145,4 @@
>  
>                      # Set the masterbuilds 
>  if [ -z "$UX_MASTERBUILD" ]
>  then                     

These changes sneaked by mistake because the way I test and didn't cleanup afterwards. Remove them in the next round.

::: tests/iopr/ssl_iopr.sh
@@ +277,2 @@
>  # No return value
>  #  

These changes sneaked by mistake because the way I test and didn't cleanup afterwards. Remove them in the next round.

::: tests/memleak/ignored
@@ +47,5 @@
>  #463631
>  vfychain/main/PL_CreateOptState/**
>  
>  #486298
> +selfserv_9460/main/PORT_Strdup_Util**

These changes sneaked by mistake because the way I test and didn't cleanup afterwards. Remove them in the next round.

::: tests/memleak/memleak.sh
@@ +322,2 @@
>  	echo "==0=="
>  	

These changes sneaked by mistake because the way I test and didn't cleanup afterwards. Remove them in the next round.

::: tests/nssqa
@@ +36,4 @@
>  . `dirname $0`/header   # utilities, shellfunctions etc, global to NSS QA
>  
>  if [ -z "$O_TBX" -o "$O_TBX" != "ON" ] ; then
>      is_running ${TMP}/nssqa 

These changes sneaked by mistake because the way I test and didn't cleanup afterwards. Remove them in the next round.

::: tests/qa_stat
@@ +772,5 @@
>        grep "bgcolor=red" $BCT_RESULT | 
>             sed -e 's/.results.html:<TR><TD>/ /' -e 's/<[^>]*>/ /g'
>        grep 'cache hits; .* cache misses, .* cache not reusable' \
>          $BCT_LOG |
> +        grep -v selfserv_9460 |

These changes sneaked by mistake because the way I test and didn't cleanup afterwards. Remove them in the next round.

::: tests/qaclean
@@ +76,5 @@
>  i=0
>  while [ $i -lt $1 ]
>  do
>      kill_by_name nssqa
> +    kill_by_name selfserv_9460

These changes sneaked by mistake because the way I test and didn't cleanup afterwards. Remove them in the next round.

::: tests/run_niscc.sh
@@ +197,5 @@
>  
>      # Directory where to write all the output data (around 650MiB for each run)
>      export TEST_OUTPUT=${TEST_OUTPUT:-"$HOME/out"}
>  
> +    # How many threads to use in selfserv_9460 and strsclnt (max. 10)

These changes sneaked by mistake because the way I test and didn't cleanup afterwards. Remove them in the next round.

::: tests/ssl/ssl.sh
@@ +114,3 @@
>        fi
>    fi
>    

These changes sneaked by mistake because the way I test and didn't cleanup afterwards. Remove them in the next round.

Comment 35

[Elio Maldonado]

Comment on attachment 8738191 [details] [diff] [review]
client authentication changes due to sha384 support

The need to submit a new version of the other patch renders this one obsolete.

Comment 36

[Elio Maldonado]

Comment on attachment 8738190 [details] [diff] [review]
adds tls12 sha384 prf support to softoken & ssl libraries

SSL2 support is now removed.

Comment 37

[Elio Maldonado]

Attachment: add test cases for new supported cipher suites

Comment 38

[Elio Maldonado]

Attachment: adds tls12 sha384 prf support libsssl - basic part

does not include authentication changes which may end up in another bug.

Comment 39

[Elio Maldonado]

Attachment: tls12 sha384 prf support libsssl - plus client auth

For comparison with the other patch. Not all tests pass yet.

Comment 40

[Elio Maldonado]

Comment on attachment 8738863 [details] [diff] [review]
adds tls12 sha384 prf support libsssl - basic part

Review of attachment 8738863 [details] [diff] [review]:
-----------------------------------------------------------------

New version coming.

::: lib/ssl/ssl3con.c
@@ +10011,5 @@
>          const SSLSignatureAndHashAlg *alg = &ss->ssl3.signatureAlgorithms[i];
>          /* Note that we don't support a handshake hash with anything other than
>           * SHA-256, so asking for a signature from clients for something else
>           * would be inviting disaster. */
> +        if (alg->hashAlg == ssl_hash_sha256 || alg->hashAlg == ssl_hash_sha384) {

Change 
ssl_hash_sha256 to ssl_hash_sha256 and 
ssl_hash_sha384 to tls_hash_sha384.

Comment 41

[Elio Maldonado]

Comment on attachment 8738865 [details] [diff] [review]
tls12 sha384 prf support libsssl - plus client auth

Review of attachment 8738865 [details] [diff] [review]:
-----------------------------------------------------------------

Testing those changes.

::: lib/ssl/ssl3con.c
@@ +3863,5 @@
> +    SSL3PRF prf_alg = ss->ssl3.hs.suite_def->prf_alg;
> +
> +   if (prf_alg == 0)
> +        return CKM_SHA256;
> +    

Remove white space.

@@ +5178,5 @@
> +                ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
> +                rv = SECFailure;
> +                goto tls12_loser;
> +            }
> +        }

Change 
ssl_hash_sha256 to ssl_hash_sha256 and 
ssl_hash_sha384 to tls_hash_sha384.

@@ +7670,5 @@
> +        } else if (hashOid->offset == SEC_OID_SHA384) {
> +            suitePRFHash = ssl_hash_sha384;
> +            suitePRFIs256Or384 = PR_TRUE;
> +        }
> +    }

Change 
ssl_hash_sha256 to ssl_hash_sha256 and 
ssl_hash_sha384 to tls_hash_sha384.

@@ +10173,5 @@
> +            PORT_Assert(hashOid->offset == SEC_OID_SHA256 ||
> +                        hashOid->offset == SEC_OID_SHA384);
> +            return SECFailure; /* err set by AppendHandshake. */
> +        }
> +    }

Change 
ssl_hash_sha256 to ssl_hash_sha256 and 
ssl_hash_sha384 to tls_hash_sha384.

Comment 42

[Elio Maldonado]

Last Cahnge above is bogus.

Comment 43

[Elio Maldonado]

Last comment above is bogus.

Comment 44

[Elio Maldonado]

Bob, I'm still modifying and retesting changes but don't let that keep you from answering the two questions above in Comment 29 and Comment 30. Thanks, -Elio

Comment 45

[Elio Maldonado]

I should point out that when Kai backed out the 3 change sets for bug 1237514 the number of failures I was having when testing this went down substantially. That on top of my own fixes to the patches. Still trying to figure out the remaining failures.

Comment 46

[Elio Maldonado]

Comment on attachment 8738865 [details] [diff] [review]
tls12 sha384 prf support libsssl - plus client auth

Review of attachment 8738865 [details] [diff] [review]:
-----------------------------------------------------------------

I get Assertion failure: PR_FALSE, at ssl3con.c:813 many times. For a while the number had gone down substantilaly and now is back up again. It's in ssl_LookupCipherSuiteDef where the search through cipher_suite_defs didn't find the cipher suite.

::: lib/ssl/ssl3con.c
@@ +544,5 @@
>      {hmac_md5,  mmech_md5_hmac,   0,  MD5_LENGTH,  SEC_OID_HMAC_MD5},
>      {hmac_sha,  mmech_sha_hmac,   0,  SHA1_LENGTH, SEC_OID_HMAC_SHA1},
>      {hmac_sha256, mmech_sha256_hmac, 0, SHA256_LENGTH, SEC_OID_HMAC_SHA256},
>      { mac_aead, mmech_invalid,    0,  0, 0 },
> +    {hmac_sha384, mmech_sha384_hmac, 0, SHA384_LENGTH}

Missing a field. Change it to
 {hmac_sha384, mmech_sha384_hmac, 0, SHA384_LENGTH, SEC_OID_HMAC_SHA384}

::: lib/ssl/ssl3prot.h
@@ +232,5 @@
> +    tls_hash_sha224 = 3,
> +    tls_hash_sha256 = 4,
> +    tls_hash_sha384 = 5,
> +    tls_hash_sha512 = 6
> +} TLSHashAlgorithm;

This type is not needed because SSLHashType from sslt.h defines the same values. Remove it and use that one instead.

Comment 47

[Elio Maldonado]

Comment on attachment 8738865 [details] [diff] [review]
tls12 sha384 prf support libsssl - plus client auth

Review of attachment 8738865 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the reasons given before.

Comment 48

[Elio Maldonado]

Eric, I noticed a possible inconsistency among various tables when it comes to TLS 1.3 ECDHE_PSK.

https://hg.mozilla.org/projects/nss/file/tip/lib/ssl/sslinfo.c
look at lines 230 through 234 for static const SSLCipherSuiteInfo suiteInfo[] = {
...
230  #ifndef NSS_DISABLE_ECC

The TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256 entry was added inside the #ifndef NSS_DISABLE_ECC


but that is not the case in https://hg.mozilla.org/projects/nss/file/tip/lib/ssl/sslinfo.c

static ssl3CipherSuiteCfg cipherSuites[ssl_V3_SUITES_IMPLEMENTED] = {
   /*      cipher_suite                     policy       enabled   isPresent */

 /* ECDHE-PSK from [draft-mattsson-tls-ecdhe-psk-aead]. Only enabled if
  * we are doing TLS 1.3 PSK-resumption.
  */
 { TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256,   SSL_ALLOWED, PR_TRUE,  PR_FALSE}, 

where it's outside the #ifndef NSS_DISABLE_ECC

#ifndef NSS_DISABLE_ECC
 { TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, SSL_ALLOWED, PR_TRUE, PR_FALSE},


not on https://hg.mozilla.org/projects/nss/file/tip/lib/ssl/enum.c
const PRUint16 SSL_ImplementedCiphers[] = {
    /* ECDHE-PSK from [draft-mattsson-tls-ecdhe-psk-aead]. */
    TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256,      

#ifndef NSS_DISABLE_ECC
    TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
    ....
where it's outside the #ifndef NSS_DISABLE_ECC

Comment 49

[Elio Maldonado]

Attachment: tls12 sha384 prf support libsssl - plus client auth hacks

This is not to be committed as some changes need to be moved to a separate patch as Bob suggested and I/m still investigating some test failures. 

In some cases the server won't start due to a couple of
Assertion failure: PR_FALSE, at ssl3con.c:814
which tells me that not all the affected tables are in sync.

I attach it here in order to get some needed feedback and make sure I'm on the right track.

Comment 50

[Elio Maldonado]

Comment on attachment 8740117 [details] [diff] [review]
tls12 sha384 prf support libsssl - plus client auth hacks

Review of attachment 8740117 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/sslinfo.c
@@ +263,5 @@
>      {0,CS(TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA),    S_RSA, K_ECDHE, C_AES, B_256, M_SHA, 1, 0, 0 },
>      {0,CS(TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256), S_RSA, K_ECDHE, C_CHACHA20, B_256, M_AEAD_128, 0, 0, 0 },
> +    {0,CS(TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384),  S_RSA, K_ECDHE, C_AES, B_128, M_SHA384, 1, 0, 0 },
> +    {0,CS(TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384), S_ECDSA, K_ECDHE, C_AES, B_128, M_SHA384, 1, 0, 0 },
> +    {0,CS(TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384), S_ECDSA, K_ECDHE, C_AES, B_256, M_SHA384, 1, 0, 0 },

change C_AES to AESGCM

@@ +264,5 @@
>      {0,CS(TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256), S_RSA, K_ECDHE, C_CHACHA20, B_256, M_AEAD_128, 0, 0, 0 },
> +    {0,CS(TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384),  S_RSA, K_ECDHE, C_AES, B_128, M_SHA384, 1, 0, 0 },
> +    {0,CS(TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384), S_ECDSA, K_ECDHE, C_AES, B_128, M_SHA384, 1, 0, 0 },
> +    {0,CS(TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384), S_ECDSA, K_ECDHE, C_AES, B_256, M_SHA384, 1, 0, 0 },
> +    {0,CS(TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256), S_RSA, K_ECDHE, C_AES, B_128, M_SHA256, 1, 0, 0 },

change C_AES to AESGCM

@@ +265,5 @@
> +    {0,CS(TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384),  S_RSA, K_ECDHE, C_AES, B_128, M_SHA384, 1, 0, 0 },
> +    {0,CS(TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384), S_ECDSA, K_ECDHE, C_AES, B_128, M_SHA384, 1, 0, 0 },
> +    {0,CS(TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384), S_ECDSA, K_ECDHE, C_AES, B_256, M_SHA384, 1, 0, 0 },
> +    {0,CS(TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256), S_RSA, K_ECDHE, C_AES, B_128, M_SHA256, 1, 0, 0 },
> +    {0,CS(TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384), S_RSA, K_ECDHE, C_AES, B_128, M_SHA384, 1, 0, 0 },

change C_AES to AESGCM

Comment 51

[Martin Thomson [:mt:]]

Comment on attachment 8740117 [details] [diff] [review]
tls12 sha384 prf support libsssl - plus client auth hacks

Review of attachment 8740117 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +109,4 @@
>   { TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, SSL_ALLOWED, PR_FALSE, PR_FALSE},
> + { TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,   SSL_ALLOWED, PR_FALSE, PR_FALSE},

I would very much prefer to see the CBC modes below ChaCha.  Maybe also the 256-bit GCM modes as well.

@@ +494,2 @@
>  #endif /* NSS_DISABLE_ECC */
> +    {TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_ecdhe_psk}

This is an ECC suite, why did you move it?

@@ +3860,5 @@
>  
> +CK_MECHANISM_TYPE
> +ssl3_GetPrfHashMechanism(sslSocket *ss)
> +{
> +    SSL3PRF prf_alg = ss->ssl3.hs.suite_def->prf_alg;

You should update the prf_alg value on all of the suite_def entries rather than do this.

@@ +4315,5 @@
>               * then this will need to be updated. */
> +            HASH_HashType ht;
> +            CK_MECHANISM_TYPE hm;
> +            SECOidTag ot;
> +            SECOidData *hashOid;

const

@@ +4811,5 @@
>      SSLHashType tlsHash;
>      SECOidTag oid;
>  } tlsHashOIDMap[] = {
>      { ssl_hash_sha1, SEC_OID_SHA1 },
> +    { ssl_hash_sha224, SEC_OID_SHA224 },

We specifically removed this one previously.  Do you actually need it?

@@ +4840,5 @@
> + * identifier. If the hash is not recognised, zero is returned.
> + *
> + * See https://tools.ietf.org/html/rfc5246#section-7.4.1.4.1 */
> +static int
> +ssl3_OIDToTLSHashAlgorithm(SECOidTag oid)

Why do you need this?

@@ +5161,5 @@
>              rv = SECFailure;
>              goto tls12_loser;
>          }
> +/* TODO: activate in a separate patch */
> +        /* a function perhaps? */

Yes.  Please extract this, you have subtle variations on this three times already.

@@ +7669,5 @@
> +        suitePRFIs256Or384 = PR_TRUE;
> +    } else if (hashOid->offset == SEC_OID_SHA384) {
> +        suitePRFHash = ssl_hash_sha384;
> +        suitePRFIs256Or384 = PR_TRUE;
> +    }

Rather than use the suitePRFIs256Or384, I think that you should have the PRF hash defined on the ssl3CipherSuiteDef instance that we are using; then you can just do a straight lookup to see if an algorithm matches the current suite.

@@ +10082,5 @@
>      *len = 0;
>      for (i = 0; i < ss->ssl3.signatureAlgorithmCount; ++i) {
>          const SSLSignatureAndHashAlg *alg = &ss->ssl3.signatureAlgorithms[i];
>          /* Note that we don't support a handshake hash with anything other than
>           * SHA-256, so asking for a signature from clients for something else

Fix comment.

@@ +10084,5 @@
>          const SSLSignatureAndHashAlg *alg = &ss->ssl3.signatureAlgorithms[i];
>          /* Note that we don't support a handshake hash with anything other than
>           * SHA-256, so asking for a signature from clients for something else
>           * would be inviting disaster. */
> +        if (alg->hashAlg == allowedHashAlg) {

Suggest s/allowHashAlg/suiteHashAlg/

@@ +10157,5 @@
>  
>      length = 1 + certTypesLength + 2 + calen;
> +
> +/* TODO: activate in a separate patch */
> +    /* a function perhaps? */

ss->ssl3.hs.suite_def->prfHashAlg

::: lib/ssl/tls13con.c
@@ +734,5 @@
>      /* Fixed context value. */
>      ss->ssl3.hs.certReqContext[0] = 0;
>      ss->ssl3.hs.certReqContextLen = 1;
>  
> +    /* TODO: separate patch, a function perhaps? move it to ssl3con.c */

as I mentioned before, please use ss->ssl3.hs.suite_def->prfHashAlg.  Also, I would move this lookup to ssl3_EncodeCertificateRequestSigAlgs() rather than pass the extra arg.

Comment 52

[Elio Maldonado]

(In reply to Martin Thomson [:mt:] from comment #51)
> Comment on attachment 8740117 [details] [diff] [review]
> tls12 sha384 prf support libsssl - plus client auth hacks
> 
> Review of attachment 8740117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/ssl/ssl3con.c
> @@ +109,4 @@
> >   { TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
> > + { TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
> > + { TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, SSL_ALLOWED, PR_FALSE, PR_FALSE},
> > + { TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,   SSL_ALLOWED, PR_FALSE, PR_FALSE},
> 
> I would very much prefer to see the CBC modes below ChaCha.  Maybe also the
> 256-bit GCM modes as well.

Yes, will do.
> 
> @@ +494,2 @@
> >  #endif /* NSS_DISABLE_ECC */
> > +    {TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_ecdhe_psk}
> 
> This is an ECC suite, why did you move it?

I had two versions to solve the inconsistency I had mentioned and I did it backwards, Will fix that and make sure order is consistent.

> 
> @@ +3860,5 @@
> >  
> > +CK_MECHANISM_TYPE
> > +ssl3_GetPrfHashMechanism(sslSocket *ss)
> > +{
> > +    SSL3PRF prf_alg = ss->ssl3.hs.suite_def->prf_alg;
> 
> You should update the prf_alg value on all of the suite_def entries rather
> than do this.

Yes, in an earlier version Bob had addedthe prf has algorithm was an extra field to static ssl3CipherSuiteCfg cipherSuites[ssl_V3_SUITES_IMPLEMENTED] = {
table. If I am not mistaken at one point you had done something similar. Need to revise.
Let me see if I can get rid of ssl3_GetPrfHashMechanism.


> 
> @@ +4315,5 @@
> >               * then this will need to be updated. */
> > +            HASH_HashType ht;
> > +            CK_MECHANISM_TYPE hm;
> > +            SECOidTag ot;
> > +            SECOidData *hashOid;
> 
> const
> 
> @@ +4811,5 @@
> >      SSLHashType tlsHash;
> >      SECOidTag oid;
> >  } tlsHashOIDMap[] = {
> >      { ssl_hash_sha1, SEC_OID_SHA1 },
> > +    { ssl_hash_sha224, SEC_OID_SHA224 },
> 
> We specifically removed this one previously.  Do you actually need it?
Not for here but we may need it upstream. That's something that I think is frowned upon on TLS 1.3. I'll remove for now.

> 
> @@ +4840,5 @@
> > + * identifier. If the hash is not recognised, zero is returned.
> > + *
> > + * See https://tools.ietf.org/html/rfc5246#section-7.4.1.4.1 */
> > +static int
> > +ssl3_OIDToTLSHashAlgorithm(SECOidTag oid)
> 
> Why do you need this?
It may not be needed after all.


> 
> @@ +5161,5 @@
> >              rv = SECFailure;
> >              goto tls12_loser;
> >          }
> > +/* TODO: activate in a separate patch */
> > +        /* a function perhaps? */
> 
> Yes.  Please extract this, you have subtle variations on this three times
> already.

Yes I have tree variants and haven't been able to crate a function with common code. Your other comment below on getting rid of suitePRFIs256Or384 will help.

> 
> @@ +7669,5 @@
> > +        suitePRFIs256Or384 = PR_TRUE;
> > +    } else if (hashOid->offset == SEC_OID_SHA384) {
> > +        suitePRFHash = ssl_hash_sha384;
> > +        suitePRFIs256Or384 = PR_TRUE;
> > +    }
> 
> Rather than use the suitePRFIs256Or384, I think that you should have the PRF
> hash defined on the ssl3CipherSuiteDef instance that we are using; then you
> can just do a straight lookup to see if an algorithm matches the current
> suite.

Next version will have it there as we had before.

> 
> @@ +10082,5 @@
> >      *len = 0;
> >      for (i = 0; i < ss->ssl3.signatureAlgorithmCount; ++i) {
> >          const SSLSignatureAndHashAlg *alg = &ss->ssl3.signatureAlgorithms[i];
> >          /* Note that we don't support a handshake hash with anything other than
> >           * SHA-256, so asking for a signature from clients for something else
> 
> Fix comment.
Will do.

> 
> @@ +10084,5 @@
> >          const SSLSignatureAndHashAlg *alg = &ss->ssl3.signatureAlgorithms[i];
> >          /* Note that we don't support a handshake hash with anything other than
> >           * SHA-256, so asking for a signature from clients for something else
> >           * would be inviting disaster. */
> > +        if (alg->hashAlg == allowedHashAlg) {
> 
> Suggest s/allowHashAlg/suiteHashAlg/
Yes
> 
> @@ +10157,5 @@
> >  
> >      length = 1 + certTypesLength + 2 + calen;
> > +
> > +/* TODO: activate in a separate patch */
> > +    /* a function perhaps? */
> 
> ss->ssl3.hs.suite_def->prfHashAlg
> 
> ::: lib/ssl/tls13con.c
> @@ +734,5 @@
> >      /* Fixed context value. */
> >      ss->ssl3.hs.certReqContext[0] = 0;
> >      ss->ssl3.hs.certReqContextLen = 1;
> >  
> > +    /* TODO: separate patch, a function perhaps? move it to ssl3con.c */
> 
> as I mentioned before, please use ss->ssl3.hs.suite_def->prfHashAlg.  Also,
> I would move this lookup to ssl3_EncodeCertificateRequestSigAlgs() rather
> than pass the extra arg.

Definitely.

Thank you you Martin for your prompt feedback.

Comment 53

[Elio Maldonado]

Martin,

I'm adding the prf field to two of the cipher_suite_defs[] table., I'm using prf_256 for
TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, 
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, and
LS_ECDHE_PSK_WITH_AES_128_GCM_SHA256

For example
    {TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_ecdhe_psk},
becomes
    {TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_ecdhe_psk, prf_256},


Is that correct?

-Elio

Comment 54

[Martin Thomson [:mt:]]

Yes, those cipher suites use SHA-256.  Please use the name prf_sha256 instead of prf_256.  Note that in TLS 1.3, we use a different PRF to TLS 1.2, but the underlying hash function remains the same.  Therefore, be careful when you comment the enum to point out that this is the hash function used by the PRF, and not the actual PRF.

Comment 55

[Elio Maldonado]

Attachment: tls12 sha384 prf support in libsssl - with client auth changes

Incorporates some of the feedback and cleans up code a bit. The sha384 test aren't working yet. The new ciphers are now marked as not enabled by default to match what we had last time Bob reviewed over a year ago and for early testing that this won't break existing functionality when not in use. I ran test without applying the sslcov.txt patch and the regular suites pass but the ssl_gtests fails. Will offer details on that soon.

Comment 56

[Hubert Kario]

While for merging the new ciphers probably should be disabled by default. We will want them enabled by default soon after.

The reason is that I'd prefer not to delay merging because of a discussion whether to enable them by default or not.

Comment 57

[Masatoshi Kimura [:emk]]

TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 should be under TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
TLS_DHE_DSS_WITH_AES_256_GCM_SHA384 should be under TLS_DHE_DSS_WITH_AES_128_GCM_SHA256, and TLS_RSA_WITH_AES_256_GCM_SHA384 should be under TLS_RSA_WITH_AES_128_GCM_SHA256 for consistency.

Comment 58

[Elio Maldonado]

I noticed the patch didn't apply cleanly.

Comment 59

[Elio Maldonado]

Attachment: tls12 sha384 prf support in libsssl - with client auth changes

This one applies cleanly, all standard non-sha384 tests regression pass (Passed: 17081) and the ssl-gtest failures are now gone. I'll start enabling the sha384 prf ciphers by default and test them.

Comment 60

[Masatoshi Kimura [:emk]]

Comment on attachment 8742464 [details] [diff] [review]
tls12 sha384 prf support in libsssl - with client auth changes

Review of attachment 8742464 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +133,5 @@
>   { TLS_DHE_RSA_WITH_AES_128_CBC_SHA,        SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>   { TLS_DHE_DSS_WITH_AES_128_CBC_SHA,        SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>   { TLS_DHE_RSA_WITH_AES_128_CBC_SHA256,     SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>   { TLS_DHE_DSS_WITH_AES_128_CBC_SHA256,     SSL_ALLOWED, PR_FALSE, PR_FALSE},
> + { TLS_DHE_DSS_WITH_AES_256_GCM_SHA384,     SSL_ALLOWED, PR_FALSE, PR_FALSE},

This is still misplaced (not TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 but TLS_DHE_DSS_WITH_AES_128_GCM_SHA256).

Comment 61

[Masatoshi Kimura [:emk]]

Comment on attachment 8742464 [details] [diff] [review]
tls12 sha384 prf support in libsssl - with client auth changes

Review of attachment 8742464 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +109,5 @@
>   { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, SSL_ALLOWED, PR_FALSE, PR_FALSE},
> + { TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,   SSL_ALLOWED, PR_FALSE, PR_FALSE},
> + { TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, SSL_ALLOWED, PR_FALSE, PR_FALSE},
> + { TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,   SSL_ALLOWED, PR_FALSE, PR_FALSE},

TLS_ECDHE_*_WITH_AES_256_CBC_SHA384 should be placed under TLS_ECDHE_*_WITH_AES_128_CBC_SHA256.

Comment 62

[Robert Relyea]

Elio, no need to split the basic client auth out of this patch. It's still needed, at least until kai does his work.

Comment 63

[Elio Maldonado]

Comment on attachment 8738863 [details] [diff] [review]
adds tls12 sha384 prf support libsssl - basic part

>diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
>--- a/lib/ssl/ssl3con.c
>+++ b/lib/ssl/ssl3con.c
>@@ -100,17 +100,21 @@ static ssl3CipherSuiteCfg cipherSuites[s
> 
>  /* ECDHE-PSK from [draft-mattsson-tls-ecdhe-psk-aead]. Only enabled if
>   * we are doing TLS 1.3 PSK-resumption.
>   */
>  { TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256,   SSL_ALLOWED, PR_TRUE,  PR_FALSE},
> 
> #ifndef NSS_DISABLE_ECC
>  { TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, SSL_ALLOWED, PR_TRUE, PR_FALSE},
>+ { TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
>+ { TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,   SSL_ALLOWED, PR_FALSE, PR_FALSE},
>+ { TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, SSL_ALLOWED, PR_FALSE, PR_FALSE},
>+ { TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,   SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, SSL_ALLOWED, PR_TRUE, PR_FALSE},
>  { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
>    /* TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA is out of order to work around
>     * bug 946147.
>     */
>  { TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,    SSL_ALLOWED, PR_TRUE, PR_FALSE},
>  { TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,    SSL_ALLOWED, PR_TRUE, PR_FALSE},
>  { TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,      SSL_ALLOWED, PR_TRUE, PR_FALSE},
>@@ -127,16 +131,18 @@ static ssl3CipherSuiteCfg cipherSuites[s
>  { TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256,SSL_ALLOWED,PR_TRUE,  PR_FALSE},
>  { TLS_DHE_DSS_WITH_AES_128_GCM_SHA256,     SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_DHE_RSA_WITH_AES_128_CBC_SHA,        SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_DHE_DSS_WITH_AES_128_CBC_SHA,        SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_DHE_RSA_WITH_AES_128_CBC_SHA256,     SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_DHE_DSS_WITH_AES_128_CBC_SHA256,     SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_DHE_RSA_WITH_CAMELLIA_128_CBC_SHA,   SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_DHE_DSS_WITH_CAMELLIA_128_CBC_SHA,   SSL_ALLOWED, PR_FALSE, PR_FALSE},
>+ { TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,     SSL_ALLOWED, PR_FALSE, PR_FALSE},
>+ { TLS_DHE_DSS_WITH_AES_256_GCM_SHA384,     SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_DHE_RSA_WITH_AES_256_CBC_SHA,        SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_DHE_DSS_WITH_AES_256_CBC_SHA,        SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_DHE_RSA_WITH_AES_256_CBC_SHA256,     SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_DHE_DSS_WITH_AES_256_CBC_SHA256,     SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA,   SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_DHE_DSS_WITH_CAMELLIA_256_CBC_SHA,   SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA,       SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA,       SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>@@ -149,16 +155,17 @@ static ssl3CipherSuiteCfg cipherSuites[s
>  { TLS_ECDH_RSA_WITH_AES_256_CBC_SHA,       SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA,    SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA,      SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_ECDH_ECDSA_WITH_RC4_128_SHA,         SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_ECDH_RSA_WITH_RC4_128_SHA,           SSL_ALLOWED, PR_FALSE, PR_FALSE},
> #endif /* NSS_DISABLE_ECC */
> 
>  /* RSA */
>+ { TLS_RSA_WITH_AES_256_GCM_SHA384,         SSL_ALLOWED, PR_FALSE,  PR_FALSE},
>  { TLS_RSA_WITH_AES_128_GCM_SHA256,         SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_RSA_WITH_AES_128_CBC_SHA,            SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_RSA_WITH_AES_128_CBC_SHA256,         SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_RSA_WITH_CAMELLIA_128_CBC_SHA,       SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_RSA_WITH_AES_256_CBC_SHA,            SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_RSA_WITH_AES_256_CBC_SHA256,         SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>  { TLS_RSA_WITH_CAMELLIA_256_CBC_SHA,       SSL_ALLOWED, PR_FALSE, PR_FALSE},
>  { TLS_RSA_WITH_SEED_CBC_SHA,               SSL_ALLOWED, PR_FALSE, PR_FALSE},
>@@ -298,16 +305,17 @@ static const ssl3BulkCipherDef bulk_ciph
>     {cipher_des40,        calg_des,          8, 5, type_block,  8, 8, 0, 0, SEC_OID_DES_40_CBC},
>     {cipher_idea,         calg_idea,        16,16, type_block,  8, 8, 0, 0, SEC_OID_IDEA_CBC},
>     {cipher_aes_128,      calg_aes,         16,16, type_block, 16,16, 0, 0, SEC_OID_AES_128_CBC},
>     {cipher_aes_256,      calg_aes,         32,32, type_block, 16,16, 0, 0, SEC_OID_AES_256_CBC},
>     {cipher_camellia_128, calg_camellia,    16,16, type_block, 16,16, 0, 0, SEC_OID_CAMELLIA_128_CBC},
>     {cipher_camellia_256, calg_camellia,    32,32, type_block, 16,16, 0, 0, SEC_OID_CAMELLIA_256_CBC},
>     {cipher_seed,         calg_seed,        16,16, type_block, 16,16, 0, 0, SEC_OID_SEED_CBC},
>     {cipher_aes_128_gcm,  calg_aes_gcm,     16,16, type_aead,   4, 0,16, 8, SEC_OID_AES_128_GCM},
>+    {cipher_aes_256_gcm,  calg_aes_gcm,     32,32, type_aead,   4, 0,16, 8, SEC_OID_AES_256_GCM},
>     {cipher_chacha20,     calg_chacha20,    32,32, type_aead,  12, 0,16, 0, SEC_OID_CHACHA20_POLY1305},
>     {cipher_missing,      calg_null,         0, 0, type_stream, 0, 0, 0, 0, 0},
> };
> 
> static const ssl3KEADef kea_defs[] =
> { /* indexed by SSL3KeyExchangeAlgorithm */
>     /* kea            exchKeyType signKeyType is_limited limit tls_keygen ephemeral  oid */
>     {kea_null,           kt_null, ssl_sign_null,  PR_FALSE,   0, PR_FALSE, PR_FALSE, 0},
>@@ -427,20 +435,28 @@ static const ssl3CipherSuiteDef cipher_s
>     {SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA, cipher_3des, mac_sha, kea_rsa_fips},
>     {SSL_RSA_FIPS_WITH_DES_CBC_SHA, cipher_des,    mac_sha, kea_rsa_fips},
> 
>     {TLS_DHE_RSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_dhe_rsa},
>     {TLS_RSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_rsa},
>     {TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_ecdhe_rsa},
>     {TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_ecdhe_ecdsa},
> 
>+    {TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_ecdhe_rsa},
>+    {TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_ecdhe_ecdsa},
>+    {TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, cipher_aes_256_gcm, mac_aead, kea_ecdhe_ecdsa},
>+    {TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, cipher_aes_256_gcm, mac_aead, kea_ecdhe_rsa},
>+    {TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, cipher_aes_256, hmac_sha384, kea_ecdhe_ecdsa},
>+    {TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384, cipher_aes_256, hmac_sha384, kea_ecdhe_rsa},
>     {TLS_DHE_DSS_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_dhe_dss},
>+    {TLS_DHE_DSS_WITH_AES_256_GCM_SHA384, cipher_aes_256_gcm, mac_aead, kea_dhe_dss},
>     {TLS_DHE_DSS_WITH_AES_128_CBC_SHA256, cipher_aes_128, hmac_sha256, kea_dhe_dss},
>     {TLS_DHE_DSS_WITH_AES_256_CBC_SHA256, cipher_aes_256, hmac_sha256, kea_dhe_dss},
> 
>+    {TLS_RSA_WITH_AES_256_GCM_SHA384, cipher_aes_256_gcm, mac_aead, kea_rsa},
>     {TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256, cipher_chacha20, mac_aead, kea_dhe_rsa},
>     {TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, cipher_chacha20, mac_aead, kea_ecdhe_rsa},
>     {TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, cipher_chacha20, mac_aead, kea_ecdhe_ecdsa},
> 
> #ifndef NSS_DISABLE_ECC
>     {TLS_ECDH_ECDSA_WITH_NULL_SHA,        cipher_null, mac_sha, kea_ecdh_ecdsa},
>     {TLS_ECDH_ECDSA_WITH_RC4_128_SHA,      cipher_rc4, mac_sha, kea_ecdh_ecdsa},
>     {TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA, cipher_3des, mac_sha, kea_ecdh_ecdsa},
>@@ -511,28 +527,30 @@ static const SSLCipher2Mech alg2Mech[] =
> };
> 
> #define mmech_invalid (CK_MECHANISM_TYPE)0x80000000L
> #define mmech_md5 CKM_SSL3_MD5_MAC
> #define mmech_sha CKM_SSL3_SHA1_MAC
> #define mmech_md5_hmac CKM_MD5_HMAC
> #define mmech_sha_hmac CKM_SHA_1_HMAC
> #define mmech_sha256_hmac CKM_SHA256_HMAC
>+#define mmech_sha384_hmac CKM_SHA384_HMAC
> 
> /* clang-format off */
> static const ssl3MACDef mac_defs[] = { /* indexed by SSL3MACAlgorithm */
>     /* pad_size is only used for SSL 3.0 MAC. See RFC 6101 Sec. 5.2.3.1. */
>     /* mac      mmech       pad_size  mac_size                       */
>     { mac_null, mmech_invalid,    0,  0         ,  0},
>     { mac_md5,  mmech_md5,       48,  MD5_LENGTH,  SEC_OID_HMAC_MD5 },
>     { mac_sha,  mmech_sha,       40,  SHA1_LENGTH, SEC_OID_HMAC_SHA1},
>     {hmac_md5,  mmech_md5_hmac,   0,  MD5_LENGTH,  SEC_OID_HMAC_MD5},
>     {hmac_sha,  mmech_sha_hmac,   0,  SHA1_LENGTH, SEC_OID_HMAC_SHA1},
>     {hmac_sha256, mmech_sha256_hmac, 0, SHA256_LENGTH, SEC_OID_HMAC_SHA256},
>     { mac_aead, mmech_invalid,    0,  0, 0 },
>+    {hmac_sha384, mmech_sha384_hmac, 0, SHA384_LENGTH}
> };
> /* clang-format on */
> 
> /* indexed by SSL3BulkCipher */
> const char *const ssl3_cipherName[] = {
>     "NULL",
>     "RC4",
>     "RC4-40",
>@@ -719,29 +737,36 @@ ssl3_CipherSuiteAllowedForVersionRange(
>              *   TLS_DH_anon_EXPORT_WITH_RC4_40_MD5:     never implemented
>              *   TLS_DH_anon_EXPORT_WITH_DES40_CBC_SHA:  never implemented
>              */
>             return vrange->min <= SSL_LIBRARY_VERSION_TLS_1_0;
> 
>         case TLS_DHE_RSA_WITH_AES_256_CBC_SHA256:
>         case TLS_RSA_WITH_AES_256_CBC_SHA256:
>         case TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256:
>+        case TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384:
>         case TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256:
>+        case TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384:
>         case TLS_DHE_RSA_WITH_AES_128_CBC_SHA256:
>         case TLS_RSA_WITH_AES_128_CBC_SHA256:
>         case TLS_RSA_WITH_AES_128_GCM_SHA256:
>+        case TLS_RSA_WITH_AES_256_GCM_SHA384:
>         case TLS_DHE_DSS_WITH_AES_128_CBC_SHA256:
>         case TLS_DHE_DSS_WITH_AES_256_CBC_SHA256:
>         case TLS_RSA_WITH_NULL_SHA256:
>         case TLS_DHE_DSS_WITH_AES_128_GCM_SHA256:
>             return vrange->max == SSL_LIBRARY_VERSION_TLS_1_2;
> 
>         case TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256:
>+        case TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384:
>         case TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256:
>+        case TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384:
>         case TLS_DHE_RSA_WITH_AES_128_GCM_SHA256:
>+        case TLS_DHE_RSA_WITH_AES_256_GCM_SHA384:
>+        case TLS_DHE_DSS_WITH_AES_256_GCM_SHA384:
>             return vrange->max >= SSL_LIBRARY_VERSION_TLS_1_2;
> 
>         /* RFC 4492: ECC cipher suites need TLS extensions to negotiate curves and
>          * point formats.*/
>         case TLS_ECDH_ECDSA_WITH_NULL_SHA:
>         case TLS_ECDH_ECDSA_WITH_RC4_128_SHA:
>         case TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA:
>         case TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA:
>@@ -2507,16 +2532,19 @@ ssl3_ComputeRecordMAC(
>                 hashObj = HASH_GetRawHashObject(HASH_AlgMD5);
>                 break;
>             case ssl_hmac_sha: /* used with TLS */
>                 hashObj = HASH_GetRawHashObject(HASH_AlgSHA1);
>                 break;
>             case ssl_hmac_sha256: /* used with TLS */
>                 hashObj = HASH_GetRawHashObject(HASH_AlgSHA256);
>                 break;
>+            case ssl_hmac_sha384: /* used with TLS */
>+                hashObj = HASH_GetRawHashObject(HASH_AlgSHA384);
>+                break;
>             default:
>                 break;
>         }
>         if (!hashObj) {
>             PORT_Assert(0);
>             PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
>             return SECFailure;
>         }
>@@ -3824,16 +3852,27 @@ ssl3_HandleChangeCipherSpecs(sslSocket *
>      */
>     if (ss->ssl3.prSpec == ss->ssl3.pwSpec) {
>         ssl3_DestroyCipherSpec(ss->ssl3.prSpec, PR_FALSE /*freeSrvName*/);
>     }
>     ssl_ReleaseSpecWriteLock(ss); /*************************************/
>     return SECSuccess;
> }
> 
>+CK_MECHANISM_TYPE
>+ssl3_GetPrfHashMechanism(sslSocket *ss)
>+{
>+    SSL3PRF prf_alg = ss->ssl3.hs.suite_def->prf_alg;
>+
>+   if (prf_alg == 0)
>+        return CKM_SHA256;
>+    
>+    return prf_alg;
>+}
>+
> /* This method completes the derivation of the MS from the PMS.
> **
> ** 1. Derive the MS, if possible, else return an error.
> **
> ** 2. Check the version if |pms_version| is non-zero and if wrong,
> **    return an error.
> **
> ** 3. If |msp| is nonzero, return MS in |*msp|.
>@@ -3947,17 +3986,17 @@ ssl3_ComputeMasterSecretInt(sslSocket *s
>     }
> 
>     master_params.pVersion = pms_version_ptr;
>     master_params.RandomInfo.pClientRandom = cr;
>     master_params.RandomInfo.ulClientRandomLen = SSL3_RANDOM_LENGTH;
>     master_params.RandomInfo.pServerRandom = sr;
>     master_params.RandomInfo.ulServerRandomLen = SSL3_RANDOM_LENGTH;
>     if (isTLS12) {
>-        master_params.prfHashMechanism = CKM_SHA256;
>+        master_params.prfHashMechanism = ssl3_GetPrfHashMechanism(ss);
>         master_params_len = sizeof(CK_TLS12_MASTER_KEY_DERIVE_PARAMS);
>     } else {
>         /* prfHashMechanism is not relevant with this PRF */
>         master_params_len = sizeof(CK_SSL3_MASTER_KEY_DERIVE_PARAMS);
>     }
> 
>     params.data = (unsigned char *)&master_params;
>     params.len = master_params_len;
>@@ -4190,17 +4229,17 @@ ssl3_DeriveConnectionKeysPKCS11(sslSocke
>         returnedKeys.pIVServer = NULL;
>     }
> 
>     calg = cipher_def->calg;
>     bulk_mechanism = ssl3_Alg2Mech(calg);
> 
>     if (isTLS12) {
>         key_derive = CKM_TLS12_KEY_AND_MAC_DERIVE;
>-        key_material_params.prfHashMechanism = CKM_SHA256;
>+        key_material_params.prfHashMechanism = ssl3_GetPrfHashMechanism(ss);
>         key_material_params_len = sizeof(CK_TLS12_KEY_MAT_PARAMS);
>     } else if (isTLS) {
>         key_derive = CKM_TLS_KEY_AND_MAC_DERIVE;
>         key_material_params_len = sizeof(CK_SSL3_KEY_MAT_PARAMS);
>     } else {
>         key_derive = CKM_SSL3_KEY_AND_MAC_DERIVE;
>         key_material_params_len = sizeof(CK_SSL3_KEY_MAT_PARAMS);
>     }
>@@ -4268,17 +4307,31 @@ ssl3_InitHandshakeHashes(sslSocket *ss)
> 
>     PORT_Assert(ss->ssl3.hs.hashType == handshake_hash_unknown);
> #ifndef NO_PKCS11_BYPASS
>     if (ss->opt.bypassPKCS11) {
>         PORT_Assert(!ss->ssl3.hs.sha_obj && !ss->ssl3.hs.sha_clone);
>         if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_2) {
>             /* If we ever support ciphersuites where the PRF hash isn't SHA-256
>              * then this will need to be updated. */
>-            ss->ssl3.hs.sha_obj = HASH_GetRawHashObject(HASH_AlgSHA256);
>+            HASH_HashType ht;
>+            CK_MECHANISM_TYPE hm;
>+            SECOidTag ot;
>+            SECOidData *hashOid;
>+
>+            hm = ssl3_GetPrfHashMechanism(ss);
>+            hashOid = SECOID_FindOIDByMechanism(hm);
>+
>+            if (hashOid == NULL) {
>+                ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
>+                return SECFailure;
>+            }
>+            ot = hashOid->offset;
>+            ht = HASH_GetHashTypeByOidTag(ot);
>+            ss->ssl3.hs.sha_obj = HASH_GetRawHashObject(ht);
>             if (!ss->ssl3.hs.sha_obj) {
>                 ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
>                 return SECFailure;
>             }
>             ss->ssl3.hs.sha_clone = (void (*)(void *, void *))SHA256_Clone;
>             ss->ssl3.hs.hashType = handshake_hash_single;
>             ss->ssl3.hs.sha_obj->begin(ss->ssl3.hs.sha_cx);
>         } else {
>@@ -4613,16 +4666,21 @@ ssl3_AppendHandshakeHeader(sslSocket *ss
> 
> /* ssl3_AppendSignatureAndHashAlgorithm appends the serialisation of
>  * |sigAndHash| to the current handshake message. */
> SECStatus
> ssl3_AppendSignatureAndHashAlgorithm(
>     sslSocket *ss, const SSLSignatureAndHashAlg *sigAndHash)
> {
>     PRUint8 serialized[2];
>+    SECOidTag hashAlg = ssl3_TLSHashAlgorithmToOID(sigAndHash->hashAlg);
>+    if (hashAlg == SEC_OID_UNKNOWN) {
>+        PORT_SetError(SSL_ERROR_UNSUPPORTED_HASH_ALGORITHM);
>+        return SECFailure;
>+    }
> 
>     serialized[0] = (PRUint8)sigAndHash->hashAlg;
>     serialized[1] = (PRUint8)sigAndHash->sigAlg;
> 
>     return ssl3_AppendHandshake(ss, serialized, sizeof(serialized));
> }
> 
> /**************************************************************************
>@@ -6777,23 +6835,16 @@ ssl3_HandleServerHello(sslSocket *ss, SS
>         desc = (version > SSL_LIBRARY_VERSION_3_0) ? protocol_version
>                                                    : handshake_failure;
>         errCode = SSL_ERROR_UNSUPPORTED_VERSION;
>         goto alert_loser;
>     }
>     ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_version;
>     isTLS = (ss->version > SSL_LIBRARY_VERSION_3_0);
> 
>-    rv = ssl3_InitHandshakeHashes(ss);
>-    if (rv != SECSuccess) {
>-        desc = internal_error;
>-        errCode = PORT_GetError();
>-        goto alert_loser;
>-    }
>-
>     rv = ssl3_ConsumeHandshake(
>         ss, &ss->ssl3.hs.server_random, SSL3_RANDOM_LENGTH, &b, &length);
>     if (rv != SECSuccess) {
>         goto loser; /* alert has been sent */
>     }
> 
>     /* Check the ServerHello.random per
>      * [draft-ietf-tls-tls13-11 Section 6.3.1.1].
>@@ -6898,16 +6949,24 @@ ssl3_HandleServerHello(sslSocket *ss, SS
>             errCode = SSL_ERROR_NO_COMPRESSION_OVERLAP;
>             goto alert_loser;
>         }
>         ss->ssl3.hs.compression = (SSLCompressionMethod)temp;
>     } else {
>         ss->ssl3.hs.compression = ssl_compression_null;
>     }
> 
>+    /* Wait until we've figured out the cipher suite before we initialize the handshake hashes */
>+    rv = ssl3_InitHandshakeHashes(ss);
>+    if (rv != SECSuccess) {
>+        desc = internal_error;
>+        errCode = PORT_GetError();
>+        goto alert_loser;
>+    }
>+
>     /* Note that if !isTLS and the extra stuff is not extensions, we
>      * do NOT goto alert_loser.
>      * There are some old SSL 3.0 implementations that do send stuff
>      * after the end of the server hello, and we deliberately ignore
>      * such stuff in the interest of maximal interoperability (being
>      * "generous in what you accept").
>      * Update: Starting in NSS 3.12.6, we handle the renegotiation_info
>      * extension in SSL 3.0.
>@@ -8576,23 +8635,16 @@ ssl3_HandleClientHello(sslSocket *ss, SS
>         desc = (version > SSL_LIBRARY_VERSION_3_0) ? protocol_version
>                                                    : handshake_failure;
>         errCode = SSL_ERROR_UNSUPPORTED_VERSION;
>         goto alert_loser;
>     }
>     isTLS13 = ss->version >= SSL_LIBRARY_VERSION_TLS_1_3;
>     ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_version;
> 
>-    rv = ssl3_InitHandshakeHashes(ss);
>-    if (rv != SECSuccess) {
>-        desc = internal_error;
>-        errCode = PORT_GetError();
>-        goto alert_loser;
>-    }
>-
>     /* Generate the Server Random now so it is available
>      * when we process the ClientKeyShare in TLS 1.3 */
>     rv = ssl3_GetNewRandom(&ss->ssl3.hs.server_random);
>     if (rv != SECSuccess) {
>         errCode = SSL_ERROR_GENERATE_RANDOM_FAILURE;
>         goto loser;
>     }
> 
>@@ -8980,16 +9032,24 @@ static SECStatus ssl3_HandleClientHelloP
>     errCode = SSL_ERROR_NO_COMPRESSION_OVERLAP;
>     /* null compression must be supported */
>     goto alert_loser;
> 
> compression_found:
>     suites->data = NULL;
>     comps->data = NULL;
> 
>+    /* Wait until we've figured out the cipher suite before we initialize the handshake hashes */
>+    rv = ssl3_InitHandshakeHashes(ss);
>+    if (rv != SECSuccess) {
>+        desc = internal_error;
>+        errCode = PORT_GetError();
>+        goto alert_loser;
>+    }
>+
>     /* If there are any failures while processing the old sid,
>      * we don't consider them to be errors.  Instead, We just behave
>      * as if the client had sent us no sid to begin with, and make a new one.
>      * The exception here is attempts to resume extended_master_secret
>      * sessions without the extension, which causes an alert.
>      */
>     if (sid != NULL)
>         do {
>@@ -9333,23 +9393,16 @@ ssl3_HandleV2ClientHello(sslSocket *ss, 
>         /* send back which ever alert client will understand. */
>         desc = (version > SSL_LIBRARY_VERSION_3_0) ? protocol_version
>                                                    : handshake_failure;
>         errCode = SSL_ERROR_UNSUPPORTED_VERSION;
>         goto alert_loser;
>     }
>     ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_version;
> 
>-    rv = ssl3_InitHandshakeHashes(ss);
>-    if (rv != SECSuccess) {
>-        desc = internal_error;
>-        errCode = PORT_GetError();
>-        goto alert_loser;
>-    }
>-
>     /* if we get a non-zero SID, just ignore it. */
>     if (length != total) {
>         SSL_DBG(("%d: SSL3[%d]: bad v2 client hello message, len=%d should=%d",
>                  SSL_GETPID(), ss->fd, length, total));
>         desc = illegal_parameter;
>         errCode = SSL_ERROR_RX_MALFORMED_CLIENT_HELLO;
>         goto alert_loser;
>     }
>@@ -9444,16 +9497,24 @@ suite_found:
>         !ssl3_ExtensionNegotiated(ss, ssl_renegotiation_info_xtn)) {
>         desc = handshake_failure;
>         errCode = SSL_ERROR_UNSAFE_NEGOTIATION;
>         goto alert_loser;
>     }
> 
>     ss->ssl3.hs.compression = ssl_compression_null;
> 
>+    /* Wait until we've figured out the cipher suite before we initialize the handshake hashes */
>+    rv = ssl3_InitHandshakeHashes(ss);
>+    if (rv != SECSuccess) {
>+        desc = internal_error;
>+        errCode = PORT_GetError();
>+        goto alert_loser;
>+    }
>+
>     /* we don't even search for a cache hit here.  It's just a miss. */
>     SSL_AtomicIncrementLong(&ssl3stats.hch_sid_cache_misses);
>     sid = ssl3_NewSessionID(ss, PR_TRUE);
>     if (sid == NULL) {
>         errCode = PORT_GetError();
>         goto loser; /* memory error is set. */
>     }
>     ss->sec.ci.sid = sid;
>@@ -9946,17 +10007,17 @@ ssl3_EncodeCertificateRequestSigAlgs(ssl
>     }
> 
>     *len = 0;
>     for (i = 0; i < ss->ssl3.signatureAlgorithmCount; ++i) {
>         const SSLSignatureAndHashAlg *alg = &ss->ssl3.signatureAlgorithms[i];
>         /* Note that we don't support a handshake hash with anything other than
>          * SHA-256, so asking for a signature from clients for something else
>          * would be inviting disaster. */
>-        if (alg->hashAlg == ssl_hash_sha256) {
>+        if (alg->hashAlg == ssl_hash_sha256 || alg->hashAlg == ssl_hash_sha384) {
>             buf[(*len)++] = (PRUint8)alg->hashAlg;
>             buf[(*len)++] = (PRUint8)alg->sigAlg;
>         }
>     }
> 
>     if (*len == 0) {
>         PORT_SetError(SSL_ERROR_NO_SUPPORTED_SIGNATURE_ALGORITHM);
>         return SECFailure;
>@@ -10017,17 +10078,18 @@ ssl3_SendCertificateRequest(sslSocket *s
>     isTLS12 = (PRBool)(ss->ssl3.pwSpec->version >= SSL_LIBRARY_VERSION_TLS_1_2);
> 
>     ssl3_GetCertificateRequestCAs(ss, &calen, &names, &nnames);
>     certTypes = certificate_types;
>     certTypesLength = sizeof certificate_types;
> 
>     length = 1 + certTypesLength + 2 + calen;
>     if (isTLS12) {
>-        rv = ssl3_EncodeCertificateRequestSigAlgs(ss, sigAlgs, sizeof(sigAlgs),
>+        rv = ssl3_EncodeCertificateRequestSigAlgs(ss,
>+                                                  sigAlgs, sizeof(sigAlgs),
>                                                   &sigAlgsLength);
>         if (rv != SECSuccess) {
>             return rv;
>         }
>         length += 2 + sigAlgsLength;
>     }
> 
>     rv = ssl3_AppendHandshakeHeader(ss, certificate_request, length);
>@@ -11501,17 +11563,17 @@ ssl3_AuthCertificateComplete(sslSocket *
> done:
>     ssl_ReleaseSSL3HandshakeLock(ss);
>     ssl_ReleaseRecvBufLock(ss);
> 
>     return rv;
> }
> 
> static SECStatus
>-ssl3_ComputeTLSFinished(ssl3CipherSpec *spec,
>+ssl3_ComputeTLSFinished(sslSocket *ss, ssl3CipherSpec *spec,
>                         PRBool isServer,
>                         const SSL3Hashes *hashes,
>                         TLSFinished *tlsFinished)
> {
>     SECStatus rv;
>     CK_TLS_MAC_PARAMS tls_mac_params;
>     SECItem param = { siBuffer, NULL, 0 };
>     PK11Context *prf_context;
>@@ -11524,17 +11586,17 @@ ssl3_ComputeTLSFinished(ssl3CipherSpec *
>         return ssl3_TLSPRFWithMasterSecret(spec, label, len, hashes->u.raw,
>                                            hashes->len, tlsFinished->verify_data,
>                                            sizeof tlsFinished->verify_data);
>     }
> 
>     if (spec->version < SSL_LIBRARY_VERSION_TLS_1_2) {
>         tls_mac_params.prfMechanism = CKM_TLS_PRF;
>     } else {
>-        tls_mac_params.prfMechanism = CKM_SHA256;
>+        tls_mac_params.prfMechanism = ssl3_GetPrfHashMechanism(ss);
>     }
>     tls_mac_params.ulMacLength = 12;
>     tls_mac_params.ulServerOrClient = isServer ? 1 : 2;
>     param.data = (unsigned char *)&tls_mac_params;
>     param.len = sizeof(tls_mac_params);
>     prf_context = PK11_CreateContextBySymKey(CKM_TLS_MAC, CKA_SIGN,
>                                              spec->master_secret, &param);
>     if (!prf_context)
>@@ -11727,17 +11789,17 @@ ssl3_SendFinished(sslSocket *ss, PRInt32
>     PORT_Assert(ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
>     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
> 
>     ssl_GetSpecReadLock(ss);
>     cwSpec = ss->ssl3.cwSpec;
>     isTLS = (PRBool)(cwSpec->version > SSL_LIBRARY_VERSION_3_0);
>     rv = ssl3_ComputeHandshakeHashes(ss, cwSpec, &hashes, sender);
>     if (isTLS && rv == SECSuccess) {
>-        rv = ssl3_ComputeTLSFinished(cwSpec, isServer, &hashes, &tlsFinished);
>+        rv = ssl3_ComputeTLSFinished(ss, cwSpec, isServer, &hashes, &tlsFinished);
>     }
>     ssl_ReleaseSpecReadLock(ss);
>     if (rv != SECSuccess) {
>         goto fail; /* err code was set by ssl3_ComputeHandshakeHashes */
>     }
> 
>     if (isTLS) {
>         if (isServer)
>@@ -11898,17 +11960,17 @@ ssl3_HandleFinished(sslSocket *ss, SSL3O
>     if (isTLS) {
>         TLSFinished tlsFinished;
> 
>         if (length != sizeof tlsFinished) {
>             (void)SSL3_SendAlert(ss, alert_fatal, decode_error);
>             PORT_SetError(SSL_ERROR_RX_MALFORMED_FINISHED);
>             return SECFailure;
>         }
>-        rv = ssl3_ComputeTLSFinished(ss->ssl3.crSpec, !isServer,
>+        rv = ssl3_ComputeTLSFinished(ss, ss->ssl3.crSpec, !isServer,
>                                      hashes, &tlsFinished);
>         if (!isServer)
>             ss->ssl3.hs.finishedMsgs.tFinished[1] = tlsFinished;
>         else
>             ss->ssl3.hs.finishedMsgs.tFinished[0] = tlsFinished;
>         ss->ssl3.hs.finishedBytes = sizeof tlsFinished;
>         if (rv != SECSuccess ||
>             0 != NSS_SecureMemcmp(&tlsFinished, b, length)) {
>diff --git a/lib/ssl/ssl3ecc.c b/lib/ssl/ssl3ecc.c
>--- a/lib/ssl/ssl3ecc.c
>+++ b/lib/ssl/ssl3ecc.c
>@@ -1061,50 +1061,58 @@ static const ssl3CipherSuite ecdh_rsa_su
>     0 /* end of list marker */
> };
> 
> static const ssl3CipherSuite ecdhe_ecdsa_suites[] = {
>     TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA,
>     TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
>     TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,
>     TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
>+    TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
>     TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
>+    TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384,
>     TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,
>     TLS_ECDHE_ECDSA_WITH_NULL_SHA,
>     TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,
>     0 /* end of list marker */
> };
> 
> static const ssl3CipherSuite ecdhe_rsa_suites[] = {
>     TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
>     TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
>     TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,
>     TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
>+    TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
>     TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
>+    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,
>     TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
>     TLS_ECDHE_RSA_WITH_NULL_SHA,
>     TLS_ECDHE_RSA_WITH_RC4_128_SHA,
>     0 /* end of list marker */
> };
> 
> /* List of all ECC cipher suites */
> static const ssl3CipherSuite ecSuites[] = {
>     TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA,
>     TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
>     TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,
>     TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
>     TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
>     TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,
>     TLS_ECDHE_ECDSA_WITH_NULL_SHA,
>     TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,
>+    TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384,
>+    TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
>     TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
>     TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
>     TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,
>     TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
>+    TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
>     TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
>+    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,
>     TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
>     TLS_ECDHE_RSA_WITH_NULL_SHA,
>     TLS_ECDHE_RSA_WITH_RC4_128_SHA,
>     TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA,
>     TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA,
>     TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA,
>     TLS_ECDH_ECDSA_WITH_NULL_SHA,
>     TLS_ECDH_ECDSA_WITH_RC4_128_SHA,
>diff --git a/lib/ssl/ssl3prot.h b/lib/ssl/ssl3prot.h
>--- a/lib/ssl/ssl3prot.h
>+++ b/lib/ssl/ssl3prot.h
>@@ -217,16 +217,42 @@ typedef struct {
> 
> typedef struct {
>     union {
>         SSL3ServerDHParams dh;
>         SSL3ServerRSAParams rsa;
>     } u;
> } SSL3ServerParams;
> 
>+/* This enum reflects HashAlgorithm enum from
>+ * https://tools.ietf.org/html/rfc5246#section-7.4.1.4.1
>+ *
>+ * When updating, be sure to also update ssl3_TLSHashAlgorithmToOID. */
>+typedef enum {
>+    tls_hash_md5 = 1,
>+    tls_hash_sha1 = 2,
>+    tls_hash_sha224 = 3,
>+    tls_hash_sha256 = 4,
>+    tls_hash_sha384 = 5,
>+    tls_hash_sha512 = 6
>+} TLSHashAlgorithm;
>+
>+/* This enum reflects SignatureAlgorithm enum from
>+ * https://tools.ietf.org/html/rfc5246#section-7.4.1.4.1 */
>+typedef enum {
>+    tls_sig_rsa = 1,
>+    tls_sig_dsa = 2,
>+    tls_sig_ecdsa = 3
>+} TLSSignatureAlgorithm;
>+
>+typedef struct {
>+    SECOidTag hashAlg;
>+    TLSSignatureAlgorithm sigAlg;
>+} SSL3SignatureAndHashAlgorithm;
>+
> /* SSL3HashesIndividually contains a combination MD5/SHA1 hash, as used in TLS
>  * prior to 1.2. */
> typedef struct {
>     PRUint8 md5[16];
>     PRUint8 sha[20];
> } SSL3HashesIndividually;
> 
> /* SSL3Hashes contains an SSL hash value. The digest is contained in |u.raw|
>diff --git a/lib/ssl/sslenum.c b/lib/ssl/sslenum.c
>--- a/lib/ssl/sslenum.c
>+++ b/lib/ssl/sslenum.c
>@@ -50,17 +50,21 @@
>  */
> const PRUint16 SSL_ImplementedCiphers[] = {
>     /* ECDHE-PSK from [draft-mattsson-tls-ecdhe-psk-aead]. */
>     TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256,
> 
> 
> #ifndef NSS_DISABLE_ECC
>     TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
>+    TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
>     TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
>+    TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
>+    TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384,
>+    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,
>     TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,
>     TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
>     /* TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA must appear before
>      * TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA to work around bug 946147.
>      */
>     TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
>     TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
>     TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
>@@ -77,16 +81,18 @@ const PRUint16 SSL_ImplementedCiphers[] 
>     TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
>     TLS_DHE_DSS_WITH_AES_128_GCM_SHA256,
>     TLS_DHE_RSA_WITH_AES_128_CBC_SHA,
>     TLS_DHE_DSS_WITH_AES_128_CBC_SHA,
>     TLS_DHE_RSA_WITH_AES_128_CBC_SHA256,
>     TLS_DHE_DSS_WITH_AES_128_CBC_SHA256,
>     TLS_DHE_RSA_WITH_CAMELLIA_128_CBC_SHA,
>     TLS_DHE_DSS_WITH_CAMELLIA_128_CBC_SHA,
>+    TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,
>+    TLS_DHE_DSS_WITH_AES_256_GCM_SHA384,
>     TLS_DHE_RSA_WITH_AES_256_CBC_SHA,
>     TLS_DHE_DSS_WITH_AES_256_CBC_SHA,
>     TLS_DHE_RSA_WITH_AES_256_CBC_SHA256,
>     TLS_DHE_DSS_WITH_AES_256_CBC_SHA256,
>     TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA,
>     TLS_DHE_DSS_WITH_CAMELLIA_256_CBC_SHA,
>     TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA,
>     TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA,
>@@ -98,16 +104,17 @@ const PRUint16 SSL_ImplementedCiphers[] 
>     TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA,
>     TLS_ECDH_RSA_WITH_AES_256_CBC_SHA,
>     TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA,
>     TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA,
>     TLS_ECDH_ECDSA_WITH_RC4_128_SHA,
>     TLS_ECDH_RSA_WITH_RC4_128_SHA,
> #endif /* NSS_DISABLE_ECC */
> 
>+    TLS_RSA_WITH_AES_256_GCM_SHA384,
>     TLS_RSA_WITH_AES_128_GCM_SHA256,
>     TLS_RSA_WITH_AES_128_CBC_SHA,
>     TLS_RSA_WITH_AES_128_CBC_SHA256,
>     TLS_RSA_WITH_CAMELLIA_128_CBC_SHA,
>     TLS_RSA_WITH_AES_256_CBC_SHA,
>     TLS_RSA_WITH_AES_256_CBC_SHA256,
>     TLS_RSA_WITH_CAMELLIA_256_CBC_SHA,
>     TLS_RSA_WITH_SEED_CBC_SHA,
>diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
>--- a/lib/ssl/sslimpl.h
>+++ b/lib/ssl/sslimpl.h
>@@ -53,16 +53,17 @@ typedef SSLMACAlgorithm SSL3MACAlgorithm
> #define calg_chacha20 ssl_calg_chacha20
> 
> #define mac_null ssl_mac_null
> #define mac_md5 ssl_mac_md5
> #define mac_sha ssl_mac_sha
> #define hmac_md5 ssl_hmac_md5
> #define hmac_sha ssl_hmac_sha
> #define hmac_sha256 ssl_hmac_sha256
>+#define hmac_sha384 ssl_hmac_sha384
> #define mac_aead ssl_mac_aead
> 
> #define SET_ERROR_CODE    /* reminder */
> #define SEND_ALERT        /* reminder */
> #define TEST_FOR_FAILURE  /* reminder */
> #define DEAL_WITH_FAILURE /* reminder */
> 
> #if defined(DEBUG) || defined(TRACE)
>@@ -271,19 +272,19 @@ typedef struct {
>     ssl3CipherSuite cipher_suite;
>     PRUint8 policy;
>     unsigned char enabled : 1;
>     unsigned char isPresent : 1;
> #endif
> } ssl3CipherSuiteCfg;
> 
> #ifndef NSS_DISABLE_ECC
>-#define ssl_V3_SUITES_IMPLEMENTED 68
>+#define ssl_V3_SUITES_IMPLEMENTED 75
> #else
>-#define ssl_V3_SUITES_IMPLEMENTED 41
>+#define ssl_V3_SUITES_IMPLEMENTED 44
> #endif /* NSS_DISABLE_ECC */
> 
> #define MAX_DTLS_SRTP_CIPHER_SUITES 4
> 
> /* MAX_SIGNATURE_ALGORITHMS allows for a large number of combinations of
>  * SSLSignType and SSLHashType, but not all combinations (specifically, this
>  * doesn't allow space for combinations with MD5). */
> #define MAX_SIGNATURE_ALGORITHMS 15
>@@ -435,21 +436,29 @@ typedef enum {
>     cipher_des40,
>     cipher_idea,
>     cipher_aes_128,
>     cipher_aes_256,
>     cipher_camellia_128,
>     cipher_camellia_256,
>     cipher_seed,
>     cipher_aes_128_gcm,
>+    cipher_aes_256_gcm,
>     cipher_chacha20,
>     cipher_missing /* reserved for no such supported cipher */
>     /* This enum must match ssl3_cipherName[] in ssl3con.c.  */
> } SSL3BulkCipher;
> 
>+/* The TLS PRF definition */
>+typedef enum {
>+    prf_null = 0, /* use default prf */
>+    prf_256 = CKM_SHA256,
>+    prf_384 = CKM_SHA384
>+} SSL3PRF;
>+
> typedef enum { type_stream,
>                type_block,
>                type_aead } CipherType;
> 
> #define MAX_IV_LENGTH 24
> 
> /*
>  * Do not depend upon 64 bit arithmetic in the underlying machine.
>@@ -685,16 +694,17 @@ struct sslSessionIDStr {
>     } u;
> };
> 
> typedef struct ssl3CipherSuiteDefStr {
>     ssl3CipherSuite cipher_suite;
>     SSL3BulkCipher bulk_cipher_alg;
>     SSL3MACAlgorithm mac_alg;
>     SSL3KeyExchangeAlgorithm key_exchange_alg;
>+    SSL3PRF prf_alg;
> } ssl3CipherSuiteDef;
> 
> /*
> ** There are tables of these, all const.
> */
> typedef struct {
>     SSL3KeyExchangeAlgorithm kea;
>     SSL3KEAType exchKeyType;
>@@ -1913,16 +1923,17 @@ SECStatus ssl3_SetupPendingCipherSpec(ss
> SECStatus ssl3_FlushHandshake(sslSocket *ss, PRInt32 flags);
> SECStatus ssl3_SendCertificate(sslSocket *ss);
> SECStatus ssl3_CompleteHandleCertificate(sslSocket *ss,
>                                          SSL3Opaque *b, PRUint32 length);
> SECStatus ssl3_SendEmptyCertificate(sslSocket *ss);
> SECStatus ssl3_SendCertificateStatus(sslSocket *ss);
> SECStatus ssl3_CompleteHandleCertificateStatus(sslSocket *ss, SSL3Opaque *b,
>                                                PRUint32 length);
>+CK_MECHANISM_TYPE ssl3_GetPrfHashMechanism(sslSocket *ss);
> SECStatus ssl3_EncodeCertificateRequestSigAlgs(sslSocket *ss, PRUint8 *buf,
>                                                unsigned maxLen, PRUint32 *len);
> void ssl3_GetCertificateRequestCAs(sslSocket *ss, int *calenp, SECItem **namesp,
>                                    int *nnamesp);
> SECStatus ssl3_ParseCertificateRequestCAs(sslSocket *ss, SSL3Opaque **b,
>                                           PRUint32 *length, PLArenaPool *arena,
>                                           CERTDistNames *ca_list);
> SECStatus ssl3_CompleteHandleCertificateRequest(sslSocket *ss,
>diff --git a/lib/ssl/sslproto.h b/lib/ssl/sslproto.h
>--- a/lib/ssl/sslproto.h
>+++ b/lib/ssl/sslproto.h
>@@ -159,18 +159,21 @@
> #define TLS_DH_RSA_WITH_CAMELLIA_256_CBC_SHA    0x0086
> #define TLS_DHE_DSS_WITH_CAMELLIA_256_CBC_SHA   0x0087
> #define TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA   0x0088
> #define TLS_DH_anon_WITH_CAMELLIA_256_CBC_SHA   0x0089
> 
> #define TLS_RSA_WITH_SEED_CBC_SHA               0x0096
> 
> #define TLS_RSA_WITH_AES_128_GCM_SHA256         0x009C
>+#define TLS_RSA_WITH_AES_256_GCM_SHA384         0x009D
> #define TLS_DHE_RSA_WITH_AES_128_GCM_SHA256     0x009E
>+#define TLS_DHE_RSA_WITH_AES_256_GCM_SHA384     0x009F
> #define TLS_DHE_DSS_WITH_AES_128_GCM_SHA256     0x00A2
>+#define TLS_DHE_DSS_WITH_AES_256_GCM_SHA384     0x00A3
> 
> /* TLS "Signaling Cipher Suite Value" (SCSV). May be requested by client.
>  * Must NEVER be chosen by server.  SSL 3.0 server acknowledges by sending
>  * back an empty Renegotiation Info (RI) server hello extension.
>  */
> #define TLS_EMPTY_RENEGOTIATION_INFO_SCSV       0x00FF
> 
> /* TLS_FALLBACK_SCSV is a signaling cipher suite value that indicates that a
>@@ -207,21 +210,25 @@
> 
> #define TLS_ECDH_anon_WITH_NULL_SHA             0xC015
> #define TLS_ECDH_anon_WITH_RC4_128_SHA          0xC016
> #define TLS_ECDH_anon_WITH_3DES_EDE_CBC_SHA     0xC017
> #define TLS_ECDH_anon_WITH_AES_128_CBC_SHA      0xC018
> #define TLS_ECDH_anon_WITH_AES_256_CBC_SHA      0xC019
> 
> #define TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256 0xC023
>+#define TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 0xC024
> #define TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256   0xC027
>+#define TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384   0xC028
> 
> #define TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 0xC02B
>+#define TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 0xC02C
> #define TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256  0xC02D
> #define TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256   0xC02F
>+#define TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384   0xC030
> #define TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256    0xC031
> 
> #define TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256   0xCCA8
> #define TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 0xCCA9
> #define TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256     0xCCAA
> 
> /* Experimental PSK support for [draft-mattsson-tls-ecdhe-psk-aead] */
> #define TLS_ECDHE_PSK_WITH_AES_128_GCM_SHA256   0xD001
>diff --git a/lib/ssl/sslt.h b/lib/ssl/sslt.h
>--- a/lib/ssl/sslt.h
>+++ b/lib/ssl/sslt.h
>@@ -111,17 +111,18 @@ typedef enum {
> 
> typedef enum {
>     ssl_mac_null = 0,
>     ssl_mac_md5 = 1,
>     ssl_mac_sha = 2,
>     ssl_hmac_md5 = 3, /* TLS HMAC version of mac_md5 */
>     ssl_hmac_sha = 4, /* TLS HMAC version of mac_sha */
>     ssl_hmac_sha256 = 5,
>-    ssl_mac_aead = 6
>+    ssl_mac_aead = 6,
>+    ssl_hmac_sha384 = 7
> } SSLMACAlgorithm;
> 
> typedef enum {
>     ssl_compression_null = 0,
>     ssl_compression_deflate = 1 /* RFC 3749 */
> } SSLCompressionMethod;
> 
> typedef struct SSLChannelInfoStr {

Comment 64

[Elio Maldonado]

I wasn't expecting the just obsoleted patch too be shown here, I obsoleted it because of Comment 62.

Comment 65

[Elio Maldonado]

Attachment: sha384 prf support in libsssl + plus tests changes

Not ready to be checked in as it isn't working yet. I do attach for review and to get some help identifying the cause of the failures.

Comment 66

[Elio Maldonado]

Here is an excerpt from the test output log with some of the failures:
------------------------------------------------------------------------------------------------

selfserv: normal termination
selfserv_9270 -b -p 9270 2>/dev/null;
selfserv_9270 with PID 11344 killed at Thu Apr 21 13:11:12 PDT 2016
ssl.sh: SSL3 Require client auth on 2nd hs (EC) (client auth) ----
selfserv_9270 starting at Thu Apr 21 13:11:12 PDT 2016
selfserv_9270 -D -p 9270 -d ../server -n localhost.localdomain  \
         -e localhost.localdomain-ec -S localhost.localdomain-dsa -w nss -r -r -r -r -i ../tests_pid.25017\
          -H 1 &
trying to connect to selfserv_9270 at Thu Apr 21 13:11:12 PDT 2016
tstclnt -p 9270 -h localhost.localdomain  -q \
        -d ../client -v < /home/emaldona/work4nss/upstream/one-sha384-test-enabled/nss/tests/ssl/sslreq.dat
tstclnt: connecting to localhost.localdomain:9270 (address=127.0.0.1)
kill -0 12766 >/dev/null 2>/dev/null
selfserv_9270 with PID 12766 found at Thu Apr 21 13:11:12 PDT 2016
selfserv_9270 with PID 12766 started at Thu Apr 21 13:11:12 PDT 2016
trying to kill selfserv_9270 with PID 12766 at Thu Apr 21 13:11:12 PDT 2016
kill -USR1 12766
./ssl.sh: line 175: 12766 User defined signal 1   ${PROFTOOL} ${BINDIR}/selfserv_9270 -D -p ${PORT} -d ${P_R_SERVERDIR} -n ${HOSTADDR} ${SERVER_OPTIONS} ${ECC_OPTIONS} -S ${HOSTADDR}-dsa -w nss ${sparam} -i ${R_SERVERPID} $verbose -H 1
selfserv_9270 -b -p 9270 2>/dev/null;
selfserv_9270 with PID 12766 killed at Thu Apr 21 13:11:12 PDT 2016
ssl.sh: SSL Cipher Coverage  - server bypass/client normal  - with ECC ===============================
selfserv_9270 starting at Thu Apr 21 13:11:12 PDT 2016
selfserv_9270 -D -p 9270 -d ../server -n localhost.localdomain -B -s \
         -e localhost.localdomain-ec -S localhost.localdomain-dsa -w nss -c :C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014:C023:C024:C027:C028:C02B:C02C:C02F:C030:CCA8:CCA9:CCAA:0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B:0084:009C:009E:00A2cdefgijklmnvyz -i ../tests_pid.25017\
          -H 1 &
trying to connect to selfserv_9270 at Thu Apr 21 13:11:12 PDT 2016
tstclnt -p 9270 -h localhost.localdomain  -q \
        -d ../client -v < /home/emaldona/work4nss/upstream/one-sha384-test-enabled/nss/tests/ssl/sslreq.dat
tstclnt: connecting to localhost.localdomain:9270 (address=127.0.0.1)
kill -0 12814 >/dev/null 2>/dev/null
selfserv_9270 with PID 12814 found at Thu Apr 21 13:11:12 PDT 2016
selfserv_9270 with PID 12814 started at Thu Apr 21 13:11:12 PDT 2016
ssl.sh: running TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 ----------------------------
kill -0 12814 >/dev/null 2>/dev/null
selfserv_9270 with PID 12814 found at Thu Apr 21 13:11:13 PDT 2016
tstclnt -p 9270 -h localhost.localdomain -c :009F -V ssl3:tls1.2  \
        -f -d ../client -v -w nss < /home/emaldona/work4nss/upstream/one-sha384-test-enabled/nss/tests/ssl/sslreq.dat
selfserv: HDX PR_Read returned error -12260:
SSL received a malformed Client Hello handshake message.
tstclnt: connecting to localhost.localdomain:9270 (address=127.0.0.1)
tstclnt: connect: PR_IN_PROGRESS_ERROR: Operation is still in progress (probably a non-blocking connect)
tstclnt: about to call PR_Poll for connect completion!
tstclnt: PR_Poll returned 0x02 for socket out_flags.
tstclnt: ready...
tstclnt: about to call PR_Poll !
tstclnt: PR_Poll returned!
tstclnt: PR_Poll returned 0x01 for stdin out_flags.
tstclnt: PR_Poll returned 0x00 for socket out_flags.
tstclnt: stdin read 18 bytes
tstclnt: Writing 18 bytes to server
tstclnt: write to SSL socket failed: SSL_ERROR_NO_CYPHER_OVERLAP: Cannot communicate securely with peer: no common encryption algorithm(s).
tstclnt: exiting with return code 254
ssl.sh: #1727: TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 produced a returncode of 254, expected is 0 - FAILED
ssl.sh: running TLS_DHE_DSS_WITH_AES_256_GCM_SHA384 ----------------------------
kill -0 12814 >/dev/null 2>/dev/null
selfserv_9270 with PID 12814 found at Thu Apr 21 13:11:13 PDT 2016
tstclnt -p 9270 -h localhost.localdomain -c :00A3 -V ssl3:tls1.2  \
        -f -d ../client -v -w nss < /home/emaldona/work4nss/upstream/one-sha384-test-enabled/nss/tests/ssl/sslreq.dat
selfserv: HDX PR_Read returned error -12260:
SSL received a malformed Client Hello handshake message.
tstclnt: connecting to localhost.localdomain:9270 (address=127.0.0.1)
tstclnt: connect: PR_IN_PROGRESS_ERROR: Operation is still in progress (probably a non-blocking connect)
tstclnt: about to call PR_Poll for connect completion!
tstclnt: PR_Poll returned 0x02 for socket out_flags.
tstclnt: ready...
tstclnt: about to call PR_Poll !
tstclnt: PR_Poll returned!
tstclnt: PR_Poll returned 0x01 for stdin out_flags.
tstclnt: PR_Poll returned 0x00 for socket out_flags.
tstclnt: stdin read 18 bytes
tstclnt: Writing 18 bytes to server
tstclnt: write to SSL socket failed: SSL_ERROR_NO_CYPHER_OVERLAP: Cannot communicate securely with peer: no common encryption algorithm(s).
tstclnt: exiting with return code 254
ssl.sh: #1728: TLS_DHE_DSS_WITH_AES_256_GCM_SHA384 produced a returncode of 254, expected is 0 - FAILED
ssl.sh: running TLS_RSA_WITH_AES_256_GCM_SHA384 ----------------------------
kill -0 12814 >/dev/null 2>/dev/null
selfserv_9270 with PID 12814 found at Thu Apr 21 13:11:13 PDT 2016
tstclnt -p 9270 -h localhost.localdomain -c :009D -V ssl3:tls1.2  \
        -f -d ../client -v -w nss < /home/emaldona/work4nss/upstream/one-sha384-test-enabled/nss/tests/ssl/sslreq.dat
selfserv: HDX PR_Read returned error -12260:
SSL received a malformed Client Hello handshake message.
tstclnt: connecting to localhost.localdomain:9270 (address=127.0.0.1)
tstclnt: connect: PR_IN_PROGRESS_ERROR: Operation is still in progress (probably a non-blocking connect)
tstclnt: about to call PR_Poll for connect completion!
tstclnt: PR_Poll returned 0x02 for socket out_flags.
tstclnt: ready...
tstclnt: about to call PR_Poll !
tstclnt: PR_Poll returned!
tstclnt: PR_Poll returned 0x01 for stdin out_flags.
tstclnt: PR_Poll returned 0x00 for socket out_flags.
tstclnt: stdin read 18 bytes
tstclnt: Writing 18 bytes to server
tstclnt: write to SSL socket failed: SSL_ERROR_NO_CYPHER_OVERLAP: Cannot communicate securely with peer: no common encryption algorithm(s).
tstclnt: exiting with return code 254
ssl.sh: #1729: TLS_RSA_WITH_AES_256_GCM_SHA384 produced a returncode of 254, expected is 0 - FAILED
ssl.sh: running TLS12_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 ----------------------------
kill -0 12814 >/dev/null 2>/dev/null
selfserv_9270 with PID 12814 found at Thu Apr 21 13:11:13 PDT 2016
tstclnt -p 9270 -h localhost.localdomain -c :C024 -V ssl3:tls1.2  \
        -f -d ../client -v -w nss < /home/emaldona/work4nss/upstream/one-sha384-test-enabled/nss/tests/ssl/sslreq.dat
./ssl.sh: line 260: 13688 Aborted                 (core dumped) ${PROFTOOL} ${BINDIR}/tstclnt -p ${PORT} -h ${HOSTADDR} -c ${param} -V ${VMIN}:${VMAX} ${CLIENT_OPTIONS} -f -d ${P_R_CLIENTDIR} -v -w nss < ${REQUEST_FILE} > ${TMP}/$HOST.tmp.$$ 2>&1
tstclnt: connecting to localhost.localdomain:9270 (address=127.0.0.1)
tstclnt: connect: PR_IN_PROGRESS_ERROR: Operation is still in progress (probably a non-blocking connect)
tstclnt: about to call PR_Poll for connect completion!
tstclnt: PR_Poll returned 0x02 for socket out_flags.
tstclnt: ready...
tstclnt: about to call PR_Poll !
tstclnt: PR_Poll returned!
tstclnt: PR_Poll returned 0x01 for stdin out_flags.
tstclnt: PR_Poll returned 0x00 for socket out_flags.
tstclnt: stdin read 18 bytes
tstclnt: Writing 18 bytes to server
tstclnt: about to call PR_Poll on writable socket !
tstclnt: PR_Poll returned with writable socket !
tstclnt: using asynchronous certificate validation
ERROR: bock_need=160 > key_block=1052988032
 Assertion failure: block_needed <= sizeof key_block, at pkcs11c.c:6539
ssl.sh: #1730: TLS12_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 produced a returncode of 134, expected is 0 - FAILED
ssl.sh: running TLS12_ECDHE_RSA_WITH_AES_256_CBC_SHA384 ----------------------------
kill -0 12814 >/dev/null 2>/dev/null
selfserv_9270 with PID 12814 found at Thu Apr 21 13:11:13 PDT 2016
tstclnt -p 9270 -h localhost.localdomain -c :C028 -V ssl3:tls1.2  \
        -f -d ../client -v -w nss < /home/emaldona/work4nss/upstream/one-sha384-test-enabled/nss/tests/ssl/sslreq.dat
tstclnt: connecting to localhost.localdomain:9270 (address=127.0.0.1)
tstclnt: connect: PR_IN_PROGRESS_ERROR: Operation is still in progress (probably a non-blocking connect)
tstclnt: about to call PR_Poll for connect completion!
tstclnt: PR_Poll returned 0x02 for socket out_flags.
tstclnt: ready...
tstclnt: about to call PR_Poll !
tstclnt: PR_Poll returned!
tstclnt: PR_Poll returned 0x01 for stdin out_flags.
tstclnt: PR_Poll returned 0x00 for socket out_flags.
tstclnt: stdin read 18 bytes
tstclnt: Writing 18 bytes to server
tstclnt: about to call PR_Poll on writable socket !
tstclnt: PR_Poll returned with writable socket !
tstclnt: write to SSL socket failed: PR_END_OF_FILE_ERROR: Encountered end of file
tstclnt: exiting with return code 254
./ssl.sh: line 260: 12814 Segmentation fault      (core dumped) ${PROFTOOL} ${BINDIR}/selfserv_9270 -D -p ${PORT} -d ${P_R_SERVERDIR} -n ${HOSTADDR} ${SERVER_OPTIONS} ${ECC_OPTIONS} -S ${HOSTADDR}-dsa -w nss ${sparam} -i ${R_SERVERPID} $verbose -H 1
ssl.sh: #1731: TLS12_ECDHE_RSA_WITH_AES_256_CBC_SHA384 produced a returncode of 254, expected is 0 - FAILED
ssl.sh: running TLS12_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 ----------------------------
kill -0 12814 >/dev/null 2>/dev/null
ssl.sh: Exit: 10 Fatal - selfserv_9270 process not detectable - FAILED
ssl.sh: #1732: 10 Fatal - selfserv_9270 process not detectable - FAILED
./init.sh: line 155: kill: (12814) - No such process
------------------------------------------------------------

Comment 67

[Elio Maldonado]

To nss/tests/ssl/sslcov.txt I have added these tests:
   ECC   TLS12  :C024 TLS12_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384
   ECC   TLS12  :C028 TLS12_ECDHE_RSA_WITH_AES_256_CBC_SHA384
   ECC   TLS12  :C02C TLS12_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
   ECC   TLS12  :C030 TLS12_ECDHE_RSA_WITH_AES_256_GCM_SHA384

  noECC  TLS12 :009F  TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
  noECC  TLS12 :00A3  TLS_DHE_DSS_WITH_AES_256_GCM_SHA384
  noECC  TLS12 :009D  TLS_RSA_WITH_AES_256_GCM_SHA384

Looking at nss/tests/ssl/ssl.sh it seems to me that I should also add
:C024, :C028, :C02C, and :C030 in their proper places to CIPHER_SUITES
inside the 
  if [ -z "$NSS_DISABLE_ECC" ] ; then
      ECC_STRING=" - with ECC"
      # List of cipher suites to test, including ECC cipher suites.
      CIPHER_SUITES="-c .....
part and and add :009F, :00A3, and :009D in their proper places to CIPHER_SUITES for
  else
      ECC_STRING=""
      # List of cipher suites to test, excluding ECC cipher suites.
      CIPHER_SUITES="-c : .....

I see that :CCA8:CCA9: for TLS12_ECDHE_{RSA|ECDsa}_WITH_CHACHA20_POLY1305_SHA256 were added.

Martin, what do you think?

Comment 68

[Martin Thomson [:mt:]]

Yes, that looks correct.  I would look for one of the existing cipher suite definitions throughout the whole tree just to be doubly sure.

Comment 69

[Martin Thomson [:mt:]]

Attachment: tests.patch

This should apply on top of attachment 8744036 [details] [diff] [review] and let you run tests.  I usually run the script in tests/ssl_gtests then save the directory created, e.g.: ../tests_results/security/localhost.80/ssl_gtests

Then it's a matter of running the binary.  You can set a filter to make it easier.  Here's a snippet from the script that I use, you will need to tweak all the ~ directories.  ~/ssl_gtest is a saved copy of the certificate DB.

filter="$1"
v="$(uname -s)$(uname -r | cut -f 1-2 -d . -)_$(uname -m)_${CC:-cc}_glibc_PTH_64_$([ -n "$BUILD_OPT" ] && echo OPT || echo DBG).OBJ"
export LD_LIBRARY_PATH=~/code/dist/$v/lib:"$LD_LIBRARY_PATH"
exec ~/code/dist/$v/bin/ssl_gtest -d ~/ssl_gtests --gtest_filter="$filter"

Comment 70

[Masatoshi Kimura [:emk]]

Comment on attachment 8744036 [details] [diff] [review]
sha384 prf support in libsssl + plus tests changes

Review of attachment 8744036 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like comments #60 and #61 are ignored, So I'll add more explanations.

Comments above SSL_ImplementedCiphers should be updated.
Proposed text:

* No-encryption cipher suites last
* Export/weak/obsolete cipher suites before no-encryption cipher suites
* Order by key exchange algorithm: ECDHE, then DHE, then ECDH, RSA.
* Within key agreement sections, prefer AEAD cipher suites over non-AEAD
  cipher suites.
* Within AEAD sections, order by symmetric encryption algorithm which
  integrates message authentication algorithm: AES-128-GCM, then
  ChaCha20-Poly1305, then AES-256-GCM,
* Within non-AEAD sections, order by symmetric encryption algorithm:
  AES-128, then Camellia-128, then AES-256, then Camellia-256, then SEED,
  then FIPS-3DES, then 3DES, then RC4. AES is commonly accepted as a
  strong cipher internationally, and is often hardware-accelerated.
  Camellia also has wide international support across standards
  organizations. SEED is only recommended by the Korean government. 3DES
  only provides 112 bits of security. RC4 is now deprecated or forbidden
  by many standards organizations.
* Within non-AEAD symmetric algorithm sections, order by message
  authentication algorithm: HMAC-SHA1, then HMAC-SHA256, then
  HMAC-SHA384, then HMAC-MD5.
* Within message authentication algorithm sections, order by asymmetric
  signature algorithm: ECDSA, then RSA, then DSS.
* As a special case, the PSK ciphers, which are only enabled when
  TLS 1.3 PSK-resumption is in use, come first.

According to this order, some added cipher suites are out of order.

::: lib/ssl/ssl3con.c
@@ +109,5 @@
>   { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,   SSL_ALLOWED, PR_TRUE, PR_FALSE},

TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 and TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 should be placed after TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA.

@@ +128,5 @@
>  
>   { TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,     SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>   { TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256,SSL_ALLOWED,PR_TRUE,  PR_FALSE},
>   { TLS_DHE_DSS_WITH_AES_128_GCM_SHA256,     SSL_ALLOWED, PR_FALSE, PR_FALSE},
> + { TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,     SSL_ALLOWED, PR_TRUE,  PR_FALSE},

TLS_DHE_DSS_WITH_AES_256_GCM_SHA384 should be placed here (that is, after TLS_DHE_RSA_WITH_AES_256_GCM_SHA384).

Comment 71

[ggramaiz]

(In reply to Masatoshi Kimura [:emk] from comment #70)
> Comment on attachment 8744036 [details] [diff] [review]
> sha384 prf support in libsssl + plus tests changes
> 
> * Within non-AEAD symmetric algorithm sections, order by message
>   authentication algorithm: HMAC-SHA1, then HMAC-SHA256, then
>   HMAC-SHA384, then HMAC-MD5.

Not an expert here, but shouldn't we prefer HMAC-SHA256, then HMAC-SHA384, then HMAC-SHA1, then HMAC-MD5?

The French information security agency recommends the MAC to be ordered that way ( http://www.ssi.gouv.fr/uploads/IMG/pdf/SSL_TLS_etat_des_lieux_et_recommandations.pdf for reference).

I agree with you on the other points.

Comment 72

[Martin Thomson [:mt:]]

Attachment: progress.patch

I couldn't make much progress, but the tests are such that you can get going pretty quickly once you have the gtests setup.

This needs a fair bit of work to get things working.  I also encourage you to compile with NSS_ENABLE_TLS_1_3=1 before you are done.  There is a lot missing there.  I started, but I'm now out of time.

Comment 73

[Martin Thomson [:mt:]]

+1 to the ordering proposed by :emk with the modification :ggramaiz adds.

However, I think that review is a little premature.  This patch is quite a long way from working just yet.  Then we can add the finishing touches.

Comment 74

[Elio Maldonado]

Attachment: progressV2.patch

Thank you Martin for the attachments which are helping a lot. Five of the sha384 test are now passing.  This is a slight variant of your progress patch that I'm using.

The tests still failing are the ones for
ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 and 
ECDHE_RSA_WITH_AES_256_CBC_SHA384 

which fail with 
ERROR: bock_need=160 > key_block=915175232
 Assertion failure: block_needed <= sizeof key_block, at pkcs11c.c:6539
the key_block seems not getting initialized. See you in a while on IRC

Comment 75

[Masatoshi Kimura [:emk]]

IIRC the reason of the current HMAC order is that HMAC-SHA1 is not known to be vulnerable yet (unlike plain SHA-1) and HMAC-SHA2 is slow.

That said, I have no strong objection about the modified order. Firefox will not enable any HMAC-SHA2 cipher suites anyway.

Comment 76

[Elio Maldonado]

(In reply to Elio Maldonado from comment #74)
> Created attachment 8744456 [details] [diff] [review]
> The tests still failing are the ones for
> ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 and 
> ECDHE_RSA_WITH_AES_256_CBC_SHA384 
> 
> which fail with 
> ERROR: bock_need=160 > key_block=915175232
>  Assertion failure: block_needed <= sizeof key_block, at pkcs11c.c:6539
> the key_block seems not getting initialized. See you in a while on IRC

The problem seems to be that the key_block array is too small for the new SHA384 support, the size is NUM_MIXERS * MD5_LENGTH where MD5_LENGTH = 16. Downstream we use SFTK_MAX_MAC_LENGTH which is 64.

With this change
-    unsigned char   key_block[NUM_MIXERS * MD5_LENGTH];
+    unsigned char   key_block[NUM_MIXERS * SFTK_MAX_MAC_LENGTH];
Those tests are now passing. 

New patch coming as soon as I get more testing done. It will include the changes to the ordering proposed by :emk with the modification :ggramaiz adds to the comments.

Comment 77

[Elio Maldonado]

Attachment: sha384 prf support - full patch

This version, though not ready for check-in yet, is worth reviewing. 

Incorporated feedback received regarding cipher suites preference order plus  improvements and cleanup from Martin. Have included "for reviewer" comments on things that I am not sure about.

Things we aren't working yet: The tests with NSS_ENABLE_TLS_1_3=1 aren't passing but I tried those without this patch and they failed as well. The new ssl_gtests cipher suites unit tests that Martin is working on are not complete yet. I also saw a test failure in OS X and Windows 7 which I need to investigate, need to rule any possible miss-configuration on my systems.

Comment 78

[Martin Thomson [:mt:]]

Comment on attachment 8744737 [details] [diff] [review]
sha384 prf support - full patch

Review of attachment 8744737 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking pretty good now.  Most of what I have here is polish.  I'd like to see the changes to the tests in bug 1266633 as well; maybe I can land that one ahead of you.

::: lib/softoken/pkcs11c.c
@@ +6534,5 @@
> +    if (block_needed > sizeof key_block) {
> +        PR_fprintf(PR_STDERR, "ERROR: block_need=%d > key_block=%d\n ",
> +                   block_needed, key_block);
> +    }
> +#endif

You should turn this into a PORT_Assert call.

::: lib/ssl/ssl3con.c
@@ +118,5 @@
>   { TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, SSL_ALLOWED, PR_TRUE, PR_FALSE},
>   { TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
>   { TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,      SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,   SSL_ALLOWED, PR_TRUE, PR_FALSE},

Hmm, ordering here is tricky.  My inclination is to put the SHA384 variants about the SHA1 variants, but that requires more changes to the existing cipher suites.  Maybe:
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA

That would need discussion on nss-dev though.

One unfortunately consequence of the existing order is that we prefer SHA1 suites over the newer SHA256 suites added in TLS 1.2.  I think that's wrong.

@@ +4367,5 @@
> +
> +            hm = ssl3_GetPrfHashMechanism(ss);
> +            hashOid = SECOID_FindOIDByMechanism(hm);
> +
> +            if (hashOid == NULL) {

No PORT_Assert here too?

@@ +4850,5 @@
>      return SECSuccess;
>  }
>  
>  /* tlsHashOIDMap contains the mapping between TLS hash identifiers and the
> + * SECOidTag used internally by NSS. 

trailing space

@@ +4893,5 @@
> + * identifier. If the hash is not recognised, zero is returned.
> + *
> + * See https://tools.ietf.org/html/rfc5246#section-7.4.1.4.1 */
> +static int
> +ssl3_OIDToTLSHashAlgorithm(SECOidTag oid)

I remember removing this.  Why is it needed?

@@ +5224,5 @@
> +            ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
> +            rv = SECFailure;
> +            goto tls12_loser;
> +        }
> +        hashes->hashAlg = ssl3_OIDToTLSHashAlgorithm(hashOid->offset);

You have the hash function already on line 5201.  See above comment.

@@ +5232,5 @@
> +            hashes->hashAlg != ssl_hash_sha384) {
> +            ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
> +            rv = SECFailure;
> +            goto tls12_loser;
> +        }

These checks can't fail, so they can go.

@@ +7722,5 @@
>      if (rv != SECSuccess) {
>          goto done;
>      }
>  
> +    hashOid = SECOID_FindOIDByMechanism(ssl3_GetPrfHashMechanism(ss));

Just add a switch statement based on ss->ssl3.hs.suite_def->prf_alg.  You don't need hashOid that way.

You could use ssl3_GetSuiteHashAlg(), except that you want prf_null to turn into ssl_hash_none/null.

@@ +7733,5 @@
> +        suitePRFHash = ssl_hash_sha256;
> +        suitePRFIs256Or384 = PR_TRUE;
> +    } else if (hashOid->offset == SEC_OID_SHA384) {
> +        suitePRFHash = ssl_hash_sha384;
> +        suitePRFIs256Or384 = PR_TRUE;

As above, you don't actually need this block, or suitePRFIs256or384.

@@ +10300,5 @@
>      certTypesLength = sizeof certificate_types;
>  
>      length = 1 + certTypesLength + 2 + calen;
> +
> +    PORT_Assert(suiteHashAlg != ssl_hash_none);

Probably not needed.

::: lib/ssl/sslimpl.h
@@ +474,5 @@
>      cipher_missing /* reserved for no such supported cipher */
>      /* This enum must match ssl3_cipherName[] in ssl3con.c.  */
>  } SSL3BulkCipher;
>  
> +/* The TLS PRF definition */

This needs more explanation, particularly around the null value.  That null value is generally used for suites that were designed before TLS 1.2.  You need to explain that null means 256 when in TLS 1.2.

::: lib/ssl/sslinfo.c
@@ +297,5 @@
> +    { 0, CS(ECDHE_RSA_WITH_AES_256_CBC_SHA384),  S_RSA, K_ECDHE, C_AES, B_256, M_SHA384, F_FIPS_STD, A_RSAS },
> +    { 0, CS(ECDHE_ECDSA_WITH_AES_256_CBC_SHA384), S_ECDSA, K_ECDHE, C_AES, B_256, M_SHA384, F_FIPS_STD, A_ECDSA },
> +    { 0, CS(ECDHE_ECDSA_WITH_AES_256_GCM_SHA384), S_ECDSA, K_ECDHE, C_AESGCM, B_256, M_SHA384, F_FIPS_STD, A_ECDSA },
> +    { 0, CS(ECDHE_RSA_WITH_AES_128_GCM_SHA256), S_RSA, K_ECDHE, C_AESGCM, B_128, M_SHA256, F_FIPS_STD, A_RSAS },
> +    { 0, CS(ECDHE_RSA_WITH_AES_256_GCM_SHA384), S_RSA, K_ECDHE, C_AESGCM, B_256, M_SHA384, F_FIPS_STD, A_RSAS },

M_AEAD128 for all three of the GCM versions here.  https://tools.ietf.org/html/rfc5116#section-5.2

::: tests/ssl/ssl.sh
@@ +85,5 @@
>  
>    if [ -z "$NSS_DISABLE_ECC" ] ; then
>        ECC_STRING=" - with ECC"
>        # List of cipher suites to test, including ECC cipher suites.
> +      CIPHER_SUITES="-c :C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014:C023:C024:C027:C028:C02B:C02C:C02F:C030:CCA8:CCA9:CCAA:0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B:0084:009C:009D:009E:009F:00A2:00A3cdefgijklmnvyz"

This is getting too long now, can you split it please?

 EC_SUITES=":C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D"
 EC_SUITES="${EC_SUITES}:C00E:C00F:C010:C011:C012:C013:C014:C023:C024:C027"
 EC_SUITES="${EC_SUITES}:C028:C02B:C02C:C02F:C030:CCA8:CCA9:CCAA"
 NON_EC_SUITES=":0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B"
 NON_EC_SUITES="${NON_EC_SUITES}:0084:009C:009D:009E:009F:00A2:00A3cdefgijklmnvyz"
 CIPHER_SUITES="-c $(EC_SUITES}${NON_EC_SUITES}"

@@ +90,4 @@
>    else
>        ECC_STRING=""
>        # List of cipher suites to test, excluding ECC cipher suites.
> +      CIPHER_SUITES="-c :0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B:0084:009C:009D:009E:009F:00A2:00A3:CCAAcdefgijklmnvyz"

CIPHER_SUITES="-c ${NON_EC_SUITES}"

Comment 79

[Martin Thomson [:mt:]]

Oh, I'd like to verify that this works with TLS 1.3.  When I tried this before, TLS 1.3 was crashing on unrelated suites, so something in the patch was interacting badly with the 1.3 code.  I'll try to remember to run the patch before it lands.

Comment 80

[Elio Maldonado]

Comment on attachment 8744737 [details] [diff] [review]
sha384 prf support - full patch

Review of attachment 8744737 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/softoken/pkcs11c.c
@@ +6088,2 @@
>      unsigned char   key_block2[MD5_LENGTH];
>      PRBool          isFIPS;		

nit: whitespace

@@ +6534,5 @@
> +    if (block_needed > sizeof key_block) {
> +        PR_fprintf(PR_STDERR, "ERROR: block_need=%d > key_block=%d\n ",
> +                   block_needed, key_block);
> +    }
> +#endif

This was temporary for me to see the actual value and I forgot to remove it. There is a PORT_Assert in the next line.

::: lib/ssl/ssl3con.c
@@ +118,5 @@
>   { TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, SSL_ALLOWED, PR_TRUE, PR_FALSE},
>   { TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,   SSL_ALLOWED, PR_TRUE, PR_FALSE},
>   { TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,      SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, SSL_ALLOWED, PR_TRUE, PR_FALSE},
> + { TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,   SSL_ALLOWED, PR_TRUE, PR_FALSE},

I agree with you on that.

@@ +4367,5 @@
> +
> +            hm = ssl3_GetPrfHashMechanism(ss);
> +            hashOid = SECOID_FindOIDByMechanism(hm);
> +
> +            if (hashOid == NULL) {

Yes, I'll add one.

@@ +4893,5 @@
> + * identifier. If the hash is not recognised, zero is returned.
> + *
> + * See https://tools.ietf.org/html/rfc5246#section-7.4.1.4.1 */
> +static int
> +ssl3_OIDToTLSHashAlgorithm(SECOidTag oid)

I see being called from ssl3_ComputeHandshakeHashes
so I can do some check
        hashes->hashAlg = ssl3_OIDToTLSHashAlgorithm(hashOid->offset);
        PORT_Assert(hashes->hashAlg == ssl_hash_sha256 ||
                    hashes->hashAlg == ssl_hash_sha384);
        if (hashes->hashAlg != ssl_hash_sha256 &&
            hashes->hashAlg != ssl_hash_sha384) {
            ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
            rv = SECFailure;
            goto tls12_loser;
        }
        rv = SECSuccess;

Let me see if I can find a way to get rid of it.

@@ +7733,5 @@
> +        suitePRFHash = ssl_hash_sha256;
> +        suitePRFIs256Or384 = PR_TRUE;
> +    } else if (hashOid->offset == SEC_OID_SHA384) {
> +        suitePRFHash = ssl_hash_sha384;
> +        suitePRFIs256Or384 = PR_TRUE;

Yes, I remember you stated that in the first feedback.
I'll clean up that.

@@ +10284,5 @@
>      int i;
>      int certTypesLength;
>      PRUint8 sigAlgs[MAX_SIGNATURE_ALGORITHMS * 2];
>      unsigned int sigAlgsLength = 0;
> +    SSLHashType suiteHashAlg = ssl3_GetSuiteHashAlg(ss);

shooud add const if I want to keep at all.

::: lib/ssl/sslimpl.h
@@ +474,5 @@
>      cipher_missing /* reserved for no such supported cipher */
>      /* This enum must match ssl3_cipherName[] in ssl3con.c.  */
>  } SSL3BulkCipher;
>  
> +/* The TLS PRF definition */

Yes, and adapting your own comments something like

/* The TLS PRF definition */

/* Note that the prf_null value is generally used for suites designed before
 * TLS 1.2 and for TLS 1.2 prf_null actually means CKM_SHA256. */

::: lib/ssl/sslinfo.c
@@ +297,5 @@
> +    { 0, CS(ECDHE_RSA_WITH_AES_256_CBC_SHA384),  S_RSA, K_ECDHE, C_AES, B_256, M_SHA384, F_FIPS_STD, A_RSAS },
> +    { 0, CS(ECDHE_ECDSA_WITH_AES_256_CBC_SHA384), S_ECDSA, K_ECDHE, C_AES, B_256, M_SHA384, F_FIPS_STD, A_ECDSA },
> +    { 0, CS(ECDHE_ECDSA_WITH_AES_256_GCM_SHA384), S_ECDSA, K_ECDHE, C_AESGCM, B_256, M_SHA384, F_FIPS_STD, A_ECDSA },
> +    { 0, CS(ECDHE_RSA_WITH_AES_128_GCM_SHA256), S_RSA, K_ECDHE, C_AESGCM, B_128, M_SHA256, F_FIPS_STD, A_RSAS },
> +    { 0, CS(ECDHE_RSA_WITH_AES_256_GCM_SHA384), S_RSA, K_ECDHE, C_AESGCM, B_256, M_SHA384, F_FIPS_STD, A_RSAS },

Yes, I thought I had done it. I was testing several slightly different versions of the patch and most likely attached the wrong one.

::: tests/ssl/ssl.sh
@@ +85,5 @@
>  
>    if [ -z "$NSS_DISABLE_ECC" ] ; then
>        ECC_STRING=" - with ECC"
>        # List of cipher suites to test, including ECC cipher suites.
> +      CIPHER_SUITES="-c :C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014:C023:C024:C027:C028:C02B:C02C:C02F:C030:CCA8:CCA9:CCAA:0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B:0084:009C:009D:009E:009F:00A2:00A3cdefgijklmnvyz"

I agree, it hurts my eyes.

Comment 81

[Elio Maldonado]

Comment on attachment 8744456 [details] [diff] [review]
progressV2.patch

Review of attachment 8744456 [details] [diff] [review]:
-----------------------------------------------------------------

I know the attachment is now obsolete but it's worth mentioning a few things caught by the compiler.

::: lib/ssl/tls13con.c
@@ +244,5 @@
> +            return 48;
> +        default:
> +            PORT_Assert(0);
> +            return 0;
> +    }

The compiler complained because in a after the switch no value is returned. Maybe
        default:
            PORT_Assert(0);
    }
    return 0; /* How about ssl_hash_sha256? */
}

@@ +258,5 @@
> +            return CKM_NSS_HKDF_SHA384;
> +        default:
> +            PORT_Assert(0);
> +            return CKM_INVALID_MECHANISM;
> +    }

Same problem here.
Maybe
        default:
            PORT_Assert(0);
    }
    return CKM_NSS_HKDF_SHA256;

@@ +272,5 @@
> +            return CKM_SHA384_HMAC;
> +        default:
> +            PORT_Assert(0);
> +            return CKM_INVALID_MECHANISM;
> +    }

And here also
Maybe
        default:
            PORT_Assert(0);
    }
    return CKM_SHA256_HMAC;

Comment 82

[Julien Pierre]

Comment on attachment 8744737 [details] [diff] [review]
sha384 prf support - full patch

Review of attachment 8744737 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +793,1 @@
>              return vrange->max >= SSL_LIBRARY_VERSION_TLS_1_2;

Can you consolidate the two case blocks that have the following in common ? I know this wasn't changed by your patch, but the existing code didn't make sense, IMO.

return vrange->max == SSL_LIBRARY_VERSION_TLS_1_2;

@@ +838,5 @@
>          if (cipher_suite_defs[i].cipher_suite == suite)
>              return &cipher_suite_defs[i];
>      }
> +#ifdef DEBUG
> +    PR_fprintf(PR_STDERR, "*** ERROR: Can't find suite %04x\n", suite);

Use SSL_TRC here .

@@ +4358,5 @@
>  #ifndef NO_PKCS11_BYPASS
>      if (ss->opt.bypassPKCS11) {
>          PORT_Assert(!ss->ssl3.hs.sha_obj && !ss->ssl3.hs.sha_clone);
>          if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_2) {
> +            /* we support ciphersuites where the PRF hash isn't SHA-256 */

It seems this means bypass with TLS 1.2 will be broken for those cases.

This isn't the first case where some things are broken. I understand that the Mozilla community doesn't want to write the code to make new features work in bypass mode. But IMO, the current state where parts of TLS 1.2 are broken when bypass is turned on is untenable.

IMO, we should do one of the following :
1) either make the entire TLS 1.2 protocol work in bypass mode - which means writing additional code
2) not make TLS 1.2 work at all in bypass mode. IMO, this should mean that if the application enables both TLS 1.2 and bypass, then bypass should be automatically disabled, so that TLS 1.2 fully works. It would be a lot less code, and a lot more transparent to applications.

It doesn't appear likely that Oracle will be able to contribute the code to do 1), so perhaps 2) should be done.

::: lib/ssl/ssl3ecc.c
@@ +1073,5 @@
>      0 /* end of list marker */
>  };
>  
>  /* List of all ECC cipher suites */
>  static const ssl3CipherSuite ecSuites[] = {

Suggestion:

Maybe we should have separate arrays for :

ecdh_rsa
and
ecdh_ecdsa

Then this ecSuites array could just point to the other 4 arrays (ecdh_rsa, ecdh_ecdsa, ecdhe_rsa, ecdhe_ecdsa).

This way, we wouldn't have to add the same cipher suites in more than one place in the future, which seems like it might be an error-prone process.

::: lib/ssl/sslenum.c
@@ +23,5 @@
>   *    * Export/weak/obsolete cipher suites before no-encryption cipher suites
>   *    * Order by key exchange algorithm: ECDHE, then DHE, then ECDH, RSA.
> + *    * Within key agreement sections, prefer AEAD over non-AEAD cipher suites.
> + *    * Within AEAD sections, order by symmetric encryption algorithm which
> + *      integrates message authentication algorithm: AES-128-GCM, then

Do we really want to have AES-128-GCM before AES-256-GCM ?

::: lib/ssl/sslimpl.h
@@ +314,5 @@
>  #endif
>  } ssl3CipherSuiteCfg;
>  
>  #ifndef NSS_DISABLE_ECC
> +#define ssl_V3_SUITES_IMPLEMENTED 75

Any chance we could change this to :

#define ssl_V3_SUITES_IMPLEMENTED SSL_GetNumImplementedCiphers()

This would get rid of the requirement to manually keep track of the number of cipher suites, which is error-prone.  Maybe this is best done as part of the patch that deletes SSL2 code.

Comment 83

[Eric Rescorla (:ekr)]

(In reply to Julien Pierre from comment #82)
> Comment on attachment 8744737 [details] [diff] [review]
> sha384 prf support - full patch
> 
> Review of attachment 8744737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/ssl/ssl3con.c
> @@ +793,1 @@
> >              return vrange->max >= SSL_LIBRARY_VERSION_TLS_1_2;
> 
> Can you consolidate the two case blocks that have the following in common ?
> I know this wasn't changed by your patch, but the existing code didn't make
> sense, IMO.
> 
> return vrange->max == SSL_LIBRARY_VERSION_TLS_1_2;

That doesn't sound right. One of these is == and one is >=. == is for things
that we banned in TLS 1.3.


> 
> ::: lib/ssl/sslenum.c
> @@ +23,5 @@
> >   *    * Export/weak/obsolete cipher suites before no-encryption cipher suites
> >   *    * Order by key exchange algorithm: ECDHE, then DHE, then ECDH, RSA.
> > + *    * Within key agreement sections, prefer AEAD over non-AEAD cipher suites.
> > + *    * Within AEAD sections, order by symmetric encryption algorithm which
> > + *      integrates message authentication algorithm: AES-128-GCM, then
> 
> Do we really want to have AES-128-GCM before AES-256-GCM ?

Yes, I think so. It's much faster.


> #define ssl_V3_SUITES_IMPLEMENTED SSL_GetNumImplementedCiphers()
> 
> This would get rid of the requirement to manually keep track of the number
> of cipher suites, which is error-prone.  Maybe this is best done as part of
> the patch that deletes SSL2 code.

We have already removed SSL2.

Comment 84

[Martin Thomson [:mt:]]

(In reply to Elio Maldonado from comment #81)
> Maybe
>     return 0; /* How about ssl_hash_sha256? */
> Maybe
>     return CKM_NSS_HKDF_SHA256;
> Maybe
>     return CKM_SHA256_HMAC;

TLS 1.3 will never use a cipher suite that has prf_null, so return CKM_INVALID_MECHANISM or 0 or whatever is most appropriate.

(In reply to Julien Pierre from comment #82)
> 2) not make TLS 1.2 work at all in bypass mode. IMO, this should mean that
> if the application enables both TLS 1.2 and bypass, then bypass should be
> automatically disabled, so that TLS 1.2 fully works. It would be a lot less
> code, and a lot more transparent to applications.

I believe that we have tried to keep bypass mode either working or disabled.  For instance, it should be disabled for TLS 1.3, though that is not tested.  If it fails, then we should do this.

> >  /* List of all ECC cipher suites */
> >  static const ssl3CipherSuite ecSuites[] = {
> 
> Suggestion:
> 
> Maybe we should have separate arrays for :
> 
> ecdh_rsa
> and
> ecdh_ecdsa

We do already.  However, I believe that we can remove these arrays entirely and filter based on a combination of the kea_def and suite_def tables.  Unfortunately, those are in ssl3con.c, so we'll need some extra code for that.  Happy to take a patch of course.

Comment 85

[Julien Pierre]

(In reply to Eric Rescorla (:ekr) from comment #83)
> That doesn't sound right. One of these is == and one is >=. == is for things
> that we banned in TLS 1.3.

Sorry, my bad, didn't realize that.

> > Do we really want to have AES-128-GCM before AES-256-GCM ?
> 
> Yes, I think so. It's much faster.

Shouldn't this decision be left to the application ?
Servers can override the ClientHello order, but it is not simple to setup. One can disable AES_128_GCM ciphers, of course, but then there will be a fallback to CBC for clients that don't support AES_256_GCM (like Firefox with the current NSS), so that is not great.

As far as the client goes, can an NSS client application programmatically change the desired order of these cipher suites in the ClientHello ? We have client applications that talk to backend where this would be desirable.

> We have already removed SSL2.

Hadn't realized that - not in the tree we are currently using at Oracle (3.21) . In that case, my suggestion should be implemented. Or we could remove ssl_V3_SUITES_IMPLEMENTED altogether as it seems it may be duplicate functionality, though that may break source compatibility with some apps. We don't use this macro in any Oracle app, AFIAK. In any case, SSL v3 will probably be next to go, so that may not be an ideal macro to keep around.

Comment 86

[Martin Thomson [:mt:]]

(In reply to Julien Pierre from comment #85)
> Shouldn't this decision be left to the application ?

Well, that's debatable.  NSS has always taken the position that it knows best in this matter.  We have an interesting open question regarding non-Intel devices and the split between ChaCha20 and AES-GCM, but I think that the intent is to solve it inside NSS for the moment.

Comment 87

[Julien Pierre]

(In reply to Martin Thomson [:mt:] from comment #84)
> > 2) not make TLS 1.2 work at all in bypass mode. IMO, this should mean that
> > if the application enables both TLS 1.2 and bypass, then bypass should be
> > automatically disabled, so that TLS 1.2 fully works. It would be a lot less
> > code, and a lot more transparent to applications.
> 
> I believe that we have tried to keep bypass mode either working or disabled.
> For instance, it should be disabled for TLS 1.3, though that is not tested. 
> If it fails, then we should do this.

TLS 1.2 does not fully work in bypass mode today. Some features are just broken with that combination. For example, client auth with SHA-1 certificates, which I discussed recently on the nssdev mailing list, and that we have had escalations about. I find this inconsistency problematic. It would be best to either fully support the protocol or not.

Where it gets messy is when multiple versions of TLS are enabled. With the state of the code currently, there is no way to disable bypass only if TLS 1.2 is negotiated, but leave bypass enabled if TLS < 1.2 is negotiated. That's because the bypass bit is set before the handshake starts, and never changes. It might be complicated to switch from bypass to PKCS#11 dynamically based on this case - and not necessarily desirable. But maybe it's simpler than all the #ifdefs we have right now for TLS 1.2 & bypass. But if we don't want to do that, just disabling bypass if TLS 1.2 is enabled at all (before the handshake start) is simpler. At least it allows the TLS 1.2 protocol to function properly.

> We do already.  However, I believe that we can remove these arrays entirely
> and filter based on a combination of the kea_def and suite_def tables. 

Yes, removing all these is another good solution.

Comment 88

[Julien Pierre]

(In reply to Martin Thomson [:mt:] from comment #86)
> (In reply to Julien Pierre from comment #85)
> > Shouldn't this decision be left to the application ?
> 
> Well, that's debatable.  NSS has always taken the position that it knows
> best in this matter.  We have an interesting open question regarding
> non-Intel devices and the split between ChaCha20 and AES-GCM, but I think
> that the intent is to solve it inside NSS for the moment.

Of course, NSS can try to do what's best for the app, but ultimately sometimes there are decisions that are going to be application-specific, or machine specific. For example, one might prefer Chaha20 over AES-GCM on a CPU that doesn't have AES-NI . There are other CPUs which accelerate various ciphers and hash algorithms, too.

As far as Oracle is concerned, in our OTD 12c release - which no longer uses NSS - we have added configurable support for ordering of the cipher suites, both on the client side, when talking to backend, and on the server side, for purpose of overriding the client's choice. But of course, we don't require the administrator to set this up - we do have a default order for the client side, and by default for the server side, we follow the order of the ClientHello.

While we won't be adding this level of configurability to our 11x products that use NSS, we probably would change the default order to prioritize AES-256 over AES-128 for our client side, if an API was available to do so. Even if Oracle never takes advantage of this, IMO, there is a good case to be made for some applications to want a different order. The only alternative today is to disable a whole bunch of cipher suites, possibly all the way down to a single one, but that reduces compatibility with peers dramatically, as well as reduce security for some hosts that are still compatible. Thus, IMO it is not a good compromise. 

Perhaps this is an RFE best addressed in a separate bug, though. But I still find it odd that the default list prefers an arguably weaker cipher, even if it is faster. If speed was the main criteria for the default list, RC4-MD5 should be at the top of the list, assuming it hadn't been badly compromised. But even before they were significant exploits on it, it never was prioritized over, say, AES.

Comment 89

[Martin Thomson [:mt:]]

There's a view that you pick the fastest cipher suite that is strong enough.  RC4 doesn't even come close to strong enough.

And yes, if you want to be able to reorder cipher suites, then we should talk about what it would take.  It's not a priority for us.

Comment 90

[Julien Pierre]

I will file an RFE for configuring the order, if there isn't one already.

My point was that speed was not historically a significant factor in the order of the list. Still doesn't seem to be, given that PFS cipher suites are listed first over non-PFS. Which I agree with, but PFS suites are slower.

"Strong enough" is a relative judgment.

Anyway, it does seem curious that while we are adding SHA384 support, which will enable AES_256_GCM_SHA384 cipher suites to function, but if both sides are using the latest NSS, and AES_128_GCM_SHA256 suites are also enabled, which they are by default, the AES_256_GCM_SHA384 suites will never be negotiated .

And disabling AES_128_GCM_SHA256 on the server side will negatively impact clients that support only AES_128_GCM_SHA256 but not AES_256_GCM_SHA384, causing a fallback to CBC. Thus, with the current default list, and without the server having the ability to override the client's chosen order, NSS servers that want to use AES_256_GCM_SHA384 are left with no good configuration choice. I think that's something that should be given some consideration. Admittedly, it's not a brand new issue as we already had the same problem with AES-128-CBC being prioritized over AES-256-CBC. But from a server developer's point of view, it is limiting.

Comment 91

[Masatoshi Kimura [:emk]]

It is not obvious that AES-256 is more secure than AES-128; See bug 975832.

Comment 92

[Julien Pierre]

Masatoshi,
Thanks for that link. I read some of the discussions and various attacks. Much is beyond my math knowledge. It seems that there is a category of attacks that is effective against AES-256 but not AES-128, but even so, not quite clear that AES-128 is more secure either.

Comment 93

[Martin Thomson [:mt:]]

Elio, one thing that your patch is missing is this obscure comment in tls13_AllowPskCipher() in tls13con.c:

    /* TODO([e-mail]): Check the KDF code whenever we have
     * adjustable KDFs. */

We need to check that the PSK suite uses the same PRF hash that was used in the original session.  That means checking that the PRF associated with sid->u.ssl3.cipherSuite matches the PRF for the PSK suite.

Comment 94

[Tom Mraz]

(In reply to Masatoshi Kimura [:emk] from comment #91)
> It is not obvious that AES-256 is more secure than AES-128; See bug 975832.

The problem with AES-128 is that in Post Quantum computing world it will be considered broken as 128 bits for a key is too low when divided by two.

I do not know whether the attacks on AES-256 key schedule can be sped up by quantum computer though.

Comment 95

[Elio Maldonado]

I a have a new version of the patch with many improvements suggested by Martin. Not all test are passing and looking at some of the failures I found
Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:88 and witch it
tstclnt: write to SSL socket failed: SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION: SSL feature not supported for the protocol version
            /* TODO([e-mail]): Reenable for TLS 1.3 */
            if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
                errCode = SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION;
                goto loser;
            }
That let me to search for more cases I found this
lib/ssl/ssl3prot.h: * TODO([e-mail]): Remove when TLS 1.3 is published. */
lib/ssl/tls13con.c:    /* TODO([e-mail]): This needs to actually be looked up. */
lib/ssl/tls13con.c:    /* TODO([e-mail]): This needs to actually be looked up. */
lib/ssl/tls13con.c:    /* TODO([e-mail]): This needs to actually be looked up. */
lib/ssl/tls13con.c:    /* TODO([e-mail]): Handle multiple curves here. */
lib/ssl/tls13con.c:    /* TODO([e-mail]): Would it be better to check all the states here? */
lib/ssl/tls13con.c:     * TODO([e-mail]): Make a version with the "true" values.
lib/ssl/tls13con.c:    /* TODO([e-mail]): Check the KDF code whenever we have
lib/ssl/tls13con.c:        /* TODO([e-mail]): Free resumeSID. */
lib/ssl/tls13con.c:         * TODO([e-mail]): Write code to correct client.
lib/ssl/tls13con.c:    /* TODO([e-mail]): This first clause is futureproofing for
lib/ssl/tls13con.c:     * TODO([e-mail]): This should be easy to relax in TLS 1.3 by
lib/ssl/tls13con.c:    /* TODO([e-mail]): Record key log */
lib/ssl/tls13con.c:            /* TODO([e-mail]): Add support for new tickets in PSK. */
lib/ssl/tls13con.c:            /* TODO([e-mail]): Handle pending auth */
lib/ssl/tls13con.c:    /* TODO([e-mail]): Re-enable new tickets when PSK mode is
lib/ssl/tls13con.c:        ExtensionNotUsed /* TODO([e-mail]): Disabled because broken
lib/ssl/tls13hkdf.c:// TODO([e-mail]): Export this separately.
lib/ssl/tls13hkdf.c:        /* TODO([e-mail]): This violates the PKCS#11 key boundary
lib/ssl/ssl3ext.c:    /* TODO: add a handler for ssl_ec_point_formats_xtn */
lib/ssl/ssl3ext.c:    /* TODO: server side NPN support would require calling
lib/ssl/ssl3ext.c:        /* TODO: crash */
lib/ssl/ssl3ext.c:            /* TODO: else send an empty ticket. */
lib/ssl/ssl3ext.c: * TODO([e-mail]): Remove when TLS 1.3 is published. */
lib/ssl/ssl3ext.c: * TODO([e-mail]): Remove when TLS 1.3 is published. */
lib/ssl/os2_err.c: * OS2TODO Stub everything for now to build. HCT
lib/ssl/ssl3con.c:/* TODO(ekr): Implement HelloVerifyRequest on server side. OK for now. */
lib/ssl/ssl3con.c:     * TODO([e-mail]): Verify that the slot can handle this key expansion
lib/ssl/ssl3con.c:        /* TODO([e-mail]): The backup hash here contains a SHA-1 hash
lib/ssl/ssl3con.c:             * TODO([e-mail]): Add option to refuse to resume when EMS is not
lib/ssl/ssl3con.c:        /* TODO: Fix session tickets for DSS. The server code rejects the
lib/ssl/ssl3con.c:     * TODO([e-mail]): Note this change was not added in the SSLv2
lib/ssl/ssl3con.c:    /* TODO: For TLS 1.3 final, downgrade when the extension IS present. */
lib/ssl/ssl3con.c:     * TODO: send a session ticket if performing a stateful
lib/ssl/ssl3con.c:        /* TODO: Support DH_anon. It might be sufficient to drop the signature.
lib/ssl/ssl3con.c:            /* TODO([e-mail]): Reenable for TLS 1.3 */
lib/ssl/sslt.h:    ssl_tls13_key_share_xtn = 40,      /* unofficial TODO(ekr) */
lib/ssl/sslt.h:    ssl_tls13_pre_shared_key_xtn = 41, /* unofficial TODO(ekr) */

Most of then deal with TLS 1.3. 

Eric, Martin, Could it be possible that current TLS 1.3 work gets in the way of this work as we are both adding and enabling cipher suites that need to play well in the presence of each other?

Comment 96

[Elio Maldonado]

Attachment: sha384 prf support

This is work in progress.

Comment 97

[Elio Maldonado]

Attachment: sha384 prf support

Work in progress, let's see his one works better with interdiff.

Comment 98

[Elio Maldonado]

Comment on attachment 8747816 [details] [diff] [review]
sha384 prf support

wrong patch.

Comment 99

[Elio Maldonado]

Attachment: tls12 sha384 prf support - wip

Comment 100

[Elio Maldonado]

(In reply to Elio Maldonado from comment #95)
> Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:88
> tstclnt: write to SSL socket failed:
> SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION: SSL feature not supported for
> the protocol version

Saw the same failure with a build where the attached patch wasn't applied. Until told it's okay I'll refrain from enabling TLS 1.3 in my test builds.

Comment 101

[Martin Thomson [:mt:]]

Elio, there are a number of places that the usual test suite fails with TLS 1.3 enabled.  However, you should be able to build and run the gtests successfully.

Comment 102

[Elio Maldonado]

Attachment: tls12 sha384 prf support - disabled by default

Version with the new ciphers disabled by default for Kai as he starts working on Bug 1179338.

Comment 103

[Elio Maldonado]

Attachment: tls12 prf sha384 hash support - disabled by default

Updated on account of latest changes but still a work in progress.

Comment 104

[Martin Thomson [:mt:]]

Comment on attachment 8749264 [details] [diff] [review]
tls12  prf sha384 hash support -  disabled by default

Review of attachment 8749264 [details] [diff] [review]:
-----------------------------------------------------------------

I managed to get this working.  I'll upload my patch, which includes fixes for the below comments.

I'm now disqualified from reviewing this.  Maybe ask Kai or Tim.

::: lib/ssl/ssl3con.c
@@ +4849,5 @@
> + * { ssl_hash_sha224, SEC_OID_SHA224 } as one of the entries
> + * in which I haven't included as not recommended for TLS 1.3
> + * https://tools.ietf.org/html/draft-ietf-tls-tls13-08 which
> + * we plan to support. We still need to work this out, see
> + * also Bug 1179338.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1179338#c19

@@ +7700,5 @@
>      /* Determine the server's hash support for that signature algorithm. */
>      for (i = 0; i < algorithms->len; i += 2) {
>          if (algorithms->data[i + 1] == sigAlg) {
>              if (algorithms->data[i] == ssl_hash_sha1) {
>                  supportsSha1 = PR_TRUE;

This change is confusing.  You still need to check that the peer supports the PRF hash.  Here, you assume that the existence of the hash in the cipher suite is enough, but it isn't.  The signature algorithms that the server advertises need to be checked.

::: tests/ssl/ssl.sh
@@ +83,5 @@
>    USER_NICKNAME=TestUser
>    NORM_EXT=""
>  
> +  EC_SUITES=":C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D"
> +  EC_SUITES="${EC_SUITES}:C00E:C00F:C010:C011:C012:C013:C014:C023:C024:C027"   

trailing space here.

Comment 105

[Martin Thomson [:mt:]]

Attachment: bug923089-1.patch

This passes all the gtests.

Comment 106

[Martin Thomson [:mt:]]

Comment on attachment 8750574 [details] [diff] [review]
bug923089-1.patch

Review of attachment 8750574 [details] [diff] [review]:
-----------------------------------------------------------------

I realize that this doesn't include the gtests that I added for the new ciphersuites.  That's easy to fix, but a pretty serious omission.

Comment 107

[Elio Maldonado]

Attachment: adds sha384 tests - naive

Is this what you have in mind?

I tried it with TLS 1.3 enabled and had a core dump but none when it wasn't. 

I also see some comments like
 // The cipher suites of type ECDH_RSA are disabled because we don't have
 // a compatible certificate in the test fixture.
 INSTANTIATE_CIPHER_TEST_P(RC4, Stream, V10ToV12

and
  // TODO - Bug 1251136 move DHE_RSA suites to V12Plus
 INSTANTIATE_CIPHER_TEST_P(AEAD12, All, V12,

Comment 108

[Martin Thomson [:mt:]]

Comment on attachment 8751087 [details] [diff] [review]
adds sha384 tests - naive

Review of attachment 8751087 [details] [diff] [review]:
-----------------------------------------------------------------

Almost.  The problem I see might cause the crash you are seeing.

::: external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc
@@ +112,4 @@
>                            TLS_DHE_RSA_WITH_AES_128_GCM_SHA256);
>  INSTANTIATE_CIPHER_TEST_P(AEAD, All, V12Plus,
> +                          TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384,
> +                          TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384,

TLS 1.3 doesn't support CBC modes, move these to the 1.2 section as well.

Comment 109

[Martin Thomson [:mt:]]

One important thing that is missing, can you update tls13_AllowPskCipher() to check that the old cipher suite and the new one use the same PRF hash?  See comment 93.

(In reply to Elio Maldonado from comment #107)
>  // The cipher suites of type ECDH_RSA are disabled because we don't have
>  // a compatible certificate in the test fixture.
>  INSTANTIATE_CIPHER_TEST_P(RC4, Stream, V10ToV12

I'm currently working on making the DHE suites good, so that one goes away soon.  We still rely on the scripts to test ECDH_RSA suites at the moment.  The only hard part is working out how to generate a certificate for them.  If you are able to do that and add it to ssl_gtests.sh, then I can do the rest.  I just don't see it as urgent right now.

>   // TODO - Bug 1251136 move DHE_RSA suites to V12Plus
>  INSTANTIATE_CIPHER_TEST_P(AEAD12, All, V12,

I'm working on implementing DHE for TLS 1.3, almost there.  I hope that this patch lands first.  You do not want to land second in this case.

Comment 110

[Elio Maldonado]

(In reply to Martin Thomson [:mt:] from comment #104)

> I'm now disqualified from reviewing this.  Maybe ask Kai or Tim.
Let's start with Kai as this is blocking other work he's trying do do.

Comment 111

[Martin Thomson [:mt:]]

Comment on attachment 8750574 [details] [diff] [review]
bug923089-1.patch

Review of attachment 8750574 [details] [diff] [review]:
-----------------------------------------------------------------

Two important omissions in this patch:

1. No gtests.  Those should be easy to add, see the other patch.

2. In TLS 1.3, we have to check that the PRF hash is the same when resuming.  See https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/tls13con.c#454

::: lib/softoken/pkcs11c.c
@@ +6534,5 @@
> +    if (block_needed > sizeof key_block) {
> +        PR_fprintf(PR_STDERR, "ERROR: block_need=%d > key_block=%d\n ",
> +                   block_needed, key_block);
> +    }
> +#endif

Let's drop this chunk.

::: lib/ssl/ssl3con.c
@@ +832,5 @@
>              return &cipher_suite_defs[i];
>      }
> +#ifdef DEBUG
> +       SSL_TRC(3, ("*** ERROR: Can't find suite %04x\n", suite));
> +#endif

Kill this too.

@@ +3910,5 @@
> +        case ssl_hash_sha384:
> +            return CKM_SHA384;
> +        case ssl_hash_sha256:
> +        case ssl_hash_none:
> +            /* ssl_hash_none is for pre-1.2 suites, which use SHA-256. */

I think that we need to make it clear that this function is only used for TLS 1.2.

@@ +4860,5 @@
>  
>  /* tlsHashOIDMap contains the mapping between TLS hash identifiers and the
> + * SECOidTag used internally by NSS.
> + *
> + * See https://tools.ietf.org/html/whih#section-7.4.1.4.1

Broken link.

Comment 112

[Kai Engert [:KaiE:]]

Attachment: incremental on top of attachment 8750574

I haven't yet reviewed attachment 8750574 [details] [diff] [review].

However, because our local interoperability tests were failing,
I've carefully compared it to the work that we did in RHEL.

I found:
- four changes were missed
- one entry in the cipherSuites is missing the hash alg
(and we should adjust two comments)

This is an incremental patch on top of attachment 8750574 [details] [diff] [review].

Comment 113

[Kai Engert [:KaiE:]]

Another mistake:

In sslinfo.c the suggested patch adds a duplicate entry for ECDHE_RSA_WITH_AES_128_GCM_SHA256 to the suiteInfo array.

Comment 114

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #111)
> 2. In TLS 1.3, we have to check that the PRF hash is the same when resuming.
> See
> https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/tls13con.
> c#454

Is this a must have for the initial commit for this bug?

Comment 115

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #114)
> (In reply to Martin Thomson [:mt:] from comment #111)
> > 2. In TLS 1.3, we have to check that the PRF hash is the same when resuming.
> > See
> > https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/tls13con.
> > c#454
> 
> Is this a must have for the initial commit for this bug?

Ok, I see you explained in comment 93 how to do it.

Comment 116

[Elio Maldonado]

Attachment: require that PSK cipher has same PSK hash as was negotiated

incremental patch on top of the two previous ones to adddresses Comment 93.

Comment 117

[Kai Engert [:KaiE:]]

Attachment: 8753914: incremental on top of attachment 8750574

Comment 118

[Kai Engert [:KaiE:]]

Martin, I'm trying to help here.

Instead of involving more people, I consider to review the interdiff between Elio's last big patch (attachment 8749264 [details] [diff] [review]) and the modified patch that you (Martin) posted afterwards (attachment 8750574 [details] [diff] [review]). If I give r+ to your changes, you could potentially continue as the main reviewer here.

If that sounds ok, I'll do that review early tomorrow (starting in about 12 hours from now).


I just attached an incremental patch on top of attachment 8750574 [details] [diff] [review] that does several things:

- it fixes the issues I described in comment 112 and comment 113

- I added an attempted implementation for the request from comment 93,
  similar to what Elio also just attached.
  However, Elio's implemention didn't handle the scenario if
    ss->sec.isServer is true and ss->statelessResume is true
  I don't know if this scenario must be handled.
  I added a conservative implementation attempt. Let us know if that can be simplified.

- I merged in Elio's test from comment 107 and addressed Martin's request from comment 108

- I attempted to address Martin's requests from comment 111.
  Let me know if you want it done differently.


I built that code with NSS_ENABLE_TLS_1_3=1

From a previous test run, I copied the tests_results/security/localhost.6/ssl_gtests directory, cd'ed into it, and executed ssl_gtest.

That crashes with the following stack:

Program received signal SIGSEGV, Segmentation fault.
0x000000000040f141 in SSLInt_CountTls13CipherSpecs (fd=0xc04010) at libssl_internals.c:111
111	       cur_p = PR_NEXT_LINK(cur_p)) {
Missing separate debuginfos, use: dnf debuginfo-install libgcc-5.3.1-6.fc23.x86_64 libstdc++-5.3.1-6.fc23.x86_64 zlib-1.2.8-9.fc23.x86_64
(gdb) bt
#0  0x000000000040f141 in SSLInt_CountTls13CipherSpecs (fd=0xc04010) at libssl_internals.c:111
#1  0x000000000047e057 in nss_test::TlsAgent::Connected (this=0xa05300) at tls_agent.cc:519
#2  0x000000000047efd3 in nss_test::TlsAgent::Handshake (this=0xa05300) at tls_agent.cc:571
#3  0x0000000000476222 in nss_test::TlsAgent::ReadableCallback_int (this=0xa05300) at tls_agent.h:248
#4  0x00000000004761a7 in nss_test::TlsAgent::ReadableCallback (self=0xa05300, event=nss_test::READABLE_EVENT) at tls_agent.h:241
#5  0x0000000000470e3b in nss_test::Poller::Poll (this=0x9d9950) at test_io.cc:478
#6  0x0000000000486dc9 in nss_test::TlsConnectTestBase::Handshake (this=0x9dfd90) at tls_connect.cc:205
#7  0x0000000000486ff0 in nss_test::TlsConnectTestBase::Connect (this=0x9dfd90) at tls_connect.cc:219
#8  0x00000000004284ad in nss_test::TlsConnectTest_TestTls13ResumptionTwice_Test::TestBody (this=0x9dfd90) at ssl_loopback_unittest.cc:985
#9  0x00000000004b676c in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x9dfd90, method=&virtual testing::Test::TestBody(), location=0x6495db "the test body") at gtest/src/gtest.cc:2362
#10 0x00000000004b0a96 in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x9dfd90, method=&virtual testing::Test::TestBody(), location=0x6495db "the test body") at gtest/src/gtest.cc:2398
#11 0x000000000049b938 in testing::Test::Run (this=0x9dfd90) at gtest/src/gtest.cc:2435
#12 0x000000000049c0f0 in testing::TestInfo::Run (this=0x921460) at gtest/src/gtest.cc:2610
#13 0x000000000049c741 in testing::TestCase::Run (this=0x920b40) at gtest/src/gtest.cc:2728
#14 0x00000000004a36cf in testing::internal::UnitTestImpl::RunAllTests (this=0x91bc40) at gtest/src/gtest.cc:4591
#15 0x00000000004b7583 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x91bc40, 
    method=(bool (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const)) 0x4a340c <testing::internal::UnitTestImpl::RunAllTests()>, location=0x649d50 "auxiliary test code (environments or event listeners)")
    at gtest/src/gtest.cc:2362
#16 0x00000000004b17d0 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x91bc40, 
    method=(bool (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const)) 0x4a340c <testing::internal::UnitTestImpl::RunAllTests()>, location=0x649d50 "auxiliary test code (environments or event listeners)")
    at gtest/src/gtest.cc:2398
#17 0x00000000004a22c7 in testing::UnitTest::Run (this=0x9078a0 <testing::UnitTest::GetInstance()::instance>) at gtest/src/gtest.cc:4209
#18 0x000000000046ef31 in RUN_ALL_TESTS () at ../../external_tests/google_test/gtest/include/gtest/gtest.h:2304
#19 0x000000000046f044 in main (argc=1, argv=0x7fffffffdcb8) at ssl_gtest.cc:33
(gdb) print ss->ssl3.hs.cipherSpecs
$1 = {next = 0x0, prev = 0x0}

Comment 119

[Kai Engert [:KaiE:]]

The crash (from the previous comment) happened when I built with
  NSS_NO_PKCS11_BYPASS=1 NSS_FORCE_FIPS=1

I wasn't yet able to understand why that fails. I couldn't find the place where ss->ssl3.hs.cipherSpecs is set to NULL and causes the crash.


To move on, I built without the above bypass/force symbols.

In tls13_AllowPskCipher, in the client scenario, ss->sec.ci.sid may happen to be NULL, so I added another check and return FALSE.

Furthermore, in function tls13_HkdfExpandLabel, the info[100] buffer is too small, the handshakeHashLen of 48 causes the required buffer to be 102.

After I increased it to info[110], the ssl_gtest completed without crashing.

It reported the following failures:

[  FAILED  ] 5 tests, listed below:
[  FAILED  ] TlsConnectTest.TestTls13ResumptionTwice
[  FAILED  ] GenericStream/TlsConnectGeneric.ConnectResumeSupportBoth/0, where GetParam() = ("TLS", 772)
[  FAILED  ] GenericStream/TlsConnectGeneric.ConnectResumeClientBothTicketServerTicket/0, where GetParam() = ("TLS", 772)
[  FAILED  ] GenericDatagram/TlsConnectGeneric.ConnectResumeSupportBoth/0, where GetParam() = ("DTLS", 772)
[  FAILED  ] GenericDatagram/TlsConnectGeneric.ConnectResumeClientBothTicketServerTicket/0, where GetParam() = ("DTLS", 772)

Comment 120

[Kai Engert [:KaiE:]]

Attachment: incremental on top of attachment 8750574 - v3

Comment 121

[Kai Engert [:KaiE:]]

Attachment: incremental on top of attachment 8750574 - v4 - check from comment 93 only done in the client code path

I've tested the alternative approach for tls13_AllowPskCipher from Elio's patch, which limits the check to the client code path. That way ssl_gtest reports all success.

Comment 122

[Kai Engert [:KaiE:]]

Comment on attachment 8750574 [details] [diff] [review]
bug923089-1.patch

r=kaie for the changes that Martin made on top of attachment 8749264 [details] [diff] [review]

Comment 123

[Kai Engert [:KaiE:]]

Attachment: merged, with r=kaie on Martin's changes [not-for-checkin]

This patch merges Martin's latest attachment with my incremental attachment v4.

This has r=kaie on the fixes that Martin made. As a result, I believe you are no longer disqualified from reviewing.

Martin, if you are OK with attachment 8754143 [details] [diff] [review], then I think it's OK for you to r+ this combined patch.

If you request further changes, then I think it's OK to use this patch as a base for future changes made by Elio or me, and have Martin as the reviewer going forward.

Comment 124

[Martin Thomson [:mt:]]

Comment on attachment 8754325 [details] [diff] [review]
merged, with r=kaie on Martin's changes [not-for-checkin]

Review of attachment 8754325 [details] [diff] [review]:
-----------------------------------------------------------------

Probably still disqualified, but this LGTM.

::: lib/ssl/ssl3con.c
@@ +9127,5 @@
>                      /* Use the cached compression method. */
>                      ss->ssl3.hs.compression =
>                          sid->u.ssl3.compression;
> +
> +

Extra lines.

::: lib/ssl/tls13con.c
@@ +1629,5 @@
>      PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
>      /* TODO([e-mail]): This first clause is futureproofing for
>       * 0-RTT. */
>      if (ss->ssl3.hs.hashType == handshake_hash_unknown) {
> +        PORT_Assert(ss->ssl3.hs.hashType != handshake_hash_unknown);

I think that this is a good change, but we should probably just remove the if statement as a result.

::: lib/ssl/tls13hkdf.c
@@ +96,5 @@
>                        PK11SymKey **keyp)
>  {
>      CK_NSS_HKDFParams params;
>      SECItem paramsi = { siBuffer, NULL, 0 };
> +    PRUint8 info[110];

This highlights the need for a comment explaining why the buffer is this big.

Comment 125

[Martin Thomson [:mt:]]

Comment on attachment 8754143 [details] [diff] [review]
incremental on top of attachment 8750574 [details] [diff] [review] - v4 - check from comment 93 only done in the client code path

Review of attachment 8754143 [details] [diff] [review]:
-----------------------------------------------------------------

See review of entire.

Comment 126

[Kai Engert [:KaiE:]]

Regarding the crash that I mentioned in comment 118:
I have filed bug 1274313

The crash is unrelated to our work from this bug.

The crash is specific to building with the combination of both 
NSS_ENABLE_TLS_1_3=1 and NSS_NO_PKCS11_BYPASS=1

The crash happens with and without the patch from this bug, so it doesn't need to block this work.

Comment 127

[Kai Engert [:KaiE:]]

Attachment: 923089-comment124.patch [ready-for-checkin]

This patch addresses Martin's review from comment 124.

It's incremental on top of attachment 8754325 [details] [diff] [review].

Comment 128

[Kai Engert [:KaiE:]]

Attachment: 923089-comment124-incremental.patch

> Created attachment 8754445 [details] [diff] [review]
> 923089-comment124.patch
> 
> This patch addresses Martin's review from comment 124.

Actually, that was a full patch.


> It's incremental on top of attachment 8754325 [details] [diff] [review].

I'm attaching the incremental one here.

Comment 129

[Kai Engert [:KaiE:]]

Martin, I notice that you already gave r+.

Since I've exactly addressed your final requests, I think I'll go ahead and land right now, so we can get test results from buildbot.

Comment 130

[Kai Engert [:KaiE:]]

Comment on attachment 8754445 [details] [diff] [review]
923089-comment124.patch [ready-for-checkin]

Landed as
https://hg.mozilla.org/projects/nss/rev/79471a8d67fa

"inline static" doesn't seem to be supported on Windows. Removed the "inline" definition as a bustage fix. Is it worth to have a follow up, to have this inline on the platform that support it?
https://hg.mozilla.org/projects/nss/rev/95008b877a39

Comment 131

[Franziskus Kiefer [:franziskus]]

Attachment: static-ssl3con.patch

This should fix the inline issue on Windows. I didn't try it on Windows but it works in other places so it should be fine.

Comment 132

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #130)
> Comment on attachment 8754445 [details] [diff] [review]
> 923089-comment124.patch
> 
> Landed as
> https://hg.mozilla.org/projects/nss/rev/79471a8d67fa

Backed out for now:
https://hg.mozilla.org/projects/nss/rev/c375f1fa727a

We'll have to address the failure in bypass mode.

selfserv crashed in ssl3_UpdateHandshakeHashes here:
  if (ss->opt.bypassPKCS11) {
        if (ss->ssl3.hs.hashType == handshake_hash_single) {
            ss->ssl3.hs.sha_obj->update(ss->ssl3.hs.sha_cx, b, l);

Comment 133

[Franziskus Kiefer [:franziskus]]

In addition to the bypass error I'd like to see the formatting fixed before this gets re-landed https://travis-ci.org/nss-dev/nss/jobs/131474073.

Comment 134

[Kai Engert [:KaiE:]]

I have a patch that (apparently) implements the missing pieces for SHA384 with bypass mode, it passes the test suite.

I'm updating my local llvm to get the required new clang-format, and will fix the format, prior to attaching the incremental new patch.

Comment 135

[Kai Engert [:KaiE:]]

Attachment: Fix bypass, on top of attachment 8754445

This patch:
- implements the pieces that were necessary to pass the testsuite in bypass mode
- adds SHA384 stress tests
- merges Franziskus' inline fix for Windows
- fixes formatting as requested by Franziskus

Comment 136

[Elio Maldonado]

Comment on attachment 8754843 [details] [diff] [review]
Fix bypass, on top of attachment 8754445 [details] [diff] [review]

Review of attachment 8754843 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +2433,5 @@
> +ssl3_GetTls12BypassHashCloneFunc(sslSocket *ss)
> +{
> +    switch (ss->ssl3.hs.suite_def->prf_hash) {
> +        case ssl_hash_sha384:
> +            return (hash_clone_func)SHA384_Clone;

ssl3con.c: In function ‘ssl3_GetTls12BypassHashCloneFunc’:
ssl3con.c:2437:37: error: ‘SHA384_Clone’ undeclared (first use in this function)
             return (hash_clone_func)SHA384_Clone;
                                     ^
I included #include "blapi.h" which declares it as
extern void SHA384_Clone(SHA384Context *dest, SHA384Context *src)

@@ +2437,5 @@
> +            return (hash_clone_func)SHA384_Clone;
> +        case ssl_hash_sha256:
> +        case ssl_hash_none:
> +            /* ssl_hash_none is for pre-1.2 suites, which use SHA-256. */
> +            return (hash_clone_func)SHA256_Clone;

As above, including blapi.h takes care of it.

@@ +10514,5 @@
>      unsigned char rsaPmsBuf[SSL3_RSA_PMS_LENGTH];
>  #endif
>      SECStatus rv;
>      SECItem enc_pms;
> +    HASH_HashType ht = HASH_AlgNULL;

ssl3con.c: In function ‘ssl3_HandleRSAClientKeyExchange’:
ssl3con.c:10518:19: error: unused variable ‘ht’ [-Werror=unused-variable]
     HASH_HashType ht = HASH_AlgNULL;

This is because ht is only used for the NO_PKCS11_BYPASS case. Just move it up to ne inside the #ifndef NO_PKCS11_BYPASS area.

Comment 137

[Martin Thomson [:mt:]]

Comment on attachment 8754843 [details] [diff] [review]
Fix bypass, on top of attachment 8754445 [details] [diff] [review]

Review of attachment 8754843 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly OK.

::: lib/ssl/ssl3con.c
@@ +86,5 @@
>  #define MAX_SEND_BUF_LENGTH 32000 /* watch for 16-bit integer overflow */
>  #define MIN_SEND_BUF_LENGTH 4000
>  
> +#if defined(_MSC_VER) && _MSC_VER < 1900
> +#define inline __inline

Let's just not use inline.  Unless you have evidence that there is a significant performance difference.

@@ +2411,5 @@
>  }
>  #endif
>  
> +HASH_HashType
> +ssl3_GetTls12BypassHashType(sslSocket *ss)

I think that you can change this to include the version check.  Check within this function if this is TLS 1.2 or greater and return HASH_AlgNull.

@@ +2489,5 @@
>          /* Double Bypass succeeded in extracting the master_secret */
>          const ssl3KEADef *kea_def = ss->ssl3.hs.kea_def;
>          PRBool isTLS = (PRBool)(kea_def->tls_keygen ||
>                                  (pwSpec->version > SSL_LIBRARY_VERSION_3_0));
> +        HASH_HashType ht = HASH_AlgNULL;

s/ht/hashType/

@@ +2494,3 @@
>          pwSpec->bypassCiphers = PR_TRUE;
> +
> +        if (isTLS && pwSpec->version >= SSL_LIBRARY_VERSION_TLS_1_2) {

The isTLS check is redundant here.  And move the check inside the function.

@@ +10514,5 @@
>      unsigned char rsaPmsBuf[SSL3_RSA_PMS_LENGTH];
>  #endif
>      SECStatus rv;
>      SECItem enc_pms;
> +    HASH_HashType ht = HASH_AlgNULL;

s/ht/hashType

::: tests/ssl/sslstress.txt
@@ +80,5 @@
>     noECC    0      -r_-r_-c_:0032  -V_ssl3:_-c_100_-C_:0032_-N_-n_TestUser-dsa     Stress TLS DHE_DSS_WITH_AES_128_CBC_SHA (no reuse, client auth)
>     noECC    0      -r_-r_-c_:0067  -V_ssl3:_-c_1000_-C_:0067_-n_TestUser-dsamixed  Stress TLS DHE_RSA_WITH_AES_128_CBC_SHA256 (client auth)
>  
>  #  noECC    0   -r_-r_-c_:00A2_-u  -V_ssl3:_-c_1000_-C_:00A2_-n_TestUser-dsa_-u       Stress TLS DHE_DSS_WITH_AES_128_GCM_SHA256 (session ticket, client auth)
> +#  noECC    0   -r_-r_-c_:00A3_-u  -V_ssl3:_-c_1000_-C_:00A3_-n_TestUser-dsa_-u       Stress TLS DHE_DSS_WITH_AES_256_GCM_SHA384 (session ticket, client auth)

I assume that you know why these are commented out.

Comment 138

[Kai Engert [:KaiE:]]

(In reply to Elio Maldonado from comment #136)
> 
> ssl3con.c: In function ‘ssl3_GetTls12BypassHashCloneFunc’:
> ssl3con.c:2437:37: error: ‘SHA384_Clone’ undeclared (first use in this
> function)
>              return (hash_clone_func)SHA384_Clone;

I don't understand why SHA384_Clone and SHA256_Clone are undeclared on your system.

Prior to this patch, file ssl3con.c already contained a reference to SHA256_Clone.

Comment 139

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #138)
> I don't understand why SHA384_Clone and SHA256_Clone are undeclared on your
> system.
> 
> Prior to this patch, file ssl3con.c already contained a reference to
> SHA256_Clone.

Now I see it. We already have:

#ifndef NO_PKCS11_BYPASS
#include "blapi.h"
#endif

The two helper functions also should be wrapped with #ifndef NO_PKCS11_BYPASS

Thanks

Comment 140

[Elio Maldonado]

(In reply to Kai Engert (:kaie) from comment #138)
I think the problem was the system I used. I should always use my normal development system. When I moved there it compiled just fine. Sorry for that.

Comment 141

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #137)
> >  #  noECC    0   -r_-r_-c_:00A2_-u  -V_ssl3:_-c_1000_-C_:00A2_-n_TestUser-dsa_-u       Stress TLS DHE_DSS_WITH_AES_128_GCM_SHA256 (session ticket, client auth)
> > +#  noECC    0   -r_-r_-c_:00A3_-u  -V_ssl3:_-c_1000_-C_:00A3_-n_TestUser-dsa_-u       Stress TLS DHE_DSS_WITH_AES_256_GCM_SHA384 (session ticket, client auth)
> 
> I assume that you know why these are commented out.

See the explanation in the line below:

 # use the above session ticket test, once session tickets with DHE_DSS are working

Comment 142

[Kai Engert [:KaiE:]]

Attachment: Fix bypass v2, on top of attachment 8754445

Addressed Martin's change requests.

Comment 143

[Kai Engert [:KaiE:]]

Comment on attachment 8754931 [details] [diff] [review]
Fix bypass v2, on top of attachment 8754445 [details] [diff] [review]

build errors when building with NO_PKCS11_BYPASS (undefined things)

Comment 144

[Kai Engert [:KaiE:]]

Attachment: Fix bypass v3, on top of attachment 8754445

Elio was right, ssl3_GetTls12BypassHashType is used in both build modes, and therefore the include to blapi.h must be unconditional. Because it's used in non-bypass code, I've renamed it to ssl3_GetTls12HashType.

This patch builds both with and without NO_PKCS11_BYPASS=1.

Comment 145

[Kai Engert [:KaiE:]]

Attachment: Fix bypass v4, on top of attachment 8754445

Sorry for going back and forth, removed two more unnecessary versions checks prior to calling ssl3_GetTls12HashType.

Comment 146

[Kai Engert [:KaiE:]]

Attachment: Fix bypass v5, on top of attachment 8754445

Grrr, found two more places to improve. I hope this is it, and you didn't yet look at the intermediate ones.

Comment 147

[Martin Thomson [:mt:]]

Comment on attachment 8754948 [details] [diff] [review]
Fix bypass v5, on top of attachment 8754445 [details] [diff] [review]

Review of attachment 8754948 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like you fixed my other comments.

::: lib/ssl/ssl3con.c
@@ +10585,2 @@
>          rv = ssl3_MasterSecretDeriveBypass(pwSpec, cr, sr, &pmsItem, isTLS,
> +                                           hashType, PR_TRUE);

You could just inline the call here.

Comment 148

[Kai Engert [:KaiE:]]

Clearing needinfo for EKR, which Elio had added in comment 95.
It seems Martin had answered the question in comment 101.

Comment 149

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #147)
> You could just inline the call here.

Will do, thanks for the patient review!

Comment 150

[Kai Engert [:KaiE:]]

Attachment: Fix bypass v6, on top of attachment 8754445

(In reply to Martin Thomson [:mt:] from comment #147)
> You could just inline the call here.

Comment 151

[Kai Engert [:KaiE:]]

Relanded the earlier patch
https://hg.mozilla.org/projects/nss/rev/3522552c289a

and added the bypass patch:
https://hg.mozilla.org/projects/nss/rev/a76066192353

Comment 152

[Kai Engert [:KaiE:]]

Landed bustage fix for Windows
https://hg.mozilla.org/projects/nss/rev/d76a9d093c45

e:/slavedir/3-winxp-x64-OPT/hg/nss/lib/ssl/ssl3con.c(2494) : error C2275: 'HASH_HashType' : illegal use of this type as an expression
        ../../../dist/public/nss\hasht.h(28) : see declaration of 'HASH_HashType'
e:/slavedir/3-winxp-x64-OPT/hg/nss/lib/ssl/ssl3con.c(2494) : error C2146: syntax error : missing ';' before identifier 'hashType'
e:/slavedir/3-winxp-x64-OPT/hg/nss/lib/ssl/ssl3con.c(2494) : error C2065: 'hashType' : undeclared identifier
e:/slavedir/3-winxp-x64-OPT/hg/nss/lib/ssl/ssl3con.c(2500) : error C2065: 'hashType' : undeclared identifier

Comment 153

[Kai Engert [:KaiE:]]

This appears to be done, marking fixed.
Please comment if there's anything that remains to be done.