Bug 1423043 - Enable half-close, r?ttaubert
☠☠ backed out by 1dd6b420d136 ☠ ☠
authorMartin Thomson <martin.thomson@gmail.com>
Fri, 06 Apr 2018 09:11:29 +1000
changeset 14309 f5444a2e1f68de9ddc2df65b7b348e9cca6f205d
parent 14308 92ec52c839cb3ae890d7a9f228e3c12dc72b6f38
child 14310 1dd6b420d13691ef45033cafe647977393306c9d
push id3045
push usermartin.thomson@gmail.com
push dateMon, 09 Apr 2018 04:14:53 +0000
reviewersttaubert
bugs1423043
Bug 1423043 - Enable half-close, r?ttaubert Summary: TLS 1.3 explicitly changed to allow close_notify on one half of the connection. Since SSL, an endpoint was required to send close_notify if it received close_notify. The general agreement was that this was a silly requirement and that we would remove it and allow one side of the connection to be closed. This is critical for some protocols that are being moved to use TLS. NSS was almost perfect here. The only problem was that it suppressed the second close_notify. I've added a test for that. Reviewers: ttaubert Bug #: 1423043 Differential Revision: https://phabricator.services.mozilla.com/D797
gtests/ssl_gtest/ssl_loopback_unittest.cc
gtests/ssl_gtest/test_io.cc
gtests/ssl_gtest/tls_agent.cc
lib/ssl/sslsecur.c
--- a/gtests/ssl_gtest/ssl_loopback_unittest.cc
+++ b/gtests/ssl_gtest/ssl_loopback_unittest.cc
@@ -486,16 +486,95 @@ TEST_F(TlsConnectTest, OneNRecordSplitti
   big_buffer.Allocate(1 + 16384 + 20);
   server_->SendBuffer(big_buffer);
   ASSERT_EQ(3U, records->count());
   EXPECT_EQ(ExpectedCbcLen(1), records->record(0).buffer.len());
   EXPECT_EQ(ExpectedCbcLen(16384), records->record(1).buffer.len());
   EXPECT_EQ(ExpectedCbcLen(20), records->record(2).buffer.len());
 }
 
