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

RESOLVED FIXED in 3.17

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ekr, Assigned: ekr)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 5 obsolete attachments)

13.33 KB, patch
ekr
: review+
Details | Diff | Splinter Review
9.46 KB, patch
Details | Diff | Splinter Review
1.63 KB, patch
ekr
: review+
rrelyea
: superreview-
Details | Diff | Splinter Review
692 bytes, patch
ekr
: review+
rrelyea
: superreview+
Details | Diff | Splinter Review
13.87 KB, patch
Details | Diff | Splinter Review
13.88 KB, patch
ekr
: review+
Details | Diff | Splinter Review
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.
Posted patch Unit test for ECDHE reuse. (obsolete) — Splinter Review
Assignee: nobody → ekr
Status: NEW → ASSIGNED
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 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 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.
Attachment #8443987 - Flags: review?(wtc)
Attachment #8443987 - Flags: review?(rrelyea)
Posted patch Unit test for ECDHE reuse (obsolete) — Splinter Review
Attachment #8443986 - Attachment is obsolete: true
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.
Attachment #8443991 - Flags: review?(martin.thomson)
Attachment #8443991 - Flags: feedback?(wtc)
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.
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: --- → 3.16.3
Attachment #8443991 - Flags: review?(martin.thomson) → review+
Target Milestone: 3.16.3 → 3.17
Wan-Teh, here is the bug we discussed today.
Flags: needinfo?(wtc)
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
Attachment #8443987 - Flags: review?(rrelyea) → review-
(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?
Flags: needinfo?(rrelyea)
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
Flags: needinfo?(rrelyea)
Attachment #8443987 - Flags: review?(wtc)
Attachment #8443987 - Attachment is obsolete: true
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.
Attachment #8443991 - Flags: feedback?(wtc)
Attachment #8443991 - Attachment is obsolete: true
Comment on attachment 8452566 [details] [diff] [review]
Unit test for ECDHE reuse

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

Carry forward r+ from mt
Attachment #8452566 - Flags: review+
Attachment #8452567 - Flags: review?(rrelyea)
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.
Attachment #8452567 - Flags: review?(rrelyea) → review+
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.
Attachment #8452567 - Flags: review?(wtc)
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 {
Attachment #8452567 - Flags: review?(wtc) → review+
Eric: I forgot to say that I will make the changes you agree with.
So you don't need to attach a new patch.
Flags: needinfo?(wtc)
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.
This is Eric's patch generated from the upstream NSS repository.
Attachment #8452567 - Attachment is obsolete: true
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.
Attachment #8457072 - Attachment is obsolete: true
Attachment #8457251 - Flags: checked-in+
Thanks for the check-in. I will fix my unit tests and land them once we
upgrade NSS in m-c.
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.
Flags: needinfo?(ekr)
thanks
Flags: needinfo?(ekr)
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/%3C20130925125236.C4DE8238899C@eris.apache.org%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
Attachment #8465067 - Flags: superreview?(rrelyea)
Attachment #8465067 - Flags: review?(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.
Attachment #8465067 - Flags: review?(ekr) → review+
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.
Attachment #8465081 - Flags: superreview?(rrelyea)
Attachment #8465081 - Flags: review?(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
Attachment #8465081 - Flags: review?(ekr) → review+
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.
Attachment #8465067 - Flags: superreview?(rrelyea) → superreview-
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
Attachment #8465081 - Flags: superreview?(rrelyea) → superreview+
It really doesn't support DHE as a server.  I tested exactly that case yesterday and triggered a fatal assert.
The request to support server DHE is bug 102794
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/
Attachment #8467158 - Flags: review?(martin.thomson)
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?
Attachment #8467158 - Flags: review?(martin.thomson) → review+
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 on attachment 8467839 [details] [diff] [review]
Modify TransportLayerDtls to force a fresh ECDHE server key.

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

r=mt
Attachment #8467839 - Flags: review+
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.
Attachment #8465081 - Flags: checked-in+
https://hg.mozilla.org/mozilla-central/rev/8ecb3e5d5013
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Bug 1025729 provides SSL_REUSE_SERVER_ECDHE_KEY.
Depends on: 1025729
Depends on: 1053565
You need to log in before you can comment on or make changes to this bug.