238051 - Enable SSL session reuse for ECC cipher suites

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Vipul Gupta]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-us) AppleWebKit/124 (KHTML, like Gecko) Safari/125.1
Build Identifier: 

Currently, it isn't possible to do SSL session reuse for ECC cipher suites because
the getWrappingKey function in ssl3con.c returns SECFailure whenever the
exchKeyType argument is kt_ecdh.

This bug causes a performance loss for NSS-based servers and clients when
using ECC cipher suites in SSL; they must perform a public-key operation on
every SSL handshake instead of being able to use an abbreviated handshake
in some instances.

Since ECC support is not compiled in by default, this is not a major issue for now.


Reproducible: Always
Steps to Reproduce:
1. Build a version of NSS with ECC enabled (on Linux and MacOS X, for example, this requires setting 
DEFINES to -DNSS_ENABLE_ECC and NSS_ENABLE_ECC to 1 before gmake nss_build_all).
2. Set up the NSS test scripts to test ECC functionality. This is accomplished by: cd nss/tests; ./
fixtests.sh enable-ecc
3. In nss/tests/ssl, the file ecsslstress.txt gets copied to sslstress.txt and it passes the -N flag to 
strsclnt when testing ECC cipher suites. Remove the -N flag in sslstress.txt.
4. Now run the NSS tests: cd nss/tests; ./all.sh
stress tests
Actual Results:  
The "SSL Stress Test" shows failures for ECC cipher suites.

Expected Results:  
Ideally, the stress tests should pass without disabling session reuse.

We need to figure out the correct behavior for getWrappingKey for kt_ecdh key exchange
so that ECC cipher suites can successfully reuse a prior SSL session.

Comment 1

[Vipul Gupta]

Attachment: This patch enables session reuse for ECC cipher suites (under certain assumptions)

This patch enabled session reuse with ECC cipher suites. It assumes that a
server that
has ECC enabled also has RSA enabled so even when the key exchange is kt_ecdh,
one could reuse the wrapping key for kt_rsa.

The patch modifies nss/lib/ssl/ssl3con.c to call getWrappingKey with kt_rsa
(even
when the cipher suite uses the kt_ecdh key exchange mechanism). The only other
file modified by the patch is nss/tests/ssl/ecsslstress.txt where we now enable

session reuse for ECC cipher suites.

Comment 2

[Julien Pierre]

Vipul,

Isn't it possible for a server to have only the ECC cipher suites enabled, and
none of the RSA cipher suites ?

If so, how should we deal with that situation ?

Comment 3

[Vipul Gupta]

(In reply to comment #2)
> Vipul,
> 
> Isn't it possible for a server to have only the ECC cipher suites enabled, and
> none of the RSA cipher suites ?
> 
> If so, how should we deal with that situation ?
> 

Hi Julien,

  This is a possibility (although for the near future, I'd expect servers to have
RSA enabled since most clients only support RSA) and we do need a better
solution in the long term. If someone familiar with the subject of key wrapping
in NSS were willing to educate me, we should be able to figure out what the right
thing to do is for ECC and implement that.

  If you have time, please consider this as a solicitation for help :-)

vipul

Comment 4

[Julien Pierre]

Vipul,

I'll do what I can but mostly this part is beyond my expertise. Did you debug
getWrappingKey to find where it failed when using exchgKeyType = kt_ecdh ? Does
it fail because there is no code for case kt_ecdh in the switch statement ? Or
does it fail for some other reason ?

Comment 5

[Julien Pierre]

I wouldn't consider this an enhancement, since session reuse is an integral part
of the TLS protocol. Do we want to deal with this in 3.10 or in a 3.9 patch ?

Comment 6

[Julien Pierre]

We have decided to go forward with ECC for the NSS 3.9 delivery at Sun,
therefore we need to fix this in 3.9.2 .

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 144366 [details] [diff] [review]
This patch enables session reuse for ECC cipher suites (under certain assumptions)

This review comment is about the approach of the patch, rather than 
about details of the implementation of that approach.

This patch assumes that the server will also have been configured to use 
an RSA cert.  However, one of the authors of the Internet Draft for ECC 
in TLS has repeatedly stated that he believes that TLS-ECC applications
will primarily be net-new applications, implying entirely new clients and
servers.  If that prediction is correct, then the approach taken in this
patch won't help those net-new application servers.

An alternative approach that occurred to me long ago is to use ECDH among
the processes sharing the cache space, using the server's primary EC key
for that.  Similarly for the non-EC case (e.g. DSA pubkeys), use DHE 
among the processes, using the prime P from the DSA cert.

Vipul, what do you think of those ideas?

Comment 8

[Julien Pierre]

Retargetting this bug 3.10, as pending legal issues have caused us to delay
enabling ECC .

Comment 9

