Bug 1642871 - Allow tickets and PHA after resumption, r=kjacobs

The first part of this is fairly simple: we accidentally disabled sending of
session tickets after resumption.

The second part is much less obvious, because the spec is unclear.  This change
takes the interpretation that it is OK to use post-handshake authentication if
the handshake is resumed, but not OK if the handshake is based on a PSK.  (This
is based on a first-principles understanding of resumption being a continuation
of a certificate-based connection rather than a reading of the spec, see the bug
for why the spec appears to be unhelpful on this point.)

This still prohibits the use of post-handshake authentication if an external PSK
was used, but that is more an abundance of caution than anything principled.

Differential Revision: https://phabricator.services.mozilla.com/D77993

3 files changed, 96 insertions, 7 deletions

diff --git a/gtests/ssl_gtest/ssl_auth_unittest.cc b/gtests/ssl_gtest/ssl_auth_unittest.cc
index c1a810d04..cdd096915 100644
--- a/gtests/ssl_gtest/ssl_auth_unittest.cc
+++ b/gtests/ssl_gtest/ssl_auth_unittest.cc
@@ -214,8 +214,7 @@ TEST_F(TlsConnectStreamTls13, PostHandshakeAuth) {
       MakeTlsFilter<TlsCertificateRequestContextRecorder>(
           client_, kTlsHandshakeCertificate);
   client_->SetupClientAuth();
-  EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(),
-                                      SSL_ENABLE_POST_HANDSHAKE_AUTH, PR_TRUE));
+  client_->SetOption(SSL_ENABLE_POST_HANDSHAKE_AUTH, PR_TRUE);
   size_t called = 0;
   server_->SetAuthCertificateCallback(
       [&called](TlsAgent*, PRBool, PRBool) -> SECStatus {
@@ -253,6 +252,47 @@ TEST_F(TlsConnectStreamTls13, PostHandshakeAuth) {
   EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert));
 }
 
+TEST_F(TlsConnectStreamTls13, PostHandshakeAuthAfterResumption) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  Connect();
+
+  SendReceive();  // Need to read so that we absorb the session tickets.
+  CheckKeys();
+
+  // Resume the connection.
+  Reset();
+
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  ExpectResumption(RESUME_TICKET);
+
+  client_->SetupClientAuth();
+  client_->SetOption(SSL_ENABLE_POST_HANDSHAKE_AUTH, PR_TRUE);
+  Connect();
+  SendReceive();
+
+  size_t called = 0;
+  server_->SetAuthCertificateCallback(
+      [&called](TlsAgent*, PRBool, PRBool) -> SECStatus {
+        called++;
+        return SECSuccess;
+      });
+  EXPECT_EQ(SECSuccess, SSL_SendCertificateRequest(server_->ssl_fd()))
+      << "Unexpected error: " << PORT_ErrorToName(PORT_GetError());
+  server_->SendData(50);
+  client_->ReadBytes(50);
+  client_->SendData(50);
+  server_->ReadBytes(50);
+  EXPECT_EQ(1U, called);
+
+  ScopedCERTCertificate cert1(SSL_PeerCertificate(server_->ssl_fd()));
+  ASSERT_NE(nullptr, cert1.get());
+  ScopedCERTCertificate cert2(SSL_LocalCertificate(client_->ssl_fd()));
+  ASSERT_NE(nullptr, cert2.get());
+  EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert));
+}
+
 static SECStatus GetClientAuthDataHook(void* self, PRFileDesc* fd,
                                        CERTDistNames* caNames,
                                        CERTCertificate** clientCert,
diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc
index a269f0f97..a94fed17f 100644
--- a/gtests/ssl_gtest/ssl_resumption_unittest.cc
+++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc
@@ -915,8 +915,7 @@ TEST_F(TlsConnectTest, SendSessionTicketWithTicketsDisabled) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
 
-  EXPECT_EQ(SECSuccess, SSL_OptionSet(server_->ssl_fd(),
-                                      SSL_ENABLE_SESSION_TICKETS, PR_FALSE));
+  server_->SetOption(SSL_ENABLE_SESSION_TICKETS, PR_FALSE);
 
   auto nst_capture =
       MakeTlsFilter<TlsHandshakeRecorder>(server_, ssl_hs_new_session_ticket);
@@ -944,6 +943,50 @@ TEST_F(TlsConnectTest, SendSessionTicketWithTicketsDisabled) {
   NstTicketMatchesPskIdentity(nst_capture->buffer(), psk_capture->extension());
 }
 
+// Successfully send a session ticket after resuming and then use it.
+TEST_F(TlsConnectTest, SendTicketAfterResumption) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  Connect();
+
+  SendReceive();  // Need to read so that we absorb the session tickets.
+  CheckKeys();
+
+  // Resume the connection.
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  ExpectResumption(RESUME_TICKET);
+
+  // We need to capture just one ticket, so
+  // disable automatic sending of tickets at the server.
+  // ConfigureSessionCache enables this option, so revert that.
+  server_->SetOption(SSL_ENABLE_SESSION_TICKETS, PR_FALSE);
+  auto nst_capture =
+      MakeTlsFilter<TlsHandshakeRecorder>(server_, ssl_hs_new_session_ticket);
+  nst_capture->EnableDecryption();
+  Connect();
+
+  SSLInt_ClearSelfEncryptKey();
+  EXPECT_EQ(SECSuccess, SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0));
+  SendReceive();
+
+  // Reset stats so that the counters for resumptions match up.
+  ClearStats();
+  // Resume again and ensure that we get the same ticket.
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  ExpectResumption(RESUME_TICKET);
+
+  auto psk_capture =
+      MakeTlsFilter<TlsExtensionCapture>(client_, ssl_tls13_pre_shared_key_xtn);
+  Connect();
+  SendReceive();
+
+  NstTicketMatchesPskIdentity(nst_capture->buffer(), psk_capture->extension());
+}
+
 // Test calling SSL_SendSessionTicket in inappropriate conditions.
 TEST_F(TlsConnectTest, SendSessionTicketInappropriate) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index fcee5a4fc..50102d493 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -900,7 +900,8 @@ SSLExp_SendCertificateRequest(PRFileDesc *fd)
         return SECFailure;
     }
 
