Bug 1429475: Tests for delayed failure and be more aggressive about m…

…aking failures persistent. r=mt, wtc

Summary:
- Make any call to ssl3_GatherCompleteHandshake (which transitively
  means any read from the wire) return PR_IO_ERROR if an alert has
  been sent.

- Patch up a few of the tests to handle this new behavior properly.
  These tests actually were a bit harder to follow so they should
  also be a bit clearer.

- Add a new set of tests for certificate authentication failure.

Reviewers: mt

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

cpputil/tls_parser.h

@@ -47,6 +47,7 @@ const uint8_t kTlsAlertUnexpectedMessage = 10;
 const uint8_t kTlsAlertBadRecordMac = 20;
 const uint8_t kTlsAlertRecordOverflow = 22;
 const uint8_t kTlsAlertHandshakeFailure = 40;
+const uint8_t kTlsAlertBadCertificate = 42;
 const uint8_t kTlsAlertIllegalParameter = 47;
 const uint8_t kTlsAlertDecodeError = 50;
 const uint8_t kTlsAlertDecryptError = 51;

gtests/ssl_gtest/ssl_0rtt_unittest.cc

@@ -405,6 +405,9 @@ TEST_P(TlsConnectTls13, TestTls13ZeroRttDowngrade) {
 // The client should abort the connection when sending a 0-rtt handshake but
 // the servers responds with a TLS 1.2 ServerHello. (with app data)
 TEST_P(TlsConnectTls13, TestTls13ZeroRttDowngradeEarlyData) {
+  const char* k0RttData = "ABCDEF";
+  const PRInt32 k0RttDataLen = static_cast<PRInt32>(strlen(k0RttData));
+
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   server_->Set0RttEnabled(true);  // set ticket_allow_early_data
   Connect();
@@ -422,27 +425,28 @@ TEST_P(TlsConnectTls13, TestTls13ZeroRttDowngradeEarlyData) {
   // Send the early data xtn in the CH, followed by early app data. The server
   // will fail right after sending its flight, when receiving the early data.
   client_->Set0RttEnabled(true);
-  ZeroRttSendReceive(true, false, [this]() {
-    client_->ExpectSendAlert(kTlsAlertIllegalParameter);
-    if (variant_ == ssl_variant_stream) {
-      server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
-    }
-    return true;
-  });
+  client_->Handshake();  // Send ClientHello.
+  PRInt32 rv =
+      PR_Write(client_->ssl_fd(), k0RttData, k0RttDataLen);  // 0-RTT write.
+  EXPECT_EQ(k0RttDataLen, rv);
 
-  client_->Handshake();
-  server_->Handshake();
-  ASSERT_TRUE_WAIT(
-      (client_->error_code() == SSL_ERROR_DOWNGRADE_WITH_EARLY_DATA), 2000);
-
-  // DTLS will timeout as we bump the epoch when installing the early app data
-  // cipher suite. Thus the encrypted alert will be ignored.
   if (variant_ == ssl_variant_stream) {
-    // The server sends an alert when receiving the early app data record.
-    ASSERT_TRUE_WAIT(
-        (server_->error_code() == SSL_ERROR_RX_UNEXPECTED_APPLICATION_DATA),
-        2000);
+    // When the server receives the early data, it will fail.
+    server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
+    server_->Handshake();  // Consume ClientHello
+    EXPECT_EQ(TlsAgent::STATE_ERROR, server_->state());
+    server_->CheckErrorCode(SSL_ERROR_RX_UNEXPECTED_APPLICATION_DATA);
+  } else {
+    // If it's datagram, we just discard the early data.
+    server_->Handshake();  // Consume ClientHello
+    EXPECT_EQ(TlsAgent::STATE_CONNECTING, server_->state());
   }
+
+  // The client now reads the ServerHello and fails.
+  ASSERT_EQ(TlsAgent::STATE_CONNECTING, client_->state());
+  client_->ExpectSendAlert(kTlsAlertIllegalParameter);
+  client_->Handshake();
+  client_->CheckErrorCode(SSL_ERROR_DOWNGRADE_WITH_EARLY_DATA);
 }
 
 static void CheckEarlyDataLimit(const std::shared_ptr<TlsAgent>& agent,
@@ -521,6 +525,8 @@ TEST_P(TlsConnectTls13, ReceiveTooMuchEarlyData) {
   client_->Handshake();  // Send ClientHello
   CheckEarlyDataLimit(client_, limit);
 
+  server_->Handshake();  // Process ClientHello, send server flight.
+
   // Lift the limit on the client.
   EXPECT_EQ(SECSuccess,
             SSLInt_SetSocketMaxEarlyDataSize(client_->ssl_fd(), 1000));
@@ -534,21 +540,31 @@ TEST_P(TlsConnectTls13, ReceiveTooMuchEarlyData) {
     // This error isn't fatal for DTLS.
     ExpectAlert(server_, kTlsAlertUnexpectedMessage);
   }
-  server_->Handshake();  // Process ClientHello, send server flight.
-  server_->Handshake();  // Just to make sure that we don't read ahead.
+
+  server_->Handshake();  // This reads the early data and maybe throws an error.
+  if (variant_ == ssl_variant_stream) {
+    server_->CheckErrorCode(SSL_ERROR_TOO_MUCH_EARLY_DATA);
+  } else {
+    EXPECT_EQ(TlsAgent::STATE_CONNECTING, server_->state());
+  }
   CheckEarlyDataLimit(server_, limit);
 
-  // Attempt to read early data.
+  // Attempt to read early data. This will get an error.
   std::vector<uint8_t> buf(strlen(message) + 1);
   EXPECT_GT(0, PR_Read(server_->ssl_fd(), buf.data(), buf.capacity()));
   if (variant_ == ssl_variant_stream) {
-    server_->CheckErrorCode(SSL_ERROR_TOO_MUCH_EARLY_DATA);
+    EXPECT_EQ(SSL_ERROR_HANDSHAKE_FAILED, PORT_GetError());
+  } else {
+    EXPECT_EQ(PR_WOULD_BLOCK_ERROR, PORT_GetError());
   }
 
-  client_->Handshake();  // Process the handshake.
-  client_->Handshake();  // Process the alert.
+  client_->Handshake();  // Process the server's first flight.
   if (variant_ == ssl_variant_stream) {
+    client_->Handshake();  // Process the alert.
     client_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT);
+  } else {
+    server_->Handshake();  // Finish connecting.
+    EXPECT_EQ(TlsAgent::STATE_CONNECTED, server_->state());
   }
 }
 

gtests/ssl_gtest/ssl_auth_unittest.cc

@@ -622,6 +622,31 @@ TEST_P(TlsConnectGenericPre13, AuthCompleteDelayed) {
   client_->DeletePacketFilter();
 }
 
+TEST_P(TlsConnectGenericPre13, AuthCompleteFailDelayed) {
+  client_->SetAuthCertificateCallback(AuthCompleteBlock);
+
+  StartConnect();
+  client_->Handshake();  // Send ClientHello
+  server_->Handshake();  // Send ServerHello
+  client_->Handshake();  // Send ClientKeyExchange and Finished
+  server_->Handshake();  // Send Finished
+  // The server should now report that it is connected
+  EXPECT_EQ(TlsAgent::STATE_CONNECTED, server_->state());
+
+  // The client should send nothing from here on.
+  client_->SetPacketFilter(std::make_shared<EnforceNoActivity>());
+  client_->Handshake();
+  EXPECT_EQ(TlsAgent::STATE_CONNECTING, client_->state());
+
+  // Report failure.
+  client_->DeletePacketFilter();
+  client_->ExpectSendAlert(kTlsAlertBadCertificate);
+  EXPECT_EQ(SECSuccess, SSL_AuthCertificateComplete(client_->ssl_fd(),
+                                                    SSL_ERROR_BAD_CERTIFICATE));
+  client_->Handshake();  // Fail
+  EXPECT_EQ(TlsAgent::STATE_ERROR, client_->state());
+}
+
 // TLS 1.3 handles a delayed AuthComplete callback differently since the
 // shape of the handshake is different.
 TEST_P(TlsConnectTls13, AuthCompleteDelayed) {
@@ -647,6 +672,44 @@ TEST_P(TlsConnectTls13, AuthCompleteDelayed) {
   EXPECT_EQ(TlsAgent::STATE_CONNECTED, server_->state());
 }
 
+TEST_P(TlsConnectTls13, AuthCompleteFailDelayed) {
+  client_->SetAuthCertificateCallback(AuthCompleteBlock);
+
+  StartConnect();
+  client_->Handshake();  // Send ClientHello
+  server_->Handshake();  // Send ServerHello
+  EXPECT_EQ(TlsAgent::STATE_CONNECTING, client_->state());
+  EXPECT_EQ(TlsAgent::STATE_CONNECTING, server_->state());
+
+  // The client will send nothing until AuthCertificateComplete is called.
+  client_->SetPacketFilter(std::make_shared<EnforceNoActivity>());
+  client_->Handshake();
+  EXPECT_EQ(TlsAgent::STATE_CONNECTING, client_->state());
+
+  // Report failure.
+  client_->DeletePacketFilter();
+  ExpectAlert(client_, kTlsAlertBadCertificate);
+  EXPECT_EQ(SECSuccess, SSL_AuthCertificateComplete(client_->ssl_fd(),
+                                                    SSL_ERROR_BAD_CERTIFICATE));
+  client_->Handshake();  // This should now fail.
+  server_->Handshake();  // Get the error.
+  EXPECT_EQ(TlsAgent::STATE_ERROR, client_->state());
+  EXPECT_EQ(TlsAgent::STATE_ERROR, server_->state());
+}
+
+static SECStatus AuthCompleteFail(TlsAgent*, PRBool, PRBool) {
+  PORT_SetError(SSL_ERROR_BAD_CERTIFICATE);
+  return SECFailure;
+}
+
+TEST_P(TlsConnectGeneric, AuthFailImmediate) {
+  client_->SetAuthCertificateCallback(AuthCompleteFail);
+
+  StartConnect();
+  ConnectExpectAlert(client_, kTlsAlertBadCertificate);
+  client_->CheckErrorCode(SSL_ERROR_BAD_CERTIFICATE);
+}
+
 static const SSLExtraServerCertData ServerCertDataRsaPkcs1Decrypt = {
     ssl_auth_rsa_decrypt, nullptr, nullptr, nullptr};
 static const SSLExtraServerCertData ServerCertDataRsaPkcs1Sign = {

gtests/ssl_gtest/tls_connect.cc

@@ -670,7 +670,8 @@ void TlsConnectTestBase::ZeroRttSendReceive(
     EXPECT_EQ(k0RttDataLen, rv);
   } else {
     EXPECT_EQ(SECFailure, rv);
-    EXPECT_EQ(PR_WOULD_BLOCK_ERROR, PORT_GetError());
+    EXPECT_EQ(PR_WOULD_BLOCK_ERROR, PORT_GetError())
+        << "Unexpected error: " << PORT_ErrorToName(PORT_GetError());
   }
 
   // Do a second read. this should fail.

lib/ssl/SSLerrs.h

@@ -540,3 +540,6 @@ ER3(SSL_ERROR_RX_MALFORMED_KEY_UPDATE, (SSL_ERROR_BASE + 170),
 
 ER3(SSL_ERROR_TOO_MANY_KEY_UPDATES, (SSL_ERROR_BASE + 171),
     "SSL attempted too many key updates.")
+
+ER3(SSL_ERROR_HANDSHAKE_FAILED, (SSL_ERROR_BASE + 172),
+    "SSL handshake has already failed. No more operations possible.")

lib/ssl/ssl3con.c

@@ -2336,6 +2336,11 @@ ssl3_SendRecord(sslSocket *ss,
     if (ss->ssl3.fatalAlertSent) {
         SSL_TRC(3, ("%d: SSL3[%d] Suppress write, fatal alert already sent",
                     SSL_GETPID(), ss->fd));
+        if (type != content_alert) {
+            /* If we are sending an alert, then we already have an
+             * error, so don't overwrite. */
+            PORT_SetError(SSL_ERROR_HANDSHAKE_FAILED);
+        }
         return SECFailure;
     }
 

lib/ssl/ssl3gthr.c

@@ -386,6 +386,13 @@ ssl3_GatherCompleteHandshake(sslSocket *ss, int flags)
     SSL3Ciphertext cText;
     PRBool keepGoing = PR_TRUE;
 
+    if (ss->ssl3.fatalAlertSent) {
+        SSL_TRC(3, ("%d: SSL3[%d] Cannot gather data; fatal alert already sent",
+                    SSL_GETPID(), ss->fd));
+        PORT_SetError(SSL_ERROR_HANDSHAKE_FAILED);
+        return SECFailure;
+    }
+
     SSL_TRC(30, ("%d: SSL3[%d]: ssl3_GatherCompleteHandshake",
                  SSL_GETPID(), ss->fd));
 

lib/ssl/sslerr.h

@@ -260,6 +260,7 @@ typedef enum {
     SSL_ERROR_RX_UNEXPECTED_KEY_UPDATE = (SSL_ERROR_BASE + 169),
     SSL_ERROR_RX_MALFORMED_KEY_UPDATE = (SSL_ERROR_BASE + 170),
     SSL_ERROR_TOO_MANY_KEY_UPDATES = (SSL_ERROR_BASE + 171),
+    SSL_ERROR_HANDSHAKE_FAILED = (SSL_ERROR_BASE + 172),
     SSL_ERROR_END_OF_LIST   /* let the c compiler determine the value of this. */
 } SSLErrorCodes;
 #endif /* NO_SECURITY_ERROR_ENUM */