[Vipul Gupta]

(In reply to comment #4)
> Vipul,
> 
> I'll do what I can but mostly this part is beyond my expertise. Did you debug
> getWrappingKey to find where it failed when using exchgKeyType = kt_ecdh ? Does
> it fail because there is no code for case kt_ecdh in the switch statement ? Or
> does it fail for some other reason ?
> 

(In reply to comment #4)
 
> I'll do what I can but mostly this part is beyond my expertise. Did you debug
> getWrappingKey to find where it failed when using exchgKeyType = kt_ecdh ? Does
> it fail because there is no code for case kt_ecdh in the switch statement ? Or
> does it fail for some other reason ?

  Hi Julien,

   After our recent phone conversation, I realized that I still owe you
  an aanswer to this question. I stepped through the code for getWrappingKey
  again today and can confirm that the code fails because it cannot handle kt_ecdh. 
  Here is some additional information.

  For ECDH_ECDSA ciphers:

   1. The first time a server handles a new SSL connection, it calls getWrappingKey 
       (from HandleFinsihed) which results in a wrapping key being generated and
       returned. However, because the switch statement has no provision for kt_ecdh,
       the wrapping key doesn't get "saved".

    2. When a the server receives a request to renew the session, it calls getWrappingKey
       again (from HandleClientHello). But because the wrapping key wasn't saved properly,
      and *pSymWrapKey is NULL. Even ssl_GetWrappingKey returns 0. Since a NULL 
      value is passed for masterSecretSlot when getWrappingKey is called from HandleClientHello,
      it returns (see line 3434) with a NULL wrappingKey. This forces the server to engage in
      a full handshake.


    In contrast, for RSA:

       1. When getWrappingKey is called from HandleFinished (after completing the full
           handshake), a wrapping key s generated and stored.

       2. Subsequent invocations to getWrappingKey from handleClientHello (when the
           server receives a session resumption request) are able to find a non-NULL
           wrapping key pointed to by pSymWrapKey so session reuse works properly.

          pSymWrapKey = &symWrapKeys[symWrapMechIndex].symWrapKey[exchKeyType];


    [It isn't yet clear to me how the code path for kt_rsa ends up saving the 
     wrapping key in symWrapKeys].

vipul

Comment 10

[Robert Relyea]

I agree with nelson here, we need to have an ECC generated wrapped key. The
current code will assert on debug builds if you don't have an RSA key defines.

The fix should be to to use the ECH private key. Probably the safest way to do
this is:

in the switch (exchKeyType) statement in getWrappingKey add a case kt_ecdh:
     Generate an temporary ECDH pair.
     discard the generated private key of that pair.
     do an ECH key exchange with the private key and generated public key.
     use the returned key as a Ks (a.la. fortezza) and proceed as the fortezza
key exchange does.
     The final wrapped key should be packages with the generated ECDH public key.
      NOTE: make sure the wrapped key space we allocated above is the correct
size (it might be that that allocation may have to be moved into the switch
statement itself now).

in the switch (exchKeyType) statement in sslUnwrapSymWrappingKey add a case kt_ecdh:
     extract the generated ECDH public key from the data blob.
     do an ECDH key exchange with the private key and generated public key.
use the returned key as a Ks (a.la. fortezza) and proceed as the fortezza key
exchange does (unwrap the symetric portion).


It would also be possible to use the ECDH key bits as an input to a key
derivation algorithm (a.la. PBE) [with a random salt, of course], but I would
want such a scheme to be reviewed by ECDH expert to determine wether or not we
could compromize either the generated key (bad) or the ECDH key (worse).

(while we are at it we should add identical kt_dh cases as well).

The resulting keys, BTW are written to disk to be shared among multiple
clustered servers (one would envision distributing this cache across the
network, though we have never really implemented that).

bob

Comment 11

[Vipul Gupta]

(In reply to comment #10)

  Hi Bob,

    Thanks for looking at this code. I have a few questions below.

> I agree with nelson here, we need to have an ECC generated wrapped key. The
> current code will assert on debug builds if you don't have an RSA key defines.
> 
> The fix should be to to use the ECH private key. Probably the safest way to do
> this is:
> 
> in the switch (exchKeyType) statement in getWrappingKey add a case kt_ecdh:
>      Generate an temporary ECDH pair.
>      discard the generated private key of that pair.
>      do an ECH key exchange with the private key and generated public key.
>      use the returned key as a Ks (a.la. fortezza) and proceed as the fortezza
> key exchange does.

     Couldn't these three steps be replaced by a single step that generates a random
     key (the equivalent of Ks)? In the case of fortezze, how is Ks generated? Is such
     a "fixed key" also available in the slot that contains the server's ECC private key?
     If so, could one simply use the same code for both kt_ecdh and kt_fortezza in
     getWrappingKey as well as ssl_UnwrapSymWrappingKey?
    
     In getWrappingKey, how should the data and len fields of wrappedKey be set?
     Currently, for ECC, this field is set to the length of the ECC key in bytes. If I wanted
     to reuse the code in case kt_fortezza for kt_ecdh as well, how should I set these fields?

>      The final wrapped key should be packages with the generated ECDH public key.
>       NOTE: make sure the wrapped key space we allocated above is the correct
> size (it might be that that allocation may have to be moved into the switch
> statement itself now).

    This part wasn't clear to me. Could you elaborate some more? What does it mean to
    store wrapped key with the generated ECDH public key? Is this the ephemeral ECDH
    public key? If the corresponding private key has been discarded, how does one regenerate
    the ECDH equivalent of Ks (assuming it was computed by doing a "self" ECDH exch)?

> in the switch (exchKeyType) statement in sslUnwrapSymWrappingKey add a case kt_ecdh:
>      extract the generated ECDH public key from the data blob.
>      do an ECDH key exchange with the private key and generated public key.
> use the returned key as a Ks (a.la. fortezza) and proceed as the fortezza key
> exchange does (unwrap the symetric portion).

     Again, couldn't one skip the ECDH computation and directly get to Ks as the
    fortezza code does using PK11_FindFixedKey?
 
     If it helps to carry on this discussion on the phone, please call me. I'll send you
     my phone info in private email.

    thanks,

    vipul

Comment 12

[Robert Relyea]

> Couldn't these three steps be replaced by a single step that generates a random
> key (the equivalent of Ks)? In the case of fortezze, how is Ks generated?
> Is such a "fixed key" also available in the slot that contains the server's
> ECC private key?

In fortezza, Ks is part of the token. It's use is available to all processes
which have authenticated to that token. We can't just 'generate' a random key
because what we are really doing here is key exchanging the wrapping key with
the other server processes that may be sharing the work of handling the
connections. The presumption is that the only thing we can count on between
these machines is they are sharing the private key as well (or in the fortezza
case, the same or identically configured token). As I alluded to, you could
generate a fixed key using a random value (which you publish with the key), and
the private EEC key. The problem is 1) I'm not sure you have access to the bits
of the EEC private key, and 2) I'm not comfortable that we aren't providing some
possible attack on the EEC private key if we do this.

> In getWrappingKey, how should the data and len fields of wrappedKey be set?
> Currently, for ECC, this field is set to the length of the ECC key in bytes.
> If I wanted to reuse the code in case kt_fortezza for kt_ecdh as well, how
> should I set these fields?

WrappedKey is a blob of data that will be decoded in the sslUnwrapSymWrappingKey
call of another server process. The only shared data between those 2 processes
is the private key. It's up to you how you package the required data, just as
long as you are consistant in sslUnwrapSymWrappingkey. The blob is key exchange
specific, no one will try to access the kt_ecdh blob fro kt_fortezza or vice versa.


> > NOTE: make sure the wrapped key space we allocated above is the correct
> > size (it might be that that allocation may have to be moved into the switch
> > statement itself now).

> This part wasn't clear to me. Could you elaborate some more? What does it
> mean to store wrapped key with the generated ECDH public key? Is this the
> ephemeral ECDH public key? If the corresponding private key has been
> discarded, how does one regenerate the ECDH equivalent of Ks (assuming it
> was computed by doing a "self" ECDH exch)?

I presented the most conservative protocol I could think of, there are likely
other ways of doing this.

So the key was generated by taking the server's private key (pk) and doing an
ECC derive with our generated ECDH public key. This key is used to wrap our
wrapping key. Other server processes now need to regenerate the key. They have
the server's private key (pk), so all they need is the randomly generated ECDH
public key that the first server used to wrap the server wrapping key. The
generated private key is not needed. I specified a full key gen because I do not
know the characteristics of ECC if you just used random data for the public key.
Again someone very familiar with the ECC could decide the latter is safe and
skip the full key pair gen.

Another option is to always key exchange with our own public key, but I'm not
sure if that would yeild a predictable (or nearly predictable) key in ECDH. I
know the government KEA scheme never uses direct DH public and private keys
without a random key pair thrown in, and I presume there's an attack they are
preventing.

bob

Comment 13

[Vipul Gupta]

Attachment: First attempt at creating RSA-independent patch for ECC session reuse

Hi Bob,

  Thanks for explaining the key wrapping functionality in NSS over the
phone. Here is a first attempt at creating a patch along the lines
you described in Comment #10. The patch isn't working yet (I'll 
start debugging this on Monday) but I thought I should get your 
feedback on the general approach before I spend more time on it.
I'm not sure that all of the arguments to the two calls to 
PK11_PubDeriveWithKDF are correct. For example, I suspect I 
shouldn't be using CKM_SKIPJACK_CBC64 and perhaps even 
CKA_DERIVE is incorrect. I'd appreciate any feedback you 
might have.

