From 5f675201f261d5db6a58d3099a190372bb2f09f0 Mon Sep 17 00:00:00 2001 From: Julia Date: Wed, 20 Nov 2024 18:20:09 -0500 Subject: Merge commit from fork * 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 * fix: primitive 2: acceptance of cross-origin alternate Co-Authored-By: Laura Hausmann * 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 * 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 * 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 * 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 Co-authored-by: Laura Hausmann Co-authored-by: syuilo <4439005+syuilo@users.noreply.github.com> --- packages/backend/src/core/HttpRequestService.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'packages/backend/src/core/HttpRequestService.ts') diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 7f3cac7c58..08e9f46b2d 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, [finalUrl]); + + return activity; } @bindThis -- cgit v1.2.3-freya From 090e9392cdb1f584af94a6fb727fba95de3b8504 Mon Sep 17 00:00:00 2001 From: rectcoordsystem <37621004+rectcoordsystem@users.noreply.github.com> Date: Thu, 21 Nov 2024 08:27:09 +0900 Subject: Merge commit from fork * fix(backend): check target IP before sending HTTP request * fix(backend): allow accessing private IP when testing * Apply suggestions from code review Co-authored-by: anatawa12 * fix(backend): lint and typecheck * fix(backend): add isLocalAddressAllowed option to getAgentByUrl and send (HttpRequestService) * fix(backend): allow fetchSummaryFromProxy, trueMail to access local addresses --------- Co-authored-by: anatawa12 Co-authored-by: syuilo <4439005+syuilo@users.noreply.github.com> --- packages/backend/src/core/DownloadService.ts | 22 ---- packages/backend/src/core/EmailService.ts | 1 + packages/backend/src/core/HttpRequestService.ts | 134 +++++++++++++++++++-- .../backend/src/server/web/UrlPreviewService.ts | 2 +- 4 files changed, 123 insertions(+), 36 deletions(-) (limited to 'packages/backend/src/core/HttpRequestService.ts') diff --git a/packages/backend/src/core/DownloadService.ts b/packages/backend/src/core/DownloadService.ts index 93f4a38246..2e78e6d877 100644 --- a/packages/backend/src/core/DownloadService.ts +++ b/packages/backend/src/core/DownloadService.ts @@ -6,7 +6,6 @@ import * as fs from 'node:fs'; import * as stream from 'node:stream/promises'; import { Inject, Injectable } from '@nestjs/common'; -import ipaddr from 'ipaddr.js'; import chalk from 'chalk'; import got, * as Got from 'got'; import { parse } from 'content-disposition'; @@ -70,13 +69,6 @@ export class DownloadService { }, enableUnixSockets: false, }).on('response', (res: Got.Response) => { - if ((process.env.NODE_ENV === 'production' || process.env.NODE_ENV === 'test') && !this.config.proxy && res.ip) { - if (this.isPrivateIp(res.ip)) { - this.logger.warn(`Blocked address: ${res.ip}`); - req.destroy(); - } - } - const contentLength = res.headers['content-length']; if (contentLength != null) { const size = Number(contentLength); @@ -139,18 +131,4 @@ export class DownloadService { cleanup(); } } - - @bindThis - private isPrivateIp(ip: string): boolean { - const parsedIp = ipaddr.parse(ip); - - for (const net of this.config.allowedPrivateNetworks ?? []) { - const cidr = ipaddr.parseCIDR(net); - if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(ipaddr.parseCIDR(net))) { - return false; - } - } - - return parsedIp.range() !== 'unicast'; - } } diff --git a/packages/backend/src/core/EmailService.ts b/packages/backend/src/core/EmailService.ts index a176474b95..da198d0e42 100644 --- a/packages/backend/src/core/EmailService.ts +++ b/packages/backend/src/core/EmailService.ts @@ -312,6 +312,7 @@ export class EmailService { Accept: 'application/json', Authorization: truemailAuthKey, }, + isLocalAddressAllowed: true, }); const json = (await res.json()) as { diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 08e9f46b2d..0ad5667049 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -6,6 +6,7 @@ import * as http from 'node:http'; import * as https from 'node:https'; import * as net from 'node:net'; +import ipaddr from 'ipaddr.js'; import CacheableLookup from 'cacheable-lookup'; import fetch from 'node-fetch'; import { HttpProxyAgent, HttpsProxyAgent } from 'hpagent'; @@ -25,8 +26,102 @@ export type HttpRequestSendOptions = { validators?: ((res: Response) => void)[]; }; +declare module 'node:http' { + interface Agent { + createConnection(options: net.NetConnectOpts, callback?: (err: unknown, stream: net.Socket) => void): net.Socket; + } +} + +class HttpRequestServiceAgent extends http.Agent { + constructor( + private config: Config, + options?: http.AgentOptions, + ) { + super(options); + } + + @bindThis + public createConnection(options: net.NetConnectOpts, callback?: (err: unknown, stream: net.Socket) => void): net.Socket { + const socket = super.createConnection(options, callback) + .on('connect', () => { + const address = socket.remoteAddress; + if (process.env.NODE_ENV === 'production') { + if (address && ipaddr.isValid(address)) { + if (this.isPrivateIp(address)) { + socket.destroy(new Error(`Blocked address: ${address}`)); + } + } + } + }); + return socket; + }; + + @bindThis + private isPrivateIp(ip: string): boolean { + const parsedIp = ipaddr.parse(ip); + + for (const net of this.config.allowedPrivateNetworks ?? []) { + const cidr = ipaddr.parseCIDR(net); + if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(ipaddr.parseCIDR(net))) { + return false; + } + } + + return parsedIp.range() !== 'unicast'; + } +} + +class HttpsRequestServiceAgent extends https.Agent { + constructor( + private config: Config, + options?: https.AgentOptions, + ) { + super(options); + } + + @bindThis + public createConnection(options: net.NetConnectOpts, callback?: (err: unknown, stream: net.Socket) => void): net.Socket { + const socket = super.createConnection(options, callback) + .on('connect', () => { + const address = socket.remoteAddress; + if (process.env.NODE_ENV === 'production') { + if (address && ipaddr.isValid(address)) { + if (this.isPrivateIp(address)) { + socket.destroy(new Error(`Blocked address: ${address}`)); + } + } + } + }); + return socket; + }; + + @bindThis + private isPrivateIp(ip: string): boolean { + const parsedIp = ipaddr.parse(ip); + + for (const net of this.config.allowedPrivateNetworks ?? []) { + const cidr = ipaddr.parseCIDR(net); + if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(ipaddr.parseCIDR(net))) { + return false; + } + } + + return parsedIp.range() !== 'unicast'; + } +} + @Injectable() export class HttpRequestService { + /** + * Get http non-proxy agent (without local address filtering) + */ + private httpNative: http.Agent; + + /** + * Get https non-proxy agent (without local address filtering) + */ + private httpsNative: https.Agent; + /** * Get http non-proxy agent */ @@ -57,19 +152,20 @@ export class HttpRequestService { lookup: false, // nativeのdns.lookupにfallbackしない }); - this.http = new http.Agent({ + const agentOption = { keepAlive: true, keepAliveMsecs: 30 * 1000, lookup: cache.lookup as unknown as net.LookupFunction, localAddress: config.outgoingAddress, - }); + }; - this.https = new https.Agent({ - keepAlive: true, - keepAliveMsecs: 30 * 1000, - lookup: cache.lookup as unknown as net.LookupFunction, - localAddress: config.outgoingAddress, - }); + this.httpNative = new http.Agent(agentOption); + + this.httpsNative = new https.Agent(agentOption); + + this.http = new HttpRequestServiceAgent(config, agentOption); + + this.https = new HttpsRequestServiceAgent(config, agentOption); const maxSockets = Math.max(256, config.deliverJobConcurrency ?? 128); @@ -104,16 +200,22 @@ export class HttpRequestService { * @param bypassProxy Allways bypass proxy */ @bindThis - public getAgentByUrl(url: URL, bypassProxy = false): http.Agent | https.Agent { + public getAgentByUrl(url: URL, bypassProxy = false, isLocalAddressAllowed = false): http.Agent | https.Agent { if (bypassProxy || (this.config.proxyBypassHosts ?? []).includes(url.hostname)) { + if (isLocalAddressAllowed) { + return url.protocol === 'http:' ? this.httpNative : this.httpsNative; + } return url.protocol === 'http:' ? this.http : this.https; } else { + if (isLocalAddressAllowed && (!this.config.proxy)) { + return url.protocol === 'http:' ? this.httpNative : this.httpsNative; + } return url.protocol === 'http:' ? this.httpAgent : this.httpsAgent; } } @bindThis - public async getActivityJson(url: string): Promise { + public async getActivityJson(url: string, isLocalAddressAllowed = false): Promise { const res = await this.send(url, { method: 'GET', headers: { @@ -121,6 +223,7 @@ export class HttpRequestService { }, timeout: 5000, size: 1024 * 256, + isLocalAddressAllowed: isLocalAddressAllowed, }, { throwErrorWhenResponseNotOk: true, validators: [validateContentTypeSetAsActivityPub], @@ -135,7 +238,7 @@ export class HttpRequestService { } @bindThis - public async getJson(url: string, accept = 'application/json, */*', headers?: Record): Promise { + public async getJson(url: string, accept = 'application/json, */*', headers?: Record, isLocalAddressAllowed = false): Promise { const res = await this.send(url, { method: 'GET', headers: Object.assign({ @@ -143,19 +246,21 @@ export class HttpRequestService { }, headers ?? {}), timeout: 5000, size: 1024 * 256, + isLocalAddressAllowed: isLocalAddressAllowed, }); return await res.json() as T; } @bindThis - public async getHtml(url: string, accept = 'text/html, */*', headers?: Record): Promise { + public async getHtml(url: string, accept = 'text/html, */*', headers?: Record, isLocalAddressAllowed = false): Promise { const res = await this.send(url, { method: 'GET', headers: Object.assign({ Accept: accept, }, headers ?? {}), timeout: 5000, + isLocalAddressAllowed: isLocalAddressAllowed, }); return await res.text(); @@ -170,6 +275,7 @@ export class HttpRequestService { headers?: Record, timeout?: number, size?: number, + isLocalAddressAllowed?: boolean, } = {}, extra: HttpRequestSendOptions = { throwErrorWhenResponseNotOk: true, @@ -183,6 +289,8 @@ export class HttpRequestService { controller.abort(); }, timeout); + const isLocalAddressAllowed = args.isLocalAddressAllowed ?? false; + const res = await fetch(url, { method: args.method ?? 'GET', headers: { @@ -191,7 +299,7 @@ export class HttpRequestService { }, body: args.body, size: args.size ?? 10 * 1024 * 1024, - agent: (url) => this.getAgentByUrl(url), + agent: (url) => this.getAgentByUrl(url, false, isLocalAddressAllowed), signal: controller.signal, }); diff --git a/packages/backend/src/server/web/UrlPreviewService.ts b/packages/backend/src/server/web/UrlPreviewService.ts index 5d493c2c46..9b5f0acd2c 100644 --- a/packages/backend/src/server/web/UrlPreviewService.ts +++ b/packages/backend/src/server/web/UrlPreviewService.ts @@ -145,6 +145,6 @@ export class UrlPreviewService { contentLengthRequired: meta.urlPreviewRequireContentLength, }); - return this.httpRequestService.getJson(`${proxy}?${queryStr}`); + return this.httpRequestService.getJson(`${proxy}?${queryStr}`, 'application/json, */*', undefined, true); } } -- cgit v1.2.3-freya From 3a6c2aa83563515b2ce02cda289b0271d992e84e Mon Sep 17 00:00:00 2001 From: かっこかり <67428053+kakkokari-gtyih@users.noreply.github.com> Date: Thu, 21 Nov 2024 12:10:02 +0900 Subject: fix(backend): fix type error(s) in security fixes (#15009) * Fix type error in security fixes (cherry picked from commit fa3cf6c2996741e642955c5e2fca8ad785e83205) * Fix error in test function calls (cherry picked from commit 1758f29364eca3cbd13dbb5c84909c93712b3b3b) * Fix style error (cherry picked from commit 23c4aa25714af145098baa7edd74c1d217e51c1a) * Fix another style error (cherry picked from commit 36af07abe28bec670aaebf9f5af5694bb582c29a) * Fix `.punyHost` misuse (cherry picked from commit 6027b516e1c82324d55d6e54d0e17cbd816feb42) * attempt to fix test: make yaml valid --------- Co-authored-by: Julia Johannesen --- packages/backend/src/core/HttpRequestService.ts | 12 ++++++------ packages/backend/src/core/RemoteUserResolveService.ts | 2 +- .../src/core/activitypub/models/ApPersonService.ts | 15 +++++++++------ .../backend/test-federation/.config/example.default.yml | 7 +++---- packages/backend/test/unit/activitypub.ts | 4 ++-- 5 files changed, 21 insertions(+), 19 deletions(-) (limited to 'packages/backend/src/core/HttpRequestService.ts') diff --git a/packages/backend/src/core/HttpRequestService.ts b/packages/backend/src/core/HttpRequestService.ts index 0ad5667049..083153940a 100644 --- a/packages/backend/src/core/HttpRequestService.ts +++ b/packages/backend/src/core/HttpRequestService.ts @@ -54,19 +54,19 @@ class HttpRequestServiceAgent extends http.Agent { } }); return socket; - }; + } @bindThis private isPrivateIp(ip: string): boolean { const parsedIp = ipaddr.parse(ip); - + for (const net of this.config.allowedPrivateNetworks ?? []) { const cidr = ipaddr.parseCIDR(net); if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(ipaddr.parseCIDR(net))) { return false; } } - + return parsedIp.range() !== 'unicast'; } } @@ -93,19 +93,19 @@ class HttpsRequestServiceAgent extends https.Agent { } }); return socket; - }; + } @bindThis private isPrivateIp(ip: string): boolean { const parsedIp = ipaddr.parse(ip); - + for (const net of this.config.allowedPrivateNetworks ?? []) { const cidr = ipaddr.parseCIDR(net); if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(ipaddr.parseCIDR(net))) { return false; } } - + return parsedIp.range() !== 'unicast'; } } diff --git a/packages/backend/src/core/RemoteUserResolveService.ts b/packages/backend/src/core/RemoteUserResolveService.ts index 678da0cfa6..098b5e1706 100644 --- a/packages/backend/src/core/RemoteUserResolveService.ts +++ b/packages/backend/src/core/RemoteUserResolveService.ts @@ -54,7 +54,7 @@ export class RemoteUserResolveService { }) as MiLocalUser; } - host = this.utilityService.punyHost(host); + host = this.utilityService.toPuny(host); if (host === this.utilityService.toPuny(this.config.host)) { this.logger.info(`return local user: ${usernameLower}`); diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts index 026ddb6ece..8590861ca0 100644 --- a/packages/backend/src/core/activitypub/models/ApPersonService.ts +++ b/packages/backend/src/core/activitypub/models/ApPersonService.ts @@ -163,13 +163,16 @@ export class ApPersonService implements OnModuleInit { } for (const collection of ['outbox', 'followers', 'following'] as (keyof IActor)[]) { - const collectionUri = getApId((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`); + const xCollection = (x as IActor)[collection]; + if (xCollection != null) { + const collectionUri = getApId(xCollection); + if (typeof collectionUri === 'string' && collectionUri.length > 0) { + if (this.utilityService.punyHost(collectionUri) !== expectHost) { + throw new Error(`invalid Actor: ${collection} has different host`); + } + } else if (collectionUri != null) { + throw new Error(`invalid Actor: wrong ${collection}`); } - } else if (collectionUri != null) { - throw new Error(`invalid Actor: wrong ${collection}`); } } diff --git a/packages/backend/test-federation/.config/example.default.yml b/packages/backend/test-federation/.config/example.default.yml index ff1760a5a6..28d51ac86e 100644 --- a/packages/backend/test-federation/.config/example.default.yml +++ b/packages/backend/test-federation/.config/example.default.yml @@ -19,7 +19,6 @@ proxyBypassHosts: - challenges.cloudflare.com proxyRemoteFiles: true signToActivityPubGet: true -allowedPrivateNetworks: [ - '127.0.0.1/32', - '172.20.0.0/16' -] +allowedPrivateNetworks: + - 127.0.0.1/32 + - 172.20.0.0/16 diff --git a/packages/backend/test/unit/activitypub.ts b/packages/backend/test/unit/activitypub.ts index 2fc08aec91..9df947982b 100644 --- a/packages/backend/test/unit/activitypub.ts +++ b/packages/backend/test/unit/activitypub.ts @@ -176,7 +176,7 @@ describe('ActivityPub', () => { resolver.register(actor.id, actor); resolver.register(post.id, post); - const note = await noteService.createNote(post.id, resolver, true); + const note = await noteService.createNote(post.id, undefined, resolver, true); assert.deepStrictEqual(note?.uri, post.id); assert.deepStrictEqual(note.visibility, 'public'); @@ -336,7 +336,7 @@ describe('ActivityPub', () => { resolver.register(actor.featured, featured); resolver.register(firstNote.id, firstNote); - const note = await noteService.createNote(firstNote.id as string, resolver); + const note = await noteService.createNote(firstNote.id as string, undefined, resolver); assert.strictEqual(note?.uri, firstNote.id); }); }); -- cgit v1.2.3-freya