From 5f25aad273326cbc8ae69ab1d4292ba6da3f24bb Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Tue, 16 Jan 2024 05:22:45 +0100 Subject: [PATCH] Use X-Forwarded-For header on trusted proxies If the controller is behind a proxy then the socket.remoteAddress will be that of the proxy instead of the end client that connected. In this case the X-Forwarded-For header will contain the address of the end client, but this should only be relied upon if the party connecting is a trusted proxy, otherwise a malicious user could spoof the IP address that is logged by providing a forged X-Forwarded-For header. Implement a trusted proxy list consisting of a configured list of IP addresses and/or CIDR blocks and use the last address[1] in the X-Forwarded-For header if the remoteAddress address of the connecting party is in the trusted proxy list. [1]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For --- packages/controller/index.ts | 1 + packages/controller/src/Controller.ts | 34 +++++++++++- packages/controller/src/WsServer.ts | 30 ++++++++--- packages/lib/src/config/definitions.ts | 8 +++ test/controller/Controller.js | 54 +++++++++++++++++++ test/controller/WsServer.js | 73 ++++++++++++++++++++++++++ 6 files changed, 191 insertions(+), 9 deletions(-) create mode 100644 test/controller/Controller.js create mode 100644 test/controller/WsServer.js diff --git a/packages/controller/index.ts b/packages/controller/index.ts index 89287589..5218034f 100644 --- a/packages/controller/index.ts +++ b/packages/controller/index.ts @@ -5,6 +5,7 @@ export { default as ControlConnection } from "./src/ControlConnection"; export { default as InstanceInfo } from "./src/InstanceInfo"; export { default as BaseControllerPlugin } from "./src/BaseControllerPlugin"; export { default as UserManager } from "./src/UserManager"; +export { default as WsServer } from "./src/WsServer"; if (module === require.main) { bootstrap(); diff --git a/packages/controller/src/Controller.ts b/packages/controller/src/Controller.ts index f1627c1e..1c299927 100644 --- a/packages/controller/src/Controller.ts +++ b/packages/controller/src/Controller.ts @@ -1,5 +1,5 @@ import type winston from "winston"; -import type { AddressInfo } from "net"; +import { BlockList, type AddressInfo, isIPv4, isIPv6 } from "net"; import type { ControllerArgs } from "../controller"; import express, { type Request, type Response, type NextFunction, type Application } from "express"; import type { Static } from "@sinclair/typebox"; @@ -74,6 +74,7 @@ export default class Controller { /** WebSocket server */ wsServer: WsServer; + trustedProxies: BlockList; debugEvents: events.EventEmitter = new events.EventEmitter(); private _events: events.EventEmitter = new events.EventEmitter(); @@ -153,6 +154,7 @@ export default class Controller { this.app.locals.controller = this; this.app.locals.streams = new Map(); + this.trustedProxies = this.parseTrustedProxies(); this.wsServer = new WsServer(this); this.modStore.on("change", mod => { @@ -228,6 +230,8 @@ export default class Controller { this.onAutosaveIntervalChanged(); } else if (field === "controller.system_metrics_interval") { this.onSystemMetricsIntervalChanged(); + } else if (field === "controller.trusted_proxies") { + this.trustedProxies = this.parseTrustedProxies(); } lib.invokeHook(this.plugins, "onControllerConfigFieldChanged", field, curr, prev); }); @@ -404,6 +408,34 @@ export default class Controller { logger.info("Goodbye"); } + parseTrustedProxies() { + const trustedProxies = new BlockList(); + const proxiesString = this.config.get("controller.trusted_proxies"); + if (proxiesString) { + const proxies = proxiesString.split(",").map(s => s.trim()); + for (const proxy of proxies) { + const [ip, prefix] = proxy.split("/"); + // eslint-disable-next-line no-nested-ternary + const type = isIPv4(ip) ? "ipv4" : isIPv6(ip) ? "ipv6" : undefined; + if (!type) { + logger.error(`Invalid proxy '${proxy}': not an IP address`); + continue; + } + try { + if (prefix) { + trustedProxies.addSubnet(ip, Number.parseInt(prefix, 10), type); + } else { + trustedProxies.addAddress(ip, type); + } + } catch (err: any) { + logger.error(`Invalid proxy '${proxy}': ${err.message}`); + } + } + } + + return trustedProxies; + } + onSystemMetricsIntervalChanged() { if (this.systemMetricsInterval) { clearInterval(this.systemMetricsInterval); diff --git a/packages/controller/src/WsServer.ts b/packages/controller/src/WsServer.ts index 3fc0d988..4ebb257f 100644 --- a/packages/controller/src/WsServer.ts +++ b/packages/controller/src/WsServer.ts @@ -59,6 +59,20 @@ export default class WsServer { ); } + remoteAddr(req: IncomingMessage) { + const remote = req.socket.remoteAddress ?? ""; + if ( + remote && req.headers["x-forwarded-for"] + && ( + (req.socket.remoteFamily === "IPv4" && this.controller.trustedProxies.check(remote, "ipv4")) + || (req.socket.remoteFamily === "IPv6" && this.controller.trustedProxies.check(remote, "ipv6")) + ) + ) { + return (req.headers["x-forwarded-for"] as string).split(",").slice(-1)[0].trim(); + } + return remote; + } + async stop(): Promise { this.stopAcceptingNewSessions = true; @@ -100,7 +114,7 @@ export default class WsServer { } handleConnection(socket: lib.WebSocketClusterio, req: IncomingMessage) { - logger.verbose(`WsServer | new connection from ${req.socket.remoteAddress}`); + logger.verbose(`WsServer | new connection from ${this.remoteAddr(req)}`); // Track statistics wsConnectionsCounter.inc(); @@ -136,7 +150,7 @@ export default class WsServer { this.pendingSockets.add(socket); let timeoutId = setTimeout(() => { - logger.verbose(`WsServer | closing ${req.socket.remoteAddress} after timing out on handshake`); + logger.verbose(`WsServer | closing ${this.remoteAddr(req)} after timing out on handshake`); wsRejectedConnectionsCounter.inc(); socket.terminate(); this.pendingSockets.delete(socket); @@ -168,7 +182,7 @@ ${err.stack}` lib.Message.validate(json); message = lib.Message.fromJSON(json); } catch (err: any) { - logger.verbose(`WsServer | closing ${req.socket.remoteAddress} after receiving invalid message`); + logger.verbose(`WsServer | closing ${this.remoteAddr(req)} after receiving invalid message`); wsRejectedConnectionsCounter.inc(); socket.close(1002, `Invalid message: ${err.message}`); return; @@ -180,14 +194,14 @@ ${err.stack}` } if (message.type !== "registerHost" && message.type !== "registerControl") { - logger.verbose(`WsServer | closing ${req.socket.remoteAddress} after receiving invalid handshake`); + logger.verbose(`WsServer | closing ${this.remoteAddr(req)} after receiving invalid handshake`); wsRejectedConnectionsCounter.inc(); socket.close(1002, "Bad handshake"); return; } if (this.stopAcceptingNewSessions) { - logger.verbose(`WsServer | closing ${req.socket.remoteAddress}, server is shutting down`); + logger.verbose(`WsServer | closing ${this.remoteAddr(req)}, server is shutting down`); wsRejectedConnectionsCounter.inc(); socket.close(1001, "Shutting down"); return; @@ -247,7 +261,7 @@ ${err.stack}` throw new Error("invalid token"); } } catch (err: any) { - logger.verbose(`WsServer | authentication failed for ${req.socket.remoteAddress}`); + logger.verbose(`WsServer | authentication failed for ${this.remoteAddr(req)}`); wsRejectedConnectionsCounter.inc(); socket.close(4003, `Authentication failed: ${err.message}`); return; @@ -306,7 +320,7 @@ ${err.stack}` user.checkPermission("core.control.connect"); } catch (err: any) { - logger.verbose(`WsServer | authentication failed for ${req.socket.remoteAddress}`); + logger.verbose(`WsServer | authentication failed for ${this.remoteAddr(req)}`); wsRejectedConnectionsCounter.inc(); socket.close(4003, `Authentication failed: ${err.message}`); return; @@ -316,7 +330,7 @@ ${err.stack}` this.nextControlId += 1; let [connector, sessionToken] = this.createSession(new lib.Address(lib.Address.control, id)); - logger.verbose(`WsServer | registered control ${id} from ${req.socket.remoteAddress}`); + logger.verbose(`WsServer | registered control ${id} from ${this.remoteAddr(req)}`); let connection = new ControlConnection(data, connector, this.controller, user, id); connector.on("close", () => { this.controlConnections.delete(id); diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index 69f54d79..d0cbb114 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -10,6 +10,7 @@ export interface ControllerConfigFields { "controller.http_port": number | null; "controller.https_port": number | null; "controller.bind_address": string | null; + "controller.trusted_proxies": string | null; "controller.external_address": string | null; "controller.tls_certificate": string | null; "controller.tls_private_key": string | null; @@ -78,6 +79,13 @@ export class ControllerConfig extends classes.Config { type: "string", optional: true, }, + "controller.trusted_proxies": { + title: "Trusted Proxies", + description: + "Comma separated list of IP addresses and/or CIDR blocks to trust the X-Forwarded-For header on", + type: "string", + optional: true, + }, "controller.external_address": { title: "External Address", description: "Public facing address the controller is hosted on.", diff --git a/test/controller/Controller.js b/test/controller/Controller.js new file mode 100644 index 00000000..a1aadde7 --- /dev/null +++ b/test/controller/Controller.js @@ -0,0 +1,54 @@ +"use strict"; +const assert = require("assert").strict; +const { Controller } = require("@clusterio/controller"); +const { ControllerConfig } = require("@clusterio/lib"); + +describe("controller/src/Controller", function() { + describe("class Controller", function() { + /** @type {Controller} */ + let controller; + before(function() { + controller = new Controller({}, [], "", new ControllerConfig("controller")); + }); + function check(list, ip, present = true) { + if (present === false) { + assert(!list.check(ip), `${ip} unexpectedly in list`); + } else { + assert(list.check(ip), `${ip} missing from list`); + } + } + describe(".parseTrustedProxies()", function() { + it("should parse addresses", function() { + controller.config.set("controller.trusted_proxies", "10.0.0.1"); + let list = controller.parseTrustedProxies(); + check(list, "10.0.0.1"); + check(list, "10.0.0.2", false); + controller.config.set("controller.trusted_proxies", "10.0.0.1, 10.0.0.2, 10.1.2.3"); + list = controller.parseTrustedProxies(); + check(list, "10.0.0.1"); + check(list, "10.0.0.2"); + check(list, "10.0.0.3", false); + }); + it("should ignore invalid entries", function() { + controller.config.set("controller.trusted_proxies", "10.0.0.1, invalid, 0.0.0.0/200, 10.0.0.2"); + const list = controller.parseTrustedProxies(); + check(list, "10.0.0.1"); + check(list, "10.0.0.2"); + check(list, "10.0.0.3", false); + }); + it("should parse CIDR blocks", function() { + controller.config.set("controller.trusted_proxies", "10.0.0.1/24, 192.168.0.16/31"); + const list = controller.parseTrustedProxies(); + check(list, "10.0.0.0"); + check(list, "10.0.0.1"); + check(list, "10.0.0.10"); + check(list, "10.0.0.255"); + check(list, "10.0.1.0", false); + check(list, "192.168.0.15", false); + check(list, "192.168.0.16"); + check(list, "192.168.0.17"); + check(list, "192.168.0.18", false); + }); + }); + }); +}); diff --git a/test/controller/WsServer.js b/test/controller/WsServer.js new file mode 100644 index 00000000..46f0bd47 --- /dev/null +++ b/test/controller/WsServer.js @@ -0,0 +1,73 @@ +"use strict"; +const assert = require("assert").strict; +const { Controller, WsServer } = require("@clusterio/controller"); +const { ControllerConfig } = require("@clusterio/lib"); + +describe("controller/src/WsServer", function() { + describe("class WsServer", function() { + /** @type {WsServer} */ + let wsServer; + before(function() { + let config = new ControllerConfig("controller"); + config.set("controller.trusted_proxies", "127.0.0.0/8, ::1"); + let controller = new Controller({}, [], "", config); + wsServer = controller.wsServer; + }); + describe(".remoteAddr()", function() { + it("should resolve X-Forward-For header for trusted proxies", function() { + const ipv4 = wsServer.remoteAddr({ + socket: { + remoteAddress: "127.0.0.1", + remoteFamily: "IPv4", + }, + headers: { "x-forwarded-for": "1.2.3.4" }, + }); + assert.equal(ipv4, "1.2.3.4"); + const ipv6 = wsServer.remoteAddr({ + socket: { + remoteAddress: "::1", + remoteFamily: "IPv6", + }, + headers: { "x-forwarded-for": "2000::1" }, + }); + assert.equal(ipv6, "2000::1"); + }); + it("should pick the last address in X-Forward-For header for trusted proxies", function() { + const ipv4 = wsServer.remoteAddr({ + socket: { + remoteAddress: "127.0.0.1", + remoteFamily: "IPv4", + }, + headers: { "x-forwarded-for": "1.0.0.0, 2.0.0.0, 1.2.3.4" }, + }); + assert.equal(ipv4, "1.2.3.4"); + const ipv6 = wsServer.remoteAddr({ + socket: { + remoteAddress: "::1", + remoteFamily: "IPv6", + }, + headers: { "x-forwarded-for": "2001::, 2002::, 2000::1" }, + }); + assert.equal(ipv6, "2000::1"); + }); + it("should not resolve X-Forward-For header for untrusted proxies", function() { + const ipv4 = wsServer.remoteAddr({ + socket: { + remoteAddress: "1.2.3.4", + remoteFamily: "IPv4", + }, + headers: { "x-forwarded-for": "127.0.0.1" }, + }); + assert.equal(ipv4, "1.2.3.4"); + const ipv6 = wsServer.remoteAddr({ + socket: { + remoteAddress: "2000::1", + remoteFamily: "IPv6", + }, + headers: { "x-forwarded-for": "::1" }, + }); + assert.equal(ipv6, "2000::1"); + }); + }); + }); +});