Make ACL safe so that Mjolnir will not ban itself. (#213)

src/actions/ApplyAcl.ts

@@ -19,7 +19,7 @@ import { ServerAcl } from "../models/ServerAcl";
 import { RoomUpdateError } from "../models/RoomUpdateError";
 import { Mjolnir } from "../Mjolnir";
 import config from "../config";
-import { LogLevel } from "matrix-bot-sdk";
+import { LogLevel, UserID } from "matrix-bot-sdk";
 import { logMessage } from "../LogProxy";
 import { ERROR_KIND_FATAL, ERROR_KIND_PERMISSION } from "../ErrorCache";
 
@@ -31,8 +31,10 @@ import { ERROR_KIND_FATAL, ERROR_KIND_PERMISSION } from "../ErrorCache";
  * @param {Mjolnir} mjolnir The Mjolnir client to apply the ACLs with.
  */
 export async function applyServerAcls(lists: BanList[], roomIds: string[], mjolnir: Mjolnir): Promise<RoomUpdateError[]> {
+    const serverName: string = new UserID(await config.RUNTIME.client!.getUserId()).domain;
+
     // Construct a server ACL first
-    const acl = new ServerAcl().denyIpAddresses().allowServer("*");
+    const acl = new ServerAcl(serverName).denyIpAddresses().allowServer("*");
     for (const list of lists) {
         for (const rule of list.serverRules) {
             acl.denyServer(rule.entity);
@@ -41,6 +43,10 @@ export async function applyServerAcls(lists: BanList[], roomIds: string[], mjoln
 
     const finalAcl = acl.safeAclContent();
 
+    if (finalAcl.deny.length !== acl.literalAclContent().deny.length) {
+        logMessage(LogLevel.WARN, "ApplyAcl", `Mjölnir has detected and removed an ACL that would exclude itself. Please check the ACL lists.`);
+    }
+
     if (config.verboseLogging) {
         // We specifically use sendNotice to avoid having to escape HTML
         await mjolnir.client.sendNotice(mjolnir.managementRoomId, `Constructed server ACL:\n${JSON.stringify(finalAcl, null, 2)}`);

src/models/BanList.ts

@@ -16,7 +16,7 @@ limitations under the License.
 
 import { extractRequestError, LogService, MatrixClient } from "matrix-bot-sdk";
 import { EventEmitter } from "events";
-import { ListRule } from "./ListRule";
+import { ListRule, RECOMMENDATION_BAN } from "./ListRule";
 
 export const RULE_USER = "m.policy.rule.user";
 export const RULE_ROOM = "m.policy.rule.room";
@@ -130,7 +130,7 @@ class BanList extends EventEmitter {
 
     /**
      * Return all the active rules of a given kind.
-     * @param kind e.g. RULE_SERVER (m.policy.rule.server)
+     * @param kind e.g. RULE_SERVER (m.policy.rule.server). Rule types are always normalised when they are interned into the BanList.
      * @returns The active ListRules for the ban list of that kind.
      */
     private rulesOfKind(kind: string): ListRule[] {
@@ -139,7 +139,10 @@ class BanList extends EventEmitter {
         if (stateKeyMap) {
             for (const event of stateKeyMap.values()) {
                 const rule = event?.unsigned?.rule;
-                if (rule && rule.kind === kind) {
+                // README! If you are refactoring this and/or introducing a mechanism to return the list of rules,
+                // please make sure that you *only* return rules with `m.ban` or create a different method
+                // (we don't want to accidentally ban entities).
+                if (rule && rule.kind === kind && rule.recommendation === RECOMMENDATION_BAN) {
                     rules.push(rule);
                 }
             }

src/models/ListRule.ts

@@ -34,6 +34,7 @@ export class ListRule {
 
     /**
      * The recommendation for this rule, or `null` if there is no recommendation or the recommendation is invalid.
+     * Recommendations are normalised to their stable types.
      */
     public get recommendation(): string|null {
         if (RECOMMENDATION_BAN_TYPES.includes(this.action)) return RECOMMENDATION_BAN;

src/models/ServerAcl.ts

@@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+import { MatrixGlob } from "matrix-bot-sdk";
 import { setToArray } from "../utils";
 
 export interface ServerAclContent {
@@ -27,6 +28,28 @@ export class ServerAcl {
     private deniedServers: Set<string> = new Set<string>();
     private allowIps = false;
 
+    public constructor(public readonly homeserver: string) {
+
+    }
+
+    /**
+     * Checks the ACL for any entries that might ban ourself.
+     * @returns A list of deny entries that will not ban our own homeserver.
+     */
+    public safeDeniedServers(): string[] {
+        // The reason we do this check here rather than in the `denyServer` method
+        // is because `literalAclContent` exists and also we want to be defensive about someone
+        // mutating `this.deniedServers` via another method in the future.
+        const entries: string[] = []
+        for (const server of this.deniedServers) {
+            const glob = new MatrixGlob(server);
+            if (!glob.test(this.homeserver)) {
+                entries.push(server);
+            }
+        }
+        return entries;
+    }
+
     public allowIpAddresses(): ServerAcl {
         this.allowIps = true;
         return this;
@@ -72,7 +95,7 @@ export class ServerAcl {
         }
         return {
             allow: allowed,
-            deny: setToArray(this.deniedServers),
+            deny: this.safeDeniedServers(),
             allow_ip_literals: this.allowIps,
         };
     }

test/integration/banListTest.ts

@@ -2,8 +2,9 @@ import { strict as assert } from "assert";
 
 import config from "../../src/config";
 import { newTestUser } from "./clientHelper";
-import { MatrixClient } from "matrix-bot-sdk";
-import  BanList, { ChangeType, ListRuleChange, RULE_USER } from "../../src/models/BanList";
+import { MatrixClient, UserID } from "matrix-bot-sdk";
+import  BanList, { ALL_RULE_TYPES, ChangeType, ListRuleChange, RULE_SERVER, RULE_USER } from "../../src/models/BanList";
+import { ServerAcl, ServerAclContent } from "../../src/models/ServerAcl";
 
 /**
  * Create a policy rule in a policy room.
@@ -12,13 +13,14 @@ import  BanList, { ChangeType, ListRuleChange, RULE_USER } from "../../src/model
  * @param policyType The type of policy to add e.g. m.policy.rule.user. (Use RULE_USER though).
  * @param entity The entity to ban e.g. @foo:example.org
  * @param reason A reason for the rule e.g. 'Wouldn't stop posting spam links'
+ * @param template The template to use for the policy rule event.
  * @returns The event id of the newly created policy rule.
  */
-async function createPolicyRule(client: MatrixClient, policyRoomId: string, policyType: string, entity: string, reason: string) {
+async function createPolicyRule(client: MatrixClient, policyRoomId: string, policyType: string, entity: string, reason: string, template = {recommendation: 'm.ban'}) {
     return await client.sendStateEvent(policyRoomId, policyType, `rule:${entity}`, {
         entity,
         reason,
-        recommendation: 'm.ban'
+        ...template,
     });
 }
 
@@ -175,4 +177,51 @@ describe("Test: Updating the BanList", function () {
         assert.equal(changes[0].previousState['event_id'], updatedEventId, 'There should be a previous state event for a modified rule');
         assert.equal(banList.userRules.filter(rule => rule.entity === entity).length, 0, 'The rule should no longer be stored.');
     })
+    it('Test: BanList Supports all entity types.', async function () {
+        const mjolnir = config.RUNTIME.client!
+        const banListId = await mjolnir.createRoom();
+        const banList = new BanList(banListId, banListId, mjolnir);
+        for (let i = 0; i < ALL_RULE_TYPES.length; i++) {
+            await createPolicyRule(mjolnir, banListId, ALL_RULE_TYPES[i], `*${i}*`, '');
+        }
+        let changes: ListRuleChange[] = await banList.updateList();
+        assert.equal(changes.length, ALL_RULE_TYPES.length);
+        assert.equal(banList.allRules.length, ALL_RULE_TYPES.length);
+    })
 });
+
+describe('Test: We do not respond to recommendations other than m.ban in the banlist', function () {
+    it('Will not respond to a rule that has a different recommendation to m.ban (or the unstable equivalent).', async function () {
+        const mjolnir = config.RUNTIME.client!
+        const banListId = await mjolnir.createRoom();
+        const banList = new BanList(banListId, banListId, mjolnir);
+        await createPolicyRule(mjolnir, banListId, RULE_SERVER, 'exmaple.org', '', {recommendation: 'something that is not m.ban'});
+        let changes: ListRuleChange[] = await banList.updateList();
+        assert.equal(changes.length, 1, 'There should only be one change');
+        assert.equal(changes[0].changeType, ChangeType.Added);
+        assert.equal(changes[0].sender, await mjolnir.getUserId());
+        // We really don't want things that aren't m.ban to end up being accessible in these APIs.
+        assert.equal(banList.serverRules.length, 0);
+        assert.equal(banList.allRules.length, 0);
+    })
+})
+
+describe('Test: We will not be able to ban ourselves via ACL.', function () {
+    it('We do not ban ourselves when we put ourselves into the policy list.', async function () {
+        const mjolnir = config.RUNTIME.client!
+        const serverName = new UserID(await mjolnir.getUserId()).domain;
+        const banListId = await mjolnir.createRoom();
+        const banList = new BanList(banListId, banListId, mjolnir);
+        await createPolicyRule(mjolnir, banListId, RULE_SERVER, serverName, '');
+        await createPolicyRule(mjolnir, banListId, RULE_SERVER, 'evil.com', '');
+        await createPolicyRule(mjolnir, banListId, RULE_SERVER, '*', '');
+        // We should still intern the matching rules rule.
+        let changes: ListRuleChange[] = await banList.updateList();
+        assert.equal(banList.serverRules.length, 3);
+        // But when we construct an ACL, we should be safe.
+        const acl = new ServerAcl(serverName)
+        changes.forEach(change => acl.denyServer(change.rule.entity));
+        assert.equal(acl.safeAclContent().deny.length, 1);
+        assert.equal(acl.literalAclContent().deny.length, 3);
+    })
+})