From c0e194d44933bd83bf9a4b126fca68ba7bf5098c Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Tue, 11 Jan 2022 15:34:18 +0100 Subject: [PATCH] fix: properly handle invalid data sent by a malicious websocket client **IMPORTANT SECURITY FIX** A malicious client could send a specially crafted HTTP request, triggering an uncaught exception and killing the Node.js process: > RangeError: Invalid WebSocket frame: RSV2 and RSV3 must be clear > at Receiver.getInfo (/.../node_modules/ws/lib/receiver.js:176:14) > at Receiver.startLoop (/.../node_modules/ws/lib/receiver.js:136:22) > at Receiver._write (/.../node_modules/ws/lib/receiver.js:83:10) > at writeOrBuffer (internal/streams/writable.js:358:12) This bug was introduced by [1], included in `engine.io@4.0.0`, so previous releases are not impacted. [1]: https://github.com/socketio/engine.io/commit/f3c291fa613a9d50c924d74293035737fdace4f2 Thanks to Marcus Wejderot from Mevisio for the responsible disclosure. --- lib/server.ts | 3 --- test/server.js | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/lib/server.ts b/lib/server.ts index 78648dfc..c09b2e1d 100644 --- a/lib/server.ts +++ b/lib/server.ts @@ -609,9 +609,6 @@ export class Server extends BaseServer { client.maybeUpgrade(transport); } } else { - // transport error handling takes over - websocket.removeListener("error", onUpgradeError); - const closeConnection = (errorCode, errorContext) => abortUpgrade(socket, errorCode, errorContext); this.handshake(req._query.transport, req, closeConnection); diff --git a/test/server.js b/test/server.js index 80086c84..05f5d7bf 100644 --- a/test/server.js +++ b/test/server.js @@ -157,6 +157,46 @@ describe("server", () => { } ); }); + + it("should not throw when the client sends invalid data during the handshake (ws only)", done => { + listen(port => { + // will throw "RangeError: Invalid WebSocket frame: RSV2 and RSV3 must be clear" + request + .get(`http://localhost:${port}/engine.io/`) + .set("connection", "upgrade") + .set("upgrade", "websocket") + .set("Sec-WebSocket-Version", "13") + .set("Sec-WebSocket-Key", "DXR4dX615eRds8nRmlhqtw==") + .query({ transport: "websocket", EIO: 4 }) + .send("test") + .end(() => {}); + + setTimeout(done, 50); + }); + }); + + it("should not throw when the client sends invalid data during the handshake (upgrade)", done => { + listen(port => { + request + .get(`http://localhost:${port}/engine.io/`) + .query({ transport: "polling", EIO: 4 }) + .end((err, res) => { + const sid = JSON.parse(res.text.substring(1)).sid; + + request + .get(`http://localhost:${port}/engine.io/`) + .set("connection", "upgrade") + .set("upgrade", "websocket") + .set("Sec-WebSocket-Version", "13") + .set("Sec-WebSocket-Key", "DXR4dX615eRds8nRmlhqtw==") + .query({ transport: "websocket", EIO: 4, sid }) + .send("test") + .end(() => {}); + + setTimeout(done, 50); + }); + }); + }); }); describe("handshake", () => {