Skip to content

Commit

Permalink
Merge pull request #288 from HanaMisskey/merge-upstream-34
Browse files Browse the repository at this point in the history
Merge upstream 34
  • Loading branch information
kanarikanaru authored Feb 23, 2025
2 parents 05d2008 + 2ea8201 commit 1c56627
Show file tree
Hide file tree
Showing 24 changed files with 360 additions and 75 deletions.
5 changes: 5 additions & 0 deletions .config/cypress-devcontainer.yml
Original file line number Diff line number Diff line change
Expand Up @@ -220,5 +220,10 @@ allowedPrivateNetworks: [
'127.0.0.1/32'
]

# Disable automatic redirect for ActivityPub object lookup. (default: false)
# This is a strong defense against potential impersonation attacks if the viewer instance has inadequate validation.
# However it will make it impossible for other instances to lookup third-party user and notes through your URL.
#disallowExternalApRedirect: true

# Upload or download file size limits (bytes)
#maxFileSize: 262144000
5 changes: 5 additions & 0 deletions .config/docker_example.yml
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@ signToActivityPubGet: true
# '127.0.0.1/32'
#]

# Disable automatic redirect for ActivityPub object lookup. (default: false)
# This is a strong defense against potential impersonation attacks if the viewer instance has inadequate validation.
# However it will make it impossible for other instances to lookup third-party user and notes through your URL.
#disallowExternalApRedirect: true

# Upload or download file size limits (bytes)
#maxFileSize: 262144000

Expand Down
5 changes: 5 additions & 0 deletions .config/example.yml
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,11 @@ signToActivityPubGet: true
# '127.0.0.1/32'
#]

# Disable automatic redirect for ActivityPub object lookup. (default: false)
# This is a strong defense against potential impersonation attacks if the viewer instance has inadequate validation.
# However it will make it impossible for other instances to lookup third-party user and notes through your URL.
#disallowExternalApRedirect: true

