From e5ea882ed74b3cdd0e86f4dda5a8ce724f30d2ef Mon Sep 17 00:00:00 2001 From: dakkar Date: Wed, 20 Dec 2023 12:17:59 +0000 Subject: [PATCH 1/5] authorized fetch #217 the implementation is copied from the other places we already check HTTP signatures, and cross-checked with Firefish's implementation --- .config/example.yml | 2 + chart/files/default.yml | 2 + packages/backend/src/config.ts | 3 + .../src/server/ActivityPubServerService.ts | 129 ++++++++++++++++++ 4 files changed, 136 insertions(+) diff --git a/.config/example.yml b/.config/example.yml index 4b0d07ae85..28fe5b359d 100644 --- a/.config/example.yml +++ b/.config/example.yml @@ -212,6 +212,8 @@ proxyRemoteFiles: true # Sign to ActivityPub GET request (default: true) signToActivityPubGet: true +# check that inbound ActivityPub GET requests are signed ("authorized fetch") +checkActivityPubGetSignature: false # For security reasons, uploading attachments from the intranet is prohibited, # but exceptions can be made from the following settings. Default value is "undefined". diff --git a/chart/files/default.yml b/chart/files/default.yml index 4cc291e80a..9c81964736 100644 --- a/chart/files/default.yml +++ b/chart/files/default.yml @@ -194,6 +194,8 @@ id: "aidx" # Sign to ActivityPub GET request (default: true) signToActivityPubGet: true +# check that inbound ActivityPub GET requests are signed ("authorized fetch") +checkActivityPubGetSignature: false #allowedPrivateNetworks: [ # '127.0.0.1/32' diff --git a/packages/backend/src/config.ts b/packages/backend/src/config.ts index dceeac4691..4a4d7e8311 100644 --- a/packages/backend/src/config.ts +++ b/packages/backend/src/config.ts @@ -88,6 +88,7 @@ type Source = { customMOTD?: string[]; signToActivityPubGet?: boolean; + checkActivityPubGetSignature?: boolean; perChannelMaxNoteCacheCount?: number; perUserNotificationsMaxCount?: number; @@ -146,6 +147,7 @@ export type Config = { proxyRemoteFiles: boolean | undefined; customMOTD: string[] | undefined; signToActivityPubGet: boolean | undefined; + checkActivityPubGetSignature: boolean | undefined; version: string; host: string; @@ -253,6 +255,7 @@ export function loadConfig(): Config { proxyRemoteFiles: config.proxyRemoteFiles, customMOTD: config.customMOTD, signToActivityPubGet: config.signToActivityPubGet, + checkActivityPubGetSignature: config.checkActivityPubGetSignature, mediaProxy: externalMediaProxy ?? internalMediaProxy, externalMediaProxyEnabled: externalMediaProxy !== null && externalMediaProxy !== internalMediaProxy, videoThumbnailGenerator: config.videoThumbnailGenerator ? diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index 68e426b5bc..a0ad4a134f 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -5,6 +5,7 @@ import * as crypto from 'node:crypto'; import { IncomingMessage } from 'node:http'; +import { format as formatURL } from 'node:url'; import { Inject, Injectable } from '@nestjs/common'; import fastifyAccepts from '@fastify/accepts'; import httpSignature from '@peertube/http-signature'; @@ -17,9 +18,13 @@ import type { FollowingsRepository, NotesRepository, EmojisRepository, NoteReact import * as url from '@/misc/prelude/url.js'; import type { Config } from '@/config.js'; import { ApRendererService } from '@/core/activitypub/ApRendererService.js'; +import { ApDbResolverService } from '@/core/activitypub/ApDbResolverService.js'; import { QueueService } from '@/core/QueueService.js'; import type { MiLocalUser, MiRemoteUser, MiUser } from '@/models/User.js'; +import { MetaService } from '@/core/MetaService.js'; import { UserKeypairService } from '@/core/UserKeypairService.js'; +import { InstanceActorService } from '@/core/InstanceActorService.js'; +import type { MiUserPublickey } from '@/models/UserPublickey.js'; import type { MiFollowing } from '@/models/Following.js'; import { countIf } from '@/misc/prelude/array.js'; import type { MiNote } from '@/models/Note.js'; @@ -65,9 +70,12 @@ export class ActivityPubServerService { @Inject(DI.followRequestsRepository) private followRequestsRepository: FollowRequestsRepository, + private metaService: MetaService, private utilityService: UtilityService, private userEntityService: UserEntityService, + private instanceActorService: InstanceActorService, private apRendererService: ApRendererService, + private apDbResolverService: ApDbResolverService, private queueService: QueueService, private userKeypairService: UserKeypairService, private queryService: QueryService, @@ -99,6 +107,101 @@ export class ActivityPubServerService { return this.apRendererService.renderCreate(await this.apRendererService.renderNote(note, false), note); } + @bindThis + private async shouldRefuseGetRequest(request: FastifyRequest, reply: FastifyReply, userId: string | undefined = undefined): Promise { + if (!this.config.checkActivityPubGetSignature) return false; + + /* this code is inspired from the `inbox` function below, and + `queue/processors/InboxProcessorService` + + those pieces of code also check `digest`, and various bits from the + request body, but that only makes sense for requests with a body: + here we're validating GET requests + + this is also inspired by FireFish's `checkFetch` + */ + + /* we always allow requests about our instance actor, because when + a remote instance needs to check our signature on a request we + sent, it will need to fetch information about the user that + signed it (which is our instance actor), and if we try to check + their signature on *that* request, we'll fetch *their* instance + actor... leading to an infinite recursion */ + if (userId) { + const instanceActor = await this.instanceActorService.getInstanceActor(); + + if (userId === instanceActor.id) return false; + } + + let signature; + + try { + signature = httpSignature.parseRequest(request.raw, { 'headers': [] }); + } catch (e) { + // not signed, or malformed signature: refuse + reply.code(401); + return true; + } + + if (signature.params.headers.indexOf('host') === -1 + || request.headers.host !== this.config.host) { + // no destination host, or not us: refuse + reply.code(401); + return true; + } + + const keyId = new URL(signature.keyId); + const keyHost = this.utilityService.toPuny(keyId.hostname); + + const meta = await this.metaService.fetch(); + if (this.utilityService.isBlockedHost(meta.blockedHosts, keyHost)) { + /* blocked instance: refuse (we don't care if the signature is + good, if they even pretend to be from a blocked instance, + they're out) */ + reply.code(401); + return true; + } + + // do we know the signer already? + let authUser: { + user: MiRemoteUser; + key: MiUserPublickey | null; + } | null = await this.apDbResolverService.getAuthUserFromKeyId(signature.keyId); + + if (authUser == null) { + /* keyId is often in the shape `${user.uri}#${keyname}`, try + fetching information about the remote user */ + const candidate = formatURL(keyId, { fragment: false }); + authUser = await this.apDbResolverService.getAuthUserFromApId(candidate); + } + + if (authUser?.key == null) { + // we can't figure out who the signer is, or we can't get their key: refuse + reply.code(401); + return true; + } + + let httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem); + + if (!httpSignatureValidated) { + // maybe they changed their key? refetch it + authUser.key = await this.apDbResolverService.refetchPublicKeyForApId(authUser.user); + + if (authUser.key != null) { + httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem); + } + } + + if (!httpSignatureValidated) { + // bad signature: refuse + reply.code(401); + return true; + } + + // all good, don't refuse + return false; + } + @bindThis private inbox(request: FastifyRequest, reply: FastifyReply) { let signature; @@ -172,6 +275,8 @@ export class ActivityPubServerService { request: FastifyRequest<{ Params: { user: string; }; Querystring: { cursor?: string; page?: string; }; }>, reply: FastifyReply, ) { + if (await this.shouldRefuseGetRequest(request, reply, request.params.user)) return; + const userId = request.params.user; const cursor = request.query.cursor; @@ -264,6 +369,8 @@ export class ActivityPubServerService { request: FastifyRequest<{ Params: { user: string; }; Querystring: { cursor?: string; page?: string; }; }>, reply: FastifyReply, ) { + if (await this.shouldRefuseGetRequest(request, reply, request.params.user)) return; + const userId = request.params.user; const cursor = request.query.cursor; @@ -353,6 +460,8 @@ export class ActivityPubServerService { @bindThis private async featured(request: FastifyRequest<{ Params: { user: string; }; }>, reply: FastifyReply) { + if (await this.shouldRefuseGetRequest(request, reply, request.params.user)) return; + const userId = request.params.user; const user = await this.usersRepository.findOneBy({ @@ -397,6 +506,8 @@ export class ActivityPubServerService { }>, reply: FastifyReply, ) { + if (await this.shouldRefuseGetRequest(request, reply, request.params.user)) return; + const userId = request.params.user; const sinceId = request.query.since_id; @@ -551,6 +662,8 @@ export class ActivityPubServerService { // note fastify.get<{ Params: { note: string; } }>('/notes/:note', { constraints: { apOrHtml: 'ap' } }, async (request, reply) => { + if (await this.shouldRefuseGetRequest(request, reply)) return; + vary(reply.raw, 'Accept'); const note = await this.notesRepository.findOneBy({ @@ -581,6 +694,8 @@ export class ActivityPubServerService { // note activity fastify.get<{ Params: { note: string; } }>('/notes/:note/activity', async (request, reply) => { + if (await this.shouldRefuseGetRequest(request, reply)) return; + vary(reply.raw, 'Accept'); const note = await this.notesRepository.findOneBy({ @@ -623,6 +738,8 @@ export class ActivityPubServerService { // publickey fastify.get<{ Params: { user: string; } }>('/users/:user/publickey', async (request, reply) => { + if (await this.shouldRefuseGetRequest(request, reply, request.params.user)) return; + const userId = request.params.user; const user = await this.usersRepository.findOneBy({ @@ -648,6 +765,8 @@ export class ActivityPubServerService { }); fastify.get<{ Params: { user: string; } }>('/users/:user', { constraints: { apOrHtml: 'ap' } }, async (request, reply) => { + if (await this.shouldRefuseGetRequest(request, reply, request.params.user)) return; + const userId = request.params.user; const user = await this.usersRepository.findOneBy({ @@ -660,6 +779,8 @@ export class ActivityPubServerService { }); fastify.get<{ Params: { user: string; } }>('/@:user', { constraints: { apOrHtml: 'ap' } }, async (request, reply) => { + if (await this.shouldRefuseGetRequest(request, reply, request.params.user)) return; + const user = await this.usersRepository.findOneBy({ usernameLower: request.params.user.toLowerCase(), host: IsNull(), @@ -672,6 +793,8 @@ export class ActivityPubServerService { // emoji fastify.get<{ Params: { emoji: string; } }>('/emojis/:emoji', async (request, reply) => { + if (await this.shouldRefuseGetRequest(request, reply)) return; + const emoji = await this.emojisRepository.findOneBy({ host: IsNull(), name: request.params.emoji, @@ -689,6 +812,8 @@ export class ActivityPubServerService { // like fastify.get<{ Params: { like: string; } }>('/likes/:like', async (request, reply) => { + if (await this.shouldRefuseGetRequest(request, reply)) return; + const reaction = await this.noteReactionsRepository.findOneBy({ id: request.params.like }); if (reaction == null) { @@ -710,6 +835,8 @@ export class ActivityPubServerService { // follow fastify.get<{ Params: { follower: string; followee: string; } }>('/follows/:follower/:followee', async (request, reply) => { + if (await this.shouldRefuseGetRequest(request, reply)) return; + // This may be used before the follow is completed, so we do not // check if the following exists. @@ -736,6 +863,8 @@ export class ActivityPubServerService { // follow fastify.get<{ Params: { followRequestId: string ; } }>('/follows/:followRequestId', async (request, reply) => { + if (await this.shouldRefuseGetRequest(request, reply)) return; + // This may be used before the follow is completed, so we do not // check if the following exists and only check if the follow request exists. From 1984416e3efeb987c94cfefb44d800bc0521e415 Mon Sep 17 00:00:00 2001 From: dakkar Date: Thu, 21 Dec 2023 18:17:19 +0000 Subject: [PATCH 2/5] authorized fetch: let /@instance.actor through this is probably never actually used, but it still looks like a good idea (also, FireFish does it) thanks @ShittyKoper for noticing! --- packages/backend/src/server/ActivityPubServerService.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index a0ad4a134f..967a69cb77 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -131,6 +131,7 @@ export class ActivityPubServerService { const instanceActor = await this.instanceActorService.getInstanceActor(); if (userId === instanceActor.id) return false; + if (userId === instanceActor.username) return false; } let signature; From 477cda0b635ff293760f91eceb7246b8f9d456e5 Mon Sep 17 00:00:00 2001 From: dakkar Date: Sat, 23 Dec 2023 15:26:21 +0000 Subject: [PATCH 3/5] authorized fetch: log when things go wrong --- .../src/server/ActivityPubServerService.ts | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index 967a69cb77..a87796e267 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -36,12 +36,17 @@ import { IActivity } from '@/core/activitypub/type.js'; import { isPureRenote } from '@/misc/is-pure-renote.js'; import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOptions, FastifyBodyParser } from 'fastify'; import type { FindOptionsWhere } from 'typeorm'; +import type Logger from '@/logger.js'; +import { LoggerService } from '@/core/LoggerService.js'; const ACTIVITY_JSON = 'application/activity+json; charset=utf-8'; const LD_JSON = 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"; charset=utf-8'; @Injectable() export class ActivityPubServerService { + private logger: Logger; + private authlogger: Logger; + constructor( @Inject(DI.config) private config: Config, @@ -79,8 +84,11 @@ export class ActivityPubServerService { private queueService: QueueService, private userKeypairService: UserKeypairService, private queryService: QueryService, + private loggerService: LoggerService, ) { //this.createServer = this.createServer.bind(this); + this.logger = this.loggerService.getLogger('apserv', 'pink'); + this.authlogger = this.logger.createSubLogger('sigcheck'); } @bindThis @@ -130,8 +138,10 @@ export class ActivityPubServerService { if (userId) { const instanceActor = await this.instanceActorService.getInstanceActor(); - if (userId === instanceActor.id) return false; - if (userId === instanceActor.username) return false; + if (userId === instanceActor.id || userId === instanceActor.username) { + this.authlogger.debug(`${request.id} ${request.url} request to instance.actor, letting through`); + return false; + } } let signature; @@ -140,6 +150,7 @@ export class ActivityPubServerService { signature = httpSignature.parseRequest(request.raw, { 'headers': [] }); } catch (e) { // not signed, or malformed signature: refuse + this.authlogger.warn(`${request.id} ${request.url} not signed, or malformed signature: refuse`); reply.code(401); return true; } @@ -147,6 +158,7 @@ export class ActivityPubServerService { if (signature.params.headers.indexOf('host') === -1 || request.headers.host !== this.config.host) { // no destination host, or not us: refuse + this.authlogger.warn(`${request.id} ${request.url} no destination host, or not us: refuse`); reply.code(401); return true; } @@ -159,6 +171,7 @@ export class ActivityPubServerService { /* blocked instance: refuse (we don't care if the signature is good, if they even pretend to be from a blocked instance, they're out) */ + this.authlogger.warn(`${request.id} ${request.url} instance ${keyHost} is blocked: refuse`); reply.code(401); return true; } @@ -173,11 +186,13 @@ export class ActivityPubServerService { /* keyId is often in the shape `${user.uri}#${keyname}`, try fetching information about the remote user */ const candidate = formatURL(keyId, { fragment: false }); + this.authlogger.info(`${request.id} ${request.url} we don't know the user for keyId ${keyID}, trying to fetch via ${candidate}`); authUser = await this.apDbResolverService.getAuthUserFromApId(candidate); } if (authUser?.key == null) { // we can't figure out who the signer is, or we can't get their key: refuse + this.authlogger.warn(`${request.id} ${request.url} we can't figure out who the signer is, or we can't get their key: refuse`); reply.code(401); return true; } @@ -185,16 +200,20 @@ export class ActivityPubServerService { let httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem); if (!httpSignatureValidated) { + this.authlogger.info(`${request.id} ${request.url} failed to validate signature, re-fetching the key for ${authUser.user}`); // maybe they changed their key? refetch it authUser.key = await this.apDbResolverService.refetchPublicKeyForApId(authUser.user); if (authUser.key != null) { httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem); + } else { + this.authlogger.warn(`${request.id} ${request.url} failed to re-fetch key for ${authUser.user}`); } } if (!httpSignatureValidated) { // bad signature: refuse + this.authlogger.info(`${request.id} ${request.url} failed to validate signature: refuse`); reply.code(401); return true; } From e6c02909c751c70b4f825616a4143f0e956dee85 Mon Sep 17 00:00:00 2001 From: dakkar Date: Sat, 23 Dec 2023 20:11:53 +0000 Subject: [PATCH 4/5] fix typo thanks @Marie --- packages/backend/src/server/ActivityPubServerService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index a87796e267..26580e21ed 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -186,7 +186,7 @@ export class ActivityPubServerService { /* keyId is often in the shape `${user.uri}#${keyname}`, try fetching information about the remote user */ const candidate = formatURL(keyId, { fragment: false }); - this.authlogger.info(`${request.id} ${request.url} we don't know the user for keyId ${keyID}, trying to fetch via ${candidate}`); + this.authlogger.info(`${request.id} ${request.url} we don't know the user for keyId ${keyId}, trying to fetch via ${candidate}`); authUser = await this.apDbResolverService.getAuthUserFromApId(candidate); } From a3dd61dec4ba26a90a8cae6ade7a615f6d459da3 Mon Sep 17 00:00:00 2001 From: dakkar Date: Sat, 23 Dec 2023 21:27:48 +0000 Subject: [PATCH 5/5] fix logging --- packages/backend/src/server/ActivityPubServerService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index 26580e21ed..68de738238 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -200,7 +200,7 @@ export class ActivityPubServerService { let httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem); if (!httpSignatureValidated) { - this.authlogger.info(`${request.id} ${request.url} failed to validate signature, re-fetching the key for ${authUser.user}`); + this.authlogger.info(`${request.id} ${request.url} failed to validate signature, re-fetching the key for ${authUser.user.uri}`); // maybe they changed their key? refetch it authUser.key = await this.apDbResolverService.refetchPublicKeyForApId(authUser.user);