thank you,

vipul

Comment 14

[Vipul Gupta]

Attachment: Working RSA-independent patch to enable session reuse with ECC ciphers

I've created a new patch to enable session reuse with ECC cipher suites
(it does not require the server to have an RSA cert/privkey).

I had to use trial-and-error to arrive at the CKM_RC2_ECB wrapping
mechanism. I also tried CKM_DES_CBC and CKM_DES_ECB but wasn't
successful -- Ks came out to be NULL when I passed either one of
these as an argument to PK11_PubDeriveWithKDF.

I'd feel a lot better if someone more familiar with the
NSS code can help me understand why the other mechanisms
don't work and also make sure that CKM_RC2_ECB is
acceptable for wrapping the key used to wrap the 
master secret.

thanks,

vipul

Comment 15

[Vipul Gupta]

Attachment: Working RSA-independent patch to enable session reuse with ECC ciphers (includes test)

This patch includes changes to ecsslstress.txt for testing the new code.

vipul

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

This work is on hold until such time as the patent issues are resolved.

Comment 17

[Wan-Teh Chang]

Attachment: Working RSA-independent patch to enable session reuse with ECC ciphers (includes test) v1.1

This is Vipul's patch (attachment 164451 [details] [diff] [review]) updated for the
current NSS tip (NSS 3.11 pre-release).

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 198863 [details] [diff] [review]
Working RSA-independent patch to enable session reuse with ECC ciphers (includes test) v1.1

