FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls: fix out-of-bounds read in ClientHelloParser · nodejs/node@8118fc2 · GitHub

/ node Public

Commit 8118fc2

Browse files
tniessen
authored and
juanarbol
committed
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: #44580 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
1 parent debf730 commit 8118fc2

3 files changed

Lines changed: 128 additions & 0 deletions

File tree

‎node.gyp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1191,6 +1191,7 @@
11911191
'HAVE_OPENSSL=1',
11921192
],
11931193
'sources': [
1194+
'test/cctest/test_crypto_clienthello.cc',
11941195
'test/cctest/test_node_crypto.cc',
11951196
]
11961197
}],

‎src/crypto/crypto_clienthello.cc‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,11 @@ bool ClientHelloParser::ParseRecordHeader(const uint8_t* data, size_t avail) {
7575
void ClientHelloParser::ParseHeader(const uint8_t* data, size_t avail) {
7676
ClientHello hello;
7777

78+
// We need at least six bytes (one byte for kClientHello, three bytes for the
79+
// length of the handshake message, and two bytes for the protocol version).
80+
// If the client sent a frame that suggests a smaller ClientHello, give up.
81+
if (frame_len_ < 6) return End();
82+
7883
// >= 5 + frame size bytes for frame parsing
7984
if (body_offset_ + frame_len_ > avail)
8085
return;

‎test/cctest/test_crypto_clienthello.cc‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
#include "crypto/crypto_clienthello-inl.h"
2+
#include "gtest/gtest.h"
3+
4+
// If the test is being compiled with an address sanitizer enabled, it should
5+
// catch the memory violation, so do not use a guard page.
6+
#ifdef __SANITIZE_ADDRESS__
7+
#define NO_GUARD_PAGE
8+
#elif defined(__has_feature)
9+
#if __has_feature(address_sanitizer)
10+
#define NO_GUARD_PAGE
11+
#endif
12+
#endif
13+
14+
// If the test is running without an address sanitizer, see if we can use
15+
// mprotect() or VirtualProtect() to cause a segmentation fault when spatial
16+
// safety is violated.
17+
#if !defined(NO_GUARD_PAGE)
18+
#ifdef __linux__
19+
#include <sys/mman.h>
20+
#include <unistd.h>
21+
#if defined(_SC_PAGE_SIZE) && defined(PROT_NONE) && defined(PROT_READ) && \
22+
defined(PROT_WRITE)
23+
#define USE_MPROTECT
24+
#endif
25+
#elif defined(_WIN32) && defined(_MSC_VER)
26+
#include <Windows.h>
27+
#include <memoryapi.h>
28+
#define USE_VIRTUALPROTECT
29+
#endif
30+
#endif
31+
32+
template <size_t N>
33+
class OverrunGuardedBuffer {
34+
public:
35+
OverrunGuardedBuffer() {
36+
#ifdef USE_MPROTECT
37+
// Place the packet right before a guard page, which, when accessed, causes
38+
// a segmentation fault.
39+
int page = sysconf(_SC_PAGE_SIZE);
40+
EXPECT_GE(page, static_cast<int>(N));
41+
alloc_base = static_cast<uint8_t*>(aligned_alloc(page, 2 * page));
42+
EXPECT_NE(alloc_base, nullptr);
43+
uint8_t* second_page = alloc_base + page;
44+
EXPECT_EQ(mprotect(second_page, page, PROT_NONE), 0);
45+
data_base = second_page - N;
46+
#elif defined(USE_VIRTUALPROTECT)
47+
// On Windows, it works almost the same way.
48+
SYSTEM_INFO system_info;
49+
GetSystemInfo(&system_info);
50+
DWORD page = system_info.dwPageSize;
51+
alloc_base = static_cast<uint8_t*>(
52+
VirtualAlloc(nullptr, 2 * page, MEM_COMMIT, PAGE_READWRITE));
53+
EXPECT_NE(alloc_base, nullptr);
54+
uint8_t* second_page = alloc_base + page;
55+
DWORD old_prot;
56+
EXPECT_NE(VirtualProtect(second_page, page, PAGE_NOACCESS, &old_prot), 0);
57+
EXPECT_EQ(old_prot, PAGE_READWRITE);
58+
data_base = second_page - N;
59+
#else
60+
// Place the packet in a regular allocated buffer. The bug causes undefined
61+
// behavior, which might crash the process, and when it does not, address
62+
// sanitizers and valgrind will catch it.
63+
alloc_base = static_cast<uint8_t*>(malloc(N));
64+
EXPECT_NE(alloc_base, nullptr);
65+
data_base = alloc_base;
66+
#endif
67+
}
68+
69+
OverrunGuardedBuffer(const OverrunGuardedBuffer& other) = delete;
70+
OverrunGuardedBuffer& operator=(const OverrunGuardedBuffer& other) = delete;
71+
72+
~OverrunGuardedBuffer() {
73+
#ifdef USE_VIRTUALPROTECT
74+
SYSTEM_INFO system_info;
75+
GetSystemInfo(&system_info);
76+
DWORD page = system_info.dwPageSize;
77+
VirtualFree(alloc_base, 2 * system_info.dwPageSize, MEM_RELEASE);
78+
#else
79+
#ifdef USE_MPROTECT
80+
// Revert page protection such that the memory can be free()'d.
81+
int page = sysconf(_SC_PAGE_SIZE);
82+
EXPECT_GE(page, static_cast<int>(N));
83+
uint8_t* second_page = alloc_base + page;
84+
EXPECT_EQ(mprotect(second_page, page, PROT_READ | PROT_WRITE), 0);
85+
#endif
86+
free(alloc_base);
87+
#endif
88+
}
89+
90+
uint8_t* data() {
91+
return data_base;
92+
}
93+
94+
private:
95+
uint8_t* alloc_base;
96+
uint8_t* data_base;
97+
};
98+
99+
// Test that ClientHelloParser::ParseHeader() does not blindly trust the client
100+
// to send a valid frame length and subsequently does not read out-of-bounds.
101+
TEST(NodeCrypto, ClientHelloParserParseHeaderOutOfBoundsRead) {
102+
using node::crypto::ClientHelloParser;
103+
104+
// This is the simplest packet triggering the bug.
105+
const uint8_t packet[] = {0x16, 0x03, 0x01, 0x00, 0x00};
106+
OverrunGuardedBuffer<sizeof(packet)> buffer;
107+
memcpy(buffer.data(), packet, sizeof(packet));
108+
109+
// Let the ClientHelloParser parse the packet. This should not lead to a
110+
// segmentation fault or to undefined behavior.
111+
node::crypto::ClientHelloParser parser;
112+
bool end_cb_called = false;
113+
parser.Start([](void* arg, auto hello) { GTEST_FAIL(); },
114+
[](void* arg) {
115+
bool* end_cb_called = static_cast<bool*>(arg);
116+
EXPECT_FALSE(*end_cb_called);
117+
*end_cb_called = true;
118+
},
119+
&end_cb_called);
120+
parser.Parse(buffer.data(), sizeof(packet));
121+
EXPECT_TRUE(end_cb_called);
122+
}

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL