fixup! fix(backend): Fix an issue where the origin of ActivityPub lookup response was not validated correctly.
Signed-off-by: eternal-flame-AD <yume@yumechi.jp> docs & one edge case Signed-off-by: eternal-flame-AD <yume@yumechi.jp> apply suggestions Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
This commit is contained in:
parent
c31466e463
commit
3961b4299d
9 changed files with 155 additions and 65 deletions
locales
packages/backend
src
core
server
test/unit
8
locales/index.d.ts
vendored
8
locales/index.d.ts
vendored
|
|
@ -10884,13 +10884,7 @@ export interface Locale extends ILocale {
|
|||
*/
|
||||
"title": string;
|
||||
/**
|
||||
* このサーバーと通信することはできましたが、得られたデータが不正なものでした。第三者サーバーのURIを使った場合はオリジナルのURIを使用して照会し直してください。
|
||||
*/
|
||||
"description": string;
|
||||
};
|
||||
"_responseInvalidIdHostNotMatch": {
|
||||
/**
|
||||
* 入力されたURIのドメインと最終的に得られたURIのドメインとが異なります。第三者のサーバーを介してリモートのコンテンツを照会している場合は、発信元のサーバーで取得できるURIを使用して照会し直してください。
|
||||
* このサーバーと通信することはできましたが、得られたデータが不正なものでした。第三者のサーバーを介してリモートのコンテンツを照会している場合は、発信元のサーバーで取得できるURIを使用して照会し直してください。
|
||||
*/
|
||||
"description": string;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -2908,9 +2908,7 @@ _remoteLookupErrors:
|
|||
description: "このサーバーとの通信に失敗しました。相手サーバーがダウンしている可能性があります。また、不正なURIや存在しないURIを入力していないか確認してください。"
|
||||
_responseInvalid:
|
||||
title: "レスポンスが不正です"
|
||||
description: "このサーバーと通信することはできましたが、得られたデータが不正なものでした。第三者サーバーのURIを使った場合はオリジナルのURIを使用して照会し直してください。"
|
||||
_responseInvalidIdHostNotMatch:
|
||||
description: "入力されたURIのドメインと最終的に得られたURIのドメインとが異なります。第三者のサーバーを介してリモートのコンテンツを照会している場合は、発信元のサーバーで取得できるURIを使用して照会し直してください。"
|
||||
description: "このサーバーと通信することはできましたが、得られたデータが不正なものでした。第三者のサーバーを介してリモートのコンテンツを照会している場合は、発信元のサーバーで取得できるURIを使用して照会し直してください。"
|
||||
_noSuchObject:
|
||||
title: "見つかりません"
|
||||
description: "要求されたリソースは見つかりませんでした。URIをもう一度お確かめください。"
|
||||
|
|
|
|||
|
|
@ -16,7 +16,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, FetchSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
|
||||
import { assertActivityMatchesUrls, FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
|
||||
import type { IObject } from '@/core/activitypub/type.js';
|
||||
import type { Response } from 'node-fetch';
|
||||
import { URL } from 'node:url';
|
||||
|
|
@ -182,7 +182,7 @@ export class HttpRequestService {
|
|||
}
|
||||
|
||||
@bindThis
|
||||
public async getActivityJson(url: string, isLocalAddressAllowed = false, forbid?: FetchSoftFailMask): Promise<IObject> {
|
||||
public async getActivityJson(url: string, isLocalAddressAllowed = false, allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict): Promise<IObject> {
|
||||
const res = await this.send(url, {
|
||||
method: 'GET',
|
||||
headers: {
|
||||
|
|
@ -199,7 +199,7 @@ export class HttpRequestService {
|
|||
const finalUrl = res.url; // redirects may have been involved
|
||||
const activity = await res.json() as IObject;
|
||||
|
||||
assertActivityMatchesUrls(url, activity, [finalUrl], forbid ?? FetchSoftFailMask.NonCanonicalId | FetchSoftFailMask.Downgraded);
|
||||
assertActivityMatchesUrls(url, activity, [finalUrl], allowSoftfail);
|
||||
|
||||
return activity;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ import { LoggerService } from '@/core/LoggerService.js';
|
|||
import { bindThis } from '@/decorators.js';
|
||||
import type Logger from '@/logger.js';
|
||||
import { validateContentTypeSetAsActivityPub } from '@/core/activitypub/misc/validator.js';
|
||||
import { assertActivityMatchesUrls, FetchSoftFailMask as FetchSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
|
||||
import { assertActivityMatchesUrls, FetchAllowSoftFailMask as FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
|
||||
import type { IObject } from './type.js';
|
||||
|
||||
type Request = {
|
||||
|
|
@ -185,7 +185,7 @@ export class ApRequestService {
|
|||
* @param url URL to fetch
|
||||
*/
|
||||
@bindThis
|
||||
public async signedGet(url: string, user: { id: MiUser['id'] }, forbid: FetchSoftFailMask, followAlternate?: boolean): Promise<unknown> {
|
||||
public async signedGet(url: string, user: { id: MiUser['id'] }, allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict, followAlternate?: boolean): Promise<unknown> {
|
||||
const _followAlternate = followAlternate ?? true;
|
||||
const keypair = await this.userKeypairService.getUserKeypair(user.id);
|
||||
|
||||
|
|
@ -243,7 +243,7 @@ export class ApRequestService {
|
|||
if (alternate) {
|
||||
const href = alternate.getAttribute('href');
|
||||
if (href && this.utilityService.punyHost(url) === this.utilityService.punyHost(href)) {
|
||||
return await this.signedGet(href, user, forbid, false);
|
||||
return await this.signedGet(href, user, allowSoftfail, false);
|
||||
}
|
||||
}
|
||||
} catch (e) {
|
||||
|
|
@ -258,7 +258,7 @@ export class ApRequestService {
|
|||
const finalUrl = res.url; // redirects may have been involved
|
||||
const activity = await res.json() as IObject;
|
||||
|
||||
assertActivityMatchesUrls(url, activity, [finalUrl], forbid);
|
||||
assertActivityMatchesUrls(url, activity, [finalUrl], allowSoftfail);
|
||||
|
||||
return activity;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -75,15 +75,13 @@ export class Resolver {
|
|||
}
|
||||
|
||||
@bindThis
|
||||
private async resolveNotNormalized(value: string | IObject, forbid?: FetchAllowSoftFailMask): Promise<IUnsanitizedObject> {
|
||||
// ActivityPub Server-to-Server communication should always be canonical object IDs.
|
||||
//
|
||||
// Exception is user-initiated lookup
|
||||
let _forbid = forbid ?? FetchAllowSoftFailMask.NonCanonicalId | FetchAllowSoftFailMask.MisalignedOrigin | FetchAllowSoftFailMask.CrossOrigin;
|
||||
private async resolveNotNormalized(value: string | IObject, allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict): Promise<IUnsanitizedObject> {
|
||||
if (typeof value !== 'string') {
|
||||
return value;
|
||||
}
|
||||
|
||||
yumeAssertAcceptableURL(value);
|
||||
|
||||
if (value.includes('#')) {
|
||||
// URLs with fragment parts cannot be resolved correctly because
|
||||
// the fragment part does not get transmitted over HTTP(S).
|
||||
|
|
@ -115,8 +113,8 @@ export class Resolver {
|
|||
}
|
||||
|
||||
const object = (this.user
|
||||
? await this.apRequestService.signedGet(value, this.user, _forbid) as IUnsanitizedObject
|
||||
: await this.httpRequestService.getActivityJson(value, undefined, _forbid)) as IUnsanitizedObject;
|
||||
? await this.apRequestService.signedGet(value, this.user, allowSoftfail) as IUnsanitizedObject
|
||||
: await this.httpRequestService.getActivityJson(value, undefined, allowSoftfail)) as IUnsanitizedObject;
|
||||
|
||||
if (
|
||||
Array.isArray(object['@context']) ?
|
||||
|
|
@ -126,37 +124,20 @@ export class Resolver {
|
|||
throw new IdentifiableError('72180409-793c-4973-868e-5a118eb5519b', '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 == null) {
|
||||
throw new IdentifiableError('ad2dc287-75c1-44c4-839d-3d2e64576675', 'invalid AP object: missing id');
|
||||
}
|
||||
|
||||
if (this.utilityService.punyHost(object.id) !== this.utilityService.punyHost(value)) {
|
||||
throw new IdentifiableError('fd93c2fa-69a8-440f-880b-bf178e0ec877', `invalid AP object ${value}: id ${object.id} has different host`);
|
||||
}
|
||||
|
||||
// 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 == null) {
|
||||
throw new Error('invalid AP object: missing id');
|
||||
}
|
||||
|
||||
yumeAssertAcceptableURL(object.id);
|
||||
yumeAssertAcceptableURL(value);
|
||||
|
||||
return object;
|
||||
}
|
||||
|
||||
@bindThis
|
||||
public async resolve(value: string | IObject): Promise<IObject> {
|
||||
const object = await this.resolveNotNormalized(value);
|
||||
public async resolve(value: string | IObject, allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict): Promise<IObject> {
|
||||
try {
|
||||
const object = await this.resolveNotNormalized(value, allowSoftfail);
|
||||
|
||||
return yumeNormalizeObject(object);
|
||||
return yumeNormalizeObject(object);
|
||||
} catch (e) {
|
||||
this.logger.error(`Failed to resolve ${value}: ${e}`);
|
||||
throw e;
|
||||
}
|
||||
}
|
||||
|
||||
@bindThis
|
||||
|
|
|
|||
|
|
@ -323,7 +323,6 @@ export class ServerService implements OnApplicationShutdown {
|
|||
// if the requester looks like to be performing an ActivityPub object lookup, reject all external redirects
|
||||
//
|
||||
// this will break lookup that involve copying a URL from a third-party server, like trying to lookup http://charlie.example.com/@alice@alice.com
|
||||
// but I think we should not allow this regardless, open redirect is not a good pattern especially in machine-to-machine communication.
|
||||
//
|
||||
// this is not required by standard but protect us from peers that did not validate final URL.
|
||||
if (this.config.disallowExternalApRedirect) {
|
||||
|
|
@ -341,7 +340,7 @@ export class ServerService implements OnApplicationShutdown {
|
|||
}
|
||||
|
||||
const effectiveLocation = process.env.NODE_ENV === 'production' ? location : location.replace(/^http:\/\//, 'https://');
|
||||
if (effectiveLocation.startsWith(`https://${this.config.host}`)) {
|
||||
if (effectiveLocation.startsWith(`https://${this.config.host}/`)) {
|
||||
done();
|
||||
return;
|
||||
}
|
||||
|
|
@ -349,11 +348,11 @@ export class ServerService implements OnApplicationShutdown {
|
|||
reply.status(406);
|
||||
reply.removeHeader('location');
|
||||
reply.header('content-type', 'text/plain; charset=utf-8');
|
||||
reply.header('link', `<${encodeURI(effectiveLocation)}>; rel="canonical"`);
|
||||
reply.header('link', `<${encodeURI(location)}>; rel="canonical"`);
|
||||
done(null, [
|
||||
"Refusing to relay remote ActivityPub object lookup.",
|
||||
"",
|
||||
`Please remove 'application/activity+json' and 'application/ld+json' from the Accept header or fetch using the authoritative URL at ${effectiveLocation}.`,
|
||||
`Please remove 'application/activity+json' and 'application/ld+json' from the Accept header or fetch using the authoritative URL at ${location}.`,
|
||||
].join('\n'));
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@ import { Injectable } from '@nestjs/common';
|
|||
import ms from 'ms';
|
||||
import { Endpoint } from '@/server/api/endpoint-base.js';
|
||||
import { ApResolverService } from '@/core/activitypub/ApResolverService.js';
|
||||
import { FetchSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
|
||||
import { FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
|
||||
|
||||
export const meta = {
|
||||
tags: ['federation'],
|
||||
|
|
@ -45,7 +45,7 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
|
|||
) {
|
||||
super(meta, paramDef, async (ps, me) => {
|
||||
const resolver = this.apResolverService.createResolver();
|
||||
const object = await resolver.resolve(ps.uri, FetchSoftFailMask.Downgraded);
|
||||
const object = await resolver.resolve(ps.uri);
|
||||
return object;
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -20,7 +20,7 @@ import { UtilityService } from '@/core/UtilityService.js';
|
|||
import { bindThis } from '@/decorators.js';
|
||||
import { ApiError } from '../../error.js';
|
||||
import { IdentifiableError } from '@/misc/identifiable-error.js';
|
||||
import { FetchSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
|
||||
import { FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
|
||||
|
||||
export const meta = {
|
||||
tags: ['federation'],
|
||||
|
|
@ -54,11 +54,6 @@ export const meta = {
|
|||
code: 'RESPONSE_INVALID',
|
||||
id: '70193c39-54f3-4813-82f0-70a680f7495b',
|
||||
},
|
||||
responseInvalidIdHostNotMatch: {
|
||||
message: 'Requested URI and response URI host does not match.',
|
||||
code: 'RESPONSE_INVALID_ID_HOST_NOT_MATCH',
|
||||
id: 'a2c9c61a-cb72-43ab-a964-3ca5fddb410a',
|
||||
},
|
||||
noSuchObject: {
|
||||
message: 'No such object.',
|
||||
code: 'NO_SUCH_OBJECT',
|
||||
|
|
@ -154,7 +149,8 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
|
|||
|
||||
// リモートから一旦オブジェクトフェッチ
|
||||
const resolver = this.apResolverService.createResolver();
|
||||
const object = await resolver.resolve(uri, FetchSoftFailMask.Downgraded).catch((err) => {
|
||||
// allow ap/show exclusively to lookup URLs that might be misaligned (http://example.com/@user -> https://activitypub.example.com/@user)
|
||||
const object = await resolver.resolve(uri, FetchAllowSoftFailMask.MisalignedOrigin).catch((err) => {
|
||||
if (err instanceof IdentifiableError) {
|
||||
switch (err.id) {
|
||||
// resolve
|
||||
|
|
@ -166,10 +162,7 @@ export default class extends Endpoint<typeof meta, typeof paramDef> { // eslint-
|
|||
case '09d79f9e-64f1-4316-9cfa-e75c4d091574':
|
||||
throw new ApiError(meta.errors.federationNotAllowed);
|
||||
case '72180409-793c-4973-868e-5a118eb5519b':
|
||||
case 'ad2dc287-75c1-44c4-839d-3d2e64576675':
|
||||
throw new ApiError(meta.errors.responseInvalid);
|
||||
case 'fd93c2fa-69a8-440f-880b-bf178e0ec877':
|
||||
throw new ApiError(meta.errors.responseInvalidIdHostNotMatch);
|
||||
|
||||
// resolveLocal
|
||||
case '02b40cd0-fa92-4b0c-acc9-fb2ada952ab8':
|
||||
|
|
|
|||
|
|
@ -8,6 +8,8 @@ import httpSignature from '@peertube/http-signature';
|
|||
|
||||
import { genRsaKeyPair } from '@/misc/gen-key-pair.js';
|
||||
import { ApRequestCreator } from '@/core/activitypub/ApRequestService.js';
|
||||
import { assertActivityMatchesUrls, FetchAllowSoftFailMask } from '@/core/activitypub/misc/check-against-url.js';
|
||||
import { IObject } from '@/core/activitypub/type.js';
|
||||
|
||||
export const buildParsedSignature = (signingString: string, signature: string, algorithm: string) => {
|
||||
return {
|
||||
|
|
@ -24,6 +26,10 @@ export const buildParsedSignature = (signingString: string, signature: string, a
|
|||
};
|
||||
};
|
||||
|
||||
function cartesianProduct<T, U>(a: T[], b: U[]): [T, U][] {
|
||||
return a.flatMap(a => b.map(b => [a, b] as [T, U]));
|
||||
}
|
||||
|
||||
describe('ap-request', () => {
|
||||
test('createSignedPost with verify', async () => {
|
||||
const keypair = await genRsaKeyPair();
|
||||
|
|
@ -58,4 +64,123 @@ describe('ap-request', () => {
|
|||
const result = httpSignature.verifySignature(parsed, keypair.publicKey);
|
||||
assert.deepStrictEqual(result, true);
|
||||
});
|
||||
|
||||
test('rejects non matching domain', () => {
|
||||
assert.doesNotThrow(() => assertActivityMatchesUrls(
|
||||
'https://alice.example.com/abc',
|
||||
{ id: 'https://alice.example.com/abc' } as IObject,
|
||||
[
|
||||
'https://alice.example.com/abc',
|
||||
],
|
||||
FetchAllowSoftFailMask.Strict,
|
||||
), 'validation should pass base case');
|
||||
assert.throws(() => assertActivityMatchesUrls(
|
||||
'https://alice.example.com/abc',
|
||||
{ id: 'https://bob.example.com/abc' } as IObject,
|
||||
[
|
||||
'https://alice.example.com/abc',
|
||||
],
|
||||
FetchAllowSoftFailMask.Any,
|
||||
), 'validation should fail no matter what if the response URL is inconsistent with the object ID');
|
||||
|
||||
// fix issues like threads
|
||||
// https://github.com/misskey-dev/misskey/issues/15039
|
||||
const withOrWithoutWWW = [
|
||||
'https://alice.example.com/abc',
|
||||
'https://www.alice.example.com/abc',
|
||||
];
|
||||
|
||||
cartesianProduct(
|
||||
cartesianProduct(
|
||||
withOrWithoutWWW,
|
||||
withOrWithoutWWW,
|
||||
),
|
||||
withOrWithoutWWW,
|
||||
).forEach(([[a, b], c]) => {
|
||||
assert.doesNotThrow(() => assertActivityMatchesUrls(
|
||||
a,
|
||||
{ id: b } as IObject,
|
||||
[
|
||||
c,
|
||||
],
|
||||
FetchAllowSoftFailMask.Strict,
|
||||
), 'validation should pass with or without www. subdomain');
|
||||
});
|
||||
});
|
||||
|
||||
test('cross origin lookup', () => {
|
||||
assert.doesNotThrow(() => assertActivityMatchesUrls(
|
||||
'https://alice.example.com/abc',
|
||||
{ id: 'https://bob.example.com/abc' } as IObject,
|
||||
[
|
||||
'https://bob.example.com/abc',
|
||||
],
|
||||
FetchAllowSoftFailMask.CrossOrigin | FetchAllowSoftFailMask.NonCanonicalId,
|
||||
), 'validation should pass if the response is otherwise consistent and cross-origin is allowed');
|
||||
assert.throws(() => assertActivityMatchesUrls(
|
||||
'https://alice.example.com/abc',
|
||||
{ id: 'https://bob.example.com/abc' } as IObject,
|
||||
[
|
||||
'https://bob.example.com/abc',
|
||||
],
|
||||
FetchAllowSoftFailMask.Strict,
|
||||
), 'validation should fail if the response is otherwise consistent and cross-origin is not allowed');
|
||||
});
|
||||
|
||||
test('rejects non-canonical ID', () => {
|
||||
assert.throws(() => assertActivityMatchesUrls(
|
||||
'https://alice.example.com/@alice',
|
||||
{ id: 'https://alice.example.com/users/alice' } as IObject,
|
||||
[
|
||||
'https://alice.example.com/users/alice'
|
||||
],
|
||||
FetchAllowSoftFailMask.Strict,
|
||||
), 'throws if the response ID did not exactly match the expected ID');
|
||||
assert.doesNotThrow(() => assertActivityMatchesUrls(
|
||||
'https://alice.example.com/@alice',
|
||||
{ id: 'https://alice.example.com/users/alice' } as IObject,
|
||||
[
|
||||
'https://alice.example.com/users/alice',
|
||||
],
|
||||
FetchAllowSoftFailMask.NonCanonicalId,
|
||||
), 'does not throw if non-canonical ID is allowed');
|
||||
});
|
||||
|
||||
test('origin relaxed alignment', () => {
|
||||
assert.doesNotThrow(() => assertActivityMatchesUrls(
|
||||
'https://alice.example.com/abc',
|
||||
{ id: 'https://ap.alice.example.com/abc' } as IObject,
|
||||
[
|
||||
'https://ap.alice.example.com/abc',
|
||||
],
|
||||
FetchAllowSoftFailMask.MisalignedOrigin | FetchAllowSoftFailMask.NonCanonicalId,
|
||||
), 'validation should pass if response is a subdomain of the expected origin');
|
||||
assert.throws(() => assertActivityMatchesUrls(
|
||||
'https://alice.multi-tenant.example.com/abc',
|
||||
{ id: 'https://alice.multi-tenant.example.com/abc' } as IObject,
|
||||
[
|
||||
'https://bob.multi-tenant.example.com/abc',
|
||||
],
|
||||
FetchAllowSoftFailMask.MisalignedOrigin | FetchAllowSoftFailMask.NonCanonicalId,
|
||||
), 'validation should fail if response is a disjoint domain of the expected origin');
|
||||
assert.throws(() => assertActivityMatchesUrls(
|
||||
'https://alice.example.com/abc',
|
||||
{ id: 'https://ap.alice.example.com/abc' } as IObject,
|
||||
[
|
||||
'https://ap.alice.example.com/abc',
|
||||
],
|
||||
FetchAllowSoftFailMask.Strict,
|
||||
), 'throws if relaxed origin is forbidden');
|
||||
});
|
||||
|
||||
test('resist HTTP downgrade', () => {
|
||||
assert.throws(() => assertActivityMatchesUrls(
|
||||
'https://alice.example.com/abc',
|
||||
{ id: 'https://alice.example.com/abc' } as IObject,
|
||||
[
|
||||
'http://alice.example.com/abc',
|
||||
],
|
||||
FetchAllowSoftFailMask.Strict,
|
||||
), 'throws if HTTP downgrade is detected');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue