Bug 1304604 - Send client-side handshake alerts with handshake keys, …

…r=mt Differential Revision: https://nss-review.dev.mozaws.net/D397 --HG-- branch : NSS_TLS13_DRAFT19_BRANCH extra : amend_source : 7a11ee24cb99e8cc2c4038d16d1e061df984522e
sailfishos-mirror · Aug 9, 2017 · 1d1717d · 1d1717d
1 parent 648d27b
commit 1d1717d
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 36 deletions.
diff --git a/gtests/ssl_gtest/ssl_0rtt_unittest.cc b/gtests/ssl_gtest/ssl_0rtt_unittest.cc
@@ -264,6 +264,14 @@ TEST_P(TlsConnectTls13, TestTls13ZeroRttAlpn) {
   CheckAlpn("a");
 }
 
+// NOTE: In this test and those below, the client always sends
+// post-ServerHello alerts with the handshake keys, even if the server
+// has accepted 0-RTT.  In some cases, as with errors in
+// EncryptedExtensions, the client can't know the server's behavior,
+// and in others it's just simpler.  What the server is expecting
+// depends on whether it accepted 0-RTT or not. Eventually, we may
+// make the server trial decrypt.
+//
 // Have the server negotiate a different ALPN value, and therefore
 // reject 0-RTT.
 TEST_P(TlsConnectTls13, TestTls13ZeroRttAlpnChangeServer) {
@@ -302,12 +310,17 @@ TEST_P(TlsConnectTls13, TestTls13ZeroRttNoAlpnServer) {
     client_->CheckAlpn(SSL_NEXT_PROTO_EARLY_VALUE, "a");
     EXPECT_EQ(SECSuccess, SSLInt_Set0RttAlpn(client_->ssl_fd(), b, sizeof(b)));
     client_->CheckAlpn(SSL_NEXT_PROTO_EARLY_VALUE, "b");
-    ExpectAlert(client_, kTlsAlertIllegalParameter);
+    client_->ExpectSendAlert(kTlsAlertIllegalParameter);
     return true;
   });
-  Handshake();
+  if (variant_ == ssl_variant_stream) {
+    server_->ExpectSendAlert(kTlsAlertBadRecordMac);
+    Handshake();
+    server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
+  } else {
+    client_->Handshake();
+  }
   client_->CheckErrorCode(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID);
-  server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT);
 }
 
 // Set up with no ALPN and then set the client so it thinks it has ALPN.
@@ -322,12 +335,17 @@ TEST_P(TlsConnectTls13, TestTls13ZeroRttNoAlpnClient) {
     PRUint8 b[] = {'b'};
     EXPECT_EQ(SECSuccess, SSLInt_Set0RttAlpn(client_->ssl_fd(), b, 1));
     client_->CheckAlpn(SSL_NEXT_PROTO_EARLY_VALUE, "b");
-    ExpectAlert(client_, kTlsAlertIllegalParameter);
+    client_->ExpectSendAlert(kTlsAlertIllegalParameter);
     return true;
   });
-  Handshake();
+  if (variant_ == ssl_variant_stream) {
+    server_->ExpectSendAlert(kTlsAlertBadRecordMac);
+    Handshake();
+    server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
+  } else {
+    client_->Handshake();
+  }
   client_->CheckErrorCode(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID);
-  server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT);
 }
 
 // Remove the old ALPN value and so the client will not offer early data.

diff --git a/gtests/ssl_gtest/ssl_damage_unittest.cc b/gtests/ssl_gtest/ssl_damage_unittest.cc
@@ -51,16 +51,12 @@ TEST_F(TlsConnectTest, DamageSecretHandleServerFinished) {
                            SSL_LIBRARY_VERSION_TLS_1_3);
   server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
                            SSL_LIBRARY_VERSION_TLS_1_3);
-  client_->ExpectSendAlert(kTlsAlertDecryptError);
-  // The server can't read the client's alert, so it also sends an alert.
-  server_->ExpectSendAlert(kTlsAlertBadRecordMac);
   server_->SetPacketFilter(std::make_shared<AfterRecordN>(
       server_, client_,
       0,  // ServerHello.
       [this]() { SSLInt_DamageServerHsTrafficSecret(client_->ssl_fd()); }));
-  ConnectExpectFail();
+  ConnectExpectAlert(client_, kTlsAlertDecryptError);
   client_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE);
