Skip to content

Commit

Permalink
Merge commit from fork
Browse files Browse the repository at this point in the history 
* enhance: Add a few validation fixes from Sharkey

See the original MR on the GitLab instance:
https://activitypub.software/TransFem-org/Sharkey/-/merge_requests/484

Co-Authored-By: Dakkar <[email protected]>

* fix: primitive 2: acceptance of cross-origin alternate

Co-Authored-By: Laura Hausmann <[email protected]>

* fix: primitive 3: validation of non-final url

* fix: primitive 4: missing same-origin identifier validation of collection-wrapped activities

* fix: primitives 5 & 8: reject activities with non
string identifiers

Co-Authored-By: Laura Hausmann <[email protected]>

* fix: primitive 6: reject anonymous objects that were fetched by their id

* fix: primitives 9, 10 & 11: http signature validation
doesn't enforce required headers or specify auth header name

Co-Authored-By: Laura Hausmann <[email protected]>

* fix: primitive 14: improper validation of outbox, followers, following & shared inbox collections

* fix: code style for primitive 14

* fix: primitive 15: improper same-origin validation for
note uri and url

Co-Authored-By: Laura Hausmann <[email protected]>

* fix: primitive 16: improper same-origin validation for user uri and url

* fix: primitive 17: note same-origin identifier validation can be bypassed by wrapping the id in an array

* fix: code style for primitive 17

* fix: check attribution against actor in notes

While this isn't strictly required to fix the exploits at hand, this
mirrors the fix in `ApQuestionService` for GHSA-5h8r-gq97-xv69, as a
preemptive countermeasure.

* fix: primitive 18: `ap/get` bypasses access checks

One might argue that we could make this one actually preform access
checks against the returned activity object, but I feel like that's a
lot more work than just restricting it to administrators, since, to me
at least, it seems more like a debugging tool than anything else.

* fix: primitive 19 & 20: respect blocks and hide more

Ideally, the user property should also be hidden (as leaving it in leaks
information slightly), but given the schema of the note endpoint, I
don't think that would be possible without introducing some kind of
"ghost" user, who is attributed for posts by users who have you blocked.

* fix: primitives 21, 22, and 23: reuse resolver

This also increases the default `recursionLimit` for `Resolver`, as it
theoretically will go higher that it previously would and could possibly
fail on non-malicious collection activities.

* fix: primitives 25-33: proper local instance checks

* revert: fix: primitive 19 & 20

This reverts commit 465a9fe6591de90f78bd3d084e3c01e65dc3cf3c.

---------

Co-authored-by: Dakkar <[email protected]>
Co-authored-by: Laura Hausmann <[email protected]>
Co-authored-by: syuilo <[email protected]>
  • Loading branch information
@K4rakara @dakkar
@zotanmew @syuilo
4 people authored Nov 20, 2024
1 parent 1c284c8 commit 5f67520
Show file tree
Hide file tree
Showing 15 changed files with 227 additions and 76 deletions.
8 changes: 7 additions & 1 deletion packages/backend/src/core/HttpRequestService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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, [finalUrl]);

return activity;
}

@bindThis
Expand Down
4 changes: 2 additions & 2 deletions packages/backend/src/core/RemoteUserResolveService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ export class RemoteUserResolveService {
}) as MiLocalUser;
}

host = this.utilityService.toPuny(host);
host = this.utilityService.punyHost(host);

