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

@@ -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");
 		}

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

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;

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;
 	}

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);
 	}

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;

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

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