1028582 - Allow server side to generate a fresh ECDHE key for each handshake

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Eric Rescorla (:ekr)]

Currently, the server side of libssl generates a single ECDHE for each curve and then reuses it for the process lifetime. This is useful for performance for servers applications but less so for P2P applications like WebRTC. We should add an option to generate a fresh key for each transaction.

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Unit test for ECDHE reuse.

Comment 2

[Eric Rescorla (:ekr)]

Attachment: Add option to NSS to not reuse ECDHE keys in server mode.

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 8443987 [details] [diff] [review]
Add option to NSS to not reuse ECDHE keys in server mode.

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

::: security/nss/lib/ssl/ssl3ecc.c
@@ +779,2 @@
>      if (rv != SECSuccess) {
> +        goto loser;

Note: this comment seemed wrong so I removed it.

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 8443986 [details] [diff] [review]
Unit test for ECDHE reuse.

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

::: media/mtransport/test/transport_unittests.cpp
@@ +177,5 @@
> +    consume(1);
> +    return true;
> +  }
> +
> +  template bool Read(uint32_t *val, size_t len) {

This needs a comment indicating that it's for integral types.

@@ +180,5 @@
> +
> +  template bool Read(uint32_t *val, size_t len) {
> +    *val = 0;
> +
> +    for (size_t i=0; i<len; ++i) {

We also need a check for len <= 4.

@@ +374,5 @@
> +      return;
> +    }
> +
> +    if ((fragment_offset != 0) || (fragment_length != length)) {
> +      // This shouldn't happen

Adjust comment to make clear that it only doesn't happen because the tests we are using this for don't fragment.

@@ +383,5 @@
> +    if (!parser.Read(tmp.get(), length)) {
> +      return;
> +    }
> +
> +    buffer_.Assign(tmp.get(), length);

Fix this to use Allocate.

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 8443987 [details] [diff] [review]
Add option to NSS to not reuse ECDHE keys in server mode.

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

First cut at patch. Please double check that PORT_SetError() etc. are handled correctly.

Comment 6

[Eric Rescorla (:ekr)]

Attachment: Unit test for ECDHE reuse

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 8443991 [details] [diff] [review]
Unit test for ECDHE reuse

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

Sorry this is still in mtransport. I am looking at building a standalone NSS unit test suite but I haven't done it yet.

Note also that this test will fail until the patches in bug 996237 land.

Comment 8

[Martin Thomson [:mt:]]

Comment on attachment 8443991 [details] [diff] [review]
Unit test for ECDHE reuse

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

::: media/mtransport/transportlayerdtls.h
@@ +95,5 @@
>    void PacketReceived(TransportLayer* layer, const unsigned char *data,
>                        size_t len);
>  
> +  // For testing use only. Forces a Setup and then returns the fd.
> +  PRFileDesc* internal_fd() { return ssl_fd_.rwget(); }

I hate friend, but this seems like a good case for it.

Comment 9

[Eric Rescorla (:ekr)]

Wan-Teh, here is the bug we discussed today.

Comment 10

[Robert Relyea]

Comment on attachment 8443987 [details] [diff] [review]
Add option to NSS to not reuse ECDHE keys in server mode.

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

I think the default was opposite in what we suggested.

::: security/nss/lib/ssl/sslsock.c
@@ +80,5 @@
>      PR_TRUE,    /* cbcRandomIV        */
>      PR_FALSE,   /* enableOCSPStapling */
>      PR_TRUE,    /* enableNPN          */
> +    PR_FALSE,   /* enableALPN         */
> +    PR_FALSE    /* reuseECDHEServerKey */

Shouldn't this default be TRUE

Comment 11

[Eric Rescorla (:ekr)]

(In reply to Robert Relyea from comment #10)
> Comment on attachment 8443987 [details] [diff] [review]
> Add option to NSS to not reuse ECDHE keys in server mode.
> 
> Review of attachment 8443987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the default was opposite in what we suggested.
> 
> ::: security/nss/lib/ssl/sslsock.c
> @@ +80,5 @@
> >      PR_TRUE,    /* cbcRandomIV        */
> >      PR_FALSE,   /* enableOCSPStapling */
> >      PR_TRUE,    /* enableNPN          */
> > +    PR_FALSE,   /* enableALPN         */
> > +    PR_FALSE    /* reuseECDHEServerKey */
> 
> Shouldn't this default be TRUE

I thought I suggested defaulting to the new behavior,
but I'm happy to have it default to the old behavior.
Would you like me to produce a new patch with this change
for your review?

Comment 12

[Robert Relyea]

I guess we weren't explicit. I'd prefer the default not change from the current behavior. If it does, I would prefer to have an environment variable affect the default.

bob

Comment 13

[Eric Rescorla (:ekr)]

Attachment: Unit test for ECDHE reuse

Comment 14

[Eric Rescorla (:ekr)]

Attachment: Add option to NSS to not reuse ECDHE keys in server mode.

Comment 15

[Eric Rescorla (:ekr)]

Comment on attachment 8443991 [details] [diff] [review]
Unit test for ECDHE reuse

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

Sorry this is still in mtransport. I am looking at building a standalone NSS unit test suite but I haven't done it yet.

Note also that this test will fail until the patches in bug 996237 land.

Comment 16

[Eric Rescorla (:ekr)]

Comment on attachment 8452566 [details] [diff] [review]
Unit test for ECDHE reuse

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

Carry forward r+ from mt

Comment 17

[Robert Relyea]

Comment on attachment 8452567 [details] [diff] [review]
Add option to NSS to not reuse ECDHE keys in server mode.

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

Looks good Eric, Thanks.

Comment 18

[Eric Rescorla (:ekr)]

Comment on attachment 8452567 [details] [diff] [review]
Add option to NSS to not reuse ECDHE keys in server mode.

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

Marking r? to WTC so he can take a look, as I am not an NSS peer.

Comment 19

[Wan-Teh Chang]

Comment on attachment 8452567 [details] [diff] [review]
Add option to NSS to not reuse ECDHE keys in server mode.

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

r=wtc.

Eric, please take a look at my suggested changes and let me
know if you disagree with any of them.

In particular, for the issue marked with "IMPORTANT", please
let me know if I should fix the code or the comment. I think
the code is correct (so we can preserve the current behavior).

::: security/nss/lib/ssl/ssl.h
@@ +184,5 @@
>  
> +/* SSL_REUSE_ECDHE_SERVER_KEY controls whether the ECDHE server key is
> +   reused for multiple handshakes or generated each time.
> +   SSL_REUSE_ECHDE_SERVER_KEY is currently disabled by default.
> +*/

1. IMPORTANT: In sslsock.c you enable this option by default,
contrary to this comment.

2. Format the multiple-line comment block as follows:

/* Line 1
 * Line 2
 * Line 3
 */

or

/*
 * Line 1
 * Line 2
 * Line 3
 */

::: security/nss/lib/ssl/ssl3ecc.c
@@ +511,4 @@
>  {
>      SECKEYPrivateKey *    privKey  = NULL;
>      SECKEYPublicKey *     pubKey   = NULL;
> +

Delete this blank line.

@@ +532,5 @@
> +    }
> +
> +    return SECSuccess;
> +}
> +/* CallOnce function, called once for each named curve. */

Add a blank line between the two functions.

@@ +534,5 @@
> +    return SECSuccess;
> +}
> +/* CallOnce function, called once for each named curve. */
> +static PRStatus
> +ssl3_CreateECDHEphemeralKeyPairCallOnce(void * arg)

Nit: shorten "CallOnce" to "Once".

@@ +772,5 @@
> +
> +    if (ss->opt.reuseECDHEServerKey) {
> +        rv = ssl3_CreateECDHEphemeralKeys(ss, curve);
> +    }
> +    else {

Format this as
    } else {

Comment 20

[Wan-Teh Chang]

Eric: I forgot to say that I will make the changes you agree with.
So you don't need to attach a new patch.

Comment 21

[Eric Rescorla (:ekr)]

Comment on attachment 8452567 [details] [diff] [review]
Add option to NSS to not reuse ECDHE keys in server mode.

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

Thank you for your review. I concur with these changes.

::: security/nss/lib/ssl/ssl.h
@@ +184,5 @@
>  
> +/* SSL_REUSE_ECDHE_SERVER_KEY controls whether the ECDHE server key is
> +   reused for multiple handshakes or generated each time.
> +   SSL_REUSE_ECHDE_SERVER_KEY is currently disabled by default.
> +*/

Yes, I concur with this change. I changed the value but not the comment.

Comment 22

[Wan-Teh Chang]

Attachment: Add option to NSS to not reuse ECDHE keys in server mode, by Eric Rescorla

This is Eric's patch generated from the upstream NSS repository.

Comment 23

[Wan-Teh Chang]

Attachment: Add option to NSS to not reuse ECDHE keys in server mode, v2, by Eric Rescorla

Patch checked in: https://hg.mozilla.org/projects/nss/rev/550c11d54672

Eric: I also renamed the option SSL_REUSE_SERVER_ECDHE_KEY (swapping
"ECDHE" and "SERVER"), so you need to update your code using this
option.

Comment 24

[Eric Rescorla (:ekr)]

Thanks for the check-in. I will fix my unit tests and land them once we
upgrade NSS in m-c.

Comment 25

[Wan-Teh Chang]

Comment on attachment 8457251 [details] [diff] [review]
Add option to NSS to not reuse ECDHE keys in server mode, v2, by Eric Rescorla

The NSS patch has been pushed to mozilla-central as part
of NSS_3_17_BETA1:
https://hg.mozilla.org/mozilla-central/rev/3e672df388ed

Eric, you can check in your unit tests now.

Comment 26

[Eric Rescorla (:ekr)]

thanks

Comment 27

[Wan-Teh Chang]

Attachment: Disable SSL_REUSE_SERVER_ECDHE_KEY by default

I propose that we disable SSL_REUSE_SERVER_ECDHE_KEY by default.
I know this could degrade the performance of NSS-based servers,
but I think "secure by default" is a good security principle.

Adam Langley told me both Apache and nginx disable the reuse of
ephemeral ECDH keys. The following web search results for
"Apache mod_ssl SSL_OP_SINGLE_ECDH_USE" and "nginx SSL_OP_SINGLE_ECDH_USE"
confirmed this statement:

http://mail-archives.apache.org/mod_mbox/httpd-cvs/201309.mbox/[e-mail]%3E

http://forum.nginx.org/read.php?29,234842

Apache Tomcat also sets the OpenSSL SSL_OP_SINGLE_ECDH_USE option:

https://github.com/apache/tomcat-native/commit/d7a6362015b5c8340590da1fe4e7789b3a1cd389

So these are strong evidence that NSS users will also want to
disable ephemeral key reuse, so that should be the default

Comment 28

[Eric Rescorla (:ekr)]

Comment on attachment 8465067 [details] [diff] [review]
Disable SSL_REUSE_SERVER_ECDHE_KEY by default

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

This patch appears to work as advertised.

Wan-Teh, I'm generally persuaded by your arguments, but if Bob
feels really strongly about keeping the current default, I can
live with that as well.

Comment 29

[Wan-Teh Chang]

Attachment: Free the server's ephemeral ECDH key pair as soon as we're finished with it

I verified that on the client side, the ephemeral ECDH key pair is
created and destroyed within the ssl3_SendECDHClientKeyExchange
function. So only the server side needs changing.

Comment 30

[Eric Rescorla (:ekr)]

Comment on attachment 8465081 [details] [diff] [review]
Free the server's ephemeral ECDH key pair as soon as we're finished with it

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

lgtm

Comment 31

[Robert Relyea]

Comment on attachment 8465067 [details] [diff] [review]
Disable SSL_REUSE_SERVER_ECDHE_KEY by default

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

r- The only affect of this patch would be a negative impact on Red Hat Server Performance.

If the current code was actually insecure, I would agree we should flip the default, but there isn't anything inherently insecure about the current code, so then it comes down the the performance issue.

There is a pending patch that allows us to administratively change NSS defaults (as well as lock down admin decided policy) that would make these issues less of a problem since these kinds of defaults could then be handled in the field without changing applications.

I'm willing to change my mind if you have an existing SSL user who can't just use the new preference to change this behavior and prefer's the lower performing, but less risky default pref.

Comment 32

[Robert Relyea]

Comment on attachment 8465081 [details] [diff] [review]
Free the server's ephemeral ECDH key pair as soon as we're finished with it

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

I was worried that we were freeing our cached key, but ssl3 key pairs are ref counted, so this is safe for both modes.

Did we want to do the same for DHE keys as well?

bob

Comment 33

[Eric Rescorla (:ekr)]

It appears that NSS does not currently support DHE as a server:

http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c?from=ssl3con.c&case=true#8877

Comment 34

[Martin Thomson [:mt:]]

It really doesn't support DHE as a server.  I tested exactly that case yesterday and triggered a fatal assert.

Comment 35

[Chris Newman]

The request to support server DHE is bug 102794

Comment 36

[Eric Rescorla (:ekr)]

Attachment: WIP: modify TransportLayerDtls to frob settings + unit test

Comment 37

[Eric Rescorla (:ekr)]

Comment on attachment 8467158 [details] [diff] [review]
WIP: modify TransportLayerDtls to frob settings + unit test

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

::: media/mtransport/test/transport_unittests.cpp
@@ +353,5 @@
> +    }
> +
> +    TlsParser parser(data, len);
> +    unsigned char message_type;
> +    // Read the content type.

s/content type/message type/

Comment 38

[Martin Thomson [:mt:]]

Comment on attachment 8467158 [details] [diff] [review]
WIP: modify TransportLayerDtls to frob settings + unit test

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

::: media/mtransport/test/transport_unittests.cpp
@@ +167,5 @@
>      EXPECT_GE(remaining(), expected); \
>      if (remaining() < expected) return false; \
>    } while(0)
>  
> +class TlsParser{

ws

@@ +182,5 @@
> +    return true;
> +  }
> +
> +  // Read an integral type of specified width.
> +  template bool Read(uint32_t *val, size_t len) {

template?

@@ +195,5 @@
> +      (*val) <<= 8;
> +      if (!Read(&tmp))
> +        return false;
> +
> +      *val += tmp;

*val = ((*val)<<8) + tmp; // too complicated?

Or are you looking to read past the end of the buffer and use the shifted value somewhere?

@@ +721,5 @@
>  
> +  void SetInspector(Inspector* in) {
> +    UniquePtr<Inspector> inspector(in);
> +
> +    lossy_->SetInspector(Move(inspector));

Did you just do this because you want to use UniquePtr?

Comment 39

[Eric Rescorla (:ekr)]

Comment on attachment 8467158 [details] [diff] [review]
WIP: modify TransportLayerDtls to frob settings + unit test

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

::: media/mtransport/test/transport_unittests.cpp
@@ +182,5 @@
> +    return true;
> +  }
> +
> +  // Read an integral type of specified width.
> +  template bool Read(uint32_t *val, size_t len) {

I had the same thought. Unfortunately, one of the things I need to parse is uint24 and there's no standard  uint24_t.

@@ +195,5 @@
> +      (*val) <<= 8;
> +      if (!Read(&tmp))
> +        return false;
> +
> +      *val += tmp;

No, I agree that that would be better.

@@ +721,5 @@
>  
> +  void SetInspector(Inspector* in) {
> +    UniquePtr<Inspector> inspector(in);
> +
> +    lossy_->SetInspector(Move(inspector));

We're already using UniquePtr<T>. This is just to ease the transition

Comment 40

[Eric Rescorla (:ekr)]

Attachment: Modify TransportLayerDtls to force a fresh ECDHE server key.

Comment 41

[Eric Rescorla (:ekr)]

Comment on attachment 8467839 [details] [diff] [review]
Modify TransportLayerDtls to force a fresh ECDHE server key.

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

r=mt

Comment 42

[Eric Rescorla (:ekr)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ecb3e5d5013

Comment 43

[Wan-Teh Chang]

Comment on attachment 8465081 [details] [diff] [review]
Free the server's ephemeral ECDH key pair as soon as we're finished with it

Patch checked in: https://hg.mozilla.org/projects/nss/rev/684747ac0358

(In reply to Robert Relyea from comment #32)
>
> Did we want to do the same for DHE keys as well?

NSS doesn't support DHE on the server side yet. When NSS supports that,
we should do the same for DHE keys as well.

On the client side, I verified that NSS creates and destroys the DHE
keys entirely within the sendDHClientKeyExchange function, so we are
good.

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ecb3e5d5013

Comment 45

[Karl Tomlinson (:karlt)]

Bug 1025729 provides SSL_REUSE_SERVER_ECDHE_KEY.