Bug 1309054 - Don't attempt resumption if we don't have the right cipher suite, r=ttaubert
authorMartin Thomson <martin.thomson@gmail.com>
Wed, 12 Oct 2016 15:20:04 +1100
changeset 12739 9db514015c763ff47f09581a2f6869917f38d460
parent 12738 235237ca61ed3e6ba24609d78e1e73bc89d184f5
child 12740 e690a15b0f7b9fc09cdd6153a49e28fa214adc7a
push id1681
push usermartin.thomson@gmail.com
push dateWed, 19 Oct 2016 02:06:37 +0000
reviewersttaubert
bugs1309054
Bug 1309054 - Don't attempt resumption if we don't have the right cipher suite, r=ttaubert
external_tests/ssl_gtest/ssl_resumption_unittest.cc
lib/ssl/ssl3con.c
--- a/external_tests/ssl_gtest/ssl_resumption_unittest.cc
+++ b/external_tests/ssl_gtest/ssl_resumption_unittest.cc
@@ -345,18 +345,27 @@ TEST_P(TlsConnectGeneric, TestResumeClie
   Connect();
   SendReceive();
   CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);
 
   Reset();
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   ExpectResumption(RESUME_NONE);
   client_->EnableSingleCipher(ChooseAnotherCipher(version_));
+  uint16_t ticket_extension;
+  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    ticket_extension = ssl_tls13_pre_shared_key_xtn;
+  } else {
+    ticket_extension = ssl_session_ticket_xtn;
+  }
+  auto ticket_capture = new TlsExtensionCapture(ticket_extension);
+  client_->SetPacketFilter(ticket_capture);
   Connect();
   CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);
+  EXPECT_EQ(0U, ticket_capture->extension().len());
 }
 
 // Test that we don't resume when we can't negotiate the same cipher.
 TEST_P(TlsConnectGeneric, TestResumeServerDifferentCipher) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   server_->EnableSingleCipher(ChooseOneCipher(version_));
   Connect();
   SendReceive();  // Need to read so that we absorb the session ticket.
