Merge branch 'security/20240330' into 'develop'

security/20240330

Co-authored-by: Laura Hausmann <laura@hausmann.dev>
Co-authored-by: mei23 <m@m544.net>

See merge request firefish/firefish!10718

docs/changelog.md

@@ -5,9 +5,10 @@ Critical security updates are indicated by the :warning: icon.
 - Server administrators should check [notice-for-admins.md](./notice-for-admins.md) as well.
 - Third-party client/bot developers may want to check [api-change.md](./api-change.md) as well.
 
-## Unreleased
+## :warning: Unreleased
 
-- Fix bugs
+- Fix bugs (including a critical security issue)
+	- We are very thankful to Oneric (the reporter of the security issue) and Laura Hausmann (Iceshrimp maintainer) for kindly and securely sharing the information to fix the issue.
 
 ## [v20240326](https://firefish.dev/firefish/firefish/-/merge_requests/10713/commits)
 

packages/backend/src/misc/download-url.ts

@@ -8,10 +8,15 @@ import chalk from "chalk";
 import Logger from "@/services/logger.js";
 import IPCIDR from "ip-cidr";
 import PrivateIp from "private-ip";
+import { isValidUrl } from "./is-valid-url.js";
 
 const pipeline = util.promisify(stream.pipeline);
 
 export async function downloadUrl(url: string, path: string): Promise<void> {
+	if (!isValidUrl(url)) {
+		throw new StatusError("Invalid URL", 400);
+	}
+
 	const logger = new Logger("download");
 
 	logger.info(`Downloading ${chalk.cyan(url)} ...`);
@@ -44,6 +49,12 @@ export async function downloadUrl(url: string, path: string): Promise<void> {
 				limit: 0,
 			},
 		})
