merge: Allow port ranges in allowedPrivateIps (!1025)

View MR for information: https://activitypub.software/TransFem-org/Sharkey/-/merge_requests/1025

Approved-by: dakkar <dakkar@thenautilus.net>
Approved-by: Marie <github@yuugi.dev>

.config/ci.yml

@@ -321,9 +321,24 @@ attachLdSignatureForRelays: true
 # For security reasons, uploading attachments from the intranet is prohibited,
 # but exceptions can be made from the following settings. Default value is "undefined".
 # Read changelog to learn more (Improvements of 12.90.0 (2021/09/04)).
-#allowedPrivateNetworks: [
+# Some example configurations:
-#  '127.0.0.1/32'
+#allowedPrivateNetworks:
-#]
+#  # Allow connections to 127.0.0.1 on any port
+#  - '127.0.0.1/32'
+#  # Allow connections to 127.0.0.* on any port
+#  - '127.0.0.1/24'
+#  # Allow connections to 127.0.0.1 on any port
+#  - '127.0.0.1'
+#  # Allow connections to 127.0.0.1 on any port
+#  - network: '127.0.0.1'
+#  # Allow connections to 127.0.0.1 on port 80
+#  - network: '127.0.0.1'
+#    ports: [80]
+#  # Allow connections to 127.0.0.1 on port 80 or 443
+#  - network: '127.0.0.1'
+#    ports:
+#      - 80
+#      - 443
 
 #customMOTD: ['Hello World', 'The sharks rule all', 'Shonks']
 

.config/cypress-devcontainer.yml

@@ -269,9 +269,27 @@ proxyRemoteFiles: true
 # Sign to ActivityPub GET request (default: true)
 signToActivityPubGet: true
 
-allowedPrivateNetworks: [
+# For security reasons, uploading attachments from the intranet is prohibited,
-  '127.0.0.1/32'
+# but exceptions can be made from the following settings. Default value is "undefined".
-]
+# Read changelog to learn more (Improvements of 12.90.0 (2021/09/04)).
+# Some example configurations:
+allowedPrivateNetworks:
+  # Allow connections to 127.0.0.1 on any port
+  - '127.0.0.1/32'
+#  # Allow connections to 127.0.0.* on any port
+#  - '127.0.0.1/24'
+#  # Allow connections to 127.0.0.1 on any port
+#  - '127.0.0.1'
+#  # Allow connections to 127.0.0.1 on any port
+#  - network: '127.0.0.1'
+#  # Allow connections to 127.0.0.1 on port 80
+#  - network: '127.0.0.1'
+#    ports: [80]
+#  # Allow connections to 127.0.0.1 on port 80 or 443
+#  - network: '127.0.0.1'
+#    ports:
+#      - 80
+#      - 443
 
 # 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.

.config/docker_example.yml

@@ -378,9 +378,24 @@ attachLdSignatureForRelays: true
 # For security reasons, uploading attachments from the intranet is prohibited,
 # but exceptions can be made from the following settings. Default value is "undefined".
 # Read changelog to learn more (Improvements of 12.90.0 (2021/09/04)).
-#allowedPrivateNetworks: [
+# Some example configurations:
-#  '127.0.0.1/32'
+#allowedPrivateNetworks:
-#]
+#  # Allow connections to 127.0.0.1 on any port
+#  - '127.0.0.1/32'
+#  # Allow connections to 127.0.0.* on any port
+#  - '127.0.0.1/24'
+#  # Allow connections to 127.0.0.1 on any port
+#  - '127.0.0.1'
+#  # Allow connections to 127.0.0.1 on any port
+#  - network: '127.0.0.1'
+#  # Allow connections to 127.0.0.1 on port 80
+#  - network: '127.0.0.1'
+#    ports: [80]
+#  # Allow connections to 127.0.0.1 on port 80 or 443
+#  - network: '127.0.0.1'
+#    ports:
+#      - 80
+#      - 443
 
 #customMOTD: ['Hello World', 'The sharks rule all', 'Shonks']
 

.config/example.yml