r- 
I have emailed my review comments to Vipul and Wan-Teh.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

ReTargetting at 3.12

Comment 20

[Vipul Gupta]

Attachment: ECC session reuse patch (v1.2) with tests

This new patch addresses Nelson's review comments.
Nelson, please take a careful look at the code that
deals with assertion failures. I'm not sure if it needs
to do anything more than "go to loser". These parts
of the patch are marked with "XXX ???"

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 210529 [details] [diff] [review]
ECC session reuse patch (v1.2) with tests

Vipul, as I begin this review, a few questions come to mind, including
a) RC2 ???    :-o
   If we're going to claim Suite B compliance, don't we need to use AES?
b) Has this been tested in a multi-process server, e.g. selfserv -M maxProcs ?
   I wonder if we should add a multi-process test case to ssl.sh.

Comment 22

[Vipul Gupta]

(In reply to comment #21)
> (From update of attachment 210529 [details] [diff] [review] [edit])
> Vipul, as I begin this review, a few questions come to mind, including
> a) RC2 ???    :-o
>    If we're going to claim Suite B compliance, don't we need to use AES?

As mentioned in comment 14, I arrived at this mechanism by trial
and error (tried a few other options but encountered problems) and
am not very comfortable with the choice. I'd really like someone 
with more thorough knowledge of NSS (such as yourself or Bob 
Relyea) to look at this and suggest what I should be trying.

> b) Has this been tested in a multi-process server, e.g. selfserv -M maxProcs ?
>    I wonder if we should add a multi-process test case to ssl.sh.

I've only run the regular ./all.sh tests. If you tell me what else to try
I'll give it a shot (please include as much detail as you can since I'm
not familar with all of the options to selfserv).

vipul

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 210529 [details] [diff] [review]
ECC session reuse patch (v1.2) with tests

This patch may be very close to the final result, but presently it 
uses RC2 for encrypting the cache contents, and it is untested in a 
multi-process server.  I can't give it r+ until those are resolved.
If you run into specific errors while trying to switch from rc2 to AES,
I will help you with them.

Now, here are some specific comments about this patch.

>Index: mozilla/security/nss/lib/ssl/ssl3con.c

>+#ifdef NSS_ENABLE_ECC
>+/* The ECCWrappedKeyInfo structure defines how various pieces of 
>+ * information are laid out within wrappedSymmetricWrappingkey 
>+ * for ECDH key exchange. Since wrappedSymmetricWrappingkey is 
>+ * a 512-byte buffer (see sslimpl.h), the variable length field 
>+ * in ECCWrappedKeyInfo can be at most (512 - 8) = 504 bytes.

Where does that 8 come from?  


>+        if (ecWrapped->encodedParamLen + ecWrapped->pubValueLen + 
>+          ecWrapped->wrappedKeyLen > MAX_EC_WRAPPED_KEY_BUFLEN) {
>+            /* XXXX ??? */

              PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
>+            goto loser;
>+        }

