CVE-2019-11934: Out-of-bounds Read (CWE-125)
value: Improper handling of close_notify alerts can result in an out-of-bounds read in AsyncSSLSocket. This issue affects folly prior to v2019.11.04.00.
CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 9.8
Published: 2019-12-04T17:16Z
Repository: https://github.com/facebook/folly
Language: C++
Files changed: folly/io/async/AsyncSSLSocket.cpp, folly/io/async/test/AsyncSSLSocketTest.cpp, folly/io/async/test/AsyncSSLSocketTest.h
Commit date: 2019-10-28 15:49:03 -0700
Commit message: Handle close_notify as standard writeErr in AsyncSSLSocket.

Summary: Fixes CVE-2019-11934

Reviewed By: mingtaoy

Differential Revision: D18020613

fbshipit-source-id: db82bb250e53f0d225f1280bd67bc74abd417836

Diff: diff --git a/folly/io/async/AsyncSSLSocket.cpp b/folly/io/async/AsyncSSLSocket.cpp
index 88cfcb9a2..2fc58b2fc 100644
--- a/folly/io/async/AsyncSSLSocket.cpp
+++ b/folly/io/async/AsyncSSLSocket.cpp
@@ -1448,13 +1448,10 @@ AsyncSocket::WriteResult AsyncSSLSocket::interpretSSLError(int rc, int error) {
                << "unsupported SSL renegotiation during write";
     return WriteResult(
         WRITE_ERROR,
         std::make_unique<SSLException>(SSLError::INVALID_RENEGOTIATION));
   } else {
-    if (zero_return(error, rc, errno)) {
-      return WriteResult(0);
-    }
     auto errError = ERR_get_error();
     VLOG(3) << "ERROR: AsyncSSLSocket(fd=" << fd_ << ", state=" << int(state_)
             << ", sslState=" << sslState_ << ", events=" << eventFlags_ << "): "
             << "SSL error: " << error << ", errno: " << errno
             << ", func: " << ERR_func_error_string(errError)
@@ -1587,14 +1584,11 @@ AsyncSocket::WriteResult AsyncSSLSocket::performWrite(
       if (error == SSL_ERROR_WANT_WRITE) {
         // The caller will register for write event if not already.
         *partialWritten = uint32_t(offset);
         return WriteResult(totalWritten);
       }
-      auto writeResult = interpretSSLError(int(bytes), error);
-      if (writeResult.writeReturn < 0) {
-        return writeResult;
-      } // else fall through to below to correctly record totalWritten
+      return interpretSSLError(int(bytes), error);
     }
 
     totalWritten += bytes;
 
     if (bytes == (ssize_t)len) {
diff --git a/folly/io/async/test/AsyncSSLSocketTest.cpp b/folly/io/async/test/AsyncSSLSocketTest.cpp
index 7c6310612..7544282df 100644
--- a/folly/io/async/test/AsyncSSLSocketTest.cpp
+++ b/folly/io/async/test/AsyncSSLSocketTest.cpp
@@ -806,10 +806,118 @@ TEST(AsyncSSLSocketTest, SSLClientTimeoutTest) {
   EXPECT_EQ(client->getHit(), 0);
 
   cerr << "SSLClientTimeoutTest test completed" << endl;
 }
 
+class PerLoopReadCallback : public AsyncTransportWrapper::ReadCallback {
+ public:
+  void getReadBuffer(void** bufReturn, size_t* lenReturn) override {
+    *bufReturn = buf_.data();
+    *lenReturn = buf_.size();
+  }
+
+  void readDataAvailable(size_t len) noexcept override {
+    VLOG(3) << "Read of size: " << len;
+    s_->setReadCB(nullptr);
+    s_->getEventBase()->runInLoop([this]() { s_->setReadCB(this); });
+  }
+
+  void readErr(const AsyncSocketException&) noexcept override {}
+
+  void readEOF() noexcept override {}
+
+  void setSocket(AsyncSocket* s) {
+    s_ = s;
+  }
+
+ private:
+  AsyncSocket* s_;
+  std::array<uint8_t, 1000> buf_;
+};
+
+class CloseNotifyConnector : public AsyncSocket::ConnectCallback {
+ public:
+  CloseNotifyConnector(EventBase* evb, const SocketAddress& addr) {
+    evb_ = evb;
+    ssl_ = AsyncSSLSocket::newSocket(std::make_shared<SSLContext>(), evb_);
+    ssl_->connect(this, addr);
+  }
+
+  void connectSuccess() noexcept override {
+    ssl_->writeChain(nullptr, IOBuf::copyBuffer("hi"));
+    auto ssl = const_cast<SSL*>(ssl_->getSSL());
+    SSL_shutdown(ssl);
+    auto fd = ssl_->detachNetworkSocket();
+    tcp_.reset(new AsyncSocket(evb_, fd), AsyncSocket::Destructor());
+    evb_->runAfterDelay(
+        [this]() {
+          perLoopReads_.setSocket(tcp_.get());
+          tcp_->setReadCB(&perLoopReads_);
+          evb_->runAfterDelay([this]() { tcp_->closeNow(); }, 10);
+        },
+        100);
+  }
+
+  void connectErr(const AsyncSocketException& ex) noexcept override {
+    FAIL() << ex.what();
+  }
+
+ private:
+  EventBase* evb_;
+  std::shared_ptr<AsyncSSLSocket> ssl_;
+  std::shared_ptr<AsyncSocket> tcp_;
+  PerLoopReadCallback perLoopReads_;
+};
+
+class ErrorCheckingWriteCallback : public AsyncSocket::WriteCallback {
+ public:
+  void writeSuccess() noexcept override {}
+
+  void writeErr(size_t, const AsyncSocketException& ex) noexcept override {
+    LOG(ERROR) << "write error: " << ex.what();
+    EXPECT_NE(
+        ex.getType(),
+        AsyncSocketException::AsyncSocketExceptionType::SSL_ERROR);
+  }
+};
+
+class WriteOnEofReadCallback : public ReadCallback {
+ public:
+  using ReadCallback::ReadCallback;
+
+  void readEOF() noexcept override {
+    LOG(INFO) << "Got EOF";
+    auto chain = IOBuf::create(0);
+    for (size_t i = 0; i < 1000 * 1000; i++) {
+      auto buf = IOBuf::create(10);
+      buf->append(10);
+      memset(buf->writableData(), 'x', 10);
+      chain->prependChain(std::move(buf));
+    }
+    socket_->writeChain(&writeCallback_, std::move(chain));
+  }
+
+  void readErr(const AsyncSocketException& ex) noexcept override {
+    LOG(ERROR) << ex.what();
+  }
+
+ private:
+  ErrorCheckingWriteCallback writeCallback_;
+};
+
+TEST(AsyncSSLSocketTest, EarlyCloseNotify) {
+  WriteOnEofReadCallback readCallback(nullptr);
+  HandshakeCallback handshakeCallback(&readCallback);
+  SSLServerAcceptCallback acceptCallback(&handshakeCallback);
+  TestSSLServer server(&acceptCallback);
+
+  EventBase eventBase;
+  CloseNotifyConnector cnc(&eventBase, server.getAddress());
+
+  eventBase.loop();
+}
+
 /**
  * Verify Client Ciphers obtained using SSL MSG Callback.
  */
 TEST(AsyncSSLSocketTest, SSLParseClientHelloSuccess) {
   EventBase eventBase;
diff --git a/folly/io/async/test/AsyncSSLSocketTest.h b/folly/io/async/test/AsyncSSLSocketTest.h
index d5eba82a5..8684f6ad9 100644
--- a/folly/io/async/test/AsyncSSLSocketTest.h
+++ b/folly/io/async/test/AsyncSSLSocketTest.h
@@ -387,11 +387,13 @@ class ReadCallback : public ReadCallbackBase {
   void readDataAvailable(size_t len) noexcept override {
     std::cerr << "readDataAvailable, len " << len << std::endl;
 
     currentBuffer.length = len;
 
-    wcb_->setSocket(socket_);
+    if (wcb_) {
+      wcb_->setSocket(socket_);
+    }
 
     // Write back the same data.
     socket_->write(wcb_, currentBuffer.buffer, len, writeFlags);
 
     buffers.push_back(currentBuffer);

Vulnerable code:     if (zero_return(error, rc, errno)) {
      return WriteResult(0);
    }
      auto writeResult = interpretSSLError(int(bytes), error);
      if (writeResult.writeReturn < 0) {
        return writeResult;
      } // else fall through to below to correctly record totalWritten
    wcb_->setSocket(socket_);
Fixed code:       return interpretSSLError(int(bytes), error);
class PerLoopReadCallback : public AsyncTransportWrapper::ReadCallback {
 public:
  void getReadBuffer(void** bufReturn, size_t* lenReturn) override {
    *bufReturn = buf_.data();
    *lenReturn = buf_.size();
  }

  void readDataAvailable(size_t len) noexcept override {
    VLOG(3) << "Read of size: " << len;
    s_->setReadCB(nullptr);
    s_->getEventBase()->runInLoop([this]() { s_->setReadCB(this); });
  }

  void readErr(const AsyncSocketException&) noexcept override {}

  void readEOF() noexcept override {}

  void setSocket(AsyncSocket* s) {
    s_ = s;
  }

 private:
  AsyncSocket* s_;
  std::array<uint8_t, 1000> buf_;
};

class CloseNotifyConnector : public AsyncSocket::ConnectCallback {
 public:
  CloseNotifyConnector(EventBase* evb, const SocketAddress& addr) {
    evb_ = evb;
    ssl_ = AsyncSSLSocket::newSocket(std::make_shared<SSLContext>(), evb_);
    ssl_->connect(this, addr);
  }

  void connectSuccess() noexcept override {
    ssl_->writeChain(nullptr, IOBuf::copyBuffer("hi"));
    auto ssl = const_cast<SSL*>(ssl_->getSSL());
    SSL_shutdown(ssl);
    auto fd = ssl_->detachNetworkSocket();
    tcp_.reset(new AsyncSocket(evb_, fd), AsyncSocket::Destructor());
    evb_->runAfterDelay(
        [this]() {
          perLoopReads_.setSocket(tcp_.get());
          tcp_->setReadCB(&perLoopReads_);
          evb_->runAfterDelay([this]() { tcp_->closeNow(); }, 10);
        },
        100);
  }

  void connectErr(const AsyncSocketException& ex) noexcept override {
    FAIL() << ex.what();
  }

 private:
  EventBase* evb_;
  std::shared_ptr<AsyncSSLSocket> ssl_;
  std::shared_ptr<AsyncSocket> tcp_;
  PerLoopReadCallback perLoopReads_;
};

class ErrorCheckingWriteCallback : public AsyncSocket::WriteCallback {
 public:
  void writeSuccess() noexcept override {}

  void writeErr(size_t, const AsyncSocketException& ex) noexcept override {
    LOG(ERROR) << "write error: " << ex.what();
    EXPECT_NE(
        ex.getType(),
        AsyncSocketException::AsyncSocketExceptionType::SSL_ERROR);
  }
};

class WriteOnEofReadCallback : public ReadCallback {
 public:
  using ReadCallback::ReadCallback;

  void readEOF() noexcept override {
    LOG(INFO) << "Got EOF";
    auto chain = IOBuf::create(0);
    for (size_t i = 0; i < 1000 * 1000; i++) {
      auto buf = IOBuf::create(10);
      buf->append(10);
      memset(buf->writableData(), 'x', 10);
      chain->prependChain(std::move(buf));
    }
    socket_->writeChain(&writeCallback_, std::move(chain));
  }

  void readErr(const AsyncSocketException& ex) noexcept override {
    LOG(ERROR) << ex.what();
  }

 private:
  ErrorCheckingWriteCallback writeCallback_;
};

TEST(AsyncSSLSocketTest, EarlyCloseNotify) {
  WriteOnEofReadCallback readCallback(nullptr);
  HandshakeCallback handshakeCallback(&readCallback);
  SSLServerAcceptCallback acceptCallback(&handshakeCallback);
  TestSSLServer server(&acceptCallback);

  EventBase eventBase;
  CloseNotifyConnector cnc(&eventBase, server.getAddress());

  eventBase.loop();
}

    if (wcb_) {
      wcb_->setSocket(socket_);
    }