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 6a00029 commit e55e510
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 3 deletions.
5 changes: 3 additions & 2 deletions lib/websocket-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ class WebSocketServer extends EventEmitter {
socket.on('error', socketOnError);

const key = req.headers['sec-websocket-key'];
const upgrade = req.headers.upgrade;
const version = +req.headers['sec-websocket-version'];

if (req.method !== 'GET') {
Expand All @@ -243,13 +244,13 @@ class WebSocketServer extends EventEmitter {
return;
}

if (req.headers.upgrade.toLowerCase() !== 'websocket') {
if (upgrade === undefined || upgrade.toLowerCase() !== 'websocket') {
const message = 'Invalid Upgrade header';
abortHandshakeOrEmitwsClientError(this, req, socket, 400, message);
return;
}

if (!key || !keyRegex.test(key)) {
if (key === undefined || !keyRegex.test(key)) {
const message = 'Missing or invalid Sec-WebSocket-Key header';
abortHandshakeOrEmitwsClientError(this, req, socket, 400, message);
return;
Expand Down
4 changes: 3 additions & 1 deletion lib/websocket.js
Original file line number Diff line number Diff line change
Expand Up @@ -928,7 +928,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
44 changes: 44 additions & 0 deletions test/websocket-server.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,50 @@ describe('WebSocketServer', () => {
});
});

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(),
'Invalid Upgrade header'
);
server.close(done);
});
});
});
});

it('fails if the Upgrade header field value is not "websocket"', (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 @@ -757,6 +757,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 e55e510

@Loboistaken
Copy link

Choose a reason for hiding this comment

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

Please sign in to comment.