fix(backend): pin the restore S3 download to its validated DNS resolution
isHostAllowed() was check-then-connect: the AWS SDK re-resolves the endpoint hostname independently when it actually connects, so a DNS rebinding condition between the preflight check and the real connection could still reach a private/internal address. Stable didn't yet have the pinnedRequestOptions() primitive main's webhookDeliveryWorker.js uses for this, so it's ported here as utils/pinnedRequest.js, plus an additive validateExternalUrlWithAddresses() in networkValidation.js that returns the resolved address set (existing exports/behavior untouched). Both get wired into the S3Client's requestHandler for the restore download path only.
This commit is contained in:
@@ -0,0 +1,147 @@
|
|||||||
|
/**
|
||||||
|
* DNS-rebinding follow-up to the blind-SSRF fix in restoreServiceS3Ssrf.test.js
|
||||||
|
* (GHSA-vm2x-c628-3cx5).
|
||||||
|
*
|
||||||
|
* isHostAllowed()/validateExternalUrlWithAddresses() are check-then-connect
|
||||||
|
* on their own: they resolve the S3 endpoint hostname once to vet it, then
|
||||||
|
* hand a bare hostname to the AWS SDK, which resolves it AGAIN when it
|
||||||
|
* actually connects. An attacker who controls DNS for the endpoint hostname
|
||||||
|
* (or an infra DNS-rebinding condition) can answer the first lookup with a
|
||||||
|
* public IP and the second with a private/metadata one.
|
||||||
|
*
|
||||||
|
* downloadFileFromS3() now builds pinned http/https agents (pinnedRequest.js
|
||||||
|
* — the same primitive webhookDeliveryWorker.js on main uses for outbound
|
||||||
|
* HTTP; ported here since stable didn't have it yet) from the validated
|
||||||
|
* address and passes them into S3StorageAdapter, which threads them into
|
||||||
|
* the S3Client's NodeHttpHandler requestHandler. This asserts that wiring:
|
||||||
|
* the agents S3StorageAdapter receives resolve the endpoint hostname to
|
||||||
|
* ONLY the address vetted during validation, and never fall through to a
|
||||||
|
* second, real DNS lookup that a rebinding attacker could answer
|
||||||
|
* differently.
|
||||||
|
*/
|
||||||
|
|
||||||
|
const path = require('path');
|
||||||
|
const fs = require('fs');
|
||||||
|
const os = require('os');
|
||||||
|
|
||||||
|
process.env.NODE_ENV = 'test';
|
||||||
|
process.env.TEST_DATABASE_PATH = path.join(
|
||||||
|
fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-restores3pin-')), 'db.sqlite',
|
||||||
|
);
|
||||||
|
process.env.JWT_SECRET = process.env.JWT_SECRET || 'restores3pin-test-secret';
|
||||||
|
|
||||||
|
jest.mock('dns', () => {
|
||||||
|
const actual = jest.requireActual('dns');
|
||||||
|
return { ...actual, promises: { ...actual.promises, lookup: jest.fn() }, lookup: jest.fn() };
|
||||||
|
});
|
||||||
|
|
||||||
|
let capturedConfig;
|
||||||
|
jest.mock('../../src/services/storage/s3Storage', () =>
|
||||||
|
jest.fn().mockImplementation((config) => {
|
||||||
|
capturedConfig = config;
|
||||||
|
return { download: jest.fn().mockResolvedValue(undefined) };
|
||||||
|
})
|
||||||
|
);
|
||||||
|
|
||||||
|
const dns = require('dns');
|
||||||
|
const promiseLookup = dns.promises.lookup;
|
||||||
|
const S3StorageAdapter = require('../../src/services/storage/s3Storage');
|
||||||
|
const { RestoreService } = require('../../src/services/restoreService');
|
||||||
|
|
||||||
|
describe('downloadFileFromS3 DNS-rebinding pinning', () => {
|
||||||
|
let restoreService;
|
||||||
|
let originalNodeEnv;
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
restoreService = new RestoreService();
|
||||||
|
capturedConfig = undefined;
|
||||||
|
promiseLookup.mockReset();
|
||||||
|
dns.lookup.mockReset();
|
||||||
|
S3StorageAdapter.mockClear();
|
||||||
|
originalNodeEnv = process.env.NODE_ENV;
|
||||||
|
process.env.NODE_ENV = 'production';
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
process.env.NODE_ENV = originalNodeEnv;
|
||||||
|
capturedConfig?.httpAgent?.destroy();
|
||||||
|
capturedConfig?.httpsAgent?.destroy();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('passes pinned http/https agents into S3StorageAdapter built from the validated address', async () => {
|
||||||
|
promiseLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }]);
|
||||||
|
|
||||||
|
await restoreService.downloadFileFromS3(
|
||||||
|
's3://backups/manifest.json',
|
||||||
|
'/tmp/whatever/manifest.json',
|
||||||
|
{ endpoint: 'rebind.example.com', accessKeyId: 'k', secretAccessKey: 's' }
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(S3StorageAdapter).toHaveBeenCalledTimes(1);
|
||||||
|
expect(capturedConfig.httpAgent).toBeInstanceOf(require('http').Agent);
|
||||||
|
expect(capturedConfig.httpsAgent).toBeInstanceOf(require('https').Agent);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('the pinned agent never performs a second DNS lookup — rebinding to a private IP on the real resolver is ignored', async () => {
|
||||||
|
// First (validation) lookup: public IP, passes the preflight.
|
||||||
|
promiseLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }]);
|
||||||
|
// If the pinned agent ever fell through to a real lookup, this would
|
||||||
|
// hand back a private/metadata address — simulating the rebind.
|
||||||
|
dns.lookup.mockImplementation((_hostname, options, callback) => {
|
||||||
|
if (typeof options === 'function') { callback = options; options = {}; }
|
||||||
|
callback(null, ...(options?.all ? [[{ address: '169.254.169.254', family: 4 }]] : ['169.254.169.254', 4]));
|
||||||
|
});
|
||||||
|
|
||||||
|
await restoreService.downloadFileFromS3(
|
||||||
|
's3://backups/manifest.json',
|
||||||
|
'/tmp/whatever/manifest.json',
|
||||||
|
{ endpoint: 'rebind.example.com', accessKeyId: 'k', secretAccessKey: 's' }
|
||||||
|
);
|
||||||
|
|
||||||
|
const pinnedLookup = capturedConfig.httpAgent.options.lookup;
|
||||||
|
expect(typeof pinnedLookup).toBe('function');
|
||||||
|
|
||||||
|
const result = await new Promise((resolve, reject) => {
|
||||||
|
pinnedLookup('rebind.example.com', {}, (err, address, family) => {
|
||||||
|
if (err) return reject(err);
|
||||||
|
resolve({ address, family });
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
// Only the address vetted during validation is ever handed back —
|
||||||
|
// never the private address the real resolver would now answer with.
|
||||||
|
expect(result).toEqual({ address: '93.184.216.34', family: 4 });
|
||||||
|
expect(dns.lookup).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('rejects a lookup for any hostname other than the one that was validated', async () => {
|
||||||
|
promiseLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }]);
|
||||||
|
|
||||||
|
await restoreService.downloadFileFromS3(
|
||||||
|
's3://backups/manifest.json',
|
||||||
|
'/tmp/whatever/manifest.json',
|
||||||
|
{ endpoint: 'rebind.example.com', accessKeyId: 'k', secretAccessKey: 's' }
|
||||||
|
);
|
||||||
|
|
||||||
|
const pinnedLookup = capturedConfig.httpAgent.options.lookup;
|
||||||
|
|
||||||
|
await expect(new Promise((resolve, reject) => {
|
||||||
|
pinnedLookup('attacker-controlled.example', {}, (err, address) => {
|
||||||
|
if (err) return reject(err);
|
||||||
|
resolve(address);
|
||||||
|
});
|
||||||
|
})).rejects.toThrow(/hostname changed/i);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not pin agents when no custom endpoint is configured (default AWS, no rebinding surface)', async () => {
|
||||||
|
await restoreService.downloadFileFromS3(
|
||||||
|
's3://backups/manifest.json',
|
||||||
|
'/tmp/whatever/manifest.json',
|
||||||
|
{ accessKeyId: 'k', secretAccessKey: 's' }
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(promiseLookup).not.toHaveBeenCalled();
|
||||||
|
expect(capturedConfig.httpAgent).toBeUndefined();
|
||||||
|
expect(capturedConfig.httpsAgent).toBeUndefined();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -1566,20 +1566,35 @@ END $$;`
|
|||||||
// cloud-metadata address for unauthenticated egress via the server.
|
// cloud-metadata address for unauthenticated egress via the server.
|
||||||
// Prod-only, matching S3StorageAdapter's own gate (dev points at
|
// Prod-only, matching S3StorageAdapter's own gate (dev points at
|
||||||
// localhost MinIO deliberately).
|
// localhost MinIO deliberately).
|
||||||
|
//
|
||||||
|
// A boolean isHostAllowed() preflight is check-then-connect: the AWS
|
||||||
|
// SDK re-resolves the endpoint hostname on its own when it actually
|
||||||
|
// connects, so a DNS-rebinding attacker (or an infra rebinding
|
||||||
|
// condition) could answer the preflight lookup with a public address
|
||||||
|
// and the SDK's own later lookup with a private/metadata one.
|
||||||
|
// validateExternalUrlWithAddresses's resolved addresses get pinned
|
||||||
|
// into the S3Client's requestHandler via pinnedRequestOptions, so the
|
||||||
|
// connection can only land on an address that was actually vetted.
|
||||||
|
let pinnedAgents = {};
|
||||||
if (process.env.NODE_ENV === 'production' && s3Config && s3Config.endpoint) {
|
if (process.env.NODE_ENV === 'production' && s3Config && s3Config.endpoint) {
|
||||||
const { isHostAllowed } = require('../utils/networkValidation');
|
const { validateExternalUrlWithAddresses } = require('../utils/networkValidation');
|
||||||
const { hostname } = new URL(
|
const { pinnedRequestOptions } = require('../utils/pinnedRequest');
|
||||||
/^https?:\/\//.test(s3Config.endpoint) ? s3Config.endpoint : `https://${s3Config.endpoint}`
|
const endpointUrl = /^https?:\/\//.test(s3Config.endpoint)
|
||||||
);
|
? s3Config.endpoint
|
||||||
if (!(await isHostAllowed(hostname))) {
|
: `https://${s3Config.endpoint}`;
|
||||||
|
const urlCheck = await validateExternalUrlWithAddresses(endpointUrl);
|
||||||
|
if (!urlCheck.valid) {
|
||||||
throw new Error('S3 endpoint resolves to a private or internal network address');
|
throw new Error('S3 endpoint resolves to a private or internal network address');
|
||||||
}
|
}
|
||||||
|
const { httpAgent, httpsAgent } = pinnedRequestOptions(urlCheck);
|
||||||
|
pinnedAgents = { httpAgent, httpsAgent };
|
||||||
}
|
}
|
||||||
|
|
||||||
const [, bucket, key] = s3PathMatch;
|
const [, bucket, key] = s3PathMatch;
|
||||||
const s3Client = new S3StorageAdapter({
|
const s3Client = new S3StorageAdapter({
|
||||||
...s3Config,
|
...s3Config,
|
||||||
bucket
|
bucket,
|
||||||
|
...pinnedAgents
|
||||||
});
|
});
|
||||||
|
|
||||||
await s3Client.download(key, localPath);
|
await s3Client.download(key, localPath);
|
||||||
|
|||||||
@@ -42,6 +42,10 @@ class S3StorageAdapter extends stream.EventEmitter {
|
|||||||
* @param {number} [config.retryDelay=1000] - Initial retry delay in milliseconds
|
* @param {number} [config.retryDelay=1000] - Initial retry delay in milliseconds
|
||||||
* @param {number} [config.connectionTimeout=120000] - Ms to acquire+establish a socket
|
* @param {number} [config.connectionTimeout=120000] - Ms to acquire+establish a socket
|
||||||
* @param {number} [config.socketTimeout=60000] - Ms of socket inactivity before a request fails
|
* @param {number} [config.socketTimeout=60000] - Ms of socket inactivity before a request fails
|
||||||
|
* @param {http.Agent} [config.httpAgent] - Pre-built http.Agent to pin connections to a
|
||||||
|
* DNS-resolved address set (see utils/pinnedRequest). Opt-in; when omitted the SDK's
|
||||||
|
* default agent (its own DNS resolution) is used, matching prior behavior.
|
||||||
|
* @param {https.Agent} [config.httpsAgent] - Same as httpAgent, for TLS connections.
|
||||||
*/
|
*/
|
||||||
constructor(config) {
|
constructor(config) {
|
||||||
super();
|
super();
|
||||||
@@ -90,7 +94,13 @@ class S3StorageAdapter extends stream.EventEmitter {
|
|||||||
// into a bounded failure, not to enforce latency targets.
|
// into a bounded failure, not to enforce latency targets.
|
||||||
requestHandler: {
|
requestHandler: {
|
||||||
connectionTimeout: this.config.connectionTimeout,
|
connectionTimeout: this.config.connectionTimeout,
|
||||||
socketTimeout: this.config.socketTimeout
|
socketTimeout: this.config.socketTimeout,
|
||||||
|
// Opt-in DNS pinning (see utils/pinnedRequest): only set when a
|
||||||
|
// caller explicitly passes agents built from a resolved address
|
||||||
|
// set. Every other caller leaves these undefined and gets the
|
||||||
|
// SDK's default agent behavior, unchanged.
|
||||||
|
...(this.config.httpAgent && { httpAgent: this.config.httpAgent }),
|
||||||
|
...(this.config.httpsAgent && { httpsAgent: this.config.httpsAgent })
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|||||||
@@ -244,4 +244,52 @@ async function validateExternalUrlAsync(urlString) {
|
|||||||
return { valid: true, reason: 'ok' };
|
return { valid: true, reason: 'ok' };
|
||||||
}
|
}
|
||||||
|
|
||||||
module.exports = { isPrivateIP, validateExternalUrl, isHostAllowed, validateExternalUrlAsync, classifyHost };
|
/**
|
||||||
|
* Same DNS-resolving vetting as classifyHost, but also returns the exact
|
||||||
|
* addresses that were checked — the piece classifyHost intentionally
|
||||||
|
* discards. Needed by any caller that then wants to PIN its connection to
|
||||||
|
* those addresses (utils/pinnedRequest.js) rather than trust a second,
|
||||||
|
* independent resolution done later by the underlying client — closing the
|
||||||
|
* TOCTOU/DNS-rebinding gap classifyHost's own doc comment calls out as
|
||||||
|
* residual risk. Purely additive: existing classifyHost/validateExternalUrlAsync
|
||||||
|
* callers and their return shapes are untouched.
|
||||||
|
* @param {string} urlString
|
||||||
|
* @returns {Promise<{ valid: boolean, error?: string, reason: string, hostname?: string, addresses?: Array<{address: string, family: number}> }>}
|
||||||
|
*/
|
||||||
|
async function validateExternalUrlWithAddresses(urlString) {
|
||||||
|
let parsed;
|
||||||
|
try {
|
||||||
|
parsed = new URL(urlString);
|
||||||
|
} catch {
|
||||||
|
return { valid: false, error: 'Invalid URL format', reason: 'invalid' };
|
||||||
|
}
|
||||||
|
const hostname = parsed.hostname.replace(/^\[|\]$/g, '');
|
||||||
|
if (isPrivateIP(parsed.hostname)) {
|
||||||
|
return { valid: false, error: 'URL points to a private or internal network address', reason: 'private' };
|
||||||
|
}
|
||||||
|
if (net.isIP(hostname)) {
|
||||||
|
return { valid: true, reason: 'ok', hostname, addresses: [{ address: hostname, family: net.isIP(hostname) }] };
|
||||||
|
}
|
||||||
|
let addresses;
|
||||||
|
try {
|
||||||
|
addresses = await dns.lookup(parsed.hostname, { all: true });
|
||||||
|
} catch {
|
||||||
|
return { valid: false, error: 'URL points to a private or internal network address', reason: 'unresolved' };
|
||||||
|
}
|
||||||
|
if (!addresses.length) {
|
||||||
|
return { valid: false, error: 'URL points to a private or internal network address', reason: 'unresolved' };
|
||||||
|
}
|
||||||
|
if (addresses.some((a) => isPrivateIP(a.address))) {
|
||||||
|
return { valid: false, error: 'URL points to a private or internal network address', reason: 'private' };
|
||||||
|
}
|
||||||
|
return { valid: true, reason: 'ok', hostname, addresses };
|
||||||
|
}
|
||||||
|
|
||||||
|
module.exports = {
|
||||||
|
isPrivateIP,
|
||||||
|
validateExternalUrl,
|
||||||
|
isHostAllowed,
|
||||||
|
validateExternalUrlAsync,
|
||||||
|
validateExternalUrlWithAddresses,
|
||||||
|
classifyHost,
|
||||||
|
};
|
||||||
|
|||||||
@@ -0,0 +1,28 @@
|
|||||||
|
/** Axios/Node lookup: connect only to the addresses vetted for this delivery.
|
||||||
|
* Keep the original URL for Host, TLS SNI and certificate verification.
|
||||||
|
* Disable environment proxies (which would resolve the destination themselves)
|
||||||
|
* and redirects. No reusable agent/socket can carry an old DNS decision.
|
||||||
|
*/
|
||||||
|
const http = require('http');
|
||||||
|
const https = require('https');
|
||||||
|
function pinnedRequestOptions(check) {
|
||||||
|
if (!check?.valid || !check.hostname || !check.addresses?.length) {
|
||||||
|
throw new Error('A validated destination is required');
|
||||||
|
}
|
||||||
|
const addresses = check.addresses.map(({ address, family }) => ({ address, family }));
|
||||||
|
const lookup = (hostname, options, callback) => {
|
||||||
|
if (typeof options === 'function') { callback = options; options = {}; }
|
||||||
|
if (hostname !== check.hostname) return callback(new Error('Destination hostname changed'));
|
||||||
|
const family = typeof options === 'number' ? options : options?.family;
|
||||||
|
const matches = family ? addresses.filter(a => a.family === family) : addresses;
|
||||||
|
if (!matches.length) return callback(new Error('No validated address for requested family'));
|
||||||
|
if (options?.all) return callback(null, matches);
|
||||||
|
callback(null, matches[0].address, matches[0].family);
|
||||||
|
};
|
||||||
|
return {
|
||||||
|
proxy: false, maxRedirects: 0,
|
||||||
|
httpAgent: new http.Agent({ lookup, keepAlive: false }),
|
||||||
|
httpsAgent: new https.Agent({ lookup, keepAlive: false }),
|
||||||
|
};
|
||||||
|
}
|
||||||
|
module.exports = { pinnedRequestOptions };
|
||||||
Reference in New Issue
Block a user