@@ -381,9 +381,24 @@ attachLdSignatureForRelays: true
 # For security reasons, uploading attachments from the intranet is prohibited,
 # but exceptions can be made from the following settings. Default value is "undefined".
 # Read changelog to learn more (Improvements of 12.90.0 (2021/09/04)).
-#allowedPrivateNetworks: [
+# Some example configurations:
-#  '127.0.0.1/32'
+#allowedPrivateNetworks:
-#]
+#  # Allow connections to 127.0.0.1 on any port
+#  - '127.0.0.1/32'
+#  # Allow connections to 127.0.0.* on any port
+#  - '127.0.0.1/24'
+#  # Allow connections to 127.0.0.1 on any port
+#  - '127.0.0.1'
+#  # Allow connections to 127.0.0.1 on any port
+#  - network: '127.0.0.1'
+#  # Allow connections to 127.0.0.1 on port 80
+#  - network: '127.0.0.1'
+#    ports: [80]
+#  # Allow connections to 127.0.0.1 on port 80 or 443
+#  - network: '127.0.0.1'
+#    ports:
+#      - 80
+#      - 443
 
 #customMOTD: ['Hello World', 'The sharks rule all', 'Shonks']
 

packages/backend/src/config.ts

@@ -8,9 +8,11 @@ import { fileURLToPath } from 'node:url';
 import { dirname, resolve } from 'node:path';
 import * as yaml from 'js-yaml';
 import { globSync } from 'glob';
+import ipaddr from 'ipaddr.js';
 import type * as Sentry from '@sentry/node';
 import type * as SentryVue from '@sentry/vue';
 import type { RedisOptions } from 'ioredis';
