Dealing with untrusted content - resolves #456

src/Mjolnir.ts

@@ -39,6 +39,7 @@ import { RoomMemberManager } from "./RoomMembers";
 import ProtectedRoomsConfig from "./ProtectedRoomsConfig";
 import { MatrixEmitter, MatrixSendClient } from "./MatrixEmitter";
 import { OpenMetrics } from "./webapis/OpenMetrics";
+import * as UntrustedContent from "./UntrustedContent";
 
 export const STATE_NOT_STARTED = "not_started";
 export const STATE_CHECKING_PERMISSIONS = "checking_permissions";
@@ -50,6 +51,9 @@ export const STATE_RUNNING = "running";
  * to store that for pagination on further polls
  */
 export const REPORT_POLL_EVENT_TYPE = "org.matrix.mjolnir.report_poll";
+const REPORT_POLL_EXPECTED_CONTENT = new UntrustedContent.SubTypeObjectContent({
+    "from": UntrustedContent.NUMBER_CONTENT
+});
 
 export class Mjolnir {
     private displayName: string;
@@ -279,6 +283,12 @@ export class Mjolnir {
                 let reportPollSetting: { from: number } = { from: 0 };
                 try {
                     reportPollSetting = await this.client.getAccountData(REPORT_POLL_EVENT_TYPE);
+                    reportPollSetting = REPORT_POLL_EXPECTED_CONTENT.fallback(reportPollSetting,
+                        () => {
+                            this.managementRoomOutput.logMessage(LogLevel.INFO, "Mjolnir@startup", "invalid report poll settings, ignoring");
+                            return ({ from: 0 })
+                        }
+                    );
                 } catch (err) {
                     if (err.body?.errcode !== "M_NOT_FOUND") {
                         throw err;

src/ProtectedRoomsConfig.ts

@@ -18,7 +18,12 @@ import AwaitLock from 'await-lock';
 import { extractRequestError, LogService, Permalinks } from "matrix-bot-sdk";
 import { IConfig } from "./config";
 import { MatrixSendClient } from './MatrixEmitter';
+import * as UntrustedContent from './UntrustedContent';
+
 const PROTECTED_ROOMS_EVENT_TYPE = "org.matrix.mjolnir.protected_rooms";
+const PROTECTED_ROOMS_EXPECTED_CONTENT = new UntrustedContent.SubTypeObjectContent({
+    rooms: UntrustedContent.STRING_CONTENT.array().optional()
+});
 
 /**
  * Manages the set of rooms that the user has EXPLICITLY asked to be protected.
@@ -65,7 +70,12 @@ export default class ProtectedRoomsConfig {
     public async loadProtectedRoomsFromAccountData(): Promise<void> {
         LogService.debug("ProtectedRoomsConfig", "Loading protected rooms...");
         try {
-            const data: { rooms?: string[] } | null = await this.client.getAccountData(PROTECTED_ROOMS_EVENT_TYPE);
+            let data: { rooms?: string[] } | null = PROTECTED_ROOMS_EXPECTED_CONTENT.fallback(
+                await this.client.getAccountData(PROTECTED_ROOMS_EVENT_TYPE),
+            () => {
+                LogService.warn("ProtectedRoomsConfig", "Invalid data, assuming empty data");
+                return null;
+            });
             if (data && data['rooms']) {
                 for (const roomId of data['rooms']) {
                     this.explicitlyProtectedRooms.add(roomId);
@@ -116,10 +126,19 @@ export default class ProtectedRoomsConfig {
         //       but it doesn't stop a third party client on the same account racing with us instead.
         await this.accountDataLock.acquireAsync();
         try {
-            const additionalProtectedRooms: string[] = await this.client.getAccountData(PROTECTED_ROOMS_EVENT_TYPE)
-                .then((rooms: {rooms?: string[]}) => Array.isArray(rooms?.rooms) ? rooms.rooms : [])
-                .catch(e => (LogService.warn("ProtectedRoomsConfig", "Could not load protected rooms from account data", extractRequestError(e)), []));
-
+            const untrustedAdditionalProtectedRooms = await
+                this
+                    .client.getAccountData(PROTECTED_ROOMS_EVENT_TYPE)
+                    .catch(e => {
+                        LogService.warn("ProtectedRoomsConfig", "Could not load protected rooms from account data", extractRequestError(e));
+                        return [];
+                    });
+            let additionalProtectedRooms = PROTECTED_ROOMS_EXPECTED_CONTENT.fallback(untrustedAdditionalProtectedRooms,
+                () => {
+                    LogService.warn("ProtectedRoomsConfig", "Invalid list of protected rooms, restarting with an empty list");
+                    return [];
+                }
+            );
             const roomsToSave = new Set([...this.explicitlyProtectedRooms.keys(), ...additionalProtectedRooms]);
             excludeRooms.forEach(roomsToSave.delete, roomsToSave);
             await this.client.setAccountData(PROTECTED_ROOMS_EVENT_TYPE, { rooms: Array.from(roomsToSave.keys()) });

src/UntrustedContent.ts

@@ -0,0 +1,195 @@
+/*
+Copyright 2023 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+/**
+ * Utilities to deal with untrusted values coming from Matrix events.
+ *
+ * e.g. to confirm that a value `foo` has type `{ bar: string[]}`,
+ * run
+ * ```ts
+ * new SubTypeObjectContent({
+ *   bar: STRING_CONTENT.array()
+ * }).check_type(value)
+ * ```
+ */
+
+/**
+ * The abstract root class for all content we wish to validate against.
+ */
+abstract class AbstractContent {
+    /**
+     * Validate the type of a value against `this`.
+     * @param value
+     */
+    abstract checkType(value: any): boolean;
+
+    /**
+     * If `value` has `this` type, return `value`, otherwise
+     * return `defaults()`.
+     */
+    fallback(value: any, defaults: () => any): any {
+        if (this.checkType(value)) {
+            return value;
+        }
+        return defaults();
+    }
+
+    /**
+     * Return an `AbstractContent` for values of type `this | null | undefined`.
+     */
+    optional(): AbstractContent {
+        return new OptionalContent(this);
+    }
+
+    /**
+     * Return a `AbstractContent` for values of type `this[]`.
+     *
+     * This is a shortcut for `new OptionalContent(this)`
+     */
+    array(): AbstractContent {
+        return new ArrayContent(this);
+    }
+};
+
+/**
+ * A content validator for numbers.
+ */
+class StringContent extends AbstractContent {
+    /**
+     * Check that `value` is a string.
+     */
+    checkType(value: any): boolean {
+        return typeof value === "string";
+    }
+};
+/**
+ * A content validator for strings (singleton).
+ */
+export const STRING_CONTENT = new StringContent();
+
+
+/**
+ * A content validator for numbers.
+ */
+class NumberContent extends AbstractContent {
+    checkType(value: any): boolean {
+        return typeof value === "number";
+    }
+};
+
+/**
+ * A content validator for numbers (singleton).
+ */
+export const NUMBER_CONTENT = new NumberContent();
+
+/**
+ * A content validator for arrays.
+ */
+class ArrayContent extends AbstractContent {
+    constructor(public readonly content: AbstractContent) {
+        super()
+    }
+    /**
+     * Check that `value` is an array and that each value it contains
+     * has type `type.content`.
+     */
+    checkType(value: any): boolean {
+        if (!Array.isArray(value)) {
+            return false;
+        }
+        for (let item of value) {
+            if (!this.content.checkType(item)) {
+                return false;
+            }
+        }
+        return true;
+    }
+}
+class OptionalContent extends AbstractContent {
+    constructor(public readonly content: AbstractContent) {
+        super()
+    }
+    optional(): AbstractContent {
+        return this;
+    }
+    /**
+     * Check that value either has type `this.content` or is `null` or `undefined`.
+     */
+    checkType(value: any): boolean {
+        if (typeof value === "undefined") {
+            return true;
+        }
+        if (value === null) {
+            return true;
+        }
+        if (this.content.checkType(value)) {
+            return true;
+        }
+        return false;
+    }
+}
+export class SubTypeObjectContent extends AbstractContent {
+    constructor(public readonly fields: Record<string, AbstractContent>) {
+        super()
+    }
+    /**
+     * Check that `value` contains **at least** the fields of `this.fields`
+     * and that each field specified in `this.fields` holds a value that
+     * matches the type specified in `this.fields`.
+     */
+    checkType(value: any): boolean {
+        if (typeof value !== "object") {
+            return false;
+        }
+        if (value === null) {
+            // Let's not forget that `typeof null === "object"`
+            return false;
+        }
+        if (Array.isArray(value)) {
+            // Let's not forget that `typeof [...] === "object"`
+            return false;
+        }
+        for (let [k, expected] of Object.entries(this.fields)) {
+            if (!expected.checkType(value[k])) {
+                return false;
+            }
+        }
+        return true;
+    }
+}
+
+export class ExactTypeObjectContent extends SubTypeObjectContent {
+    constructor(public readonly fields: Record<string, AbstractContent>) {
+        super(fields)
+    }
+    /**
+     * Check that `value` contains **exactly** the fields of `this.fields`
+     * and that each field specified in `this.fields` holds a value that
+     * matches the type specified in `this.fields`.
+     */
+    checkType(value: any): boolean {
+        if (!super.checkType(value)) {
+            return false;
+        }
+        // Check that we don't have any field we're not expecting.
+        for (let k of Object.keys(value)) {
+            if (!(k in this.fields)) {
+                return false;
+            }
+        }
+        return true;
+    }
+}

src/commands/UnbanBanCommand.ts

@@ -19,6 +19,10 @@ import PolicyList from "../models/PolicyList";
 import { extractRequestError, LogLevel, LogService, MatrixGlob, RichReply } from "matrix-bot-sdk";
 import { RULE_ROOM, RULE_SERVER, RULE_USER, USER_RULE_TYPES } from "../models/ListRule";
 import { DEFAULT_LIST_EVENT_TYPE } from "./SetDefaultBanListCommand";
+import * as UntrustedContent from "../UntrustedContent";
+const DEFAULT_LIST_EXPECTED_CONTENT = new UntrustedContent.SubTypeObjectContent({
+    shortcode: UntrustedContent.STRING_CONTENT
+});
 
 interface Arguments {
     list: PolicyList | null;
@@ -31,7 +35,8 @@ interface Arguments {
 export async function parseArguments(roomId: string, event: any, mjolnir: Mjolnir, parts: string[]): Promise<Arguments | null> {
     let defaultShortcode: string | null = null;
     try {
-        const data: { shortcode: string } = await mjolnir.client.getAccountData(DEFAULT_LIST_EVENT_TYPE);
+        const untrustedData = await mjolnir.client.getAccountData(DEFAULT_LIST_EVENT_TYPE);
+        const data: { shortcode: string | null } = DEFAULT_LIST_EXPECTED_CONTENT.fallback(untrustedData, () => ({ shortcode: null }));
         defaultShortcode = data['shortcode'];
     } catch (e) {
         LogService.warn("UnbanBanCommand", "Non-fatal error getting default ban list");

src/models/PolicyList.ts

@@ -21,6 +21,7 @@ import { MatrixSendClient } from "../MatrixEmitter";
 import AwaitLock from "await-lock";
 import { monotonicFactory } from "ulidx";
 import { Mjolnir } from "../Mjolnir";
+import * as UntrustedContent from "../UntrustedContent";
 
 /**
  * Account data event type used to store the permalinks to each of the policylists.
@@ -33,6 +34,9 @@ import { Mjolnir } from "../Mjolnir";
  * ```
  */
 export const WATCHED_LISTS_EVENT_TYPE = "org.matrix.mjolnir.watched_lists";
+const WATCHED_LISTS_EXPECTED_CONTENT = new UntrustedContent.SubTypeObjectContent({
+    references: UntrustedContent.STRING_CONTENT.array()
+});
 
 /**
  * A prefix used to record that we have already warned at least once that a PolicyList room is unprotected.
@@ -707,7 +711,13 @@ export class PolicyListManager {
 
         let watchedListsEvent: { references?: string[] } | null = null;
         try {
-            watchedListsEvent = await this.mjolnir.client.getAccountData(WATCHED_LISTS_EVENT_TYPE);
+            watchedListsEvent = WATCHED_LISTS_EXPECTED_CONTENT.fallback(
+                await this.mjolnir.client.getAccountData(WATCHED_LISTS_EVENT_TYPE),
+                () => {
+                    LogService.warn('Mjolnir', "Invalid account data for Mjolnir's watched lists, assuming first start.");
+                    return null;
+                }
+            );
         } catch (e) {
             if (e.statusCode === 404) {
                 LogService.warn('Mjolnir', "Couldn't find account data for Mjolnir's watched lists, assuming first start.", extractRequestError(e));

src/protections/ProtectionManager.ts

@@ -31,6 +31,7 @@ import { htmlEscape } from "../utils";
 import { ERROR_KIND_FATAL, ERROR_KIND_PERMISSION } from "../ErrorCache";
 import { RoomUpdateError } from "../models/RoomUpdateError";
 import { LocalAbuseReports } from "./LocalAbuseReports";
+import * as UntrustedContent from "../UntrustedContent";
 
 const PROTECTIONS: Protection[] = [
     new FirstMessageIsImage(),
@@ -45,6 +46,10 @@ const PROTECTIONS: Protection[] = [
 ];
 
 const ENABLED_PROTECTIONS_EVENT_TYPE = "org.matrix.mjolnir.enabled_protections";
+const ENABLED_PROTECTIONS_EXPECTED_CONTENT = new UntrustedContent.SubTypeObjectContent({
+    enabled: UntrustedContent.STRING_CONTENT.array(),
+}).optional();
+
 const CONSEQUENCE_EVENT_DATA = "org.matrix.mjolnir.consequence";
 
 /**
@@ -87,7 +92,10 @@ export class ProtectionManager {
 
         let enabledProtections: { enabled: string[] } | null = null;
         try {
-            enabledProtections = await this.mjolnir.client.getAccountData(ENABLED_PROTECTIONS_EVENT_TYPE);
+            enabledProtections = ENABLED_PROTECTIONS_EXPECTED_CONTENT.fallback(
+                await this.mjolnir.client.getAccountData(ENABLED_PROTECTIONS_EVENT_TYPE),
+                () => null
+            );
         } catch {
             // this setting either doesn't exist, or we failed to read it (bad network?)
             // TODO: retry on certain failures?