fix: harden the plugin runtime after the 2.9.0 security review

plugins/remote-desktop/src/backend/guacamole-server.ts

@@ -264,6 +264,23 @@ export function createGuacamoleServer(
 
   return {
     handleUpgrade(request, socket, head) {
+      // The route is public, so the token is the only credential: refuse
+      // anything forged, edited or expired before guacamole-lite reads it.
+      const token = new URL(
+        request.url ?? "/",
+        "http://localhost",
+      ).searchParams.get("token");
+      if (!token || !deps.tokens.verifyToken(token)) {
+        try {
+          socket.write(
+            "HTTP/1.1 401 Unauthorized\r\nConnection: close\r\n\r\n",
+          );
+        } catch {
+          // The peer may already be gone.
+        }
+        socket.destroy();
+        return;
+      }
       const wss = (
         server as unknown as {
           webSocketServer?: {

plugins/remote-desktop/src/backend/routes.ts

@@ -3,7 +3,10 @@ import net from "net";
 import path from "path";
 import type { Request, Response, Router } from "express";
 import type { PluginContext } from "@termix/plugin-sdk/backend";
-import type { GuacamoleTokenService } from "./token-service.js";
+import {
+  isServerOwnedSetting,
+  type GuacamoleTokenService,
+} from "./token-service.js";
 import type { GuacdOptions } from "./guacd-config.js";
 import type { RemoteSessions } from "./sessions.js";
 import type { RecordingsWriter } from "./guacamole-server.js";
@@ -188,6 +191,9 @@ export function registerRoutes(router: Router, deps: RouteDeps): void {
       }
       const options: Record<string, unknown> = {};
       for (const [key, value] of Object.entries(raw)) {
+        // A quick connect names a host, never where guacd is or what paths
+        // it writes to: those would let any user aim guacd anywhere.
+        if (isServerOwnedSetting(key)) continue;
         if (value !== "auto") options[key] = value;
       }
       const host = String(hostname);
@@ -199,6 +205,7 @@ export function registerRoutes(router: Router, deps: RouteDeps): void {
         for (const [key, value] of Object.entries(
           await readUserDefaults(ctx, userId),
         )) {
+          if (isServerOwnedSetting(key)) continue;
           if (options[key] === undefined) options[key] = value;
         }
       }

plugins/remote-desktop/src/backend/token-service.ts

@@ -47,6 +47,8 @@ export interface GuacamoleToken {
   connection: GuacamoleConnectionSettings;
   recording?: GuacamoleRecordingMetadata;
   termixMeta?: TermixGuacMeta;
+  /** When the token stops opening displays, in ms since the epoch. */
+  exp?: number;
 }
 
 export interface GuacamoleRecordingMetadata {
@@ -63,6 +65,26 @@ export interface GuacamoleRecordingMetadata {
 const CIPHER = "aes-256-cbc";
 const KEY_LENGTH = 32;
 
+/**
+ * How long a token opens a display for. Short, because a token for a saved
+ * host carries its password and ends up in the display URL.
+ */
+export const TOKEN_TTL_MS = 5 * 60 * 1000;
+
+/**
+ * Settings the server decides, never the caller: where guacd is, and every
+ * path guacd writes or reads on its own filesystem.
+ */
+export function isServerOwnedSetting(key: string): boolean {
+  return (
+    key.startsWith("guacd") ||
+    key.endsWith("-path") ||
+    key.startsWith("create-") ||
+    key.startsWith("recording-") ||
+    key.startsWith("typescript-")
+  );
+}
+
 const silentLogger: TokenLogger = { warn: () => {}, error: () => {} };
 
 /**
@@ -71,9 +93,17 @@ const silentLogger: TokenLogger = { warn: () => {}, error: () => {} };
  */
 export class GuacamoleTokenService {
   private encryptionKey: Buffer;
+  private macKey: Buffer;
 
-  constructor(private readonly log: TokenLogger = silentLogger) {
+  constructor(
+    private readonly log: TokenLogger = silentLogger,
+    private readonly now: () => number = Date.now,
+  ) {
     this.encryptionKey = this.initializeKey();
+    this.macKey = crypto
+      .createHmac("sha256", this.encryptionKey)
+      .update("termix-guacamole-token-mac")
+      .digest();
   }
 
   private initializeKey(): Buffer {
@@ -105,12 +135,17 @@ export class GuacamoleTokenService {
     return this.encryptionKey;
   }
 
+  /**
+   * guacamole-lite only knows AES-CBC, which does not notice tampering, so
+   * the ciphertext also carries an HMAC that verifyToken checks before
+   * guacamole-lite ever sees the token.
+   */
   encryptToken(tokenObject: GuacamoleToken): string {
     const iv = crypto.randomBytes(16);
     const cipher = crypto.createCipheriv(CIPHER, this.encryptionKey, iv);
 
     let encrypted = cipher.update(
-      JSON.stringify(tokenObject),
+      JSON.stringify({ ...tokenObject, exp: this.now() + TOKEN_TTL_MS }),
       "utf8",
       "base64",
     );
@@ -119,11 +154,50 @@ export class GuacamoleTokenService {
     const data = {
       iv: iv.toString("base64"),
       value: encrypted,
+      mac: this.mac(iv.toString("base64"), encrypted),
     };
 
     return Buffer.from(JSON.stringify(data)).toString("base64");
   }
 
+  private mac(iv: string, value: string): string {
+    return crypto
+      .createHmac("sha256", this.macKey)
+      .update(`${iv}.${value}`)
+      .digest("base64");
+  }
+
+  /**
+   * The token if it is one this server minted, untouched and not expired,
+   * else null. The gate in front of guacamole-lite.
+   */
+  verifyToken(token: string): GuacamoleToken | null {
+    let data: { iv?: unknown; value?: unknown; mac?: unknown };
+    try {
+      data = JSON.parse(Buffer.from(token, "base64").toString("utf8"));
+    } catch {
+      return null;
+    }
+    if (
+      typeof data?.iv !== "string" ||
+      typeof data.value !== "string" ||
+      typeof data.mac !== "string"
+    ) {
+      return null;
+    }
+    const expected = Buffer.from(this.mac(data.iv, data.value), "base64");
+    const given = Buffer.from(data.mac, "base64");
+    if (
+      expected.length !== given.length ||
+      !crypto.timingSafeEqual(expected, given)
+    ) {
+      return null;
+    }
+    const decoded = this.decryptToken(token);
+    if (!decoded || typeof decoded.exp !== "number") return null;
+    return decoded.exp > this.now() ? decoded : null;
+  }
+
   decryptToken(token: string): GuacamoleToken | null {
     try {
       const data = JSON.parse(Buffer.from(token, "base64").toString("utf8"));

plugins/remote-desktop/tests/backend/routes.test.ts

@@ -261,6 +261,26 @@ describe("the rest of the routes", () => {
     ).toMatchObject({ hostname: "10.0.0.9", port: 3389, "color-depth": 24 });
   });
 
+  it("ignores guacd and path settings a quick connect tries to set", async () => {
+    server = await start();
+    const response = await server.request("POST", "/token", {
+      type: "rdp",
+      hostname: "10.0.0.9",
+      guacdHost: "attacker.example",
+      guacdPort: 4822,
+      "drive-path": "/etc",
+      "recording-path": "/tmp/x",
+      width: 800,
+    });
+    expect(response.status).toBe(200);
+    const token = tokens.decryptToken(response.body.token);
+    expect(token?.connection.guacdHost).toBeUndefined();
+    expect(token?.connection.guacdPort).toBeUndefined();
+    expect(token?.connection.settings["drive-path"]).toBeUndefined();
+    expect(token?.connection.settings["recording-path"]).toBeUndefined();
+    expect(token?.connection.settings.width).toBe(800);
+  });
+
   it("offers native RDP only in the desktop app", async () => {
     server = await start();
     expect((await server.request("GET", "/native-rdp")).body).toEqual({

plugins/remote-desktop/tests/backend/token-security.test.ts

@@ -0,0 +1,93 @@
+/**
+ * A display token is the only credential on the public /display socket, so
+ * it has to be one this server minted, unedited and recent.
+ */
+
+import { describe, expect, it } from "vitest";
+import {
+  GuacamoleTokenService,
+  TOKEN_TTL_MS,
+  isServerOwnedSetting,
+} from "../../src/backend/token-service.js";
+
+process.env.JWT_SECRET = "remote-desktop-test-secret";
+
+function decode(token: string) {
+  return JSON.parse(Buffer.from(token, "base64").toString("utf8")) as {
+    iv: string;
+    value: string;
+    mac: string;
+  };
+}
+
+function encode(data: unknown): string {
+  return Buffer.from(JSON.stringify(data)).toString("base64");
+}
+
+describe("verifyToken", () => {
+  it("accepts a fresh token it minted", () => {
+    const tokens = new GuacamoleTokenService();
+    const token = tokens.createRdpToken("10.0.0.7", "admin", "secret");
+    expect(tokens.verifyToken(token)?.connection.settings.hostname).toBe(
+      "10.0.0.7",
+    );
+  });
+
+  it("refuses an edited ciphertext or IV", () => {
+    const tokens = new GuacamoleTokenService();
+    const data = decode(tokens.createRdpToken("10.0.0.7", "admin", "secret"));
+    const iv = Buffer.from(data.iv, "base64");
+    iv[0] ^= 0xff;
+    expect(
+      tokens.verifyToken(encode({ ...data, iv: iv.toString("base64") })),
+    ).toBeNull();
+    expect(
+      tokens.verifyToken(encode({ ...data, value: `A${data.value.slice(1)}` })),
+    ).toBeNull();
+  });
+
+  it("refuses a token with no MAC, as guacamole-lite alone would accept", () => {
+    const tokens = new GuacamoleTokenService();
+    const { iv, value } = decode(tokens.createRdpToken("h", "u", "p"));
+    expect(tokens.verifyToken(encode({ iv, value }))).toBeNull();
+  });
+
+  it("refuses a token once it is older than the TTL", () => {
+    let now = 1_000_000;
+    const tokens = new GuacamoleTokenService(undefined, () => now);
+    const token = tokens.createJoinToken("session-1", true);
+    now += TOKEN_TTL_MS - 1;
+    expect(tokens.verifyToken(token)).not.toBeNull();
+    now += 2;
+    expect(tokens.verifyToken(token)).toBeNull();
+  });
+
+  it("refuses garbage", () => {
+    const tokens = new GuacamoleTokenService();
+    expect(tokens.verifyToken("not-a-token")).toBeNull();
+    expect(tokens.verifyToken(encode({ iv: 1 }))).toBeNull();
+  });
+});
+
+describe("isServerOwnedSetting", () => {
+  it.each([
+    "guacdHost",
+    "guacdPort",
+    "drive-path",
+    "create-drive-path",
+    "recording-path",
+    "recording-name",
+    "create-recording-path",
+    "typescript-path",
+    "sftp-private-key-path",
+  ])("keeps %s for the server", (key) => {
+    expect(isServerOwnedSetting(key)).toBe(true);
+  });
+
+  it.each(["width", "security", "ignore-cert", "color-depth", "enable-drive"])(
+    "lets a caller set %s",
+    (key) => {
+      expect(isServerOwnedSetting(key)).toBe(false);
+    },
+  );
+});