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() {