Skip to content

Commit 22c2876

Browse files
committed
[security] Fix crash when the Upgrade header cannot be read (#2231)
It is possible that the Upgrade header is correctly received and handled (the `'upgrade'` event is emitted) without its value being returned to the user. This can happen if the number of received headers exceed the `server.maxHeadersCount` or `request.maxHeadersCount` threshold. In this case `incomingMessage.headers.upgrade` may not be set. Handle the case correctly and abort the handshake. Fixes #2230
1 parent 8a78f87 commit 22c2876

File tree

4 files changed

+73
-2
lines changed

4 files changed

+73
-2
lines changed

lib/websocket-server.js

+3-1
Original file line numberDiff line numberDiff line change
@@ -210,12 +210,14 @@ class WebSocketServer extends EventEmitter {
210210
req.headers['sec-websocket-key'] !== undefined
211211
? req.headers['sec-websocket-key'].trim()
212212
: false;
213+
const upgrade = req.headers.upgrade;
213214
const version = +req.headers['sec-websocket-version'];
214215
const extensions = {};
215216

216217
if (
217218
req.method !== 'GET' ||
218-
req.headers.upgrade.toLowerCase() !== 'websocket' ||
219+
upgrade === undefined ||
220+
upgrade.toLowerCase() !== 'websocket' ||
219221
!key ||
220222
!keyRegex.test(key) ||
221223
(version !== 8 && version !== 13) ||

lib/websocket.js

+3-1
Original file line numberDiff line numberDiff line change
@@ -799,7 +799,9 @@ function initAsClient(websocket, address, protocols, options) {
799799

800800
req = websocket._req = null;
801801

802-
if (res.headers.upgrade.toLowerCase() !== 'websocket') {
802+
const upgrade = res.headers.upgrade;
803+
804+
if (upgrade === undefined || upgrade.toLowerCase() !== 'websocket') {
803805
abortHandshake(websocket, socket, 'Invalid Upgrade header');
804806
return;
805807
}

test/websocket-server.test.js

+41
Original file line numberDiff line numberDiff line change
@@ -494,6 +494,47 @@ describe('WebSocketServer', () => {
494494
});
495495

496496
describe('Connection establishing', () => {
497+
it('fails if the Upgrade header field value cannot be read', (done) => {
498+
const server = http.createServer();
499+
const wss = new WebSocket.Server({ noServer: true });
500+
501+
server.maxHeadersCount = 1;
502+
503+
server.on('upgrade', (req, socket, head) => {
504+
assert.deepStrictEqual(req.headers, { foo: 'bar' });
505+
wss.handleUpgrade(req, socket, head, () => {
506+
done(new Error('Unexpected callback invocation'));
507+
});
508+
});
509+
510+
server.listen(() => {
511+
const req = http.get({
512+
port: server.address().port,
513+
headers: {
514+
foo: 'bar',
515+
bar: 'baz',
516+
Connection: 'Upgrade',
517+
Upgrade: 'websocket'
518+
}
519+
});
520+
521+
req.on('response', (res) => {
522+
assert.strictEqual(res.statusCode, 400);
523+
524+
const chunks = [];
525+
526+
res.on('data', (chunk) => {
527+
chunks.push(chunk);
528+
});
529+
530+
res.on('end', () => {
531+
assert.strictEqual(Buffer.concat(chunks).toString(), 'Bad Request');
532+
server.close(done);
533+
});
534+
});
535+
});
536+
});
537+
497538
it('fails if the Sec-WebSocket-Key header is invalid (1/2)', (done) => {
498539
const wss = new WebSocket.Server({ port: 0 }, () => {
499540
const req = http.get({

test/websocket.test.js

+26
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,32 @@ describe('WebSocket', () => {
528528
beforeEach((done) => server.listen(0, done));
529529
afterEach((done) => server.close(done));
530530

531+
it('fails if the Upgrade header field value cannot be read', (done) => {
532+
server.once('upgrade', (req, socket) => {
533+
socket.on('end', socket.end);
534+
socket.write(
535+
'HTTP/1.1 101 Switching Protocols\r\n' +
536+
'Connection: Upgrade\r\n' +
537+
'Upgrade: websocket\r\n' +
538+
'\r\n'
539+
);
540+
});
541+
542+
const ws = new WebSocket(`ws://localhost:${server.address().port}`);
543+
544+
ws._req.maxHeadersCount = 1;
545+
546+
ws.on('upgrade', (res) => {
547+
assert.deepStrictEqual(res.headers, { connection: 'Upgrade' });
548+
549+
ws.on('error', (err) => {
550+
assert.ok(err instanceof Error);
551+
assert.strictEqual(err.message, 'Invalid Upgrade header');
552+
done();
553+
});
554+
});
555+
});
556+
531557
it('fails if the Upgrade header field value is not "websocket"', (done) => {
532558
server.once('upgrade', (req, socket) => {
533559
socket.on('end', socket.end);

0 commit comments

Comments
 (0)