Commit cf9543d2 authored by Martin Thomson's avatar Martin Thomson

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.

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

--HG--
extra : source : f3122e5bfb5e5c9d1c6ca4f37fde170d7e289b77
extra : amend_source : 3debaa587e2aeda7b7c4440b03cb38952ecc8d41
parent 1140660a
......@@ -629,6 +629,85 @@ TEST_P(TlsConnectGeneric, CheckRandoms) {
EXPECT_NE(0, memcmp(srandom1, srandom2, random_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,
......
......@@ -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;
}
......
......@@ -939,8 +939,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) {
......@@ -999,28 +999,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);
......
......@@ -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);
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment