diff --git a/docs/changelog.md b/docs/changelog.md
index 4edb1f2446..b5a66b7680 100644
--- a/docs/changelog.md
+++ b/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)
diff --git a/packages/backend/src/misc/download-url.ts b/packages/backend/src/misc/download-url.ts
index 83680c175f..ab04e8aa9c 100644
--- a/packages/backend/src/misc/download-url.ts
+++ b/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" ||
diff --git a/packages/backend/src/misc/fetch.ts b/packages/backend/src/misc/fetch.ts
index 75eb2e17f4..8108b13d9e 100644
--- a/packages/backend/src/misc/fetch.ts
+++ b/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}`,
diff --git a/packages/backend/src/misc/is-valid-url.ts b/packages/backend/src/misc/is-valid-url.ts
new file mode 100644
index 0000000000..5aebefcb71
--- /dev/null
+++ b/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;
+ }
+}
diff --git a/packages/backend/src/remote/activitypub/request.ts b/packages/backend/src/remote/activitypub/request.ts
index 9fc2a8115c..f6d33b8549 100644
--- a/packages/backend/src/remote/activitypub/request.ts
+++ b/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 {
diff --git a/packages/backend/src/remote/activitypub/resolver.ts b/packages/backend/src/remote/activitypub/resolver.ts
index 7ddb4b8022..7c0bd6e102 100644
--- a/packages/backend/src/remote/activitypub/resolver.ts
+++ b/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`);
}
}
}