Skip to content

Commit 894a62e

Browse files
committed
fix(security): harden protocol-download SSRF guard
Address the commit-review follow-ups: - TOCTOU/DNS-rebinding: pin the socket to the validated IP via a custom lookup on Node http/https (hostname still used for Host header + TLS SNI), instead of letting net.fetch re-resolve after validation. - Range coverage: classify resolved addresses with ipaddr.js and allow only public unicast, rejecting all special-use IPv4/IPv6 ranges (CGNAT, reserved, multicast, IPv6 ULA, IPv4-mapped, ...) — replaces the bespoke checks. - Redirects are still not followed (Node http.get does not auto-follow).
1 parent 5f74495 commit 894a62e

4 files changed

Lines changed: 61 additions & 37 deletions

File tree

.eslintrc.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,8 @@
123123
"reduxstore",
124124
"renderer",
125125
"utils",
126-
"loopback"
126+
"loopback",
127+
"unicast"
127128
],
128129
"skipIfMatch": [
129130
"http(s)?://[^s]*",

package-lock.json

Lines changed: 10 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@
128128
"archiver": "^4.0.1",
129129
"d3-force": "~3.0.0",
130130
"electron-devtools-installer": "^4.0.0",
131+
"ipaddr.js": "~2.4.0",
131132
"path-browserify": "~1.0.1"
132133
},
133134
"overrides": {

src/main/protocolDownload.js

Lines changed: 48 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,43 +1,60 @@
1-
import { app, net } from 'electron';
1+
import { app } from 'electron';
22
import fs from 'node:fs';
33
import path from 'node:path';
4+
import http from 'node:http';
5+
import https from 'node:https';
46
import dns from 'node:dns';
5-
import { isIP } from 'node:net';
67
import { randomUUID } from 'node:crypto';
8+
import ipaddr from 'ipaddr.js';
79

8-
const isPrivateIPv4 = (ip) => {
9-
const [a, b] = ip.split('.').map(Number);
10-
if (a === 0 || a === 127 || a === 10) { return true; } // unspecified, loopback, private
11-
if (a === 172 && b >= 16 && b <= 31) { return true; } // private
12-
if (a === 192 && b === 168) { return true; } // private
13-
if (a === 169 && b === 254) { return true; } // link-local
14-
return false;
15-
};
16-
17-
const isPrivateIPv6 = (ip) => {
18-
const addr = ip.toLowerCase();
19-
if (addr === '::1' || addr === '::') { return true; } // loopback, unspecified
20-
const mapped = addr.match(/::ffff:(\d+\.\d+\.\d+\.\d+)$/); // IPv4-mapped
21-
if (mapped) { return isPrivateIPv4(mapped[1]); }
22-
if (/^f[cd]/.test(addr)) { return true; } // unique-local fc00::/7
23-
if (/^fe[89ab]/.test(addr)) { return true; } // link-local fe80::/10
24-
return false;
10+
// Only ordinary public unicast addresses are allowed; every special-use range
11+
// (loopback, private, link-local, CGNAT, reserved, multicast, IPv6 ULA, ...) is
12+
// rejected. ipaddr.process() also unmaps IPv4-in-IPv6 before classifying.
13+
const isPublicAddress = (address) => {
14+
try {
15+
return ipaddr.process(address).range() === 'unicast';
16+
} catch {
17+
return false;
18+
}
2519
};
2620

27-
const isBlockedAddress = (address) => {
28-
const kind = isIP(address);
29-
if (kind === 4) { return isPrivateIPv4(address); }
30-
if (kind === 6) { return isPrivateIPv6(address); }
31-
return true; // not a resolvable IP literal: refuse
32-
};
21+
const fetchToBuffer = (parsed, pinned) => new Promise((resolve, reject) => {
22+
const lib = parsed.protocol === 'https:' ? https : http;
23+
const request = lib.get(
24+
parsed.href,
25+
{
26+
// Pin the socket to the address we already validated, so a DNS rebind
27+
// between validation and connection cannot point us at an internal host.
28+
// The hostname is still used for the Host header and TLS SNI.
29+
lookup: (_hostname, _options, callback) => callback(null, pinned.address, pinned.family),
30+
timeout: 60000,
31+
},
32+
(response) => {
33+
const { statusCode } = response;
34+
// Node http does not follow redirects; any non-200 (incl. a 3xx that
35+
// could point at a blocked host) is treated as a failure.
36+
if (statusCode !== 200) {
37+
response.resume();
38+
reject(new Error(`Unexpected response status ${statusCode}`));
39+
return;
40+
}
41+
const chunks = [];
42+
response.on('data', (chunk) => chunks.push(chunk));
43+
response.on('end', () => resolve(Buffer.concat(chunks)));
44+
response.on('error', reject);
45+
},
46+
);
47+
request.on('timeout', () => request.destroy(new Error('Protocol download timed out')));
48+
request.on('error', reject);
49+
});
3350

3451
// Downloads a remote protocol to a temp file. Runs in the main process, where
3552
// fetch is not subject to the renderer's CORS policy. Returns the temp path,
3653
// which is inside app temp so the extract step's path guard permits it.
3754
//
38-
// SSRF guard: the URL is user-supplied, so we resolve the host and refuse
39-
// private/loopback/link-local targets, and reject redirects (which could
40-
// otherwise bounce a public host to an internal address).
55+
// SSRF guard: the URL is user-supplied, so we resolve the host, refuse any
56+
// non-public address, and pin the connection to the validated IP (defeating
57+
// DNS rebinding). Redirects are not followed.
4158
const downloadProtocol = async (url) => {
4259
let parsed;
4360
try {
@@ -50,16 +67,11 @@ const downloadProtocol = async (url) => {
5067
}
5168

5269
const addresses = await dns.promises.lookup(parsed.hostname, { all: true });
53-
if (addresses.length === 0 || addresses.some(({ address }) => isBlockedAddress(address))) {
54-
throw new Error('Refusing to download from a private, loopback, or link-local address.');
55-
}
56-
57-
const response = await net.fetch(parsed.href, { redirect: 'error' });
58-
if (!response.ok) {
59-
throw new Error(`Unexpected response status ${response.status}`);
70+
if (addresses.length === 0 || addresses.some(({ address }) => !isPublicAddress(address))) {
71+
throw new Error('Refusing to download from a non-public address.');
6072
}
6173

62-
const buffer = Buffer.from(await response.arrayBuffer());
74+
const buffer = await fetchToBuffer(parsed, addresses[0]);
6375
const destination = path.join(app.getPath('temp'), `${randomUUID()}.netcanvas`);
6476
await fs.promises.writeFile(destination, buffer);
6577
return destination;

0 commit comments

Comments
 (0)