Skip to content

Commit

Permalink
[security] Fix crash when the Upgrade header cannot be read (#2231)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lpinca committed Jun 16, 2024
1 parent 8a78f87 commit 22c2876
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 2 deletions.
4 changes: 3 additions & 1 deletion lib/websocket-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,12 +210,14 @@ class WebSocketServer extends EventEmitter {
req.headers['sec-websocket-key'] !== undefined
? req.headers['sec-websocket-key'].trim()
: false;
const upgrade = req.headers.upgrade;
const version = +req.headers['sec-websocket-version'];
const extensions = {};

if (
req.method !== 'GET' ||
req.headers.upgrade.toLowerCase() !== 'websocket' ||
upgrade === undefined ||
upgrade.toLowerCase() !== 'websocket' ||
!key ||
!keyRegex.test(key) ||
(version !== 8 && version !== 13) ||
Expand Down
4 changes: 3 additions & 1 deletion lib/websocket.js
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,9 @@ function initAsClient(websocket, address, protocols, options) {

req = websocket._req = null;

if (res.headers.upgrade.toLowerCase() !== 'websocket') {
const upgrade = res.headers.upgrade;

if (upgrade === undefined || upgrade.toLowerCase() !== 'websocket') {
abortHandshake(websocket, socket, 'Invalid Upgrade header');
return;
}
Expand Down
41 changes: 41 additions & 0 deletions test/websocket-server.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,47 @@ describe('WebSocketServer', () => {
});

describe('Connection establishing', () => {
it('fails if the Upgrade header field value cannot be read', (done) => {
const server = http.createServer();
const wss = new WebSocket.Server({ noServer: true });

server.maxHeadersCount = 1;

server.on('upgrade', (req, socket, head) => {
assert.deepStrictEqual(req.headers, { foo: 'bar' });
wss.handleUpgrade(req, socket, head, () => {
done(new Error('Unexpected callback invocation'));
});
});

server.listen(() => {
const req = http.get({
port: server.address().port,
headers: {
foo: 'bar',
bar: 'baz',
Connection: 'Upgrade',
Upgrade: 'websocket'
}
});

req.on('response', (res) => {
assert.strictEqual(res.statusCode, 400);

const chunks = [];

res.on('data', (chunk) => {
chunks.push(chunk);
});

res.on('end', () => {
assert.strictEqual(Buffer.concat(chunks).toString(), 'Bad Request');
server.close(done);
});
});
});
});

it('fails if the Sec-WebSocket-Key header is invalid (1/2)', (done) => {
const wss = new WebSocket.Server({ port: 0 }, () => {
const req = http.get({
Expand Down
26 changes: 26 additions & 0 deletions test/websocket.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,32 @@ describe('WebSocket', () => {
beforeEach((done) => server.listen(0, done));
afterEach((done) => server.close(done));

it('fails if the Upgrade header field value cannot be read', (done) => {
server.once('upgrade', (req, socket) => {
socket.on('end', socket.end);
socket.write(
'HTTP/1.1 101 Switching Protocols\r\n' +
'Connection: Upgrade\r\n' +
'Upgrade: websocket\r\n' +
'\r\n'
);
});

const ws = new WebSocket(`ws://localhost:${server.address().port}`);

ws._req.maxHeadersCount = 1;

ws.on('upgrade', (res) => {
assert.deepStrictEqual(res.headers, { connection: 'Upgrade' });

ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'Invalid Upgrade header');
done();
});
});
});

it('fails if the Upgrade header field value is not "websocket"', (done) => {
server.once('upgrade', (req, socket) => {
socket.on('end', socket.end);
Expand Down

1 comment on commit 22c2876

@billykuo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Please sign in to comment.