tls: fix out-of-bounds read in ClientHelloParser

tniessen · RafaelGSS · commit 42225c38a443 · 2022-09-26T13:06:32.000-03:00
ClientHelloParser::ParseHeader(data, avail) potentially accesses data beyond avail bytes because it trusts the client to transmit a valid frame length. Sending an impossibly small frame length causes the TLS server to read beyond the buffer provided by the caller. Guard against this by calling End() on the ClientHelloParser when the client transmits an impossibly small frame length. The test is designed to reliable cause a segmentation fault on Linux and Windows when the buffer overrun occurs, and to trigger a spatial safety violation when compiled with an address sanitizer enabled or when running under valgrind. PR-URL: #44580 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
diff --git a/node.gyp b/node.gyp
@@ -1250,6 +1250,7 @@
             'HAVE_OPENSSL=1',
           ],
           'sources': [
+            'test/cctest/test_crypto_clienthello.cc',
             'test/cctest/test_node_crypto.cc',
           ]
         }],
diff --git a/src/crypto/crypto_clienthello.cc b/src/crypto/crypto_clienthello.cc
@@ -75,6 +75,11 @@ bool ClientHelloParser::ParseRecordHeader(const uint8_t* data, size_t avail) {
 void ClientHelloParser::ParseHeader(const uint8_t* data, size_t avail) {
   ClientHello hello;
 
+  // We need at least six bytes (one byte for kClientHello, three bytes for the
+  // length of the handshake message, and two bytes for the protocol version).
+  // If the client sent a frame that suggests a smaller ClientHello, give up.
+  if (frame_len_ < 6) return End();
+
   // >= 5 + frame size bytes for frame parsing
   if (body_offset_ + frame_len_ > avail)
     return;
diff --git a/test/cctest/test_crypto_clienthello.cc b/test/cctest/test_crypto_clienthello.cc
@@ -0,0 +1,122 @@
+#include "crypto/crypto_clienthello-inl.h"
+#include "gtest/gtest.h"
+
+// If the test is being compiled with an address sanitizer enabled, it should
+// catch the memory violation, so do not use a guard page.
+#ifdef __SANITIZE_ADDRESS__
+#define NO_GUARD_PAGE
+#elif defined(__has_feature)
+#if __has_feature(address_sanitizer)
+#define NO_GUARD_PAGE
+#endif
+#endif
+
+// If the test is running without an address sanitizer, see if we can use
+// mprotect() or VirtualProtect() to cause a segmentation fault when spatial
+// safety is violated.
+#if !defined(NO_GUARD_PAGE)
+#ifdef __linux__
+#include <sys/mman.h>
+#include <unistd.h>
+#if defined(_SC_PAGE_SIZE) && defined(PROT_NONE) && defined(PROT_READ) &&      \
+    defined(PROT_WRITE)
+#define USE_MPROTECT
+#endif
+#elif defined(_WIN32) && defined(_MSC_VER)
+#include <Windows.h>
+#include <memoryapi.h>
+#define USE_VIRTUALPROTECT
+#endif
+#endif
+
+template <size_t N>
+class OverrunGuardedBuffer {
+ public:
+  OverrunGuardedBuffer() {
+#ifdef USE_MPROTECT
+    // Place the packet right before a guard page, which, when accessed, causes
+    // a segmentation fault.
+    int page = sysconf(_SC_PAGE_SIZE);
+    EXPECT_GE(page, static_cast<int>(N));
+    alloc_base = static_cast<uint8_t*>(aligned_alloc(page, 2 * page));
+    EXPECT_NE(alloc_base, nullptr);
+    uint8_t* second_page = alloc_base + page;
+    EXPECT_EQ(mprotect(second_page, page, PROT_NONE), 0);
+    data_base = second_page - N;
+#elif defined(USE_VIRTUALPROTECT)
+    // On Windows, it works almost the same way.
+    SYSTEM_INFO system_info;
+    GetSystemInfo(&system_info);
+    DWORD page = system_info.dwPageSize;
+    alloc_base = static_cast<uint8_t*>(
+        VirtualAlloc(nullptr, 2 * page, MEM_COMMIT, PAGE_READWRITE));
+    EXPECT_NE(alloc_base, nullptr);
+    uint8_t* second_page = alloc_base + page;
+    DWORD old_prot;
+    EXPECT_NE(VirtualProtect(second_page, page, PAGE_NOACCESS, &old_prot), 0);
+    EXPECT_EQ(old_prot, PAGE_READWRITE);
+    data_base = second_page - N;
+#else
+    // Place the packet in a regular allocated buffer. The bug causes undefined
+    // behavior, which might crash the process, and when it does not, address
+    // sanitizers and valgrind will catch it.
+    alloc_base = static_cast<uint8_t*>(malloc(N));
+    EXPECT_NE(alloc_base, nullptr);
+    data_base = alloc_base;
+#endif
+  }
+
+  OverrunGuardedBuffer(const OverrunGuardedBuffer& other) = delete;
+  OverrunGuardedBuffer& operator=(const OverrunGuardedBuffer& other) = delete;
+
+  ~OverrunGuardedBuffer() {
+#ifdef USE_VIRTUALPROTECT
+    SYSTEM_INFO system_info;
+    GetSystemInfo(&system_info);
+    DWORD page = system_info.dwPageSize;
+    VirtualFree(alloc_base, 2 * system_info.dwPageSize, MEM_RELEASE);
+#else
+#ifdef USE_MPROTECT
+    // Revert page protection such that the memory can be free()'d.
+    int page = sysconf(_SC_PAGE_SIZE);
+    EXPECT_GE(page, static_cast<int>(N));
+    uint8_t* second_page = alloc_base + page;
+    EXPECT_EQ(mprotect(second_page, page, PROT_READ | PROT_WRITE), 0);
+#endif
+    free(alloc_base);
+#endif
+  }
+
+  uint8_t* data() {
+    return data_base;
+  }
+
+ private:
+  uint8_t* alloc_base;
+  uint8_t* data_base;
+};
+
+// Test that ClientHelloParser::ParseHeader() does not blindly trust the client
+// to send a valid frame length and subsequently does not read out-of-bounds.
+TEST(NodeCrypto, ClientHelloParserParseHeaderOutOfBoundsRead) {
+  using node::crypto::ClientHelloParser;
+
+  // This is the simplest packet triggering the bug.
+  const uint8_t packet[] = {0x16, 0x03, 0x01, 0x00, 0x00};
+  OverrunGuardedBuffer<sizeof(packet)> buffer;
+  memcpy(buffer.data(), packet, sizeof(packet));
+
+  // Let the ClientHelloParser parse the packet. This should not lead to a
+  // segmentation fault or to undefined behavior.
+  node::crypto::ClientHelloParser parser;
+  bool end_cb_called = false;
+  parser.Start([](void* arg, auto hello) { GTEST_FAIL(); },
+               [](void* arg) {
+                 bool* end_cb_called = static_cast<bool*>(arg);
+                 EXPECT_FALSE(*end_cb_called);
+                 *end_cb_called = true;
+               },
+               &end_cb_called);
+  parser.Parse(buffer.data(), sizeof(packet));
+  EXPECT_TRUE(end_cb_called);
+}

Original file line number	Diff line number	Diff line change
`@@ -1250,6 +1250,7 @@`
`1250`	`1250`	`'HAVE_OPENSSL=1',`
`1251`	`1251`	`],`
`1252`	`1252`	`'sources': [`
	`1253`	`+ 'test/cctest/test_crypto_clienthello.cc',`
`1253`	`1254`	`'test/cctest/test_node_crypto.cc',`
`1254`	`1255`	`]`
`1255`	`1256`	`}],`