diff --git a/CHANGELOG.md b/CHANGELOG.md index 45c9c8da..60ebdb9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## 2023 +- [6.4.2](#642-2023-05-02) (May 2023) - [6.4.1](#641-2023-02-20) (Feb 2023) - [6.4.0](#640-2023-02-06) (Feb 2023) - [6.3.1](#631-2023-01-12) (Jan 2023) @@ -46,6 +47,39 @@ # Release notes +## [6.4.2](https://github.com/socketio/engine.io/compare/6.4.1...6.4.2) (2023-05-02) + +:warning: This release contains an important security fix :warning: + +A malicious client could send a specially crafted HTTP request, triggering an uncaught exception and killing the Node.js process: + +``` +TypeError: Cannot read properties of undefined (reading 'handlesUpgrades') + at Server.onWebSocket (build/server.js:515:67) +``` + +Please upgrade as soon as possible. + + +### Bug Fixes + +* include error handling for Express middlewares ([#674](https://github.com/socketio/engine.io/issues/674)) ([9395782](https://github.com/socketio/engine.io/commit/93957828be1252c83275b56f0c7c0bd145a0ceb9)) +* prevent crash when provided with an invalid query param ([fc480b4](https://github.com/socketio/engine.io/commit/fc480b4f305e16fe5972cf337d055e598372dc44)) +* **typings:** make clientsCount public ([#675](https://github.com/socketio/engine.io/issues/675)) ([bd6d471](https://github.com/socketio/engine.io/commit/bd6d4713b02ff646c581872cd9ffe753acff0d73)) +* **uws:** prevent crash when using with middlewares ([8b22162](https://github.com/socketio/engine.io/commit/8b2216290330b174c9e67be32765bec0c74769f9)) + + +### Credits + +Huge thanks to [@tyilo](https://github.com/tyilo) and [@cieldeville](https://github.com/cieldeville) for helping! + + +### Dependencies + +- [`ws@~8.11.0`](https://github.com/websockets/ws/releases/tag/8.11.0) (no change) + + + ## [6.4.1](https://github.com/socketio/engine.io/compare/6.4.0...6.4.1) (2023-02-20) This release contains [6e78489](https://github.com/socketio/engine.io/commit/6e78489486f0d7570861fd6002a364d1ab87da4a), which exports the `BaseServer` class in order to restore the compatibility with the `nodenext` module resolution strategy of TypeScript. diff --git a/lib/server.ts b/lib/server.ts index ac493bbc..a4a4dcf8 100644 --- a/lib/server.ts +++ b/lib/server.ts @@ -137,14 +137,14 @@ export interface ServerOptions { type Middleware = ( req: IncomingMessage, res: ServerResponse, - next: () => void + next: (err?: any) => void ) => void; export abstract class BaseServer extends EventEmitter { public opts: ServerOptions; protected clients: any; - private clientsCount: number; + public clientsCount: number; protected middlewares: Middleware[] = []; /** @@ -320,7 +320,7 @@ export abstract class BaseServer extends EventEmitter { * * @param fn */ - public use(fn: Middleware) { + public use(fn: any) { this.middlewares.push(fn); } @@ -335,7 +335,7 @@ export abstract class BaseServer extends EventEmitter { protected _applyMiddlewares( req: IncomingMessage, res: ServerResponse, - callback: () => void + callback: (err?: any) => void ) { if (this.middlewares.length === 0) { debug("no middleware to apply, skipping"); @@ -344,7 +344,11 @@ export abstract class BaseServer extends EventEmitter { const apply = (i) => { debug("applying middleware n°%d", i + 1); - this.middlewares[i](req, res, () => { + this.middlewares[i](req, res, (err?: any) => { + if (err) { + return callback(err); + } + if (i + 1 < this.middlewares.length) { apply(i + 1); } else { @@ -655,8 +659,12 @@ export class Server extends BaseServer { } }; - this._applyMiddlewares(req, res, () => { - this.verify(req, false, callback); + this._applyMiddlewares(req, res, (err) => { + if (err) { + callback(Server.errors.BAD_REQUEST, { name: "MIDDLEWARE_FAILURE" }); + } else { + this.verify(req, false, callback); + } }); } @@ -673,32 +681,37 @@ export class Server extends BaseServer { this.prepare(req); const res = new WebSocketResponse(req, socket); + const callback = (errorCode, errorContext) => { + if (errorCode !== undefined) { + this.emit("connection_error", { + req, + code: errorCode, + message: Server.errorMessages[errorCode], + context: errorContext, + }); + abortUpgrade(socket, errorCode, errorContext); + return; + } - this._applyMiddlewares(req, res as unknown as ServerResponse, () => { - this.verify(req, true, (errorCode, errorContext) => { - if (errorCode) { - this.emit("connection_error", { - req, - code: errorCode, - message: Server.errorMessages[errorCode], - context: errorContext, - }); - abortUpgrade(socket, errorCode, errorContext); - return; - } - - const head = Buffer.from(upgradeHead); - upgradeHead = null; + const head = Buffer.from(upgradeHead); + upgradeHead = null; - // some middlewares (like express-session) wait for the writeHead() call to flush their headers - // see https://github.com/expressjs/session/blob/1010fadc2f071ddf2add94235d72224cf65159c6/index.js#L220-L244 - res.writeHead(); + // some middlewares (like express-session) wait for the writeHead() call to flush their headers + // see https://github.com/expressjs/session/blob/1010fadc2f071ddf2add94235d72224cf65159c6/index.js#L220-L244 + res.writeHead(); - // delegate to ws - this.ws.handleUpgrade(req, socket, head, (websocket) => { - this.onWebSocket(req, socket, websocket); - }); + // delegate to ws + this.ws.handleUpgrade(req, socket, head, (websocket) => { + this.onWebSocket(req, socket, websocket); }); + }; + + this._applyMiddlewares(req, res as unknown as ServerResponse, (err) => { + if (err) { + callback(Server.errors.BAD_REQUEST, { name: "MIDDLEWARE_FAILURE" }); + } else { + this.verify(req, true, callback); + } }); } diff --git a/lib/transports/polling.ts b/lib/transports/polling.ts index 69a968eb..70be3411 100644 --- a/lib/transports/polling.ts +++ b/lib/transports/polling.ts @@ -75,8 +75,7 @@ export class Polling extends Transport { debug("request overlap"); // assert: this.res, '.req and .res should be (un)set together' this.onError("overlap from client"); - // TODO for the next major release: use an HTTP 400 status code (https://github.com/socketio/engine.io/issues/650) - res.writeHead(500); + res.writeHead(400); res.end(); return; } @@ -117,8 +116,7 @@ export class Polling extends Transport { if (this.dataReq) { // assert: this.dataRes, '.dataReq and .dataRes should be (un)set together' this.onError("data request overlap from client"); - // TODO for the next major release: use an HTTP 400 status code (https://github.com/socketio/engine.io/issues/650) - res.writeHead(500); + res.writeHead(400); res.end(); return; } diff --git a/lib/userver.ts b/lib/userver.ts index 29729bd1..951d3b96 100644 --- a/lib/userver.ts +++ b/lib/userver.ts @@ -92,7 +92,11 @@ export class uServer extends BaseServer { }); } - override _applyMiddlewares(req: any, res: any, callback: () => void): void { + override _applyMiddlewares( + req: any, + res: any, + callback: (err?: any) => void + ): void { if (this.middlewares.length === 0) { return callback(); } @@ -100,12 +104,12 @@ export class uServer extends BaseServer { // needed to buffer headers until the status is computed req.res = new ResponseWrapper(res); - super._applyMiddlewares(req, req.res, () => { + super._applyMiddlewares(req, req.res, (err) => { // some middlewares (like express-session) wait for the writeHead() call to flush their headers // see https://github.com/expressjs/session/blob/1010fadc2f071ddf2add94235d72224cf65159c6/index.js#L220-L244 req.res.writeHead(); - callback(); + callback(err); }); } @@ -118,28 +122,34 @@ export class uServer extends BaseServer { req.res = res; - this._applyMiddlewares(req, res, () => { - this.verify(req, false, (errorCode, errorContext) => { - if (errorCode !== undefined) { - this.emit("connection_error", { - req, - code: errorCode, - message: Server.errorMessages[errorCode], - context: errorContext, - }); - this.abortRequest(req.res, errorCode, errorContext); - return; - } + const callback = (errorCode, errorContext) => { + if (errorCode !== undefined) { + this.emit("connection_error", { + req, + code: errorCode, + message: Server.errorMessages[errorCode], + context: errorContext, + }); + this.abortRequest(req.res, errorCode, errorContext); + return; + } + + if (req._query.sid) { + debug("setting new request for existing client"); + this.clients[req._query.sid].transport.onRequest(req); + } else { + const closeConnection = (errorCode, errorContext) => + this.abortRequest(res, errorCode, errorContext); + this.handshake(req._query.transport, req, closeConnection); + } + }; - if (req._query.sid) { - debug("setting new request for existing client"); - this.clients[req._query.sid].transport.onRequest(req); - } else { - const closeConnection = (errorCode, errorContext) => - this.abortRequest(res, errorCode, errorContext); - this.handshake(req._query.transport, req, closeConnection); - } - }); + this._applyMiddlewares(req, res, (err) => { + if (err) { + callback(Server.errors.BAD_REQUEST, { name: "MIDDLEWARE_FAILURE" }); + } else { + this.verify(req, false, callback); + } }); } @@ -154,63 +164,69 @@ export class uServer extends BaseServer { req.res = res; - this._applyMiddlewares(req, res, () => { - this.verify(req, true, async (errorCode, errorContext) => { - if (errorCode) { - this.emit("connection_error", { - req, - code: errorCode, - message: Server.errorMessages[errorCode], - context: errorContext, - }); - this.abortRequest(res, errorCode, errorContext); + const callback = async (errorCode, errorContext) => { + if (errorCode !== undefined) { + this.emit("connection_error", { + req, + code: errorCode, + message: Server.errorMessages[errorCode], + context: errorContext, + }); + this.abortRequest(res, errorCode, errorContext); + return; + } + + const id = req._query.sid; + let transport; + + if (id) { + const client = this.clients[id]; + if (!client) { + debug("upgrade attempt for closed client"); + res.close(); + } else if (client.upgrading) { + debug("transport has already been trying to upgrade"); + res.close(); + } else if (client.upgraded) { + debug("transport had already been upgraded"); + res.close(); + } else { + debug("upgrading existing transport"); + transport = this.createTransport(req._query.transport, req); + client.maybeUpgrade(transport); + } + } else { + transport = await this.handshake( + req._query.transport, + req, + (errorCode, errorContext) => + this.abortRequest(res, errorCode, errorContext) + ); + if (!transport) { return; } + } - const id = req._query.sid; - let transport; - - if (id) { - const client = this.clients[id]; - if (!client) { - debug("upgrade attempt for closed client"); - res.close(); - } else if (client.upgrading) { - debug("transport has already been trying to upgrade"); - res.close(); - } else if (client.upgraded) { - debug("transport had already been upgraded"); - res.close(); - } else { - debug("upgrading existing transport"); - transport = this.createTransport(req._query.transport, req); - client.maybeUpgrade(transport); - } - } else { - transport = await this.handshake( - req._query.transport, - req, - (errorCode, errorContext) => - this.abortRequest(res, errorCode, errorContext) - ); - if (!transport) { - return; - } - } + // calling writeStatus() triggers the flushing of any header added in a middleware + req.res.writeStatus("101 Switching Protocols"); - // calling writeStatus() triggers the flushing of any header added in a middleware - req.res.writeStatus("101 Switching Protocols"); - - res.upgrade( - { - transport, - }, - req.getHeader("sec-websocket-key"), - req.getHeader("sec-websocket-protocol"), - req.getHeader("sec-websocket-extensions"), - context - ); - }); + res.upgrade( + { + transport, + }, + req.getHeader("sec-websocket-key"), + req.getHeader("sec-websocket-protocol"), + req.getHeader("sec-websocket-extensions"), + context + ); + }; + + this._applyMiddlewares(req, res, (err) => { + if (err) { + callback(Server.errors.BAD_REQUEST, { name: "MIDDLEWARE_FAILURE" }); + } else { + this.verify(req, true, callback); + } }); } @@ -278,6 +294,7 @@ class ResponseWrapper { this.res.writeStatus(status); this.statusWritten = true; this.writeBufferedHeaders(); + return this; } public writeHeader(key: string, value: string) { diff --git a/package.json b/package.json index a368fda6..0a8e2459 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "engine.io", - "version": "6.4.1", + "version": "6.4.2", "description": "The realtime engine behind Socket.IO. Provides the foundation of a bidirectional connection between client and server", "type": "commonjs", "main": "./build/engine.io.js", diff --git a/test/middlewares.js b/test/middlewares.js index a81f1354..586df621 100644 --- a/test/middlewares.js +++ b/test/middlewares.js @@ -4,6 +4,7 @@ const request = require("superagent"); const { WebSocket } = require("ws"); const helmet = require("helmet"); const session = require("express-session"); +const { ClientSocket } = require("./common"); describe("middlewares", () => { it("should apply middleware (polling)", (done) => { @@ -247,4 +248,74 @@ describe("middlewares", () => { }); }); }); + + it("should fail on errors (polling)", (done) => { + const engine = listen((port) => { + engine.use((req, res, next) => { + next(new Error("will always fail")); + }); + + request + .get(`http://localhost:${port}/engine.io/`) + .query({ EIO: 4, transport: "polling" }) + .end((err, res) => { + expect(err).to.be.an(Error); + expect(res.status).to.eql(400); + + if (engine.httpServer) { + engine.httpServer.close(); + } + done(); + }); + }); + + it("should fail on errors (websocket)", (done) => { + const engine = listen((port) => { + engine.use((req, res, next) => { + next(new Error("will always fail")); + }); + + engine.on("connection", () => { + done(new Error("should not connect")); + }); + + const socket = new WebSocket( + `ws://localhost:${port}/engine.io/?EIO=4&transport=websocket` + ); + + socket.addEventListener("error", () => { + if (engine.httpServer) { + engine.httpServer.close(); + } + done(); + }); + }); + }); + }); + + it("should not be receiving data when getting a message longer than maxHttpBufferSize when polling (with a middleware)", (done) => { + const opts = { + allowUpgrades: false, + transports: ["polling"], + maxHttpBufferSize: 5, + }; + const engine = listen(opts, (port) => { + engine.use((req, res, next) => { + next(); + }); + + const socket = new ClientSocket(`ws://localhost:${port}`); + engine.on("connection", (conn) => { + conn.on("message", () => { + done(new Error("Test invalidation (message is longer than allowed)")); + }); + }); + socket.on("open", () => { + socket.send("aasdasdakjhasdkjhasdkjhasdkjhasdkjhasdkjhasdkjha"); + }); + socket.on("close", (reason) => { + done(); + }); + }); + }); }); diff --git a/test/server.js b/test/server.js index 19dd3fbe..bfda7cd9 100644 --- a/test/server.js +++ b/test/server.js @@ -11,6 +11,7 @@ const { ClientSocket, listen, createPartialDone } = require("./common"); const expect = require("expect.js"); const request = require("superagent"); const cookieMod = require("cookie"); +const { WebSocket } = require("ws"); /** * Tests. @@ -197,6 +198,51 @@ describe("server", () => { }); }); }); + + it("should disallow `__proto__` as transport (polling)", (done) => { + const partialDone = createPartialDone(done, 2); + + engine = listen((port) => { + engine.on("connection_error", (err) => { + expect(err.req).to.be.ok(); + expect(err.code).to.be(0); + expect(err.message).to.be("Transport unknown"); + expect(err.context.transport).to.be("__proto__"); + partialDone(); + }); + + request + .get(`http://localhost:${port}/engine.io/`) + .query({ transport: "__proto__", EIO: 4 }) + .end((err, res) => { + expect(err).to.be.an(Error); + expect(res.status).to.be(400); + expect(res.body.code).to.be(0); + expect(res.body.message).to.be("Transport unknown"); + partialDone(); + }); + }); + }); + + it("should disallow `__proto__` as transport (websocket)", (done) => { + const partialDone = createPartialDone(done, 2); + + engine = listen((port) => { + engine.on("connection_error", (err) => { + expect(err.req).to.be.ok(); + expect(err.code).to.be(0); + expect(err.message).to.be("Transport unknown"); + expect(err.context.transport).to.be("__proto__"); + partialDone(); + }); + + const socket = new WebSocket( + `ws://localhost:${port}/engine.io/?EIO=4&transport=__proto__` + ); + + socket.onerror = partialDone; + }); + }); }); describe("handshake", () => {