+void FailOnCloseNotify(const PRFileDesc* fd, void* arg, const SSLAlert* alert) {
+  ADD_FAILURE() << "received alert " << alert->description;
+}
+
+void CheckCloseNotify(const PRFileDesc* fd, void* arg, const SSLAlert* alert) {
+  *reinterpret_cast<bool*>(arg) = true;
+  EXPECT_EQ(close_notify, alert->description);
+  EXPECT_EQ(alert_warning, alert->level);
+}
+
+TEST_P(TlsConnectGeneric, ShutdownOneSide) {
+  Connect();
+
+  // Setup to check alerts.
+  EXPECT_EQ(SECSuccess, SSL_AlertSentCallback(server_->ssl_fd(),
+                                              FailOnCloseNotify, nullptr));
+  EXPECT_EQ(SECSuccess, SSL_AlertReceivedCallback(client_->ssl_fd(),
+                                                  FailOnCloseNotify, nullptr));
+
+  bool client_sent = false;
+  EXPECT_EQ(SECSuccess, SSL_AlertSentCallback(client_->ssl_fd(),
+                                              CheckCloseNotify, &client_sent));
+  bool server_received = false;
+  EXPECT_EQ(SECSuccess,
+            SSL_AlertReceivedCallback(server_->ssl_fd(), CheckCloseNotify,
+                                      &server_received));
+  EXPECT_EQ(PR_SUCCESS, PR_Shutdown(client_->ssl_fd(), PR_SHUTDOWN_SEND));
+
+  // Make sure that the server reads out the close_notify.
+  uint8_t buf[10];
+  EXPECT_EQ(0, PR_Read(server_->ssl_fd(), buf, sizeof(buf)));
+
+  // Reading and writing should still work in the one open direction.
+  EXPECT_TRUE(client_sent);
+  EXPECT_TRUE(server_received);
+  server_->SendData(10, 10);
+  client_->ReadBytes(10);
+
+  // Now close the other side and do the same checks.
+  bool server_sent = false;
+  EXPECT_EQ(SECSuccess, SSL_AlertSentCallback(server_->ssl_fd(),
+                                              CheckCloseNotify, &server_sent));
+  bool client_received = false;
+  EXPECT_EQ(SECSuccess,
+            SSL_AlertReceivedCallback(client_->ssl_fd(), CheckCloseNotify,
+                                      &client_received));
+  EXPECT_EQ(PR_SUCCESS, PR_Shutdown(server_->ssl_fd(), PR_SHUTDOWN_SEND));
+
+  EXPECT_EQ(0, PR_Read(client_->ssl_fd(), buf, sizeof(buf)));
+  EXPECT_TRUE(server_sent);
+  EXPECT_TRUE(client_received);
+}
+
+TEST_P(TlsConnectGeneric, ShutdownOneSideThenCloseTcp) {
+  Connect();
+
+  bool client_sent = false;
+  EXPECT_EQ(SECSuccess, SSL_AlertSentCallback(client_->ssl_fd(),
+                                              CheckCloseNotify, &client_sent));
+  bool server_received = false;
+  EXPECT_EQ(SECSuccess,
+            SSL_AlertReceivedCallback(server_->ssl_fd(), CheckCloseNotify,
+                                      &server_received));
+  EXPECT_EQ(PR_SUCCESS, PR_Shutdown(client_->ssl_fd(), PR_SHUTDOWN_SEND));
+
+  // Make sure that the server reads out the close_notify.
+  uint8_t buf[10];
+  EXPECT_EQ(0, PR_Read(server_->ssl_fd(), buf, sizeof(buf)));
+
+  // Now simulate the underlying connection closing.
+  client_->adapter()->Reset();
+
+  // Now close the other side and see that things don't explode.
+  EXPECT_EQ(PR_SUCCESS, PR_Shutdown(server_->ssl_fd(), PR_SHUTDOWN_SEND));
+
+  EXPECT_GT(0, PR_Read(client_->ssl_fd(), buf, sizeof(buf)));
+  EXPECT_EQ(PR_NOT_CONNECTED_ERROR, PR_GetError());
+}
+
 INSTANTIATE_TEST_CASE_P(
     GenericStream, TlsConnectGeneric,
     ::testing::Combine(TlsConnectTestBase::kTlsVariantsStream,
                        TlsConnectTestBase::kTlsVAll));
 INSTANTIATE_TEST_CASE_P(
     GenericDatagram, TlsConnectGeneric,
     ::testing::Combine(TlsConnectTestBase::kTlsVariantsDatagram,
                        TlsConnectTestBase::kTlsV11Plus));
--- a/gtests/ssl_gtest/test_io.cc
+++ b/gtests/ssl_gtest/test_io.cc
@@ -26,27 +26,47 @@ namespace nss_test {
   } while (false)
 
 ScopedPRFileDesc DummyPrSocket::CreateFD() {
   static PRDescIdentity test_fd_identity =
       PR_GetUniqueIdentity("testtransportadapter");
   return DummyIOLayerMethods::CreateFD(test_fd_identity, this);
 }
 