>+        /* Derive Ks using ECDH */
>+        Ks = PK11_PubDeriveWithKDF(svrPrivKey, &pubWrapKey, PR_FALSE, NULL,
>+          NULL, CKM_ECDH1_DERIVE, CKM_RC2_ECB, CKA_DERIVE, 0, CKD_NULL, 
>+          NULL, NULL);
>+        if (Ks == NULL) {
>+            SET_ERROR_CODE  
                  no, error code should have been set by PK11_PubDeriveWithKDF
>+            goto loser;
>+        }


>+        PORT_Assert(svrPubKey->keyType == ecKey);
>+        if (svrPubKey->keyType != ecKey) {
>+            /* XXXX ??? */
              PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
>+            goto loser;
>+        }

Can the above error happen due to misconfiguration of the server?
Or does it always indicate an NSS programming error?  

>+        privWrapKey = SECKEY_CreateECPrivateKey(
>+            &svrPubKey->u.ec.DEREncodedParams, &pubWrapKey, NULL);
>+        if ((privWrapKey == NULL) || (pubWrapKey == NULL)) {
>+            SET_ERROR_CODE  /* XXXX ??? */
       error code should have been set by SECKEY_CreateECPrivateKey.
>+            goto loser;

If one of these is NULL and the other is not, will loser destroy the 
non-NULL one?

>+        }
>+        
>+        /* Set the key size in bits */
>+        if (pubWrapKey->u.ec.size == 0) {
>+            pubWrapKey->u.ec.size = SECKEY_PublicKeyStrengthInBits(svrPubKey);

What value does SECKEY_PublicKeyStrengthInBits return on failure?
Shouldn't we detect it here?

>+        }
>+
>+        PORT_Assert(pubWrapKey->u.ec.DEREncodedParams.len + 
>+          pubWrapKey->u.ec.publicValue.len < MAX_EC_WRAPPED_KEY_BUFLEN);
>+        if (pubWrapKey->u.ec.DEREncodedParams.len + 
>+          pubWrapKey->u.ec.publicValue.len >= MAX_EC_WRAPPED_KEY_BUFLEN) {
>+            /* XXX ??? */
              PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
>+            goto loser;
>+        }

Can the above error happen due to misconfiguration of the server?
Or does it always indicate an NSS programming error?  
The error code I suggest above assumes it always indicates NSS programming error.

>+
>+        /* Derive Ks using ECDH */
>+        Ks = PK11_PubDeriveWithKDF(svrPrivKey, pubWrapKey, PR_FALSE, NULL,
>+          NULL, CKM_ECDH1_DERIVE, CKM_RC2_ECB, CKA_DERIVE, 0, CKD_NULL, 
>+          NULL, NULL);
>+        if (Ks == NULL) {
>+            SET_ERROR_CODE
       error code should have been set by PK11_PubDeriveWithKDF
>+            goto loser;
>+        }

>+        wrappedKey.len = MAX_EC_WRAPPED_KEY_BUFLEN - 
>+          (ecWrapped->encodedParamLen + ecWrapped->pubValueLen);

should we test this to see if it is negative?  

>+        wrappedKey.data = ecWrapped->var + ecWrapped->encodedParamLen +
>+          ecWrapped->pubValueLen;
>+
>+        /* wrap symmetricWrapping key with the local Ks */
>+        rv = PK11_WrapSymKey(CKM_RC2_ECB, NULL, Ks,
>+          unwrappedWrappingKey, &wrappedKey);
>+
>+        /* Write down the length of wrapped key in the buffer
>+         * wswk.wrappedSymmetricWrappingkey at the appropriate offset
>+         *
>+         * XXX What happens in PK11_WrapSymKey if the wrappedKey.len
>+         * isn't big enough ???

Then it fails (returns SECFailure), which this code seems not to notice.
In that case, the value of wrappedKey.len MAY (should) have been altered 
to show the required size, and so will be larger than the input value.

>+         */
>+        ecWrapped->wrappedKeyLen = wrappedKey.len;
>+
>+        if (privWrapKey) SECKEY_DestroyPrivateKey(privWrapKey);
>+        if (pubWrapKey) SECKEY_DestroyPublicKey(pubWrapKey);
>+        PK11_FreeSymKey(Ks);
>+        asymWrapMechanism = CKM_RC2_ECB;
>+        break;

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Assigning to Vipul, who is working on it (IINM). 
Vipul, let me know if you're not.
IMO this is the highest priority RFE needed for inclusion in servers.

Comment 25

[Julien Pierre]

Reassigning to Bob per our meeting.

Comment 26

[Robert Relyea]

Attachment: ECC session reuse v1.3

The changes to this patch over the previous patch:
  1) use the selected 'best mechanism' to do all the wrapping.
  2) fix bug in ECC derive so that it will derive keys for various fixed key length algorithms.
  3) added a new error code for a corrupted SESSION cache.

