From 87172dfb8d634aa33f9446dcb14655a4ffa4bd91 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Tue, 25 Jul 2023 17:21:44 +0200 Subject: [PATCH] Implement basic ingress filtering on controller Validate messages received on the controller from hosts and control nodes before handling or forwarding them. This should prevent performing actions that are not allowed by the role system and block simple attacks such as spoofing the source address of a message. --- packages/controller/src/ControlConnection.js | 52 +++++- packages/controller/src/HostConnection.js | 30 ++++ packages/lib/data/messages_instance.js | 18 +- packages/lib/link/connectors.js | 8 +- packages/lib/link/link.js | 72 +++++++- plugins/global_chat/messages.js | 1 + test/integration/clusterio.js | 35 +++- test/lib/link/link.js | 175 ++++++++++++++++--- 8 files changed, 339 insertions(+), 52 deletions(-) diff --git a/packages/controller/src/ControlConnection.js b/packages/controller/src/ControlConnection.js index d65350c5..fcee23df 100644 --- a/packages/controller/src/ControlConnection.js +++ b/packages/controller/src/ControlConnection.js @@ -36,7 +36,6 @@ class ControlConnection extends BaseConnection { this._agent = registerData.agent; this._version = registerData.version; this.id = id; - this.dst = new libData.Address(libData.Address.instance, id); /** * The user making this connection. @@ -169,6 +168,50 @@ class ControlConnection extends BaseConnection { this.handle(libData.DebugDumpWsRequest, this.handleDebugDumpWsRequest.bind(this)); } + validateIngress(message) { + let origin = this.connector.dst; + if (origin.type !== message.src.type || origin.id !== message.src.id) { + throw new libErrors.InvalidMessage(`Received message with invalid src ${message.src} from ${origin}`); + } + } + + validatePermission(message, entry) { + try { + this.checkPermission(message, entry); + } catch (err) { + this.connector.sendResponseError(new libData.ResponseError(err.message, err.code), message.src); + logger.audit(`Permission denied for ${message.name} by ${this.user.name} from ${this.connector.dst}`); + throw err; + } + } + + checkPermission(message, entry) { + let permission; + if (message.type === "request") { + permission = entry.Request.permission; + } else if (message.type === "event") { + permission = entry.Event.permission; + } else { + return; + } + + if (permission === null) { + return; + } + + if (typeof permission === "string") { + this.user.checkPermission(permission); + return; + } + + if (typeof permission === "function") { + permission(this.user, message); + return; + } + + throw new Error("Should be unreachable"); + } + async handleControllerConfigGetRequest() { return new libData.RawConfig(this._controller.config.serialize("control")); } @@ -362,13 +405,6 @@ class ControlConnection extends BaseConnection { } async handleInstanceTransferSaveRequest(request) { - // XXX if control sends the request directly to a host these checks are bypassed - if (request.copy) { - this.user.checkPermission("core.instance.save.copy"); - } else if (request.sourceName !== request.targetName) { - this.user.checkPermission("core.instance.save.rename"); - } - if (request.sourceInstanceId === request.targetInstanceId) { throw new libErrors.RequestError("Source and target instance may not be the same"); } diff --git a/packages/controller/src/HostConnection.js b/packages/controller/src/HostConnection.js index b5c626f3..232d7c7c 100644 --- a/packages/controller/src/HostConnection.js +++ b/packages/controller/src/HostConnection.js @@ -1,6 +1,7 @@ "use strict"; const libData = require("@clusterio/lib/data"); const libConfig = require("@clusterio/lib/config"); +const libErrors = require("@clusterio/lib/errors"); const { logger } = require("@clusterio/lib/logging"); const libPlugin = require("@clusterio/lib/plugin"); const PlayerStats = require("@clusterio/lib/PlayerStats"); @@ -65,6 +66,35 @@ class HostConnection extends BaseConnection { this.handle(libData.InstancePlayerUpdateEvent, this.handleInstancePlayerUpdateEvent.bind(this)); } + validateIngress(message) { + let origin = this.connector.dst; + switch (message.src.type) { + case libData.Address.control: + case libData.Address.controller: + throw new libErrors.InvalidMessage(`Received message with invalid src ${message.src} from ${origin}`); + + case libData.Address.host: + if (message.src.id !== origin.id) { + throw new libErrors.InvalidMessage( + `Received message with invalid src ${message.src} from ${origin}` + ); + } + break; + + case libData.Address.instance: + let instance = this._controller.instances.get(message.src.id); + if (!instance || instance.config.get("instance.assigned_host") !== this._id) { + throw new libErrors.InvalidMessage( + `Received message with invalid src ${message.src} from ${origin}` + ); + } + break; + + default: + throw new Error("Should be unreachable"); + } + } + _checkPluginVersions() { let pluginInfos = new Map(this._controller.pluginInfos.map(i => [i.name, i])); for (let [name, version] of this.plugins) { diff --git a/packages/lib/data/messages_instance.js b/packages/lib/data/messages_instance.js index c288edbc..967f972a 100644 --- a/packages/lib/data/messages_instance.js +++ b/packages/lib/data/messages_instance.js @@ -86,7 +86,7 @@ class InstanceDetailsSetSubscriptionsRequest { static type = "request"; static src = "control"; static dst = "controller"; - static premission = "core.instance.subscribe"; + static permission = "core.instance.subscribe"; /** @type {boolean} */ all; @@ -144,7 +144,7 @@ class InstanceCreateRequest { static type = "request"; static src = "control"; static dst = "controller"; - static premission = "core.instance.create"; + static permission = "core.instance.create"; /** @type {object} */ config; @@ -172,7 +172,7 @@ class InstanceConfigGetRequest { static type = "request"; static src = "control"; static dst = "controller"; - static premission = "core.instance.get_config"; + static permission = "core.instance.get_config"; /** @type {number} */ instanceId; @@ -429,7 +429,7 @@ class InstanceSetSaveListSubscriptionsRequest { static type = "request"; static src = "control"; static dst = "controller"; - static premission = "core.instance.save.list_subscribe"; + static permission = "core.instance.save.list_subscribe"; /** @type {boolean} */ all; @@ -669,7 +669,15 @@ class InstanceTransferSaveRequest { static type = "request"; static src = ["control", "controller"]; static dst = ["controller", "host"]; - static permission = "core.instance.save.transfer"; + static permission(user, message) { + user.checkPermission("core.instance.save.transfer"); + if (message.data.copy) { + user.checkPermission("core.instance.save.copy"); + } else if (message.data.sourceName !== message.data.targetName) { + user.checkPermission("core.instance.save.rename"); + } + } + /** @type {number} */ sourceInstanceId; diff --git a/packages/lib/link/connectors.js b/packages/lib/link/connectors.js index 39a7ed69..a083e2c8 100644 --- a/packages/lib/link/connectors.js +++ b/packages/lib/link/connectors.js @@ -44,18 +44,18 @@ class BaseConnector extends events.EventEmitter { return seq; } - sendResponse(response, dst) { + sendResponse(response, dst, src = this.src) { let seq = this._seq; this._seq += 1; - const message = new libData.MessageResponse(seq, this.src, dst, response); + const message = new libData.MessageResponse(seq, src, dst, response); this.send(message); return seq; } - sendResponseError(error, dst) { + sendResponseError(error, dst, src = this.src) { let seq = this._seq; this._seq += 1; - const message = new libData.MessageResponseError(seq, this.src, dst, error); + const message = new libData.MessageResponseError(seq, src, dst, error); this.send(message); return seq; } diff --git a/packages/lib/link/link.js b/packages/lib/link/link.js index 5895074c..dd4bf6fe 100644 --- a/packages/lib/link/link.js +++ b/packages/lib/link/link.js @@ -96,6 +96,16 @@ class Link { } let entry = this._validateMessage(message); + if (entry && this.connector.dst.type === libData.Address.control) { + try { + this.validatePermission(message, entry); // Somewhat hacky, defined in ControlConnection + } catch (err) { + if (err instanceof libErrors.PermissionError) { + return; + } + throw err; + } + } if (message.type === "event" && this._eventSnoopers.has(entry.Event)) { let handler = this._eventSnoopers.get(entry.Event); @@ -123,7 +133,29 @@ class Link { } } + /** + * Ingress message validation + * + * Should be overridden by sub-classes to validate messages received. + * + * @param {module:lib/data.Message} message - Message to check. + * @throws {module:lib/errors.InvalidMessage} if the message is invalid. + */ + validateIngress(message) { } + _validateMessage(message) { + try { + if (message.src.type === libData.Address.broadcast) { + throw new libErrors.InvalidMessage("Message src may not be broadcast"); + } + this.validateIngress(message); + } catch (err) { + if (message.type === "request") { + this.connector.sendResponseError(new libData.ResponseError(err.message, err.code), message.src); + } + throw err; + } + if (message.type === "request") { let entry = this.constructor._requestsByName.get(message.name); if (!entry) { @@ -173,6 +205,18 @@ class Link { throw new Error("Should be unreachable"); } + /** + * Message permission validation + * + * Called when an request or event is received by a control connector. + * Should be overridden by sub-classes to validate messages received. + * + * @param {module:lib/data.Message} message - Message to check. + * @param {object} entry - Request or Event entry for this Message. + * @throws {module:lib/errors.PermissionError} if unauthorized. + */ + validatePermission(message, entry) { } + _routeMessage(message, entry) { if (!this.router) { let err = new libErrors.InvalidMessage( @@ -196,10 +240,10 @@ class Link { if (message.type !== "request") { throw new Error(`Router requested fallback handling of unsupported message type ${message.type}`); } - this._processRequest(message, entry, fallback); + this._processRequest(message, entry, fallback, message.dst); } - _processRequest(message, entry, handler) { + _processRequest(message, entry, handler, spoofedSrc) { if (!handler) { this.connector.sendResponseError( new libData.ResponseError(`No handler for ${entry.Request.name}`), message.src @@ -233,7 +277,7 @@ class Link { throw new Error(`Expected empty response from ${entry.Request.name} handler`); } } - this.connector.sendResponse(result, message.src); + this.connector.sendResponse(result, message.src, spoofedSrc); } ).catch( err => { @@ -246,7 +290,7 @@ class Link { logger.error(`Unexpected error responding to ${message.name}:\n${err.stack}`); } this.connector.sendResponseError( - new libData.ResponseError(err.message, err.code, err.stack), message.src + new libData.ResponseError(err.message, err.code, err.stack), message.src, spoofedSrc ); } ); @@ -260,6 +304,10 @@ class Link { ); } + if (!pending.dst.equals(message.src)) { + throw new libErrors.InvalidMessage(`Received reply from ${message.src} for message sent to ${pending.dst}`); + } + try { pending.resolve(pending.request.responseFromJSON(message.data)); } catch (err) { @@ -350,6 +398,7 @@ class Link { let pending = { request: entry, + dst, }; pending.promise = new Promise((resolve, reject) => { pending.resolve = resolve; @@ -516,6 +565,13 @@ class Link { allowedDstTypes: this.allowedTypes(Request.dst, name, "dst"), }; + if ( + entry.allowedSrcTypes.has(libData.Address.control) + && !(Request.permission === null || ["function", "string"].includes(typeof Request.permission)) + ) { + throw new Error(`Invalid permission specification ${typeof Request.permission} on ${name}`); + } + let Response = Request.Response; if (Response) { entry.Response = Response; @@ -563,6 +619,14 @@ class Link { allowedSrcTypes: this.allowedTypes(Event.src, name, "src"), allowedDstTypes: this.allowedTypes(Event.dst, name, "dst"), }; + + if ( + entry.allowedSrcTypes.has(libData.Address.control) + && !(Event.permission === null || ["function", "string"].includes(typeof Event.permission)) + ) { + throw new Error(`Invalid permission specification ${typeof Event.permission} on ${name}`); + } + this._eventsByName.set(name, entry); this._eventsByClass.set(Event, entry); } diff --git a/plugins/global_chat/messages.js b/plugins/global_chat/messages.js index 9b264d4d..d233fd98 100644 --- a/plugins/global_chat/messages.js +++ b/plugins/global_chat/messages.js @@ -5,6 +5,7 @@ class ChatEvent { static src = ["control", "instance"]; static dst = "instance"; static plugin = "global_chat"; + static permission = null; /** @type {string} */ instanceName; diff --git a/test/integration/clusterio.js b/test/integration/clusterio.js index 569fe844..8b9e9c6e 100644 --- a/test/integration/clusterio.js +++ b/test/integration/clusterio.js @@ -48,6 +48,11 @@ async function deleteSave(instanceId, save) { await getControl().send(new libData.InstanceDeleteSaveRequest(instanceId, save)); } +async function getUsers() { + let users = await getControl().send(new libData.UserListRequest()); + return new Map(users.map(user => [user.name, user])); +} + describe("Integration of Clusterio", function() { describe("clusteriocontroller", function() { describe("bootstrap generate-user-token", function() { @@ -455,11 +460,6 @@ describe("Integration of Clusterio", function() { }); describe("user set-admin/whitelisted/banned", function() { - async function getUsers() { - let users = await getControl().send(new libData.UserListRequest()); - return new Map(users.map(user => [user.name, user])); - } - async function getUser(name) { return await getControl().send(new libData.UserGetRequest(name)); } @@ -1090,6 +1090,31 @@ describe("Integration of Clusterio", function() { assert.deepEqual(tempUser.roles, [0]); assert.equal(getControl().userUpdates.length, 1); }); + + it("should restrict actions based on roles", async function() { + await execCtl('user set-roles temp "Player"'); + let tempControl; + try { + let tlsCa = await fs.readFile("test/file/tls/cert.pem"); + let connector = new TestControlConnector(url, 2, tlsCa); + connector.token = jwt.sign( + { aud: "user", user: "temp" }, Buffer.from("TestSecretDoNotUse", "base64") + ); + tempControl = new TestControl(connector); + await connector.connect(); + + await assert.rejects( + tempControl.send(new libData.UserCreateRequest("notallowed")), + new Error("Permission denied") + ); + + } finally { + await tempControl.connector.disconnect(); + } + + let users = await getUsers(); + assert(!users.has("notallowed"), "user was created when it should not be allowed"); + }); }); describe("user delete", function() { diff --git a/test/lib/link/link.js b/test/lib/link/link.js index 484029bf..e88df1f1 100644 --- a/test/lib/link/link.js +++ b/test/lib/link/link.js @@ -26,12 +26,14 @@ describe("lib/link/link", function() { static type = "request"; static src = ["controller", "control"]; static dst = ["controller", "control"]; + static permission = null; } libLink.Link.register(SimpleRequest); class NumberRequest { static type = "request"; static src = ["controller", "control"]; static dst = ["controller", "control"]; + static permission = null; constructor(value) { this.value = value; } static jsonSchema = { type: "number" }; toJSON() { return this.value; } @@ -48,12 +50,14 @@ describe("lib/link/link", function() { static type = "event"; static src = ["controller", "control"]; static dst = ["controller", "control"]; + static permission = null; } libLink.Link.register(SimpleEvent); class NumberEvent { static type = "event"; static src = ["controller", "control"]; static dst = ["controller", "control"]; + static permission = null; constructor(value) { this.value = value; } static jsonSchema = { type: "number" }; toJSON() { return this.value; } @@ -69,33 +73,152 @@ describe("lib/link/link", function() { it("should handle unknown message", async function() { testConnector.emit("message", { type: "unknown" }); }); - it("should give an error response back on unrecognized request", function() { - testConnector.emit("message", new libData.MessageRequest(1, dst, src, "UnhandledRequest")); - assert.deepEqual(testConnector.sentMessages, [ - new libData.MessageResponseError( - testConnector._seq - 1, - src, - dst, - new libData.ResponseError("Unrecognized request UnhandledRequest") - ), - ]); + + describe("Request handling", function() { + it("should give an error response back on unrecognized request", function() { + testConnector.emit("message", new libData.MessageRequest(1, dst, src, "UnhandledRequest")); + assert.deepEqual(testConnector.sentMessages, [ + new libData.MessageResponseError( + testConnector._seq - 1, + src, + dst, + new libData.ResponseError("Unrecognized request UnhandledRequest") + ), + ]); + }); + it("should give an error response back on unhandled request", function() { + class UnhandledRequest { + static type = "request"; + static src = "controller"; + static dst = "control"; + } + libLink.Link.register(UnhandledRequest); + testConnector.emit("message", new libData.MessageRequest(1, dst, src, "UnhandledRequest")); + assert.deepEqual(testConnector.sentMessages, [ + new libData.MessageResponseError( + testConnector._seq - 1, + src, + dst, + new libData.ResponseError("No handler for UnhandledRequest") + ), + ]); + }); + it("should give an error response back on invalid src", function() { + class InvalidSrcRequest { + static type = "request"; + static src = "host"; + static dst = "control"; + } + libLink.Link.register(InvalidSrcRequest); + testConnector.emit("message", new libData.MessageRequest(1, dst, src, "InvalidSrcRequest")); + assert.deepEqual(testConnector.sentMessages, [ + new libData.MessageResponseError( + testConnector._seq - 1, + src, + dst, + new libData.ResponseError("Source [Address controller:0] is not allowed for InvalidSrcRequest") + ), + ]); + }); + it("should give an error response back on invalid dst", function() { + class InvalidDstRequest { + static type = "request"; + static src = "controller"; + static dst = "host"; + } + libLink.Link.register(InvalidDstRequest); + testConnector.emit("message", new libData.MessageRequest(1, dst, src, "InvalidDstRequest")); + assert.deepEqual(testConnector.sentMessages, [ + new libData.MessageResponseError( + testConnector._seq - 1, + src, + dst, + new libData.ResponseError( + "Destination [Address control:1] is not allowed for InvalidDstRequest" + ) + ), + ]); + }); + it("should give an error response back on broadcast src", function() { + testConnector.emit("message", new libData.MessageRequest(1, addr("allControls"), src, "SimpleRequest")); + assert.deepEqual(testConnector.sentMessages, [ + new libData.MessageResponseError( + testConnector._seq - 1, + src, + addr("allControls"), + new libData.ResponseError("Message src may not be broadcast") + ), + ]); + }); + it("should give an error response back on requst with broadcast dst", function() { + testConnector.emit("message", new libData.MessageRequest(1, dst, addr("allControls"), "SimpleRequest")); + assert.deepEqual(testConnector.sentMessages, [ + new libData.MessageResponseError( + testConnector._seq - 1, + src, + dst, + new libData.ResponseError("Destination [Address 4:control] is not allowed for SimpleRequest") + ), + ]); + }); }); - it("should give an error response back on unhandled request", function() { - class UnhandledRequest { - static type = "request"; - static src = "controller"; - static dst = "control"; - } - libLink.Link.register(UnhandledRequest); - testConnector.emit("message", new libData.MessageRequest(1, dst, src, "UnhandledRequest")); - assert.deepEqual(testConnector.sentMessages, [ - new libData.MessageResponseError( - testConnector._seq - 1, - src, - dst, - new libData.ResponseError("No handler for UnhandledRequest") - ), - ]); + + describe("Event handling", function() { + it("should ignore unrecognized event", function() { + testConnector.emit("message", new libData.MessageEvent(1, dst, src, "UnhandledEvent")); + assert.deepEqual(testConnector.sentMessages, []); + }); + it("should ignore unhandled event", function() { + class UnhandledEvent { + static type = "event"; + static src = "controller"; + static dst = "control"; + } + libLink.Link.register(UnhandledEvent); + testConnector.emit("message", new libData.MessageEvent(1, dst, src, "UnhandledEvent")); + assert.deepEqual(testConnector.sentMessages, []); + }); + it("should ignore invalid src", function() { + class InvalidSrcEvent { + static type = "event"; + static src = "host"; + static dst = "control"; + } + let handled = false; + libLink.Link.register(InvalidSrcEvent); + testLink.snoopEvent(InvalidSrcEvent, async () => { handled = true; }); + testConnector.emit("message", new libData.MessageEvent(1, dst, src, "InvalidSrcEvent")); + assert(!handled, "event was not ignored"); + assert.deepEqual(testConnector.sentMessages, []); + }); + it("should ignore invalid dst", function() { + class InvalidDstEvent { + static type = "event"; + static src = "controller"; + static dst = "host"; + } + libLink.Link.register(InvalidDstEvent); + let handled = false; + testLink.snoopEvent(InvalidDstEvent, async () => { handled = true; }); + testConnector.emit("message", new libData.MessageEvent(1, dst, src, "InvalidDstEvent")); + testConnector.emit("message", new libData.MessageEvent(1, dst, addr("allControls"), "InvalidDstEvent")); + assert(!handled, "event was not ignored"); + assert.deepEqual(testConnector.sentMessages, []); + }); + it("should ignore broadcast src", function() { + let handled = false; + testLink.snoopEvent(SimpleEvent, async () => { handled = true; }); + testConnector.emit("message", new libData.MessageEvent(1, addr("allControls"), src, "SimpleEvent")); + assert(!handled, "event was not ignored"); + assert.deepEqual(testConnector.sentMessages, []); + }); + it("should handle event with broadcast dst", function() { + let handled = false; + testLink.snoopEvent(SimpleEvent, async () => { handled = true; }); + testConnector.emit("message", new libData.MessageEvent(1, dst, addr("allControls"), "SimpleEvent")); + assert(handled, "event was ignored"); + assert.deepEqual(testConnector.sentMessages, []); + }); }); it("should send ready on connector prepareDisconnect", async function() {