-  server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
 }
 
 TEST_P(TlsConnectGenericPre13, DamageServerSignature) {
@@ -79,15 +75,7 @@ TEST_P(TlsConnectTls13, DamageServerSignature) {
   auto filter =
       std::make_shared<TlsLastByteDamager>(kTlsHandshakeCertificateVerify);
   server_->SetTlsRecordFilter(filter);
-  client_->ExpectSendAlert(kTlsAlertDecryptError);
-  // The server can't read the client's alert, so it also sends an alert.
-  if (variant_ == ssl_variant_stream) {
-    server_->ExpectSendAlert(kTlsAlertBadRecordMac);
-    ConnectExpectFail();
-    server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
-  } else {
-    ConnectExpectFailOneSide(TlsAgent::CLIENT);
-  }
+  ConnectExpectAlert(client_, kTlsAlertDecryptError);
   client_->CheckErrorCode(SEC_ERROR_BAD_SIGNATURE);
 }
 

diff --git a/gtests/ssl_gtest/ssl_extension_unittest.cc b/gtests/ssl_gtest/ssl_extension_unittest.cc
@@ -1031,7 +1031,7 @@ class TlsBogusExtensionTestPre13 : public TlsBogusExtensionTest {
 class TlsBogusExtensionTest13 : public TlsBogusExtensionTest {
  protected:
   void ConnectAndFail(uint8_t message) override {
-    if (message == kTlsHandshakeHelloRetryRequest) {
+    if (message != kTlsHandshakeServerHello) {
       ConnectExpectAlert(client_, kTlsAlertUnsupportedExtension);
       return;
     }
@@ -1074,7 +1074,7 @@ TEST_P(TlsBogusExtensionTest13, AddBogusExtensionCertificate) {
 TEST_P(TlsBogusExtensionTest13, AddBogusExtensionCertificateRequest) {
   server_->RequestClientAuth(false);
   AddFilter(kTlsHandshakeCertificateRequest, 0xff);
-  FailWithAlert(kTlsAlertDecryptError);
+  ConnectExpectAlert(client_, kTlsAlertDecryptError);
   client_->CheckErrorCode(SEC_ERROR_BAD_SIGNATURE);
 }
 

diff --git a/gtests/ssl_gtest/ssl_skip_unittest.cc b/gtests/ssl_gtest/ssl_skip_unittest.cc
@@ -101,19 +101,10 @@ class Tls13SkipTest : public TlsConnectTestBase,
   void ServerSkipTest(std::shared_ptr<TlsRecordFilter> filter, int32_t error) {
     EnsureTlsSetup();
     server_->SetTlsRecordFilter(filter);
-    client_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
-    if (variant_ == ssl_variant_stream) {
-      server_->ExpectSendAlert(kTlsAlertBadRecordMac);
-      ConnectExpectFail();
-    } else {
-      ConnectExpectFailOneSide(TlsAgent::CLIENT);
-    }
+    ExpectAlert(client_, kTlsAlertUnexpectedMessage);
+    ConnectExpectFail();
     client_->CheckErrorCode(error);
-    if (variant_ == ssl_variant_stream) {
-      server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
-    } else {
-      ASSERT_EQ(TlsAgent::STATE_CONNECTING, server_->state());
-    }
+    server_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT);
   }
 
   void ClientSkipTest(std::shared_ptr<TlsRecordFilter> filter, int32_t error) {

diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc
@@ -93,11 +93,11 @@ TlsAgent::~TlsAgent() {
   // Add failures manually, if any, so we don't throw in a destructor.
   if (expected_received_alert_ != kTlsAlertCloseNotify ||
       expected_received_alert_level_ != kTlsAlertWarning) {
-    ADD_FAILURE() << "Wrong expected_received_alert status";
+    ADD_FAILURE() << "Wrong expected_received_alert status: " << role_str();
   }
   if (expected_sent_alert_ != kTlsAlertCloseNotify ||
       expected_sent_alert_level_ != kTlsAlertWarning) {
-    ADD_FAILURE() << "Wrong expected_sent_alert status";
+    ADD_FAILURE() << "Wrong expected_sent_alert status: " << role_str();
   }
 }
 

diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
@@ -3109,6 +3109,15 @@ SSL3_SendAlert(sslSocket *ss, SSL3AlertLevel level, SSL3AlertDescription desc)
             ss->sec.uncache(ss->sec.ci.sid);
         }
     }
+
+    rv = tls13_SetAlertCipherSpec(ss);
+    if (rv != SECSuccess) {
+        if (needHsLock) {
+            ssl_ReleaseSSL3HandshakeLock(ss);
+        }
+        return rv;
+    }
+
     ssl_GetXmitBufLock(ss);
     rv = ssl3_FlushHandshake(ss, ssl_SEND_FLAG_FORCE_INTO_BUFFER);
     if (rv == SECSuccess) {

diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
@@ -2934,6 +2934,40 @@ tls13_SetupPendingCipherSpec(sslSocket *ss)
     return SECSuccess;
 }
 
+/*
+ * Called before sending alerts to set up the right key on the client.
+ * We might encounter errors during the handshake where the current
+ * key is ClearText or EarlyApplicationData. This
+ * function switches to the Handshake key if possible.
+ */
+SECStatus
+tls13_SetAlertCipherSpec(sslSocket *ss)
+{
+    SECStatus rv;
+
+    if (ss->sec.isServer) {
+        return SECSuccess;
+    }
+    if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) {
+        return SECSuccess;
+    }
+    if (TLS13_IN_HS_STATE(ss, wait_server_hello)) {
+        return SECSuccess;
+    }
+    if ((ss->ssl3.cwSpec->epoch != TrafficKeyClearText) &&
+        (ss->ssl3.cwSpec->epoch != TrafficKeyEarlyApplicationData)) {
+        return SECSuccess;
+    }
+
+    rv = tls13_SetCipherSpec(ss, TrafficKeyHandshake,
+                             CipherSpecWrite, PR_FALSE);
+    if (rv != SECSuccess) {
+        PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
+        return SECFailure;
+    }
+    return SECSuccess;
+}
+
 /* Install a new cipher spec for this direction. */
 static SECStatus
 tls13_SetCipherSpec(sslSocket *ss, TrafficKeyType type,

diff --git a/lib/ssl/tls13con.h b/lib/ssl/tls13con.h
@@ -87,6 +87,7 @@ void tls13_DestroyEarlyData(PRCList *list);
 void tls13_CipherSpecAddRef(ssl3CipherSpec *spec);
 void tls13_CipherSpecRelease(ssl3CipherSpec *spec);
 void tls13_DestroyCipherSpecs(PRCList *list);
+SECStatus tls13_SetAlertCipherSpec(sslSocket *ss);
 tls13ExtensionStatus tls13_ExtensionStatus(PRUint16 extension,
                                            SSLHandshakeType message);
 SECStatus tls13_ProtectRecord(sslSocket *ss,