Not included in the patch: Vipul's tests. New strings in cmd/lib/sslerr for the SSL error code.

Comment 27

[Robert Relyea]

Arg!!! The patch also has my changes from bug 326482 (to many patches floating around in my tree again;(.

The 326482 patch is to use the public key value from the key pair. It countains the latest patch I intended for review anyway (sigh).

bob

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213618 [details] [diff] [review]
ECC session reuse v1.3

Please consider these to be preliminary review comments.
More review comments to come.

>Index: softoken/pkcs11c.c

It's unclear to me what the changes to pkcs11c.c have to do with
this bug (bug 238051).  Are they somehow related to the new calls
to PK11_PubDeriveWithKDF ? If these changes *are* relevant to this
bug, please add comments in the newly added code in this file,
explaining what these changes do and what problem they solve that
is relevant to this bug.

>Index: ssl/ssl3con.c

>@@ -3565,6 +3594,60 @@

>+        PORT_Assert(ecWrapped->encodedParamLen + ecWrapped->pubValueLen + 
>+          ecWrapped->wrappedKeyLen <= MAX_EC_WRAPPED_KEY_BUFLEN);
>+
>+        if (ecWrapped->encodedParamLen + ecWrapped->pubValueLen + 
>+          ecWrapped->wrappedKeyLen > MAX_EC_WRAPPED_KEY_BUFLEN) {
>+            PORT_SetError(SSL_ERROR_SESSION_CACHE_CORRUPTED);
>+            goto loser;
>+        }

In the above code, we're taking a key out of the cache.  If the length
is invalid, it means NSS made a grave error when it created this cache
entry (See that code below).  
So, I'd say that SEC_ERROR_LIBRARY_FAILURE is appropriate here.


>@@ -3700,12 +3782,12 @@
>      */
>     PORT_Memset(&wswk, 0, sizeof wswk);	/* eliminate UMRs. */
> 
>-    svrCert         = ss->serverCerts[exchKeyType].serverCert;
>-    svrPubKey       = CERT_ExtractPublicKey(svrCert);
>+    if (ss->serverCerts[exchKeyType].serverKeyPair) {
>+	svrPubKey = ss->serverCerts[exchKeyType].serverKeyPair->pubKey;
>+    }
>     if (svrPubKey == NULL) {
>-	/* CERT_ExtractPublicKey doesn't set error code */
> 	PORT_SetError(SSL_ERROR_EXTRACT_PUBLIC_KEY_FAILURE);

IMO, this is now the wrong error code to be using here, since the
code is no longer performing a key extraction here.  Maybe this is
another case for SEC_ERROR_LIBRARY_FAILURE.

>@@ -3724,6 +3812,93 @@
>+         * described in the ECCWrappedKeyInfo structure.
>+         */
>+        PORT_Assert(svrPubKey->keyType == ecKey);
>+        if (svrPubKey->keyType != ecKey) {
>+            /* XXXX ??? */

Yes, Call PORT_SetError here.

>+	    rv = SECFailure;
>+            goto ec_cleanup;
>+        }
>+
>+        privWrapKey = SECKEY_CreateECPrivateKey(
>+            &svrPubKey->u.ec.DEREncodedParams, &pubWrapKey, NULL);
>+        if ((privWrapKey == NULL) || (pubWrapKey == NULL)) {
>+	    rv = SECFailure;
>+            goto ec_cleanup;
>+        }
>+        
>+        /* Set the key size in bits */
>+        if (pubWrapKey->u.ec.size == 0) {
>+            pubWrapKey->u.ec.size = SECKEY_PublicKeyStrengthInBits(svrPubKey);
>+        }
>+
>+        PORT_Assert(pubWrapKey->u.ec.DEREncodedParams.len + 
>+          pubWrapKey->u.ec.publicValue.len < MAX_EC_WRAPPED_KEY_BUFLEN);
>+        if (pubWrapKey->u.ec.DEREncodedParams.len + 
>+          pubWrapKey->u.ec.publicValue.len >= MAX_EC_WRAPPED_KEY_BUFLEN) {
>+            /* XXX ??? */

Definitely need to call PORT_SetError here.
This is the code that creates the cache entry.  If a new error code is 
to be created, it should be added here, and should not complain of 
"corrupted" but rather of key too large for cache entry, or some such.

>+	    rv = SECFailure;
>+            goto ec_cleanup;

Your new code uses an odd mixture of spaces and tabs.  
For example, the rv = line above uses a tab, the goto uses 8 spaces.
This causes lots of apparent indentation strangeness.

>+        if (Ks == NULL) {
>+	    rv = SECFailure;
>+            goto ec_cleanup;
>+        }

Indentation strangemess.
>+        /* wrap symmetricWrapping key with the local Ks */
>+        rv = PK11_WrapSymKey(masterWrapMech, NULL, Ks,
>+          unwrappedWrappingKey, &wrappedKey);
>+
>+	if (rv != SECSuccess) {
>+	    goto ec_cleanup;
>+	}
>+
>+        /* Write down the length of wrapped key in the buffer
>+         * wswk.wrappedSymmetricWrappingkey at the appropriate offset
>+         */
>+	ecWrapped->wrappedKeyLen = wrappedKey.len;

Lots of indentation strangeness.

>@@ -6141,8 +6312,9 @@
> ssl3_HandleClientKeyExchange(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
> {
>     SECKEYPrivateKey *serverKey         = NULL;
>-    SECStatus         rv;
>+    SECStatus rv;

Please don't make that change (removing white space).

>@@ -6161,6 +6333,16 @@
> 
>     kea_def   = ss->ssl3.hs.kea_def;
> 
>+    if (ss->ssl3.hs.usedStepDownKey
>+#ifdef DEBUG
>+	 && kea_def->is_limited /* XXX OR cert is signing only */
>+	 && kea_def->exchKeyType == kt_rsa 
>+	 && ss->stepDownKeyPair != NULL
>+#endif
>+	 ) { 

The above ifdef seems undesirable.  It causes debug and optimized 
code to behave differently, taking different paths of execution 
for the same input values.  I think this code in the #ifdef 
wants to be an assert, maybe.

Comment 29

[Robert Relyea]

>>Index: softoken/pkcs11c.c

> It's unclear to me what the changes to pkcs11c.c have to do with
> this bug (bug 238051). 

pkcs11c.c changes are required so the session reuse can generate keys for specific target algorithms. The old code simply generated a key of some 'natural' length based on the ecc ciphers. Most target algorithms need keys of a specific size. This is why gupta could only get rc5 to work. The code constrains the size of the generated key to the 'keySize' variable, which is set whenever a specific size key is needed.

@@ -6161,6 +6333,16 @@
>> 
>>     kea_def   = ss->ssl3.hs.kea_def;
>> 
>>+    if (ss->ssl3.hs.usedStepDownKey
>>+#ifdef DEBUG
>>+	 && kea_def->is_limited /* XXX OR cert is signing only */
>>+	 && kea_def->exchKeyType == kt_rsa 
>>+	 && ss->stepDownKeyPair != NULL
>>+#endif
>>+	 ) { 

>The above ifdef seems undesirable.  It causes debug and optimized 
>code to behave differently, taking different paths of execution 
>for the same input values.  I think this code in the #ifdef 
>wants to be an assert, maybe.

The above ifdef preserves existing behavior. You'll see this exact ifdef in the previous code in a couple of places (I've colapsed those cases). I don't mind changing it, just let me know if they should always be defined are always undefined.

This particular patch is really part of the bug 326482 patch to use the public key value from the key pair.

Comment 30

[Robert Relyea]

review of the pck11c.c portion of this patch is blocking work on 309701.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213618 [details] [diff] [review]
ECC session reuse v1.3

(In reply to comment #30)
> review of the pck11c.c portion of this patch is blocking work on 309701.

You apparently mean attachment 213618 [details] [diff] [review].  In Comment 28, I wrote (about the
pkcs11c.c portion of that attachment):

> please add comments in the newly added code in this file,
> explaining what these changes do and what problem they solve that
> is relevant to this bug.

A code reviewer (or maintainer) should only have to answre the question:
"Does this code do what it was definedd/intended to do?", not 
"What the heck is this code doing, and 
 is that what the author really intended?"

I gather that the new code in pkcs11c.c is constructing a long key from a 
shorter one, by right-justifying the shorter data in a larger zero'ed buffer,
as if making a larger integer out of a shorter one on a big endian system.
Is that what was intended?  A comment should tell me that, and why it is
desirable.

Since the PKCS11c.c patch is aparently separable from the rest, go ahead
and add a new patch that clarifies the patch to that file.  That can then
be reviewed separately.
I'll try to finish the review for the rest of attachment 213618 [details] [diff] [review] today.

Comment 32

[Robert Relyea]

Attachment: ECC session reuse v1.4a pkcs11c.c review changes.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 215332 [details] [diff] [review]
ECC session reuse v1.4a pkcs11c.c review changes.

  12345678901234567890123456789012345678901234567890123456789012345678901234567890
>+	/*
>+	 * if keySize is supplied, then we are generating a key of a specific length.
>+	 * This is done by taking the least significant 'keySize' bytes from the unsigned
>+	 * value calculated by ECDH. Note: this may mean padding temp with extra leading
>+	 * zeros from what ECDH_Derive already returned (which itself may contain leading
>+	 * zeros).

As shown by the ruler above, these comment lines exceed 80 characters.
Fix that, then r=nelson

Comment 34

[Robert Relyea]

pkcs11c.c checked into tip:
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.77; previous revision: 1.76
done

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213618 [details] [diff] [review]
ECC session reuse v1.3

My primary review comments are in comment 28.
Please address all those comments in a new patch.
The only additional comment at this time (an extension of an
observation in comment 28) concerns this snippet:

>@@ -6161,6 +6333,16 @@
> 
>     kea_def   = ss->ssl3.hs.kea_def;
> 
>+    if (ss->ssl3.hs.usedStepDownKey
>+#ifdef DEBUG
>+	 && kea_def->is_limited /* XXX OR cert is signing only */
>+	 && kea_def->exchKeyType == kt_rsa 
>+	 && ss->stepDownKeyPair != NULL
>+#endif
>+	 ) { 
>+    	serverKeyPair = ss->stepDownKeyPair;
>+	ss->sec.keaKeyBits = EXPORT_RSA_KEY_LENGTH * BPB;
>+    } else 
> #ifdef NSS_ENABLE_ECC

That #ifdef DEBUG code really should be an assertion, 
something like this (with better indentation :)

>+    if (ss->ssl3.hs.usedStepDownKey) { 
>+       PORT_Assert(kea_def->is_limited /* OR cert is signing only */
>+	          && kea_def->exchKeyType == kt_rsa 
>+	          && ss->stepDownKeyPair != NULL);
>+       if (!kea_def->is_limited ||
>+           kea_def->exchKeyType != kt_rsa ||
>+           !ss->stepDownKeyPair) 
>+             goto oops;
>+    	serverKeyPair = ss->stepDownKeyPair;
>+	ss->sec.keaKeyBits = EXPORT_RSA_KEY_LENGTH * BPB;
>+    } else 
>+ oops:

Comment 36

[Robert Relyea]

Attachment: ECC session reuse v1.4b ssl3con.c review changes.

This one should do it.

Comment 37

[Thomas Kwan]

Comment on attachment 215332 [details] [diff] [review]
ECC session reuse v1.4a pkcs11c.c review changes.

r=nkwan

Comment 38

[Thomas Kwan]

Comment on attachment 215460 [details] [diff] [review]
ECC session reuse v1.4b ssl3con.c review changes.

r=nkwan

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 215460 [details] [diff] [review]
ECC session reuse v1.4b ssl3con.c review changes.

There are some minor indentation nits.  
You can fix them or not as you wish.
Lots of new code seems to use indentation of 2 spaces, not 4.

>+        wrappedKey.len  = ecWrapped->wrappedKeyLen;
>+        wrappedKey.data = ecWrapped->var + ecWrapped->encodedParamLen + 
>+          ecWrapped->pubValueLen;
          ^^  only indented by 2.

Also, when calling a function with lots of args, we usually 
force the args to wrap around at the point where the ( separates
the function name from the args, e.g. 

>+        /* Derive Ks using ECDH */
>+        Ks = PK11_PubDeriveWithKDF(svrPrivKey, &pubWrapKey, PR_FALSE, NULL,
>+          NULL, CKM_ECDH1_DERIVE, masterWrapMech, CKA_DERIVE, 0, CKD_NULL, 
>+          NULL, NULL);
>+        if (Ks == NULL) {
>+            goto loser;
>+        }

Maybe should be:
>+        Ks = PK11_PubDeriveWithKDF(svrPrivKey, &pubWrapKey, PR_FALSE, NULL,
>+                                   NULL, CKM_ECDH1_DERIVE, masterWrapMech, 
>+                                   CKA_DERIVE, 0, CKD_NULL, NULL, NULL);

Comment 40

[Robert Relyea]

checked into tip:

Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.46; previous revision: 1.45
done
Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.83; previous revision: 1.82
done


checked into 3.11 branch:
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.68.2.6; previous revision: 1.68.2.5
done
Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.76.2.6; previous revision: 1.76.2.5
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.42.2.3; previous revision: 1.42.2.2
done

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

For some (unknown) reason, there are lots of differences between the 
version of the patch that was checked in for the trunk and the version
checked in for the branch.  Consequently, since this checking, patches 
that apply cleanly to the branch don't to the trunk and vice versa.

So, I'm going to attach a patch to this bug that will cause the branch
to match the trunk with respect to these changes.  

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

Attachment: eliminate unnecessary diffs between trunk and branch (checked in)

The purpose of this patch is to eliminate unnecessary diffs between the 
branch and the trunk by bringing the branch to match the trunk.
These changes should be whitespace only (indentation, line wrapping, etc.)

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 217306 [details] [diff] [review]
eliminate unnecessary diffs between trunk and branch (checked in)

Checked in on branch

Checking in ssl3con.c;  new revision: 1.76.2.7; previous revision: 1.76.2.6