if (this.config.host === host) {
if (host === this.utilityService.toPuny(this.config.host)) {
this.logger.info(`return local user: ${usernameLower}`);
return await this.usersRepository.findOneBy({ usernameLower, host: IsNull() }).then(u => {
if (u == null) {
Expand Down
14 changes: 13 additions & 1 deletion packages/backend/src/core/UtilityService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ export class UtilityService {
return this.toPuny(this.config.host) === this.toPuny(host);
}

@bindThis
public isUriLocal(uri: string): boolean {
return this.punyHost(uri) === this.toPuny(this.config.host);
}

@bindThis
public isBlockedHost(blockedHosts: string[], host: string | null): boolean {
if (host == null) return false;
Expand Down Expand Up @@ -96,7 +101,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
Expand All @@ -110,6 +115,13 @@ export class UtilityService {
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;
}

@bindThis
public isFederationAllowedHost(host: string): boolean {
if (this.meta.federation === 'none') return false;
Expand Down
6 changes: 5 additions & 1 deletion packages/backend/src/core/activitypub/ApDbResolverService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type { Config } from '@/config.js';
import { MemoryKVCache } from '@/misc/cache.js';
import type { MiUserPublickey } from '@/models/UserPublickey.js';
import { CacheService } from '@/core/CacheService.js';
import { UtilityService } from '@/core/UtilityService.js';
import type { MiNote } from '@/models/Note.js';
import { bindThis } from '@/decorators.js';
import { MiLocalUser, MiRemoteUser } from '@/models/User.js';
Expand Down Expand Up @@ -53,6 +54,7 @@ export class ApDbResolverService implements OnApplicationShutdown {

private cacheService: CacheService,
private apPersonService: ApPersonService,
private utilityService: UtilityService,
) {
this.publicKeyCache = new MemoryKVCache<MiUserPublickey | null>(1000 * 60 * 60 * 12); // 12h
this.publicKeyByUserIdCache = new MemoryKVCache<MiUserPublickey | null>(1000 * 60 * 60 * 12); // 12h
Expand All @@ -63,7 +65,9 @@ export class ApDbResolverService implements OnApplicationShutdown {
const separator = '/';

const uri = new URL(getApId(value));
if (uri.origin !== this.config.url) return { local: false, uri: uri.href };
if (this.utilityService.toPuny(uri.host) !== this.utilityService.toPuny(this.config.host)) {
return { local: false, uri: uri.href };
}

const [, type, id, ...rest] = uri.pathname.split(separator);
return {
Expand Down
93 changes: 56 additions & 37 deletions packages/backend/src/core/activitypub/ApInboxService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,26 @@ export class ApInboxService {
}

@bindThis
public async performActivity(actor: MiRemoteUser, activity: IObject): Promise<string | void> {
public async performActivity(actor: MiRemoteUser, activity: IObject, resolver?: Resolver): Promise<string | void> {
let result = undefined as string | void;
if (isCollectionOrOrderedCollection(activity)) {
const results = [] as [string, string | void][];
const resolver = this.apResolverService.createResolver();
for (const item of toArray(isCollection(activity) ? activity.items : activity.orderedItems)) {
// eslint-disable-next-line no-param-reassign
resolver ??= this.apResolverService.createResolver();

const items = toArray(isCollection(activity) ? activity.items : activity.orderedItems);
if (items.length >= resolver.getRecursionLimit()) {
throw new Error(`skipping activity: collection would surpass recursion limit: ${this.utilityService.extractDbHost(actor.uri)}`);
}

for (const item of items) {
const act = await resolver.resolve(item);
if (act.id == null || this.utilityService.extractDbHost(act.id) !== this.utilityService.extractDbHost(actor.uri)) {
this.logger.debug('skipping activity: activity id is null or mismatching');
continue;
}
try {
results.push([getApId(item), await this.performOneActivity(actor, act)]);
results.push([getApId(item), await this.performOneActivity(actor, act, resolver)]);
} catch (err) {
if (err instanceof Error || typeof err === 'string') {
this.logger.error(err);
Expand All @@ -112,7 +123,7 @@ export class ApInboxService {
result = results.map(([id, reason]) => `${id}: ${reason}`).join('\n');
}
} else {
result = await this.performOneActivity(actor, activity);
result = await this.performOneActivity(actor, activity, resolver);
}

// ついでにリモートユーザーの情報が古かったら更新しておく
Expand All @@ -127,37 +138,37 @@ export class ApInboxService {
}

@bindThis
public async performOneActivity(actor: MiRemoteUser, activity: IObject): Promise<string | void> {
public async performOneActivity(actor: MiRemoteUser, activity: IObject, resolver?: Resolver): Promise<string | void> {
if (actor.isSuspended) return;

if (isCreate(activity)) {
return await this.create(actor, activity);
return await this.create(actor, activity, resolver);
} else if (isDelete(activity)) {
return await this.delete(actor, activity);
} else if (isUpdate(activity)) {
return await this.update(actor, activity);
return await this.update(actor, activity, resolver);
} else if (isFollow(activity)) {
return await this.follow(actor, activity);
} else if (isAccept(activity)) {
return await this.accept(actor, activity);
return await this.accept(actor, activity, resolver);
} else if (isReject(activity)) {
return await this.reject(actor, activity);
return await this.reject(actor, activity, resolver);
} else if (isAdd(activity)) {
return await this.add(actor, activity);
return await this.add(actor, activity, resolver);
} else if (isRemove(activity)) {
return await this.remove(actor, activity);
return await this.remove(actor, activity, resolver);
} else if (isAnnounce(activity)) {
return await this.announce(actor, activity);
return await this.announce(actor, activity, resolver);
} else if (isLike(activity)) {
return await this.like(actor, activity);
} else if (isUndo(activity)) {
return await this.undo(actor, activity);
return await this.undo(actor, activity, resolver);
} else if (isBlock(activity)) {
return await this.block(actor, activity);
} else if (isFlag(activity)) {
return await this.flag(actor, activity);
} else if (isMove(activity)) {
return await this.move(actor, activity);
return await this.move(actor, activity, resolver);
} else {
return `unrecognized activity type: ${activity.type}`;
}
Expand Down Expand Up @@ -199,12 +210,13 @@ export class ApInboxService {
}

@bindThis
private async accept(actor: MiRemoteUser, activity: IAccept): Promise<string> {
private async accept(actor: MiRemoteUser, activity: IAccept, resolver?: Resolver): Promise<string> {
const uri = activity.id ?? activity;

this.logger.info(`Accept: ${uri}`);

const resolver = this.apResolverService.createResolver();
// eslint-disable-next-line no-param-reassign
resolver ??= this.apResolverService.createResolver();

const object = await resolver.resolve(activity.object).catch(err => {
this.logger.error(`Resolution failed: ${err}`);
Expand Down Expand Up @@ -241,7 +253,7 @@ export class ApInboxService {
}

@bindThis
private async add(actor: MiRemoteUser, activity: IAdd): Promise<string | void> {
private async add(actor: MiRemoteUser, activity: IAdd, resolver?: Resolver): Promise<string | void> {
if (actor.uri !== activity.actor) {
return 'invalid actor';
}
Expand All @@ -251,7 +263,7 @@ export class ApInboxService {
}

if (activity.target === actor.featured) {
const note = await this.apNoteService.resolveNote(activity.object);
const note = await this.apNoteService.resolveNote(activity.object, { resolver });
if (note == null) return 'note not found';
await this.notePiningService.addPinned(actor, note.id);
return;
Expand All @@ -261,12 +273,13 @@ export class ApInboxService {
}

@bindThis
private async announce(actor: MiRemoteUser, activity: IAnnounce): Promise<string | void> {
private async announce(actor: MiRemoteUser, activity: IAnnounce, resolver?: Resolver): Promise<string | void> {
const uri = getApId(activity);

this.logger.info(`Announce: ${uri}`);

const resolver = this.apResolverService.createResolver();
// eslint-disable-next-line no-param-reassign
resolver ??= this.apResolverService.createResolver();

if (!activity.object) return 'skip: activity has no object property';
const targetUri = getApId(activity.object);
Expand All @@ -283,7 +296,7 @@ export class ApInboxService {
}

@bindThis
private async announceNote(actor: MiRemoteUser, activity: IAnnounce, target: IPost): Promise<string | void> {
private async announceNote(actor: MiRemoteUser, activity: IAnnounce, target: IPost, resolver?: Resolver): Promise<string | void> {
const uri = getApId(activity);

if (actor.isSuspended) {
Expand All @@ -305,7 +318,7 @@ export class ApInboxService {
// Announce対象をresolve
let renote;
try {
renote = await this.apNoteService.resolveNote(target);
renote = await this.apNoteService.resolveNote(target, { resolver });
if (renote == null) return 'announce target is null';
} catch (err) {
// 対象が4xxならスキップ
Expand All @@ -324,7 +337,7 @@ export class ApInboxService {

this.logger.info(`Creating the (Re)Note: ${uri}`);

const activityAudience = await this.apAudienceService.parseAudience(actor, activity.to, activity.cc);
const activityAudience = await this.apAudienceService.parseAudience(actor, activity.to, activity.cc, resolver);
const createdAt = activity.published ? new Date(activity.published) : null;

if (createdAt && createdAt < this.idService.parse(renote.id).date) {
Expand Down Expand Up @@ -362,7 +375,7 @@ export class ApInboxService {
}

@bindThis
private async create(actor: MiRemoteUser, activity: ICreate): Promise<string | void> {
private async create(actor: MiRemoteUser, activity: ICreate, resolver?: Resolver): Promise<string | void> {
const uri = getApId(activity);

this.logger.info(`Create: ${uri}`);
Expand All @@ -387,7 +400,8 @@ export class ApInboxService {
activity.object.attributedTo = activity.actor;
}

const resolver = this.apResolverService.createResolver();
// eslint-disable-next-line no-param-reassign
resolver ??= this.apResolverService.createResolver();

const object = await resolver.resolve(activity.object).catch(e => {
this.logger.error(`Resolution failed: ${e}`);
Expand All @@ -414,6 +428,8 @@ export class ApInboxService {
if (this.utilityService.extractDbHost(actor.uri) !== this.utilityService.extractDbHost(note.id)) {
return 'skip: host in actor.uri !== note.id';
}
} else {
return 'skip: note.id is not a string';
}
}

Expand All @@ -423,7 +439,7 @@ export class ApInboxService {
const exist = await this.apNoteService.fetchNote(note);
if (exist) return 'skip: note exists';

await this.apNoteService.createNote(note, resolver, silent);
await this.apNoteService.createNote(note, actor, resolver, silent);
return 'ok';
} catch (err) {
if (err instanceof StatusError && !err.isRetryable) {
Expand Down Expand Up @@ -555,12 +571,13 @@ export class ApInboxService {
}

@bindThis
private async reject(actor: MiRemoteUser, activity: IReject): Promise<string> {
private async reject(actor: MiRemoteUser, activity: IReject, resolver?: Resolver): Promise<string> {
const uri = activity.id ?? activity;

this.logger.info(`Reject: ${uri}`);

const resolver = this.apResolverService.createResolver();
// eslint-disable-next-line no-param-reassign
resolver ??= this.apResolverService.createResolver();

const object = await resolver.resolve(activity.object).catch(e => {
this.logger.error(`Resolution failed: ${e}`);
Expand Down Expand Up @@ -597,7 +614,7 @@ export class ApInboxService {
}

@bindThis
private async remove(actor: MiRemoteUser, activity: IRemove): Promise<string | void> {
private async remove(actor: MiRemoteUser, activity: IRemove, resolver?: Resolver): Promise<string | void> {
if (actor.uri !== activity.actor) {
return 'invalid actor';
}
Expand All @@ -607,7 +624,7 @@ export class ApInboxService {
}

if (activity.target === actor.featured) {
const note = await this.apNoteService.resolveNote(activity.object);
const note = await this.apNoteService.resolveNote(activity.object, { resolver });
if (note == null) return 'note not found';
await this.notePiningService.removePinned(actor, note.id);
return;
Expand All @@ -617,7 +634,7 @@ export class ApInboxService {
}

@bindThis
private async undo(actor: MiRemoteUser, activity: IUndo): Promise<string> {
private async undo(actor: MiRemoteUser, activity: IUndo, resolver?: Resolver): Promise<string> {
if (actor.uri !== activity.actor) {
return 'invalid actor';
}
Expand All @@ -626,7 +643,8 @@ export class ApInboxService {

this.logger.info(`Undo: ${uri}`);

const resolver = this.apResolverService.createResolver();
// eslint-disable-next-line no-param-reassign
resolver ??= this.apResolverService.createResolver();

const object = await resolver.resolve(activity.object).catch(e => {
this.logger.error(`Resolution failed: ${e}`);
Expand Down Expand Up @@ -750,14 +768,15 @@ export class ApInboxService {
}

@bindThis
private async update(actor: MiRemoteUser, activity: IUpdate): Promise<string> {
private async update(actor: MiRemoteUser, activity: IUpdate, resolver?: Resolver): Promise<string> {
if (actor.uri !== activity.actor) {
return 'skip: invalid actor';
}

this.logger.debug('Update');

const resolver = this.apResolverService.createResolver();
// eslint-disable-next-line no-param-reassign
resolver ??= this.apResolverService.createResolver();

const object = await resolver.resolve(activity.object).catch(e => {
this.logger.error(`Resolution failed: ${e}`);
Expand All @@ -776,11 +795,11 @@ export class ApInboxService {
}

@bindThis
private async move(actor: MiRemoteUser, activity: IMove): Promise<string> {
private async move(actor: MiRemoteUser, activity: IMove, resolver?: Resolver): Promise<string> {
// fetch the new and old accounts
const targetUri = getApHrefNullable(activity.target);
if (!targetUri) return 'skip: invalid activity target';

return await this.apPersonService.updatePerson(actor.uri) ?? 'skip: nothing to do';
return await this.apPersonService.updatePerson(actor.uri, resolver) ?? 'skip: nothing to do';
}
}
Loading

0 comments on commit 5f67520

Please sign in to comment.