Bug 1517714 - Properly handle ESNI with HRR, r=mt
authorEkr <ekr@rtfm.com>
Thu, 14 Mar 2019 07:58:21 +1100
changeset 15045 81275ef77c8d26a984733f7a591aa1792eb9d9a2
parent 15044 2292f1b96d97d0a229e1133a601774306965abe7
child 15049 3ed1a9e945feb1c8653a01d2bda6b55cb525bc6f
push id3295
push usermartin.thomson@gmail.com
push dateWed, 13 Mar 2019 21:14:40 +0000
reviewersmt
bugs1517714
Bug 1517714 - Properly handle ESNI with HRR, r=mt
gtests/ssl_gtest/tls_esni_unittest.cc
lib/ssl/ssl3con.c
lib/ssl/tls13con.c
lib/ssl/tls13con.h
lib/ssl/tls13esni.c
--- a/gtests/ssl_gtest/tls_esni_unittest.cc
+++ b/gtests/ssl_gtest/tls_esni_unittest.cc
@@ -300,21 +300,24 @@ TEST_P(TlsConnectTls13, ConnectEsniHrr) 
   EnsureTlsSetup();
   const std::vector<SSLNamedGroup> groups = {ssl_grp_ec_secp384r1};
   server_->ConfigNamedGroups(groups);
   SetupEsni(client_, server_);
   auto hrr_capture = MakeTlsFilter<TlsHandshakeRecorder>(
       server_, kTlsHandshakeHelloRetryRequest);
   auto filter =
       MakeTlsFilter<TlsExtensionCapture>(client_, ssl_server_name_xtn);
-  auto cfilter =
-      MakeTlsFilter<TlsExtensionCapture>(client_, ssl_server_name_xtn);
+  auto filter2 =
+      MakeTlsFilter<TlsExtensionCapture>(client_, ssl_server_name_xtn, true);
+  client_->SetFilter(std::make_shared<ChainedPacketFilter>(
+      ChainedPacketFilterInit({filter, filter2})));
   server_->SetSniCallback(SniCallback);
   Connect();
-  CheckSniExtension(cfilter->extension());
+  CheckSniExtension(filter->extension());
+  CheckSniExtension(filter2->extension());
   EXPECT_NE(0UL, hrr_capture->buffer().len());
 }
 
 TEST_P(TlsConnectTls13, ConnectEsniNoDummy) {
   EnsureTlsSetup();
   ScopedSECKEYPublicKey pub;
   ScopedSECKEYPrivateKey priv;
   DataBuffer record;
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -4978,19 +4978,18 @@ ssl3_SendClientHello(sslSocket *ss, sslC
     /* Generate a new random if this is the first attempt. */
     if (type == client_hello_initial) {
         rv = ssl3_GetNewRandom(ss->ssl3.hs.client_random);
         if (rv != SECSuccess) {
             goto loser; /* err set by GetNewRandom. */
         }
     }
 
-    if (ss->vrange.max >= SSL_LIBRARY_VERSION_TLS_1_3 &&
-        type == client_hello_initial) {
-        rv = tls13_SetupClientHello(ss);
+    if (ss->vrange.max >= SSL_LIBRARY_VERSION_TLS_1_3) {
+        rv = tls13_SetupClientHello(ss, type);
         if (rv != SECSuccess) {
             goto loser;
         }
     }
 
     if (isTLS || (ss->firstHsDone && ss->peerRequestedProtection)) {
         rv = ssl_ConstructExtensions(ss, &extensionBuf, ssl_hs_client_hello);
         if (rv != SECSuccess) {
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -411,38 +411,45 @@ SSL_SendAdditionalKeyShares(PRFileDesc *
 
 /*
  * Generate shares for ECDHE and FFDHE.  This picks the first enabled group of
  * the requisite type and creates a share for that.
  *
  * Called from ssl3_SendClientHello.
  */
 SECStatus
-tls13_SetupClientHello(sslSocket *ss)
+tls13_SetupClientHello(sslSocket *ss, sslClientHelloType chType)
 {
     unsigned int i;
     SSL3Statistics *ssl3stats = SSL_GetStatistics();
     NewSessionTicket *session_ticket = NULL;
     sslSessionID *sid = ss->sec.ci.sid;
     unsigned int numShares = 0;
     SECStatus rv;
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
-    PORT_Assert(PR_CLIST_IS_EMPTY(&ss->ephemeralKeyPairs));
-
-    /* Do encrypted SNI. This may create a key share as a side effect. */
+
+    /* Do encrypted SNI.
+     * Note: this makes a new key even though we don't need one.
+     * Maybe remove this in future for efficiency. */
     rv = tls13_ClientSetupESNI(ss);
     if (rv != SECSuccess) {
         return SECFailure;
     }
 
+    /* Everything below here is only run on the first CH. */
+    if (chType != client_hello_initial) {
+        return SECSuccess;
+    }
+
     /* Select the first enabled group.
      * TODO(ekr@rtfm.com): be smarter about offering the group
      * that the other side negotiated if we are resuming. */
+    PORT_Assert(PR_CLIST_IS_EMPTY(&ss->ephemeralKeyPairs));
     for (i = 0; i < SSL_NAMED_GROUP_COUNT; ++i) {
         if (!ss->namedGroupPreferences[i]) {
             continue;
         }
         rv = tls13_AddKeyShare(ss, ss->namedGroupPreferences[i]);
         if (rv != SECSuccess) {
             return SECFailure;
         }
--- a/lib/ssl/tls13con.h
+++ b/lib/ssl/tls13con.h
@@ -58,17 +58,17 @@ SECStatus tls13_ComputeHash(sslSocket *s
 SECStatus tls13_ComputeHandshakeHashes(sslSocket *ss,
                                        SSL3Hashes *hashes);
 SECStatus tls13_DeriveSecretNullHash(sslSocket *ss, PK11SymKey *key,
                                      const char *label,
                                      unsigned int labelLen,
                                      PK11SymKey **dest);
 void tls13_FatalError(sslSocket *ss, PRErrorCode prError,
                       SSL3AlertDescription desc);
-SECStatus tls13_SetupClientHello(sslSocket *ss);
+SECStatus tls13_SetupClientHello(sslSocket *ss, sslClientHelloType chType);
 SECStatus tls13_MaybeDo0RTTHandshake(sslSocket *ss);
 PRInt32 tls13_LimitEarlyData(sslSocket *ss, SSLContentType type, PRInt32 toSend);
 PRBool tls13_AllowPskCipher(const sslSocket *ss,
                             const ssl3CipherSuiteDef *cipher_def);
 PRBool tls13_PskSuiteEnabled(sslSocket *ss);
 SECStatus tls13_WriteExtensionsWithBinder(sslSocket *ss, sslBuffer *extensions);
 SECStatus tls13_HandleClientHelloPart2(sslSocket *ss,
                                        const SECItem *suites,
--- a/lib/ssl/tls13esni.c
+++ b/lib/ssl/tls13esni.c
@@ -579,16 +579,18 @@ tls13_ClientSetupESNI(sslSocket *ss)
     sslEphemeralKeyPair *keyPair;
     size_t i;
     PRCList *cur;
     SECStatus rv;
     TLS13KeyShareEntry *share;
     const sslNamedGroupDef *group = NULL;
     PRTime now = PR_Now() / PR_USEC_PER_SEC;
 
+    PORT_Assert(!ss->xtnData.esniPrivateKey);
+
     if (!ss->esniKeys) {
         return SECSuccess;
     }
 
     if ((ss->esniKeys->notBefore > now) || (ss->esniKeys->notAfter < now)) {
         return SECSuccess;
     }