@@ -406,32 +415,34 @@ TEST_P(TlsConnectStream, TestResumptionO
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   server_->EnableSingleCipher(ChooseOneCipher(version_));
   Connect();
   SendReceive();
   CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);
 
   Reset();
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
-  server_->SetPacketFilter(new SelectedCipherSuiteReplacer(ChooseAnotherCipher(version_)));
+  server_->SetPacketFilter(
+      new SelectedCipherSuiteReplacer(ChooseAnotherCipher(version_)));
 
   ConnectExpectFail();
   client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_SERVER_HELLO);
   if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
     // The reason this test is stream only: the server is unable to decrypt
     // the alert that the client sends, see bug 1304603.
     server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
   } else {
     server_->CheckErrorCode(SSL_ERROR_HANDSHAKE_FAILURE_ALERT);
   }
 }
 
 class SelectedVersionReplacer : public TlsHandshakeFilter {
  public:
   SelectedVersionReplacer(uint16_t version) : version_(version) {}
+
  protected:
   PacketFilter::Action FilterHandshake(const HandshakeHeader& header,
                                        const DataBuffer& input,
                                        DataBuffer* output) override {
     if (header.handshake_type() != kTlsHandshakeServerHello) {
       return KEEP;
     }
 
@@ -446,32 +457,32 @@ class SelectedVersionReplacer : public T
 
 // Test how the client handles the case where the server picks a
 // lower version number on resumption.
 TEST_P(TlsConnectGenericPre13, TestResumptionOverrideVersion) {
   uint16_t override_version = 0;
   if (mode_ == STREAM) {
     switch (version_) {
       case SSL_LIBRARY_VERSION_TLS_1_0:
-        return; // Skip the test.
+        return;  // Skip the test.
       case SSL_LIBRARY_VERSION_TLS_1_1:
         override_version = SSL_LIBRARY_VERSION_TLS_1_0;
         break;
       case SSL_LIBRARY_VERSION_TLS_1_2:
         override_version = SSL_LIBRARY_VERSION_TLS_1_1;
         break;
       default:
         ASSERT_TRUE(false) << "unknown version";
     }
   } else {
     if (version_ == SSL_LIBRARY_VERSION_TLS_1_2) {
       override_version = SSL_LIBRARY_VERSION_DTLS_1_0_WIRE;
     } else {
       ASSERT_EQ(SSL_LIBRARY_VERSION_TLS_1_1, version_);
-      return; // Skip the test.
+      return;  // Skip the test.
     }
   }
 
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   // Need to use a cipher that is plausible for the lower version.
   server_->EnableSingleCipher(TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA);
   Connect();
   CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -957,17 +957,17 @@ ssl3_config_match_init(sslSocket *ss)
     }
     return numPresent;
 }
 
 /* Return PR_TRUE if suite is usable.  This if the suite is permitted by policy,
  * enabled, has a certificate (as needed), has a viable key agreement method, is
  * usable with the negotiated TLS version, and is otherwise usable. */
 static PRBool
-config_match(ssl3CipherSuiteCfg *suite, int policy,
+config_match(const ssl3CipherSuiteCfg *suite, int policy,
              const SSLVersionRange *vrange, const sslSocket *ss)
 {
     const ssl3CipherSuiteDef *cipher_def;
     const ssl3KEADef *kea_def;
 
     PORT_Assert(policy != SSL_NOT_ALLOWED);
     if (policy == SSL_NOT_ALLOWED)
         return PR_FALSE;
@@ -4984,16 +4984,22 @@ ssl3_SendClientHello(sslSocket *ss, sslC
     /* These must be reset every handshake. */
     ss->ssl3.hs.sendingSCSV = PR_FALSE;
     ss->ssl3.hs.preliminaryInfo = 0;
     PORT_Assert(IS_DTLS(ss) || type != client_hello_retransmit);
     SECITEM_FreeItem(&ss->ssl3.hs.newSessionTicket.ticket, PR_FALSE);
     ss->ssl3.hs.receivedNewSessionTicket = PR_FALSE;
     PORT_Memset(&ss->xtnData, 0, sizeof(TLSExtensionData));
 
+    /* How many suites does our PKCS11 support (regardless of policy)? */
+    num_suites = ssl3_config_match_init(ss);
+    if (!num_suites) {
+        return SECFailure; /* ssl3_config_match_init has set error code. */
+    }
+
     /*
      * During a renegotiation, ss->clientHelloVersion will be used again to
      * work around a Windows SChannel bug. Ensure that it is still enabled.
      */
     if (ss->firstHsDone) {
         PORT_Assert(type != client_hello_initial);
         if (SSL_ALL_VERSIONS_DISABLED(&ss->vrange)) {
             PORT_SetError(SSL_ERROR_SSL_DISABLED);
@@ -5017,17 +5023,28 @@ ssl3_SendClientHello(sslSocket *ss, sslC
 
     /* We can't resume based on a different token. If the sid exists,
      * make sure the token that holds the master secret still exists ...
      * If we previously did client-auth, make sure that the token that holds
      * the private key still exists, is logged in, hasn't been removed, etc.
      */
     if (sid) {
         PRBool sidOK = PR_TRUE;
-        if (sid->u.ssl3.keys.msIsWrapped) {
+        const ssl3CipherSuiteCfg *suite;
+
+        /* Check that the cipher suite we need is enabled. */
+        suite = ssl_LookupCipherSuiteCfg(sid->u.ssl3.cipherSuite,
+                                         ss->cipherSuites);
+        PORT_Assert(suite);
+        if (!suite || !config_match(suite, ss->ssl3.policy, &ss->vrange, ss)) {
+            sidOK = PR_FALSE;
+        }
+
+        /* Check that we can recover the master secret. */
+        if (sidOK && sid->u.ssl3.keys.msIsWrapped) {
             PK11SlotInfo *slot = NULL;
             if (sid->u.ssl3.masterValid) {
                 slot = SECMOD_LookupSlot(sid->u.ssl3.masterModuleID,
                                          sid->u.ssl3.masterSlotID);
             }
             if (slot == NULL) {
                 sidOK = PR_FALSE;
             } else {
@@ -5143,21 +5160,16 @@ ssl3_SendClientHello(sslSocket *ss, sslC
     }
     ssl_ReleaseSpecWriteLock(ss);
 
     if (ss->sec.ci.sid != NULL) {
         ssl_FreeSID(ss->sec.ci.sid); /* decrement ref count, free if zero */
     }
     ss->sec.ci.sid = sid;
 
-    /* how many suites does our PKCS11 support (regardless of policy)? */
-    num_suites = ssl3_config_match_init(ss);
-    if (!num_suites)
-        return SECFailure; /* ssl3_config_match_init has set error code. */
-
     /* HACK for SCSV in SSL 3.0.  On initial handshake, prepend SCSV,
      * only if TLS is disabled.
      */
     if (!ss->firstHsDone && !isTLS) {
         /* Must set this before calling Hello Extension Senders,
          * to suppress sending of empty RI extension.
          */
         ss->ssl3.hs.sendingSCSV = PR_TRUE;
@@ -8508,17 +8520,16 @@ ssl3_HandleClientHello(sslSocket *ss, SS
     }
 
     /* Free a potentially leftover session ID from a previous handshake. */
     if (ss->sec.ci.sid) {
         ssl_FreeSID(ss->sec.ci.sid);
         ss->sec.ci.sid = NULL;
     }
 
-
     if (sid != NULL) {
         /* We've found a session cache entry for this client.
          * Now, if we're going to require a client-auth cert,
          * and we don't already have this client's cert in the session cache,
          * and this is the first handshake on this connection (not a redo),
          * then drop this old cache entry and start a new session.
          */
         if ((sid->peerCert == NULL) && ss->opt.requestCertificate &&