# Upload or download file size limits (bytes)
#maxFileSize: 262144000

Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,15 @@
- Enhance: 開発者モードでメニューからファイルIDをコピー出来るように `#15441'
- Enhance: ノートに埋め込まれたメディアのコンテキストメニューから管理者用のファイル管理画面を開けるように ( #15440 )
- Enhance: リアクションする際に確認ダイアログを表示できるように
- Enhance: CWの注釈で入力済みの文字数を表示
- Fix: コンディショナルロールを手動で割り当てできる導線を削除 `#13529`
- Fix: 埋め込みプレイヤーから外部ページに移動できない問題を修正
- Fix: Play の再読込時に UI が以前の状態を引き継いでしまう問題を修正 `#14378`
- Fix: カスタム絵文字管理画面(beta)にてisSensitive/localOnlyの絞り込みが上手くいかない問題の修正 ( #15445 )
- Fix: CWの注釈が100文字を超えている場合、ノート投稿ボタンを非アクティブに

### Server
- Enhance: 成り済まし対策として、ActivityPub照会された時にリモートのリダイレクトを拒否できるように (config.disallowExternalApRedirect)
- Fix: `following/invalidate`でフォロワーを解除しようとしているユーザーの情報を返すように
- Fix: オブジェクトストレージの設定でPrefixを設定していなかった場合nullまたは空文字になる問題を修正
- Fix: pgroongaでの検索時にはじめのキーワードのみが検索に使用される問題を修正
Expand Down
8 changes: 1 addition & 7 deletions locales/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11047,13 +11047,7 @@ export interface Locale extends ILocale {
*/
"title": string;
/**
* このサーバーと通信することはできましたが、得られたデータが不正なものでした。
*/
"description": string;
};
"_responseInvalidIdHostNotMatch": {
/**
* 入力されたURIのドメインと最終的に得られたURIのドメインとが異なります。第三者のサーバーを介してリモートのコンテンツを照会している場合は、発信元のサーバーで取得できるURIを使用して照会し直してください。
* このサーバーと通信することはできましたが、得られたデータが不正なものでした。第三者のサーバーを介してリモートのコンテンツを照会している場合は、発信元のサーバーで取得できるURIを使用して照会し直してください。
*/
"description": string;
};
Expand Down
4 changes: 1 addition & 3 deletions locales/ja-JP.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2951,9 +2951,7 @@ _remoteLookupErrors:
description: "このサーバーとの通信に失敗しました。相手サーバーがダウンしている可能性があります。また、不正なURIや存在しないURIを入力していないか確認してください。"
_responseInvalid:
title: "レスポンスが不正です"
description: "このサーバーと通信することはできましたが、得られたデータが不正なものでした。"
_responseInvalidIdHostNotMatch:
description: "入力されたURIのドメインと最終的に得られたURIのドメインとが異なります。第三者のサーバーを介してリモートのコンテンツを照会している場合は、発信元のサーバーで取得できるURIを使用して照会し直してください。"
description: "このサーバーと通信することはできましたが、得られたデータが不正なものでした。第三者のサーバーを介してリモートのコンテンツを照会している場合は、発信元のサーバーで取得できるURIを使用して照会し直してください。"
_noSuchObject:
title: "見つかりません"
description: "要求されたリソースは見つかりませんでした。URIをもう一度お確かめください。"
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "misskey",
"version": "2025.2.1-alpha.0",
"version": "2025.2.1-beta.0",
"codename": "nasubi",
"repository": {
"type": "git",
Expand Down
3 changes: 3 additions & 0 deletions packages/backend/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ type Source = {
proxyBypassHosts?: string[];

allowedPrivateNetworks?: string[];
disallowExternalApRedirect?: boolean;

maxFileSize?: number;

Expand Down Expand Up @@ -171,6 +172,7 @@ export type Config = {
proxySmtp: string | undefined;
proxyBypassHosts: string[] | undefined;
allowedPrivateNetworks: string[] | undefined;
disallowExternalApRedirect: boolean;
maxFileSize: number;
clusterLimit: number | undefined;
id: string;
Expand Down Expand Up @@ -319,6 +321,7 @@ export function loadConfig(): Config {
proxySmtp: config.proxySmtp,
proxyBypassHosts: config.proxyBypassHosts,
allowedPrivateNetworks: config.allowedPrivateNetworks,
disallowExternalApRedirect: config.disallowExternalApRedirect ?? false,
maxFileSize: config.maxFileSize ?? 262144000,
clusterLimit: config.clusterLimit,
outgoingAddress: config.outgoingAddress,
Expand Down
6 changes: 3 additions & 3 deletions packages/backend/src/core/HttpRequestService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } 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 type { URL } from 'node:url';
Expand Down Expand Up @@ -215,7 +215,7 @@ export class HttpRequestService {
}

@bindThis
public async getActivityJson(url: string, isLocalAddressAllowed = false): Promise<IObject> {
public async getActivityJson(url: string, isLocalAddressAllowed = false, allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict): Promise<IObject> {
const res = await this.send(url, {
method: 'GET',
headers: {
Expand All @@ -232,7 +232,7 @@ export class HttpRequestService {
const finalUrl = res.url; // redirects may have been involved
const activity = await res.json() as IObject;

assertActivityMatchesUrls(activity, [finalUrl]);
assertActivityMatchesUrls(url, activity, [finalUrl], allowSoftfail);

return activity;
}
Expand Down
8 changes: 4 additions & 4 deletions packages/backend/src/core/activitypub/ApRequestService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } 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 = {
Expand Down Expand Up @@ -185,7 +185,7 @@ export class ApRequestService {
* @param url URL to fetch
*/
@bindThis
public async signedGet(url: string, user: { id: MiUser['id'] }, 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);

Expand Down Expand Up @@ -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, false);
return await this.signedGet(href, user, allowSoftfail, false);
}
}
} catch (e) {
Expand All @@ -258,7 +258,7 @@ export class ApRequestService {
const finalUrl = res.url; // redirects may have been involved
const activity = await res.json() as IObject;

assertActivityMatchesUrls(activity, [finalUrl]);
assertActivityMatchesUrls(url, activity, [finalUrl], allowSoftfail);

return activity;
}
Expand Down
21 changes: 5 additions & 16 deletions packages/backend/src/core/activitypub/ApResolverService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { ApRendererService } from './ApRendererService.js';
import { ApRequestService } from './ApRequestService.js';
import type { IObject, ICollection, IOrderedCollection } from './type.js';
import { IdentifiableError } from '@/misc/identifiable-error.js';
import { FetchAllowSoftFailMask } from './misc/check-against-url.js';

export class Resolver {
private history: Set<string>;
Expand Down Expand Up @@ -72,7 +73,7 @@ export class Resolver {
}

@bindThis
public async resolve(value: string | IObject): Promise<IObject> {
public async resolve(value: string | IObject, allowSoftfail: FetchAllowSoftFailMask = FetchAllowSoftFailMask.Strict): Promise<IObject> {
if (typeof value !== 'string') {
return value;
}
Expand Down Expand Up @@ -108,8 +109,8 @@ export class Resolver {
}

const object = (this.user
? await this.apRequestService.signedGet(value, this.user) as IObject
: await this.httpRequestService.getActivityJson(value)) as IObject;
? await this.apRequestService.signedGet(value, this.user, allowSoftfail) as IObject
: await this.httpRequestService.getActivityJson(value, undefined, allowSoftfail)) as IObject;

if (
Array.isArray(object['@context']) ?
Expand All @@ -118,19 +119,7 @@ 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`);
}


return object;
}

Expand Down
126 changes: 116 additions & 10 deletions packages/backend/src/core/activitypub/misc/check-against-url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,124 @@
*/
import type { IObject } from '../type.js';

export function assertActivityMatchesUrls(activity: IObject, urls: string[]) {
const hosts = urls.map(it => new URL(it).host);
export enum FetchAllowSoftFailMask {
// Allow no softfail flags
Strict = 0,
// The values in tuple (requestUrl, finalUrl, objectId) are not all identical
//
// This condition is common for user-initiated lookups but should not be allowed in federation loop
//
// Allow variations:
// good example: https://alice.example.com/@user -> https://alice.example.com/user/:userId
// problematic example: https://alice.example.com/redirect?url=https://bad.example.com/ -> https://bad.example.com/ -> https://alice.example.com/somethingElse
NonCanonicalId = 1 << 0,
// Allow the final object to be at most one subdomain deeper than the request URL, similar to SPF relaxed alignment
//
// Currently no code path allows this flag to be set, but is kept in case of future use as some niche deployments do this, and we provide a pre-reviewed mechanism to opt-in.
//
// Allow variations:
// good example: https://example.com/@user -> https://activitypub.example.com/@user { id: 'https://activitypub.example.com/@user' }
// problematic example: https://example.com/@user -> https://untrusted.example.com/@user { id: 'https://untrusted.example.com/@user' }
MisalignedOrigin = 1 << 1,
// The requested URL has a different host than the returned object ID, although the final URL is still consistent with the object ID
//
// This condition is common for user-initiated lookups using an intermediate host but should not be allowed in federation loops
//
// Allow variations:
// good example: https://alice.example.com/@user@bob.example.com -> https://bob.example.com/@user { id: 'https://bob.example.com/@user' }
// problematic example: https://alice.example.com/definitelyAlice -> https://bob.example.com/@somebodyElse { id: 'https://bob.example.com/@somebodyElse' }
CrossOrigin = 1 << 2 | MisalignedOrigin,
// Allow all softfail flags
//
// do not use this flag on released code
Any = ~0,
}

/**
* Fuzz match on whether the candidate host has authority over the request host
*
* @param requestHost The host of the requested resources
* @param candidateHost The host of final response
* @returns Whether the candidate host has authority over the request host, or if a soft fail is required for a match
*/
function hostFuzzyMatch(requestHost: string, candidateHost: string): FetchAllowSoftFailMask {
const requestFqdn = requestHost.endsWith('.') ? requestHost : `${requestHost}.`;
const candidateFqdn = candidateHost.endsWith('.') ? candidateHost : `${candidateHost}.`;

if (requestFqdn === candidateFqdn) {
return FetchAllowSoftFailMask.Strict;
}

// allow only one case where candidateHost is a first-level subdomain of requestHost
const requestDnsDepth = requestFqdn.split('.').length;
const candidateDnsDepth = candidateFqdn.split('.').length;

if ((candidateDnsDepth - requestDnsDepth) !== 1) {
return FetchAllowSoftFailMask.CrossOrigin;
}

const idOk = activity.id !== undefined && hosts.includes(new URL(activity.id).host);
if (`.${candidateHost}`.endsWith(`.${requestHost}`)) {
return FetchAllowSoftFailMask.MisalignedOrigin;
}

// 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' && hosts.includes(new URL(activity.url).host);
return FetchAllowSoftFailMask.CrossOrigin;
}

if (!idOk && !urlOk) {
throw new Error(`bad Activity: neither id(${activity?.id}) nor url(${activity?.url}) match location(${urls})`);
// normalize host names by removing www. prefix
function normalizeSynonymousSubdomain(url: URL | string): URL {
const urlParsed = url instanceof URL ? url : new URL(url);
const host = urlParsed.host;
const normalizedHost = host.replace(/^www\./, '');
return new URL(urlParsed.toString().replace(host, normalizedHost));
}

export function assertActivityMatchesUrls(requestUrl: string | URL, activity: IObject, candidateUrls: (string | URL)[], allowSoftfail: FetchAllowSoftFailMask): FetchAllowSoftFailMask {
// must have a unique identifier to verify authority
if (!activity.id) {
throw new Error('bad Activity: missing id field');
}

let softfail = 0;

// if the flag is allowed, set the flag on return otherwise throw
const requireSoftfail = (needed: FetchAllowSoftFailMask, message: string) => {
if ((allowSoftfail & needed) !== needed) {
throw new Error(message);
}

softfail |= needed;
};

const requestUrlParsed = normalizeSynonymousSubdomain(requestUrl);
const idParsed = normalizeSynonymousSubdomain(activity.id);

const candidateUrlsParsed = candidateUrls.map(it => normalizeSynonymousSubdomain(it));

const requestUrlSecure = requestUrlParsed.protocol === 'https:';
const finalUrlSecure = candidateUrlsParsed.every(it => it.protocol === 'https:');
if (requestUrlSecure && !finalUrlSecure) {
throw new Error(`bad Activity: id(${activity.id}) is not allowed to have http:// in the url`);
}

// Compare final URL to the ID
if (!candidateUrlsParsed.some(it => it.href === idParsed.href)) {
requireSoftfail(FetchAllowSoftFailMask.NonCanonicalId, `bad Activity: id(${activity.id}) does not match response url(${candidateUrlsParsed.map(it => it.toString())})`);

// at lease host need to match exactly (ActivityPub requirement)
if (!candidateUrlsParsed.some(it => idParsed.host === it.host)) {
throw new Error(`bad Activity: id(${activity.id}) does not match response host(${candidateUrlsParsed.map(it => it.host)})`);
}
}

// Compare request URL to the ID
if (!requestUrlParsed.href.includes(idParsed.href)) {
requireSoftfail(FetchAllowSoftFailMask.NonCanonicalId, `bad Activity: id(${activity.id}) does not match request url(${requestUrlParsed.toString()})`);

// if cross-origin lookup is allowed, we can accept some variation between the original request URL to the final object ID (but not between the final URL and the object ID)
const hostResult = hostFuzzyMatch(requestUrlParsed.host, idParsed.host);

requireSoftfail(hostResult, `bad Activity: id(${activity.id}) is valid but is not the same origin as request url(${requestUrlParsed.toString()})`);
}

return softfail;
}
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,12 @@ export class InboxProcessorService implements OnApplicationShutdown {

// それでもわからなければ終了
if (authUser == null) {
throw new Bull.UnrecoverableError('skip: failed to resolve user');
throw new Bull.UnrecoverableError(`skip: failed to resolve user ${getApId(activity.actor)}`);
}

// publicKey がなくても終了
if (authUser.key == null) {
throw new Bull.UnrecoverableError('skip: failed to resolve user publicKey');
throw new Bull.UnrecoverableError(`skip: failed to resolve user publicKey ${getApId(activity.actor)}`);
}

// HTTP-Signatureの検証
Expand Down
Loading

0 comments on commit 1c56627

Please sign in to comment.