+		.on("redirect", (res: Got.Response, opts: Got.NormalizedOptions) => {
+			if (!isValidUrl(opts.url)) {
+				logger.warn(`Invalid URL: ${opts.url}`);
+				req.destroy();
+			}
+		})
 		.on("response", (res: Got.Response) => {
 			if (
 				(process.env.NODE_ENV === "production" ||

packages/backend/src/misc/fetch.ts

@@ -2,9 +2,10 @@ import * as http from "node:http";
 import * as https from "node:https";
 import type { URL } from "node:url";
 import CacheableLookup from "cacheable-lookup";
-import fetch from "node-fetch";
+import fetch, { RequestRedirect } from "node-fetch";
 import { HttpProxyAgent, HttpsProxyAgent } from "hpagent";
 import config from "@/config/index.js";
+import { isValidUrl } from "./is-valid-url.js";
 
 export async function getJson(
 	url: string,
@@ -57,7 +58,12 @@ export async function getResponse(args: {
 	headers: Record<string, string>;
 	timeout?: number;
 	size?: number;
+	redirect?: RequestRedirect;
 }) {
+	if (!isValidUrl(args.url)) {
+		throw new StatusError("Invalid URL", 400);
+	}
+
 	const timeout = args.timeout || 10 * 1000;
 
 	const controller = new AbortController();
@@ -73,8 +79,16 @@ export async function getResponse(args: {
 		size: args.size || 10 * 1024 * 1024,
 		agent: getAgentByUrl,
 		signal: controller.signal,
+		redirect: args.redirect,
 	});
 
+	if (args.redirect === "manual" && [301, 302, 307, 308].includes(res.status)) {
+		if (!isValidUrl(res.url)) {
+			throw new StatusError("Invalid URL", 400);
+		}
+		return res;
+	}
+
 	if (!res.ok) {
 		throw new StatusError(
 			`${res.status} ${res.statusText}`,

packages/backend/src/misc/is-valid-url.ts

@@ -0,0 +1,20 @@
+export function isValidUrl(url: string | URL | undefined): boolean {
+	if (process.env.NODE_ENV !== "production") return true;
+
+	try {
+		if (url == null) return false;
+
+		const u = typeof url === "string" ? new URL(url) : url;
+		if (!u.protocol.match(/^https?:$/) || u.hostname === "unix") {
+			return false;
+		}
+
+		if (u.port !== "" && !["80", "443"].includes(u.port)) {
+			return false;
+		}
+
+		return true;
+	} catch {
+		return false;
+	}
+}

packages/backend/src/remote/activitypub/request.ts

@@ -1,10 +1,12 @@
 import config from "@/config/index.js";
 import { getUserKeypair } from "@/misc/keypair-store.js";
 import type { User, ILocalUser } from "@/models/entities/user.js";
-import { getResponse } from "@/misc/fetch.js";
+import { StatusError, getResponse } from "@/misc/fetch.js";
 import { createSignedPost, createSignedGet } from "./ap-request.js";
 import type { Response } from "node-fetch";
 import type { IObject } from "./type.js";
+import { isValidUrl } from "@/misc/is-valid-url.js";
+import { apLogger } from "@/remote/activitypub/logger.js";
 
 export default async (user: { id: User["id"] }, url: string, object: any) => {
 	const body = JSON.stringify(object);
@@ -33,10 +35,19 @@ export default async (user: { id: User["id"] }, url: string, object: any) => {
 
 /**
  * Get ActivityPub object
- * @param user http-signature user
  * @param url URL to fetch
+ * @param user http-signature user
+ * @param redirects whether or not to accept redirects
  */
-export async function apGet(url: string, user?: ILocalUser): Promise<IObject> {
+export async function apGet(
+	url: string,
+	user?: ILocalUser,
+	redirects: boolean = true,
+): Promise<{ finalUrl: string; content: IObject }> {
+	if (!isValidUrl(url)) {
+		throw new StatusError("Invalid URL", 400);
+	}
+
 	let res: Response;
 
 	if (user != null) {
@@ -56,7 +67,16 @@ export async function apGet(url: string, user?: ILocalUser): Promise<IObject> {
 			url,
 			method: req.request.method,
 			headers: req.request.headers,
+			redirect: redirects ? "manual" : "error",
 		});
+
+		if (redirects && [301, 302, 307, 308].includes(res.status)) {
+			const newUrl = res.headers.get("location");
+			if (newUrl == null)
+				throw new Error("apGet got redirect but no target location");
+			apLogger.debug(`apGet is redirecting to ${newUrl}`);
+			return apGet(newUrl, user, false);
+		}
 	} else {
 		res = await getResponse({
 			url,
@@ -66,12 +86,23 @@ export async function apGet(url: string, user?: ILocalUser): Promise<IObject> {
 					'application/activity+json, application/ld+json; profile="https://www.w3.org/ns/activitystreams"',
 				"User-Agent": config.userAgent,
 			},
+			redirect: redirects ? "manual" : "error",
 		});
+
+		if (redirects && [301, 302, 307, 308].includes(res.status)) {
+			const newUrl = res.headers.get("location");
+			if (newUrl == null)
+				throw new Error("apGet got redirect but no target location");
+			apLogger.debug(`apGet is redirecting to ${newUrl}`);
+			return apGet(newUrl, undefined, false);
+		}
 	}
 
 	const contentType = res.headers.get("content-type");
 	if (contentType == null || !validateContentType(contentType)) {
-		throw new Error("Invalid Content Type");
+		throw new Error(
+			`apGet response had unexpected content-type: ${contentType}`,
+		);
 	}
 
 	if (res.body == null) throw new Error("body is null");
@@ -79,7 +110,10 @@ export async function apGet(url: string, user?: ILocalUser): Promise<IObject> {
 	const text = await res.text();
 	if (text.length > 65536) throw new Error("too big result");
 
-	return JSON.parse(text) as IObject;
+	return {
+		finalUrl: res.url,
+		content: JSON.parse(text) as IObject,
+	};
 }
 
 function validateContentType(contentType: string): boolean {

packages/backend/src/remote/activitypub/resolver.ts

@@ -6,7 +6,13 @@ import { extractDbHost, isSelfHost } from "@/misc/convert-host.js";
 import { apGet } from "./request.js";
 import type { IObject, ICollection, IOrderedCollection } from "./type.js";
 import { isCollectionOrOrderedCollection, getApId } from "./type.js";
-import { Notes, NoteReactions, Polls, Users } from "@/models/index.js";
+import {
+	FollowRequests,
+	Notes,
+	NoteReactions,
+	Polls,
+	Users,
+} from "@/models/index.js";
 import { parseUri } from "./db-resolver.js";
 import renderNote from "@/remote/activitypub/renderer/note.js";
 import { renderLike } from "@/remote/activitypub/renderer/like.js";
@@ -17,7 +23,7 @@ import { renderActivity } from "@/remote/activitypub/renderer/index.js";
 import renderFollow from "@/remote/activitypub/renderer/follow.js";
 import { shouldBlockInstance } from "@/misc/should-block-instance.js";
 import { apLogger } from "@/remote/activitypub/logger.js";
-import { In, IsNull, Not } from "typeorm";
+import { IsNull, Not } from "typeorm";
 
 export default class Resolver {
 	private history: Set<string>;
@@ -114,7 +120,7 @@ export default class Resolver {
 		apLogger.debug("Getting object from remote, authenticated as user:");
 		apLogger.debug(JSON.stringify(this.user, null, 2));
 
-		const object = await apGet(value, this.user);
+		const { finalUrl, content: object } = await apGet(value, this.user);
 
 		if (
 			object == null ||
@@ -127,61 +133,73 @@ export default class Resolver {
 			throw new Error("invalid response");
 		}
 
-		return object;
+		if (object.id == null) {
+			throw new Error("Object has no ID");
+		}
+
+		if (finalUrl === object.id) return object;
+
+		if (new URL(finalUrl).host !== new URL(object.id).host) {
+			throw new Error("Object ID host doesn't match final url host");
+		}
+
+		const finalRes = await apGet(object.id, this.user);
+
+		if (finalRes.finalUrl !== finalRes.content.id)
+			throw new Error(
+				"Object ID still doesn't match final URL after second fetch attempt",
+			);
+
+		return finalRes.content;
 	}
 
-	private resolveLocal(url: string): Promise<IObject> {
+	private async resolveLocal(url: string): Promise<IObject> {
 		const parsed = parseUri(url);
 		if (!parsed.local) throw new Error("resolveLocal: not local");
 
 		switch (parsed.type) {
 			case "notes":
-				return Notes.findOneByOrFail({ id: parsed.id }).then((note) => {
-					if (parsed.rest === "activity") {
-						// this refers to the create activity and not the note itself
-						return renderActivity(renderCreate(renderNote(note), note));
-					} else {
-						return renderNote(note);
-					}
-				});
+				const note = await Notes.findOneByOrFail({ id: parsed.id });
+				if (parsed.rest === "activity") {
+					// this refers to the create activity and not the note itself
+					return renderActivity(renderCreate(renderNote(note), note));
+				} else {
+					return renderNote(note);
+				}
 			case "users":
-				return Users.findOneByOrFail({ id: parsed.id }).then((user) =>
-					renderPerson(user as ILocalUser),
-				);
+				const user = await Users.findOneByOrFail({ id: parsed.id });
+				return await renderPerson(user as ILocalUser);
 			case "questions":
 				// Polls are indexed by the note they are attached to.
-				return Promise.all([
+				const [pollNote, poll] = await Promise.all([
 					Notes.findOneByOrFail({ id: parsed.id }),
 					Polls.findOneByOrFail({ noteId: parsed.id }),
-				]).then(([note, poll]) =>
-					renderQuestion({ id: note.userId }, note, poll),
-				);
+				]);
+				return await renderQuestion({ id: pollNote.userId }, pollNote, poll);
 			case "likes":
-				return NoteReactions.findOneByOrFail({ id: parsed.id }).then(
-					(reaction) => renderActivity(renderLike(reaction, { uri: null })),
-				);
+				const reaction = await NoteReactions.findOneByOrFail({ id: parsed.id });
+				return renderActivity(renderLike(reaction, { uri: null }));
 			case "follows":
 				// if rest is a <followee id>
 				if (parsed.rest != null && /^\w+$/.test(parsed.rest)) {
-					return Promise.all(
+					const [follower, followee] = await Promise.all(
 						[parsed.id, parsed.rest].map((id) => Users.findOneByOrFail({ id })),
-					).then(([follower, followee]) =>
-						renderActivity(renderFollow(follower, followee, url)),
 					);
+					return renderActivity(renderFollow(follower, followee, url));
 				}
 
 				// Another situation is there is only requestId, then obtained object from database.
-				const followRequest = FollowRequests.findOneBy({
+				const followRequest = await FollowRequests.findOneBy({
 					id: parsed.id,
 				});
 				if (followRequest == null) {
 					throw new Error("resolveLocal: invalid follow URI");
 				}
-				const follower = Users.findOneBy({
+				const follower = await Users.findOneBy({
 					id: followRequest.followerId,
 					host: IsNull(),
 				});
-				const followee = Users.findOneBy({
+				const followee = await Users.findOneBy({
 					id: followRequest.followeeId,
 					host: Not(IsNull()),
 				});
@@ -190,7 +208,7 @@ export default class Resolver {
 				}
 				return renderActivity(renderFollow(follower, followee, url));
 			default:
-				throw new Error(`resolveLocal: type ${type} unhandled`);
+				throw new Error(`resolveLocal: type ${parsed.type} unhandled`);
 		}
 	}
 }