-    if (ss->ssl3.hs.kea_def->authKeyType == ssl_auth_psk) {
+    /* Disallow a CertificateRequest if this connection uses an external PSK. */
+    if (ss->sec.authType == ssl_auth_psk) {
         PORT_SetError(SSL_ERROR_FEATURE_DISABLED);
         return SECFailure;
     }
@@ -2562,7 +2563,7 @@ tls13_HandleCertificateRequest(sslSocket *ss, PRUint8 *b, PRUint32 length)
     }
 
     /*  MUST NOT combine external PSKs with certificate authentication. */
-    if (ss->ssl3.hs.kea_def->authKeyType == ssl_auth_psk) {
+    if (ss->sec.authType == ssl_auth_psk) {
         FATAL_ERROR(ss, SSL_ERROR_RX_UNEXPECTED_CERT_REQUEST, unexpected_message);
         return SECFailure;
     }
@@ -2682,6 +2683,8 @@ tls13_HandleCertificateRequest(sslSocket *ss, PRUint8 *b, PRUint32 length)
 PRBool
 tls13_ShouldRequestClientAuth(sslSocket *ss)
 {
+    /* Even if we are configured to request a certificate, we can't
+     * if this handshake used a PSK, even when we are resuming. */
     return ss->opt.requestCertificate &&
            ss->ssl3.hs.kea_def->authKeyType != ssl_auth_psk;
 }
@@ -5206,7 +5209,10 @@ SSLExp_SendSessionTicket(PRFileDesc *fd, const PRUint8 *token,
         return SECFailure;
     }
 
-    if (ss->ssl3.hs.kea_def->authKeyType == ssl_auth_psk) {
+    /* Disable tickets if we can trace this connection back to a PSK.
+     * We aren't able to issue tickets (currently) without a certificate.
+     * As PSK =~ resumption, there is no reason to do this. */
+    if (ss->sec.authType == ssl_auth_psk) {
         PORT_SetError(SSL_ERROR_FEATURE_DISABLED);
         return SECFailure;
     }