BREAKING: activitypub: validate fetch signatures #399

Manually merged
Johann150 merged 2 commits from helene/FoundKey:feature/verify-fetch-signatures into main 2023-06-27 19:59:29 +00:00
10 changed files with 194 additions and 16 deletions
Showing only changes of commit f89a374e5f - Show all commits

View file

@ -0,0 +1,11 @@
export class allowUnsignedFetches1687460719276 {
name = 'allowUnsignedFetches1687460719276'
async up(queryRunner) {
await queryRunner.query(`ALTER TABLE "meta" ADD "allowUnsignedFetches" bool default false`);
}
async down(queryRunner) {
await queryRunner.query(`ALTER TABLE "meta" DROP COLUMN "allowUnsignedFetches"`);
}
}

View file

@ -82,6 +82,11 @@ export class Meta {
})
public blockedHosts: string[];
@Column('boolean', {
default: false
})
public allowUnsignedFetches: boolean;
@Column('varchar', {
length: 512,
nullable: true,

View file

@ -4,6 +4,7 @@ import { IRemoteUser } from '@/models/entities/user.js';
import { UserPublickey } from '@/models/entities/user-publickey.js';
import { uriPersonCache, userByIdCache } from '@/services/user-cache.js';
import { createPerson } from '@/remote/activitypub/models/person.js';
import { Resolver } from '@/remote/activitypub/resolver.js';
export type AuthUser = {
user: IRemoteUser;
@ -29,8 +30,8 @@ function authUserFromApId(uri: string): Promise<AuthUser | null> {
});
}
export async function getAuthUser(keyId: string, actorUri: string, resolver: Resolver): Promise<AuthUser | null> {
let authUser = await publicKeyCache.fetch(keyId)
export async function authUserFromKeyId(keyId: string): Promise<AuthUser | null> {
return await publicKeyCache.fetch(keyId)
.then(async key => {
if (!key) return null;
else return {
@ -38,6 +39,10 @@ export async function getAuthUser(keyId: string, actorUri: string, resolver: Res
key,
};
});
}
export async function getAuthUser(keyId: string, actorUri: string, resolver: Resolver): Promise<AuthUser | null> {
let authUser = await authUserFromKeyId(keyId);
if (authUser != null) return authUser;
authUser = await authUserFromApId(actorUri);

View file

@ -20,6 +20,8 @@ import Outbox from './activitypub/outbox.js';
import Followers from './activitypub/followers.js';
import Following from './activitypub/following.js';
import Featured from './activitypub/featured.js';
import { SignatureValidationResult, validateFetchSignature } from './activitypub/fetch-signature.js';
import { isInstanceActor } from '@/services/instance-actor.js';
// Init router
const router = new Router();
@ -59,6 +61,30 @@ export function setResponseType(ctx: Router.RouterContext): void {
}
}
export function handleSignatureResult(ctx: Router.RouterContext, result: SignatureValidationResult): boolean {
switch (result) {
case 'always':
ctx.set('Cache-Control', 'public, max-age=180');
return true;
case 'valid':
ctx.set('Cache-Control', 'no-store');
return true;
case 'missing':
case 'invalid':
// This would leak information on blocks. Only use for debugging.
// ctx.status = 400;
// break;
// eslint-disable-next-line no-fallthrough
case 'rejected':
default:
ctx.status = 403;
break;
}
ctx.set('Cache-Control', 'no-store');
return false;
}
// inbox
router.post('/inbox', json(), inbox);
router.post('/users/:user/inbox', json(), inbox);
@ -66,6 +92,7 @@ router.post('/users/:user/inbox', json(), inbox);
// note
router.get('/notes/:note', async (ctx, next) => {
if (!isActivityPubReq(ctx)) return await next();
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
const note = await Notes.findOneBy({
id: ctx.params.note,
@ -89,7 +116,6 @@ router.get('/notes/:note', async (ctx, next) => {
}
ctx.body = renderActivity(await renderNote(note, false));
ctx.set('Cache-Control', 'public, max-age=180');
setResponseType(ctx);
});
@ -103,6 +129,7 @@ router.get('/notes/:note/activity', async ctx => {
ctx.redirect(`/notes/${ctx.params.note}`);
return;
}
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
const note = await Notes.findOneBy({
id: ctx.params.note,
@ -117,21 +144,32 @@ router.get('/notes/:note/activity', async ctx => {
}
ctx.body = renderActivity(await renderNoteOrRenoteActivity(note));
ctx.set('Cache-Control', 'public, max-age=180');
setResponseType(ctx);
});
// outbox
router.get('/users/:user/outbox', Outbox);
router.get('/users/:user/outbox', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
return await Outbox(ctx);
});
// followers
router.get('/users/:user/followers', Followers);
router.get('/users/:user/followers', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
return await Followers(ctx);
});
// following
router.get('/users/:user/following', Following);
router.get('/users/:user/following', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
return await Following(ctx);
});
// featured
router.get('/users/:user/collections/featured', Featured);
router.get('/users/:user/collections/featured', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
return await Featured(ctx);
});
// publickey
router.get('/users/:user/publickey', async ctx => {
@ -166,7 +204,6 @@ async function userInfo(ctx: Router.RouterContext, user: User | null): Promise<v
}
ctx.body = renderActivity(await renderPerson(user as ILocalUser));
ctx.set('Cache-Control', 'public, max-age=180');
setResponseType(ctx);
}
@ -181,11 +218,26 @@ router.get('/users/:user', async (ctx, next) => {
isSuspended: false,
});
// Allow fetching the instance actor without any HTTP signature.
// Only on this route, as it is the canonical route.
// If the user could not be resolved, or is not the instance actor,
// validate and enforce signatures.
if (user == null || !isInstanceActor(user))
{
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
}
else if (isInstanceActor(user))
{
// Set cache at all times for instance actors.
ctx.set('Cache-Control', 'public, max-age=180');
}
await userInfo(ctx, user);
});
router.get('/@:user', async (ctx, next) => {
if (!isActivityPubReq(ctx)) return await next();
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
const user = await Users.findOneBy({
usernameLower: ctx.params.user.toLowerCase(),
@ -198,6 +250,9 @@ router.get('/@:user', async (ctx, next) => {
// emoji
router.get('/emojis/:emoji', async ctx => {
// Enforcing HTTP signatures on Emoji objects could cause problems for
// other software that might use those objects for copying custom emoji.
const emoji = await Emojis.findOneBy({
host: IsNull(),
name: ctx.params.emoji,
@ -215,6 +270,7 @@ router.get('/emojis/:emoji', async ctx => {
// like
router.get('/likes/:like', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
const reaction = await NoteReactions.findOneBy({ id: ctx.params.like });
if (reaction == null) {
@ -233,12 +289,12 @@ router.get('/likes/:like', async ctx => {
}
ctx.body = renderActivity(await renderLike(reaction, note));
ctx.set('Cache-Control', 'public, max-age=180');
setResponseType(ctx);
});
// follow
router.get('/follows/:follower/:followee', async ctx => {
if (!handleSignatureResult(ctx, await validateFetchSignature(ctx.req))) return;
// This may be used before the follow is completed, so we do not
// check if the following exists.
@ -259,7 +315,6 @@ router.get('/follows/:follower/:followee', async ctx => {
}
ctx.body = renderActivity(renderFollow(follower, followee));
ctx.set('Cache-Control', 'public, max-age=180');
setResponseType(ctx);
});

View file

@ -36,6 +36,5 @@ export default async (ctx: Router.RouterContext) => {
);
ctx.body = renderActivity(rendered);
ctx.set('Cache-Control', 'public, max-age=180');
setResponseType(ctx);
};

View file

@ -0,0 +1,102 @@
import httpSignature from '@peertube/http-signature';
import { toPuny } from '@/misc/convert-host.js';
import { fetchMeta } from '@/misc/fetch-meta.js';
import { shouldBlockInstance } from '@/misc/should-block-instance.js';
import { authUserFromKeyId, getAuthUser } from '@/remote/activitypub/misc/auth-user.js';
import { getApId, isActor } from '@/remote/activitypub/type.js';
import { StatusError } from '@/misc/fetch.js';
import { Resolver } from '@/remote/activitypub/resolver.js';
import { createPerson } from '@/remote/activitypub/models/person.js';
export type SignatureValidationResult = 'missing' | 'invalid' | 'rejected' | 'valid' | 'always';
async function resolveKeyId(keyId: string, resolver: Resolver): Promise<AuthUser | null> {
// Do we already know that keyId?
const authUser = await authUserFromKeyId(keyId);
if (authUser != null) return authUser;
// If not, discover it.
const keyUrl = new URL(keyId);
keyUrl.hash = ''; // Fragment should not be part of the request.
const keyObject = await resolver.resolve(keyUrl.toString());
// Does the keyId end up resolving to an Actor?
if (isActor(keyObject)) {
await createPerson(keyObject, resolver);
return await getAuthUser(keyId, getApId(keyObject), resolver);
}
// Does the keyId end up resolving to a Key-like?
const keyData = keyObject as any;
if (keyData.owner != null && keyData.publicKeyPem != null) {
await createPerson(keyData.owner, resolver);
return await getAuthUser(keyId, getApId(keyData.owner), resolver);
}
// Cannot be resolved.
return null;
}
export async function validateFetchSignature(req: IncomingMessage): Promise<SignatureValidationResult> {
Johann150 marked this conversation as resolved
Review

This should be refactored together with the code from packages/backend/src/queue/processors/inbox.ts that this was copied from to work out a way that does not duplicate most of the code.

This should be refactored together with the code from `packages/backend/src/queue/processors/inbox.ts` that this was copied from to work out a way that does not duplicate most of the code.
Review

I would've liked to replace the inbox signature code with this function, but LD signatures and the reliance on the actor field of an activity (as well as relay logic) would make it difficult without introducing bugs, unwanted behavior, unclean code and/or more network/database requests.

I would've liked to replace the `inbox` signature code with this function, but LD signatures and the reliance on the `actor` field of an activity (as well as relay logic) would make it difficult without introducing bugs, unwanted behavior, unclean code and/or more network/database requests.
const meta = await fetchMeta();
let signature;
if (meta.allowUnsignedFetches === true)
return 'always';
try {
signature = httpSignature.parseRequest(req);
} catch (e) {
// TypeScript has wrong typings for Error, meaning I can't extract `name`.
// No typings for @peertube/http-signature's Errors either.
// This means we have to report it as missing instead of invalid in cases
// where the structure is incorrect.
return 'missing';
}
// This old `keyId` format is no longer supported.
const keyIdLower = signature.keyId.toLowerCase();
if (keyIdLower.startsWith('acct:'))
return 'invalid';
const host = toPuny(new URL(keyIdLower).hostname);
Johann150 marked this conversation as resolved
Review
-	const host = toPuny(new URL(keyIdLower).hostname);
+	const host = extractDbHost(keyIdLower);

and consequently above:

-import { toPuny } from '@/misc/convert-host.js'; 
+import { extractDbHost } from '@/misc/convert-host.js'; 
```diff - const host = toPuny(new URL(keyIdLower).hostname); + const host = extractDbHost(keyIdLower); ``` and consequently above: ```diff -import { toPuny } from '@/misc/convert-host.js'; +import { extractDbHost } from '@/misc/convert-host.js'; ```
Review

I based myself on the inbox code which uses the same code. I will apply the patch to my submission, but the inbox code should probably be patched to match. (PS: I find extractDbHost to be quite a confusing name for such a function)

I based myself on the `inbox` code which uses the same code. I will apply the patch to my submission, but the `inbox` code should probably be patched to match. (PS: I find `extractDbHost` to be quite a confusing name for such a function)
Review

Do you have a suggestion for a better name (for future refactoring)?

Do you have a suggestion for a better name (for future refactoring)?
Review

extractHost, extractHostname, punyHost, or something similar? I'm not sure; I mostly think the Db in the name doesn't make much more sense when used in such contexts, even if that's where the function originated from.

`extractHost`, `extractHostname`, `punyHost`, or something similar? I'm not sure; I mostly think the `Db` in the name doesn't make much more sense when used in such contexts, even if that's where the function originated from.
// Reject if the host is blocked.
if (await shouldBlockInstance(host))
return 'rejected';
const resolver = new Resolver();
let authUser;
try {
authUser = await resolveKeyId(signature.keyId, resolver);
} catch (e) {
if (e instanceof StatusError) {
if (e.isClientError) {
// Actor is deleted.
return 'rejected';
} else {
throw new Error(`Error in signature ${signature} - ${e.statusCode || e}`);
}
}
}
if (authUser == null) {
// Key not found? Unacceptable!
return 'invalid';
} else {
// Found key!
}
// Make sure the resolved user matches the keyId host.
if (authUser.user.host !== host)
return 'rejected';
// Verify the HTTP Signature
const httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem);
if (httpSignatureValidated === true)
Johann150 marked this conversation as resolved
Review

Explicitly checking for something to equal true seems unnecessary, since httpSignature.verifySignature has the return type boolean.

-	const httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem);
-	if (httpSignatureValidated === true)
+	if (httpSignature.verifySignature(signature, authUser.key.keyPem))
Explicitly checking for something to equal true seems unnecessary, since `httpSignature.verifySignature` has the return type `boolean`. ```diff - const httpSignatureValidated = httpSignature.verifySignature(signature, authUser.key.keyPem); - if (httpSignatureValidated === true) + if (httpSignature.verifySignature(signature, authUser.key.keyPem)) ```
Review

I want to make sure the code and check condition are clear on the signature verification step, which is common practice on sensitive code. Additionally, while the typings do specify that the return type should be boolean, this is not checked at runtime, meaning that if a truthy-type is returned (for example, a non-empty string, which is common in the codebase), the check would pass. I enforce the value to be strictly the boolean value true instead of being truthy. This is mostly for future-proofing, for example, in case of replacement of the http-signature package in the future, as the dependency itself looks fairly sound on that point.

I want to make sure the code and check condition are clear on the signature verification step, which is common practice on sensitive code. Additionally, while the typings do specify that the return type should be boolean, this is not checked at runtime, meaning that if a truthy-type is returned (for example, a non-empty string, which is common in the codebase), the check would pass. I enforce the value to be strictly the boolean value `true` instead of being truthy. This is mostly for future-proofing, for example, in case of replacement of the `http-signature` package in the future, as the dependency itself looks fairly sound on that point.
return 'valid';
// Otherwise, fail.
return 'invalid';
}

View file

@ -82,7 +82,6 @@ export default async (ctx: Router.RouterContext) => {
// index page
const rendered = renderOrderedCollection(partOf, user.followersCount, `${partOf}?page=true`);
ctx.body = renderActivity(rendered);
ctx.set('Cache-Control', 'public, max-age=180');
setResponseType(ctx);
}
};

View file

@ -82,7 +82,6 @@ export default async (ctx: Router.RouterContext) => {
// index page
const rendered = renderOrderedCollection(partOf, user.followingCount, `${partOf}?page=true`);
ctx.body = renderActivity(rendered);
ctx.set('Cache-Control', 'public, max-age=180');
setResponseType(ctx);
}
};

View file

@ -90,7 +90,6 @@ export default async (ctx: Router.RouterContext) => {
`${partOf}?page=true&since_id=000000000000000000000000`,
);
ctx.body = renderActivity(rendered);
ctx.set('Cache-Control', 'public, max-age=180');
setResponseType(ctx);
}
};

View file

@ -1,5 +1,5 @@
import { IsNull } from 'typeorm';
import { ILocalUser } from '@/models/entities/user.js';
import { ILocalUser, User } from '@/models/entities/user.js';
import { Users } from '@/models/index.js';
import { getSystemUser } from './system-user.js';
@ -17,3 +17,7 @@ export async function getInstanceActor(): Promise<ILocalUser> {
return instanceActor;
}
export function isInstanceActor(user: User): boolean {
return user.username === ACTOR_USERNAME && user.host === null;
}