From 074de82bf72161528911283de7fd68dde119d251 Mon Sep 17 00:00:00 2001
From: dakkar <dakkar@thenautilus.net>
Date: Sat, 30 Mar 2024 11:05:58 +0000
Subject: [PATCH] some validation fixes
---
.../backend/src/core/HttpRequestService.ts | 8 +++++-
packages/backend/src/core/UtilityService.ts | 9 ++++++-
.../src/core/activitypub/ApRequestService.ts | 9 ++++++-
.../src/core/activitypub/ApResolverService.ts | 8 ++++++
.../activitypub/misc/check-against-url.ts | 19 +++++++++++++
.../activitypub/models/ApPersonService.ts | 27 ++++++++++++-------
.../src/server/api/endpoints/ap/show.ts | 6 ++++-
7 files changed, 72 insertions(+), 14 deletions(-)
create mode 100644 packages/backend/src/core/activitypub/misc/check-against-url.ts
diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts
index 7f3cac7c58..bea5dee6ab 100644
--- a/packages/backend/src/core/HttpRequestService.ts
+++ b/packages/backend/src/core/HttpRequestService.ts
@@ -15,6 +15,7 @@ import type { Config } from '@/config.js';
import { StatusError } from '@/misc/status-error.js';
import { bindThis } from '@/decorators.js';
import { validateContentTypeSetAsActivityPub } from '@/core/activitypub/misc/validator.js';
+import { assertActivityMatchesUrls } from '@/core/activitypub/misc/check-against-url.js';
import type { IObject } from '@/core/activitypub/type.js';
import type { Response } from 'node-fetch';
import type { URL } from 'node:url';
@@ -125,7 +126,12 @@ export class HttpRequestService {
validators: [validateContentTypeSetAsActivityPub],
});
- return await res.json() as IObject;
+ const finalUrl = res.url; // redirects may have been involved
+ const activity = await res.json() as IObject;
+
+ assertActivityMatchesUrls(activity, [url, finalUrl]);
+
+ return activity;
}
@bindThis
diff --git a/packages/backend/src/core/UtilityService.ts b/packages/backend/src/core/UtilityService.ts
index 652e8f7449..21c4af3ca5 100644
--- a/packages/backend/src/core/UtilityService.ts
+++ b/packages/backend/src/core/UtilityService.ts
@@ -86,7 +86,7 @@ export class UtilityService {
@bindThis
public extractDbHost(uri: string): string {
const url = new URL(uri);
- return this.toPuny(url.hostname);
+ return this.toPuny(url.host);
}
@bindThis
@@ -99,4 +99,11 @@ export class UtilityService {
if (host == null) return null;
return toASCII(host.toLowerCase());
}
+
+ @bindThis
+ public punyHost(url: string): string {
+ const urlObj = new URL(url);
+ const host = `${this.toPuny(urlObj.hostname)}${urlObj.port.length > 0 ? ':' + urlObj.port : ''}`;
+ return host;
+ }
}
diff --git a/packages/backend/src/core/activitypub/ApRequestService.ts b/packages/backend/src/core/activitypub/ApRequestService.ts
index 93ac8ce9a7..fd8d65445a 100644
--- a/packages/backend/src/core/activitypub/ApRequestService.ts
+++ b/packages/backend/src/core/activitypub/ApRequestService.ts
@@ -14,7 +14,9 @@ import { HttpRequestService } from '@/core/HttpRequestService.js';
import { LoggerService } from '@/core/LoggerService.js';
import { bindThis } from '@/decorators.js';
import type Logger from '@/logger.js';
+import type { IObject } from './type.js';
import { validateContentTypeSetAsActivityPub } from '@/core/activitypub/misc/validator.js';
+import { assertActivityMatchesUrls } from '@/core/activitypub/misc/check-against-url.js';
type Request = {
url: string;
@@ -201,6 +203,11 @@ export class ApRequestService {
validators: [validateContentTypeSetAsActivityPub],
});
- return await res.json();
+ const finalUrl = res.url; // redirects may have been involved
+ const activity = await res.json() as IObject;
+
+ assertActivityMatchesUrls(activity, [url, finalUrl]);
+
+ return activity;
}
}
diff --git a/packages/backend/src/core/activitypub/ApResolverService.ts b/packages/backend/src/core/activitypub/ApResolverService.ts
index bb3c40f093..b047a6c59b 100644
--- a/packages/backend/src/core/activitypub/ApResolverService.ts
+++ b/packages/backend/src/core/activitypub/ApResolverService.ts
@@ -115,6 +115,14 @@ export class Resolver {
throw new Error('invalid response');
}
+ // HttpRequestService / ApRequestService have already checked that
+ // `object.id` or `object.url` matches the URL used to fetch the
+ // object after redirects; here we double-check that no redirects
+ // bounced between hosts
+ if (object.id && (this.utilityService.punyHost(object.id) !== this.utilityService.punyHost(value))) {
+ throw new Error(`invalid AP object ${value}: id ${object.id} has different host`);
+ }
+
return object;
}
diff --git a/packages/backend/src/core/activitypub/misc/check-against-url.ts b/packages/backend/src/core/activitypub/misc/check-against-url.ts
new file mode 100644
index 0000000000..78ba891a2e
--- /dev/null
+++ b/packages/backend/src/core/activitypub/misc/check-against-url.ts
@@ -0,0 +1,19 @@
+/*
+ * SPDX-FileCopyrightText: dakkar and sharkey-project
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+import type { IObject } from '../type.js';
+
+export function assertActivityMatchesUrls(activity: IObject, urls: string[]) {
+ const idOk = activity.id !== undefined && urls.includes(activity.id);
+
+ // technically `activity.url` could be an `ApObject = IObject |
+ // string | (IObject | string)[]`, but if it's a complicated thing
+ // and the `activity.id` doesn't match, I think we're fine
+ // rejecting the activity
+ const urlOk = typeof(activity.url) === 'string' && urls.includes(activity.url);
+
+ if (!idOk && !urlOk) {
+ throw new Error(`bad Activity: neither id(${activity?.id}) nor url(${activity?.url}) match location(${urls})`);
+ }
+}
diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts
index c489d38d90..224b8e8c3f 100644
--- a/packages/backend/src/core/activitypub/models/ApPersonService.ts
+++ b/packages/backend/src/core/activitypub/models/ApPersonService.ts
@@ -127,12 +127,6 @@ export class ApPersonService implements OnModuleInit {
this.logger = this.apLoggerService.logger;
}
- private punyHost(url: string): string {
- const urlObj = new URL(url);
- const host = `${this.utilityService.toPuny(urlObj.hostname)}${urlObj.port.length > 0 ? ':' + urlObj.port : ''}`;
- return host;
- }
-
/**
* Validate and convert to actor object
* @param x Fetched object
@@ -140,7 +134,7 @@ export class ApPersonService implements OnModuleInit {
*/
@bindThis
private validateActor(x: IObject, uri: string): IActor {
- const expectHost = this.punyHost(uri);
+ const expectHost = this.utilityService.punyHost(uri);
if (!isActor(x)) {
throw new Error(`invalid Actor type '${x.type}'`);
@@ -154,6 +148,19 @@ export class ApPersonService implements OnModuleInit {
throw new Error('invalid Actor: wrong inbox');
}
+ if (this.utilityService.punyHost(x.inbox) !== expectHost) {
+ throw new Error('invalid Actor: inbox has different host');
+ }
+
+ for (const collection of ['outbox', 'followers', 'following'] as (keyof IActor)[]) {
+ const collectionUri = (x as IActor)[collection];
+ if (typeof collectionUri === 'string' && collectionUri.length > 0) {
+ if (this.utilityService.punyHost(collectionUri) !== expectHost) {
+ throw new Error(`invalid Actor: ${collection} has different host`);
+ }
+ }
+ }
+
if (!(typeof x.preferredUsername === 'string' && x.preferredUsername.length > 0 && x.preferredUsername.length <= 128 && /^\w([\w-.]*\w)?$/.test(x.preferredUsername))) {
throw new Error('invalid Actor: wrong username');
}
@@ -177,7 +184,7 @@ export class ApPersonService implements OnModuleInit {
x.summary = truncate(x.summary, summaryLength);
}
- const idHost = this.punyHost(x.id);
+ const idHost = this.utilityService.punyHost(x.id);
if (idHost !== expectHost) {
throw new Error('invalid Actor: id has different host');
}
@@ -187,7 +194,7 @@ export class ApPersonService implements OnModuleInit {
throw new Error('invalid Actor: publicKey.id is not a string');
}
- const publicKeyIdHost = this.punyHost(x.publicKey.id);
+ const publicKeyIdHost = this.utilityService.punyHost(x.publicKey.id);
if (publicKeyIdHost !== expectHost) {
throw new Error('invalid Actor: publicKey.id has different host');
}
@@ -286,7 +293,7 @@ export class ApPersonService implements OnModuleInit {
this.logger.info(`Creating the Person: ${person.id}`);
- const host = this.punyHost(object.id);
+ const host = this.utilityService.punyHost(object.id);
const fields = this.analyzeAttachments(person.attachment ?? []);
diff --git a/packages/backend/src/server/api/endpoints/ap/show.ts b/packages/backend/src/server/api/endpoints/ap/show.ts
index 364a4826e3..ca6789a464 100644
--- a/packages/backend/src/server/api/endpoints/ap/show.ts
+++ b/packages/backend/src/server/api/endpoints/ap/show.ts
@@ -113,8 +113,9 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
@bindThis
private async fetchAny(uri: string, me: MiLocalUser | null | undefined): Promise<SchemaType<typeof meta['res']> | null> {
// ブロックしてたら中断
+ const host = this.utilityService.extractDbHost(uri);
const fetchedMeta = await this.metaService.fetch();
- if (this.utilityService.isBlockedHost(fetchedMeta.blockedHosts, this.utilityService.extractDbHost(uri))) return null;
+ if (this.utilityService.isBlockedHost(fetchedMeta.blockedHosts, host)) return null;
let local = await this.mergePack(me, ...await Promise.all([
this.apDbResolverService.getUserFromApId(uri),
@@ -122,6 +123,9 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
]));
if (local != null) return local;
+ // local object, not found in db? fail
+ if (this.utilityService.isSelfHost(host)) return null;
+
// リモートから一旦オブジェクトフェッチ
const resolver = this.apResolverService.createResolver();
const object = await resolver.resolve(uri) as any;