From ce6900124392150d683a6bfae2bda7d7e1241eab Mon Sep 17 00:00:00 2001 From: mei23 <m@m544.net> Date: Tue, 26 Mar 2024 21:05:29 +0900 Subject: [PATCH 1/8] fix (backend): improve URL check https://github.com/mei23/misskey/commit/13ea67bee40b394906f55e75d5d80d6e4f7fa171 https://github.com/mei23/misskey/commit/da12d5b079b690d3705a76919ed817f6dfc03ac8 Co-authored-by: naskya <m@naskya.net> --- packages/backend/src/misc/download-url.ts | 11 ++++++++++ packages/backend/src/misc/fetch.ts | 9 +++++++++ packages/backend/src/misc/is-valid-url.ts | 20 +++++++++++++++++++ .../backend/src/remote/activitypub/request.ts | 7 ++++++- 4 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 packages/backend/src/misc/is-valid-url.ts 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..ea3506550c 100644 --- a/packages/backend/src/misc/fetch.ts +++ b/packages/backend/src/misc/fetch.ts @@ -5,6 +5,7 @@ import CacheableLookup from "cacheable-lookup"; import fetch 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, @@ -58,6 +59,10 @@ export async function getResponse(args: { timeout?: number; size?: number; }) { + if (!isValidUrl(args.url)) { + throw new StatusError("Invalid URL", 400); + } + const timeout = args.timeout || 10 * 1000; const controller = new AbortController(); @@ -83,6 +88,10 @@ export async function getResponse(args: { ); } + if (res.redirected && !isValidUrl(res.url)) { + throw new StatusError("Invalid URL", 400); + } + return res; } 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..07ccbf4e82 100644 --- a/packages/backend/src/remote/activitypub/request.ts +++ b/packages/backend/src/remote/activitypub/request.ts @@ -1,10 +1,11 @@ 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"; export default async (user: { id: User["id"] }, url: string, object: any) => { const body = JSON.stringify(object); @@ -37,6 +38,10 @@ export default async (user: { id: User["id"] }, url: string, object: any) => { * @param url URL to fetch */ export async function apGet(url: string, user?: ILocalUser): Promise<IObject> { + if (!isValidUrl(url)) { + throw new StatusError("Invalid URL", 400); + } + let res: Response; if (user != null) { From 00b15bb17c4ce24a2b6580779b2c92141015a31d Mon Sep 17 00:00:00 2001 From: naskya <m@naskya.net> Date: Wed, 27 Mar 2024 06:51:01 +0900 Subject: [PATCH 2/8] refactor (backend): mark resolveLocal as async There are many type errors that need to be fixed :( --- .../src/remote/activitypub/resolver.ts | 54 +++++++++---------- 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/packages/backend/src/remote/activitypub/resolver.ts b/packages/backend/src/remote/activitypub/resolver.ts index 7ddb4b8022..cdba3cba02 100644 --- a/packages/backend/src/remote/activitypub/resolver.ts +++ b/packages/backend/src/remote/activitypub/resolver.ts @@ -6,7 +6,7 @@ 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 +17,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>; @@ -130,58 +130,52 @@ export default class Resolver { return object; } - 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( - [parsed.id, parsed.rest].map((id) => Users.findOneByOrFail({ id })), - ).then(([follower, followee]) => - renderActivity(renderFollow(follower, followee, url)), - ); + const [follower, followee] = await Promise.all( + [parsed.id, parsed.rest].map((id) => Users.findOneByOrFail({ id }))); + 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 +184,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`); } } } From 850c52ef6383cf4bbe5801baddaf520d3a09ecb1 Mon Sep 17 00:00:00 2001 From: Laura Hausmann <laura@hausmann.dev> Date: Wed, 27 Mar 2024 07:07:19 +0900 Subject: [PATCH 3/8] feat (backend): permit redirects for AP object lookups https://iceshrimp.dev/iceshrimp/iceshrimp/commit/8d7d95fd2344ffe2954821af167305f5231f3b28 Co-authored-by: naskya <m@naskya.net> --- packages/backend/src/misc/fetch.ts | 8 +++++- .../backend/src/remote/activitypub/request.ts | 28 +++++++++++++++++-- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/packages/backend/src/misc/fetch.ts b/packages/backend/src/misc/fetch.ts index ea3506550c..ee903a79e9 100644 --- a/packages/backend/src/misc/fetch.ts +++ b/packages/backend/src/misc/fetch.ts @@ -2,7 +2,7 @@ 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"; @@ -58,6 +58,7 @@ 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); @@ -78,8 +79,13 @@ 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)) { + return res; + } + if (!res.ok) { throw new StatusError( `${res.status} ${res.statusText}`, diff --git a/packages/backend/src/remote/activitypub/request.ts b/packages/backend/src/remote/activitypub/request.ts index 07ccbf4e82..3dbad8a97a 100644 --- a/packages/backend/src/remote/activitypub/request.ts +++ b/packages/backend/src/remote/activitypub/request.ts @@ -6,6 +6,7 @@ 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); @@ -34,10 +35,15 @@ 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<IObject> { if (!isValidUrl(url)) { throw new StatusError("Invalid URL", 400); } @@ -61,7 +67,15 @@ 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, @@ -71,12 +85,20 @@ 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"); From ada0137a35f238edaea02ffb79b18e1fa93b077a Mon Sep 17 00:00:00 2001 From: Laura Hausmann <laura@hausmann.dev> Date: Wed, 27 Mar 2024 07:16:44 +0900 Subject: [PATCH 4/8] fix (backend): verify object id host matches final URL when fetching remote activities https://iceshrimp.dev/iceshrimp/iceshrimp/commit/5f6096c1b7b37b771055e6e3b9d7b15ca10a05da Co-authored-by: naskya <m@naskya.net> --- .../backend/src/remote/activitypub/request.ts | 19 ++++++++++++------ .../src/remote/activitypub/resolver.ts | 20 ++++++++++++++++--- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/packages/backend/src/remote/activitypub/request.ts b/packages/backend/src/remote/activitypub/request.ts index 3dbad8a97a..f6d33b8549 100644 --- a/packages/backend/src/remote/activitypub/request.ts +++ b/packages/backend/src/remote/activitypub/request.ts @@ -42,8 +42,8 @@ export default async (user: { id: User["id"] }, url: string, object: any) => { export async function apGet( url: string, user?: ILocalUser, - redirects: boolean = true -): Promise<IObject> { + redirects: boolean = true, +): Promise<{ finalUrl: string; content: IObject }> { if (!isValidUrl(url)) { throw new StatusError("Invalid URL", 400); } @@ -72,7 +72,8 @@ export async function apGet( 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"); + 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); } @@ -90,7 +91,8 @@ export async function apGet( 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"); + 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); } @@ -98,7 +100,9 @@ export async function apGet( const contentType = res.headers.get("content-type"); if (contentType == null || !validateContentType(contentType)) { - throw new Error(`apGet response had unexpected content-type: ${contentType}`); + throw new Error( + `apGet response had unexpected content-type: ${contentType}`, + ); } if (res.body == null) throw new Error("body is null"); @@ -106,7 +110,10 @@ export async function apGet( 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 cdba3cba02..6e781850b9 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 { FollowRequests, 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"; @@ -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,6 +133,13 @@ export default class Resolver { throw new Error("invalid response"); } + if ( + object.id != null && + new URL(finalUrl).host != new URL(object.id).host + ) { + throw new Error("Object ID host doesn't match final url host"); + } + return object; } @@ -160,7 +173,8 @@ export default class Resolver { // if rest is a <followee id> if (parsed.rest != null && /^\w+$/.test(parsed.rest)) { const [follower, followee] = await Promise.all( - [parsed.id, parsed.rest].map((id) => Users.findOneByOrFail({ id }))); + [parsed.id, parsed.rest].map((id) => Users.findOneByOrFail({ id })), + ); return renderActivity(renderFollow(follower, followee, url)); } From 2e51a33ae572eff3766a24a791bf73c64e017fd6 Mon Sep 17 00:00:00 2001 From: Laura Hausmann <laura@hausmann.dev> Date: Wed, 27 Mar 2024 07:34:57 +0900 Subject: [PATCH 5/8] fix (backend): stricter hostname checking when fetching remote objects Co-authored-by: naskya <m@naskya.net> --- .../backend/src/remote/activitypub/resolver.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/backend/src/remote/activitypub/resolver.ts b/packages/backend/src/remote/activitypub/resolver.ts index 6e781850b9..94830e1054 100644 --- a/packages/backend/src/remote/activitypub/resolver.ts +++ b/packages/backend/src/remote/activitypub/resolver.ts @@ -133,14 +133,21 @@ export default class Resolver { throw new Error("invalid response"); } - if ( - object.id != null && - new URL(finalUrl).host != new URL(object.id).host - ) { + if (object.id == null) return object; + 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"); } - return object; + 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 async resolveLocal(url: string): Promise<IObject> { From e753b313dae08d63cdfbc674d4a188e38d948de9 Mon Sep 17 00:00:00 2001 From: Laura Hausmann <laura@hausmann.dev> Date: Wed, 27 Mar 2024 23:01:51 +0900 Subject: [PATCH 6/8] fix (backend): reject anonymous objects --- packages/backend/src/remote/activitypub/resolver.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/backend/src/remote/activitypub/resolver.ts b/packages/backend/src/remote/activitypub/resolver.ts index 94830e1054..7c0bd6e102 100644 --- a/packages/backend/src/remote/activitypub/resolver.ts +++ b/packages/backend/src/remote/activitypub/resolver.ts @@ -133,7 +133,10 @@ export default class Resolver { throw new Error("invalid response"); } - if (object.id == null) 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) { From b3668f67a056e722c9dc61941bfdee30684d79eb Mon Sep 17 00:00:00 2001 From: naskya <m@naskya.net> Date: Thu, 28 Mar 2024 15:35:51 +0900 Subject: [PATCH 7/8] fix (backend): check redirect url --- packages/backend/src/misc/fetch.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/backend/src/misc/fetch.ts b/packages/backend/src/misc/fetch.ts index ee903a79e9..8108b13d9e 100644 --- a/packages/backend/src/misc/fetch.ts +++ b/packages/backend/src/misc/fetch.ts @@ -83,6 +83,9 @@ export async function getResponse(args: { }); if (args.redirect === "manual" && [301, 302, 307, 308].includes(res.status)) { + if (!isValidUrl(res.url)) { + throw new StatusError("Invalid URL", 400); + } return res; } @@ -94,10 +97,6 @@ export async function getResponse(args: { ); } - if (res.redirected && !isValidUrl(res.url)) { - throw new StatusError("Invalid URL", 400); - } - return res; } From 0f51768ff7a15a09686e011086e375cd1b942ded Mon Sep 17 00:00:00 2001 From: naskya <m@naskya.net> Date: Sat, 30 Mar 2024 10:48:32 +0900 Subject: [PATCH 8/8] docs: update changelog --- docs/changelog.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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)