From 752e1472e1913a1f375422228ca9dd703d675e7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 15 Sep 2022 19:27:04 +0200 Subject: [PATCH] tls: fix out-of-bounds read in ClientHelloParser 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: https://github.com/nodejs/node/pull/44580 Reviewed-By: Ben Noordhuis Reviewed-By: Rich Trott Reviewed-By: Minwoo Jung --- node.gyp | 1 + src/crypto/crypto_clienthello.cc | 5 + test/cctest/test_crypto_clienthello.cc | 122 +++++++++++++++++++++++++ 3 files changed, 128 insertions(+) create mode 100644 test/cctest/test_crypto_clienthello.cc diff --git a/node.gyp b/node.gyp index 2dfa91fea6606e..b94a2b1d98628c 100644 --- 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 index 5a0be70bc11e5f..7da05e9b474a64 100644 --- 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 new file mode 100644 index 00000000000000..10917fe31da1f6 --- /dev/null +++ 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 +#include +#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 +#include +#define USE_VIRTUALPROTECT +#endif +#endif + +template +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(N)); + alloc_base = static_cast(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( + 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(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(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 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(arg); + EXPECT_FALSE(*end_cb_called); + *end_cb_called = true; + }, + &end_cb_called); + parser.Parse(buffer.data(), sizeof(packet)); + EXPECT_TRUE(end_cb_called); +}