+void DummyPrSocket::Reset() {
+  auto p = peer_.lock();
+  peer_.reset();
+  if (p) {
+    p->peer_.reset();
+    p->Reset();
+  }
+  while (!input_.empty()) {
+    input_.pop();
+  }
+  filter_ = nullptr;
+  write_error_ = 0;
+}
+
 void DummyPrSocket::PacketReceived(const DataBuffer &packet) {
   input_.push(Packet(packet));
 }
 
 int32_t DummyPrSocket::Read(PRFileDesc *f, void *data, int32_t len) {
   PR_ASSERT(variant_ == ssl_variant_stream);
   if (variant_ != ssl_variant_stream) {
     PR_SetError(PR_INVALID_METHOD_ERROR, 0);
     return -1;
   }
 
+  auto dst = peer_.lock();
+  if (!dst) {
+    PR_SetError(PR_NOT_CONNECTED_ERROR, 0);
+    return -1;
+  }
+
   if (input_.empty()) {
     LOGV("Read --> wouldblock " << len);
     PR_SetError(PR_WOULD_BLOCK_ERROR, 0);
     return -1;
   }
 
   auto &front = input_.front();
   size_t to_read =
@@ -69,16 +89,22 @@ int32_t DummyPrSocket::Recv(PRFileDesc *
     PR_SetError(PR_NOT_IMPLEMENTED_ERROR, 0);
     return -1;
   }
 
   if (variant() != ssl_variant_datagram) {
     return Read(f, buf, buflen);
   }
 
+  auto dst = peer_.lock();
+  if (!dst) {
+    PR_SetError(PR_NOT_CONNECTED_ERROR, 0);
+    return -1;
+  }
+
   if (input_.empty()) {
     PR_SetError(PR_WOULD_BLOCK_ERROR, 0);
     return -1;
   }
 
   auto &front = input_.front();
   if (static_cast<size_t>(buflen) < front.len()) {
     PR_ASSERT(false);
@@ -96,17 +122,17 @@ int32_t DummyPrSocket::Recv(PRFileDesc *
 int32_t DummyPrSocket::Write(PRFileDesc *f, const void *buf, int32_t length) {
   if (write_error_) {
     PR_SetError(write_error_, 0);
     return -1;
   }
 
   auto dst = peer_.lock();
   if (!dst) {
-    PR_SetError(PR_IO_ERROR, 0);
+    PR_SetError(PR_NOT_CONNECTED_ERROR, 0);
     return -1;
   }
 
   DataBuffer packet(static_cast<const uint8_t *>(buf),
                     static_cast<size_t>(length));
   DataBuffer filtered;
   PacketFilter::Action action = PacketFilter::KEEP;
   if (filter_) {
--- a/gtests/ssl_gtest/tls_agent.cc
+++ b/gtests/ssl_gtest/tls_agent.cc
@@ -916,18 +916,18 @@ void TlsAgent::SendDirect(const DataBuff
 void TlsAgent::SendRecordDirect(const TlsRecord& record) {
   DataBuffer buf;
 
   auto rv = record.header.Write(&buf, 0, record.buffer);
   EXPECT_EQ(record.header.header_length() + record.buffer.len(), rv);
   SendDirect(buf);
 }
 
-static bool ErrorIsNonFatal(PRErrorCode code) {
-  return code == PR_WOULD_BLOCK_ERROR || code == SSL_ERROR_RX_SHORT_DTLS_READ;
+static bool ErrorIsFatal(PRErrorCode code) {
+  return code != PR_WOULD_BLOCK_ERROR && code != SSL_ERROR_RX_SHORT_DTLS_READ;
 }
 
 void TlsAgent::SendData(size_t bytes, size_t blocksize) {
   uint8_t block[4096];
 
   ASSERT_LT(blocksize, sizeof(block));
 
   while (bytes) {
@@ -976,38 +976,49 @@ bool TlsAgent::SendEncryptedRecord(const
   EXPECT_EQ(header.header_length() + ciphertext.len(), rv);
   SendDirect(record);
   return true;
 }
 
 void TlsAgent::ReadBytes(size_t amount) {
   uint8_t block[16384];
 
-  int32_t rv = PR_Read(ssl_fd(), block, (std::min)(amount, sizeof(block)));
-  LOGV("ReadBytes " << rv);
-  int32_t err;
+  size_t remaining = amount;
+  while (remaining > 0) {
+    int32_t rv = PR_Read(ssl_fd(), block, (std::min)(amount, sizeof(block)));
+    LOGV("ReadBytes " << rv);
 
-  if (rv >= 0) {
-    size_t count = static_cast<size_t>(rv);
-    for (size_t i = 0; i < count; ++i) {
-      ASSERT_EQ(recv_ctr_ & 0xff, block[i]);
-      recv_ctr_++;
-    }
-  } else {
-    err = PR_GetError();
-    LOG("Read error " << PORT_ErrorToName(err) << ": "
-                      << PORT_ErrorToString(err));
-    if (err != PR_WOULD_BLOCK_ERROR && expect_readwrite_error_) {
-      error_code_ = err;
-      expect_readwrite_error_ = false;
+    if (rv > 0) {
+      size_t count = static_cast<size_t>(rv);
+      for (size_t i = 0; i < count; ++i) {
+        ASSERT_EQ(recv_ctr_ & 0xff, block[i]);
+        recv_ctr_++;
+      }
+      remaining -= rv;
+    } else {
+      PRErrorCode err = 0;
+      if (rv < 0) {
+        err = PR_GetError();
+        LOG("Read error " << PORT_ErrorToName(err) << ": "
+                          << PORT_ErrorToString(err));
+        if (err != PR_WOULD_BLOCK_ERROR && expect_readwrite_error_) {
+          error_code_ = err;
+          expect_readwrite_error_ = false;
+        }
+      }
+      if (err != 0 && ErrorIsFatal(err)) {
+        // If we hit a fatal error, we're done.
+        remaining = 0;
+      }
+      break;
     }
   }
 
   // If closed, then don't bother waiting around.
-  if (rv > 0 || (rv < 0 && ErrorIsNonFatal(err))) {
+  if (remaining) {
     LOGV("Re-arming");
     Poller::Instance()->Wait(READABLE_EVENT, adapter_, this,
                              &TlsAgent::ReadableCallback);
   }
 }
 
 void TlsAgent::ResetSentBytes() { send_ctr_ = 0; }
 
--- a/lib/ssl/sslsecur.c
+++ b/lib/ssl/sslsecur.c
@@ -680,47 +680,29 @@ ssl_SecureConnect(sslSocket *ss, const P
     }
 
     SSL_TRC(5, ("%d: SSL[%d]: secure connect completed, rv == %d",
                 SSL_GETPID(), ss->fd, rv));
     return rv;
 }
 
 /*
- * The TLS 1.2 RFC 5246, Section 7.2.1 says:
- *
- *     Unless some other fatal alert has been transmitted, each party is
- *     required to send a close_notify alert before closing the write side
- *     of the connection.  The other party MUST respond with a close_notify
- *     alert of its own and close down the connection immediately,
- *     discarding any pending writes.  It is not required for the initiator
- *     of the close to wait for the responding close_notify alert before
- *     closing the read side of the connection.
- *
- * The second sentence requires that we send a close_notify alert when we
- * have received a close_notify alert.  In practice, all SSL implementations
- * close the socket immediately after sending a close_notify alert (which is
- * allowed by the third sentence), so responding with a close_notify alert
- * would result in a write failure with the ECONNRESET error.  This is why
- * we don't respond with a close_notify alert.
- *
  * Also, in the unlikely event that the TCP pipe is full and the peer stops
  * reading, the SSL3_SendAlert call in ssl_SecureClose and ssl_SecureShutdown
  * may block indefinitely in blocking mode, and may fail (without retrying)
  * in non-blocking mode.
  */
 
 int
 ssl_SecureClose(sslSocket *ss)
 {
     int rv;
 
     if (!(ss->shutdownHow & ssl_SHUTDOWN_SEND) &&
-        ss->firstHsDone &&
-        !ss->recvdCloseNotify) {
+        ss->firstHsDone) {
 
         /* We don't want the final alert to be Nagle delayed. */
         if (!ss->delayDisabled) {
             ssl_EnableNagleDelay(ss, PR_FALSE);
             ss->delayDisabled = 1;
         }
 
         (void)SSL3_SendAlert(ss, alert_warning, close_notify);
@@ -739,18 +721,17 @@ ssl_SecureShutdown(sslSocket *ss, int ns
 
     if ((unsigned)nsprHow > PR_SHUTDOWN_BOTH) {
         PORT_SetError(PR_INVALID_ARGUMENT_ERROR);
         return PR_FAILURE;
     }
 
     if ((sslHow & ssl_SHUTDOWN_SEND) != 0 &&
         !(ss->shutdownHow & ssl_SHUTDOWN_SEND) &&
-        ss->firstHsDone &&
-        !ss->recvdCloseNotify) {
+        ss->firstHsDone) {
 
         (void)SSL3_SendAlert(ss, alert_warning, close_notify);
     }
 
     rv = osfd->methods->shutdown(osfd, nsprHow);
 
     ss->shutdownHow |= sslHow;