+import type { IPv4, IPv6 } from 'ipaddr.js';
 
 type RedisOptionsSource = Partial<RedisOptions> & {
 	host?: string;
@@ -82,7 +84,7 @@ type Source = {
 	proxySmtp?: string;
 	proxyBypassHosts?: string[];
 
-	allowedPrivateNetworks?: string[];
+	allowedPrivateNetworks?: PrivateNetworkSource[];
 	disallowExternalApRedirect?: boolean;
 
 	maxFileSize?: number;
@@ -152,6 +154,60 @@ type Source = {
 	}
 };
 
+export type PrivateNetworkSource = string | { network?: string, ports?: number[] };
+
+export type PrivateNetwork = {
+	/**
+	 * CIDR IP/netmask definition of the IP range to match.
+	 */
+	cidr: CIDR;
+
+	/**
+	 * List of ports to match.
+	 * If undefined, then all ports match.
+	 * If empty, then NO ports match.
+	 */
+	ports?: number[];
+};
+
+export type CIDR = [ip: IPv4 | IPv6, prefixLength: number];
+
+export function parsePrivateNetworks(patterns: PrivateNetworkSource[]): PrivateNetwork[];
+export function parsePrivateNetworks(patterns: undefined): undefined;
+export function parsePrivateNetworks(patterns: PrivateNetworkSource[] | undefined): PrivateNetwork[] | undefined;
+export function parsePrivateNetworks(patterns: PrivateNetworkSource[] | undefined): PrivateNetwork[] | undefined {
+	if (!patterns) return undefined;
+	return patterns
+		.map(e => {
+			if (typeof(e) === 'string') {
+				const cidr = parseIpOrMask(e);
+				if (cidr) {
+					return { cidr } satisfies PrivateNetwork;
+				}
+			} else if (e.network) {
+				const cidr = parseIpOrMask(e.network);
+				if (cidr) {
+					return { cidr, ports: e.ports } satisfies PrivateNetwork;
+				}
+			}
+
+			console.warn('[config] Skipping invalid entry in allowedPrivateNetworks: ', e);
+			return null;
+		})
+		.filter(p => p != null);
+}
+
+function parseIpOrMask(ipOrMask: string): CIDR | null {
+	if (ipaddr.isValidCIDR(ipOrMask)) {
+		return ipaddr.parseCIDR(ipOrMask);
+	}
+	if (ipaddr.isValid(ipOrMask)) {
+		const ip = ipaddr.parse(ipOrMask);
+		return [ip, 32];
+	}
+	return null;
+}
+
 export type Config = {
 	url: string;
 	port: number;
@@ -190,7 +246,7 @@ export type Config = {
 	proxy: string | undefined;
 	proxySmtp: string | undefined;
 	proxyBypassHosts: string[] | undefined;
-	allowedPrivateNetworks: string[] | undefined;
+	allowedPrivateNetworks: PrivateNetwork[] | undefined;
 	disallowExternalApRedirect: boolean;
 	maxFileSize: number;
 	maxNoteLength: number;
@@ -382,7 +438,7 @@ export function loadConfig(): Config {
 		proxy: config.proxy,
 		proxySmtp: config.proxySmtp,
 		proxyBypassHosts: config.proxyBypassHosts,
-		allowedPrivateNetworks: config.allowedPrivateNetworks,
+		allowedPrivateNetworks: parsePrivateNetworks(config.allowedPrivateNetworks),
 		disallowExternalApRedirect: config.disallowExternalApRedirect ?? false,
 		maxFileSize: config.maxFileSize ?? 262144000,
 		maxNoteLength: config.maxNoteLength ?? 3000,

packages/backend/src/core/HttpRequestService.ts

@@ -12,7 +12,7 @@ import fetch from 'node-fetch';
 import { HttpProxyAgent, HttpsProxyAgent } from 'hpagent';
 import { Inject, Injectable } from '@nestjs/common';
 import { DI } from '@/di-symbols.js';
-import type { Config } from '@/config.js';
+import type { Config, PrivateNetwork } from '@/config.js';
 import { StatusError } from '@/misc/status-error.js';
 import { bindThis } from '@/decorators.js';
 import { validateContentTypeSetAsActivityPub } from '@/core/activitypub/misc/validator.js';
@@ -20,12 +20,36 @@ import type { IObject, IObjectWithId } from '@/core/activitypub/type.js';
 import { ApUtilityService } from './activitypub/ApUtilityService.js';
 import type { Response } from 'node-fetch';
 import type { URL } from 'node:url';
+import type { Socket } from 'node:net';
 
 export type HttpRequestSendOptions = {
 	throwErrorWhenResponseNotOk: boolean;
 	validators?: ((res: Response) => void)[];
 };
 
+export function isPrivateIp(allowedPrivateNetworks: PrivateNetwork[] | undefined, ip: string, port?: number): boolean {
+	const parsedIp = ipaddr.parse(ip);
+
+	for (const { cidr, ports } of allowedPrivateNetworks ?? []) {
+		if (cidr[0].kind() === parsedIp.kind() && parsedIp.match(cidr)) {
+			if (ports == null || (port != null && ports.includes(port))) {
+				return false;
+			}
+		}
+	}
+
+	return parsedIp.range() !== 'unicast';
+}
+
+export function validateSocketConnect(allowedPrivateNetworks: PrivateNetwork[] | undefined, socket: Socket): void {
+	const address = socket.remoteAddress;
+	if (address && ipaddr.isValid(address)) {
+		if (isPrivateIp(allowedPrivateNetworks, address, socket.remotePort)) {
+			socket.destroy(new Error(`Blocked address: ${address}`));
+		}
+	}
+}
+
 declare module 'node:http' {
 	interface Agent {
 		createConnection(options: net.NetConnectOpts, callback?: (err: unknown, stream: net.Socket) => void): net.Socket;
@@ -44,31 +68,12 @@ class HttpRequestServiceAgent extends http.Agent {
 	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)) {
+					validateSocketConnect(this.config.allowedPrivateNetworks, socket);
-						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 {
@@ -83,31 +88,12 @@ class HttpsRequestServiceAgent extends https.Agent {
 	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)) {
+					validateSocketConnect(this.config.allowedPrivateNetworks, socket);
-						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()

packages/backend/test/unit/core/HttpRequestService.ts

@@ -0,0 +1,113 @@
+/*
+ * SPDX-FileCopyrightText: hazelnoot and other Sharkey contributors
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+
+import { jest } from '@jest/globals';
+import type { Mock } from 'jest-mock';
+import type { PrivateNetwork } from '@/config.js';
+import type { Socket } from 'net';
+import { HttpRequestService, isPrivateIp, validateSocketConnect } from '@/core/HttpRequestService.js';
+import { parsePrivateNetworks } from '@/config.js';
+
+describe(HttpRequestService, () => {
+	let allowedPrivateNetworks: PrivateNetwork[] | undefined;
+
+	beforeEach(() => {
+		allowedPrivateNetworks = parsePrivateNetworks([
+			'10.0.0.1/32',
+			{ network: '127.0.0.1/32', ports: [1] },
+			{ network: '127.0.0.1/32', ports: [3, 4, 5] },
+		]);
+	});
+
+	describe('isPrivateIp', () => {
+		it('should return false when ip public', () => {
+			const result = isPrivateIp(allowedPrivateNetworks, '74.125.127.100', 80);
+			expect(result).toBeFalsy();
+		});
+
+		it('should return false when ip private and port matches', () => {
+			const result = isPrivateIp(allowedPrivateNetworks, '127.0.0.1', 1);
+			expect(result).toBeFalsy();
+		});
+
+		it('should return false when ip private and all ports undefined', () => {
+			const result = isPrivateIp(allowedPrivateNetworks, '10.0.0.1', undefined);
+			expect(result).toBeFalsy();
+		});
+
+		it('should return true when ip private and no ports specified', () => {
+			const result = isPrivateIp(allowedPrivateNetworks, '10.0.0.2', 80);
+			expect(result).toBeTruthy();
+		});
+
+		it('should return true when ip private and port does not match', () => {
+			const result = isPrivateIp(allowedPrivateNetworks, '127.0.0.1', 80);
+			expect(result).toBeTruthy();
+		});
+
+		it('should return true when ip private and port is null but ports are specified', () => {
+			const result = isPrivateIp(allowedPrivateNetworks, '127.0.0.1', undefined);
+			expect(result).toBeTruthy();
+		});
+	});
+
+	describe('validateSocketConnect', () => {
+		let fakeSocket: Socket;
+		let fakeSocketMutable: {
+			remoteAddress: string | undefined;
+			remotePort: number | undefined;
+			destroy: Mock<(error?: Error) => void>;
+		};
+
+		beforeEach(() => {
+			fakeSocketMutable = {
+				remoteAddress: '74.125.127.100',
+				remotePort: 80,
+				destroy: jest.fn<(error?: Error) => void>(),
+			};
+			fakeSocket = fakeSocketMutable as unknown as Socket;
+		});
+
+		it('should accept when IP is empty', () => {
+			fakeSocketMutable.remoteAddress = undefined;
+
+			validateSocketConnect(allowedPrivateNetworks, fakeSocket);
+
+			expect(fakeSocket.destroy).not.toHaveBeenCalled();
+		});
+
+		it('should accept when IP is invalid', () => {
+			fakeSocketMutable.remoteAddress = 'AB939ajd9jdajsdja8jj';
+
+			validateSocketConnect(allowedPrivateNetworks, fakeSocket);
+
+			expect(fakeSocket.destroy).not.toHaveBeenCalled();
+		});
+
+		it('should accept when IP is valid', () => {
+			validateSocketConnect(allowedPrivateNetworks, fakeSocket);
+
+			expect(fakeSocket.destroy).not.toHaveBeenCalled();
+		});
+
+		it('should accept when IP is private and port match', () => {
+			fakeSocketMutable.remoteAddress = '127.0.0.1';
+			fakeSocketMutable.remotePort = 1;
+
+			validateSocketConnect(allowedPrivateNetworks, fakeSocket);
+
+			expect(fakeSocket.destroy).not.toHaveBeenCalled();
+		});
+
+		it('should reject when IP is private and port no match', () => {
+			fakeSocketMutable.remoteAddress = '127.0.0.1';
+			fakeSocketMutable.remotePort = 2;
+
+			validateSocketConnect(allowedPrivateNetworks, fakeSocket);
+
+			expect(fakeSocket.destroy).toHaveBeenCalled();
+		});
+	});
+});