Bug 1423043 - Enable half-close, r=ttaubert,ekr

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, ekr

Reviewed By: ttaubert, ekr

Subscribers: ekr

Bug #: 1423043

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

4 files changed, 138 insertions, 41 deletions

diff --git a/gtests/ssl_gtest/ssl_loopback_unittest.cc b/gtests/ssl_gtest/ssl_loopback_unittest.cc
index 5adbd9dc7..ebea2bbba 100644
--- a/gtests/ssl_gtest/ssl_loopback_unittest.cc
+++ b/gtests/ssl_gtest/ssl_loopback_unittest.cc
@@ -539,6 +539,85 @@ TEST_F(TlsConnectTest, OneNRecordSplitting) {
   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,
diff --git a/gtests/ssl_gtest/test_io.cc b/gtests/ssl_gtest/test_io.cc
index d76b3526c..6d792c520 100644
--- a/gtests/ssl_gtest/test_io.cc
+++ b/gtests/ssl_gtest/test_io.cc
@@ -31,6 +31,20 @@ ScopedPRFileDesc DummyPrSocket::CreateFD() {
   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));
 }
@@ -42,6 +56,12 @@ int32_t DummyPrSocket::Read(PRFileDesc *f, void *data, int32_t len) {
     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);
@@ -74,6 +94,12 @@ int32_t DummyPrSocket::Recv(PRFileDesc *f, void *buf, int32_t buflen,
     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;
@@ -101,7 +127,7 @@ int32_t DummyPrSocket::Write(PRFileDesc *f, const void *buf, int32_t length) {
 
   auto dst = peer_.lock();
   if (!dst) {
-    PR_SetError(PR_IO_ERROR, 0);
+    PR_SetError(PR_NOT_CONNECTED_ERROR, 0);
     return -1;
   }
 
diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc
index 4f0402bf5..a62d488f6 100644
--- a/gtests/ssl_gtest/tls_agent.cc
+++ b/gtests/ssl_gtest/tls_agent.cc
@@ -919,8 +919,8 @@ void TlsAgent::SendRecordDirect(const TlsRecord& record) {
   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) {
@@ -979,28 +979,39 @@ bool TlsAgent::SendEncryptedRecord(const std::shared_ptr<TlsCipherSpec>& spec,
 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);
diff --git a/lib/ssl/sslsecur.c b/lib/ssl/sslsecur.c
index d3424a7ad..b98969b95 100644
--- a/lib/ssl/sslsecur.c
+++ b/lib/ssl/sslsecur.c
@@ -685,23 +685,6 @@ ssl_SecureConnect(sslSocket *ss, const PRNetAddr *sa)
 }
 
 /*
- * 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)
@@ -714,8 +697,7 @@ 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) {
@@ -744,8 +726,7 @@ ssl_SecureShutdown(sslSocket *ss, int nsprHow)
 
     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);
     }