fix(backend): validate the S3 endpoint host before the restore download (#1383)
* fix(backend): validate the S3 endpoint host before the restore download downloadFileFromS3() built an S3StorageAdapter and called .download() directly, skipping the isHostAllowed() private-IP/DNS-rebinding guard that testConnection() applies elsewhere — an admin with backup.restore could point the configured S3 endpoint at an internal/metadata address for unauthenticated egress via the server. * 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. Reuses the same pinnedRequestOptions() primitive webhookDeliveryWorker.js already uses, wired into the S3Client's requestHandler. --------- Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
b798d8e4c1
commit
ec03089d57
@@ -0,0 +1,146 @@
|
||||
/**
|
||||
* DNS-rebinding follow-up to the blind-SSRF fix in restoreServiceS3Ssrf.test.js
|
||||
* (GHSA-vm2x-c628-3cx5).
|
||||
*
|
||||
* isHostAllowed()/validateExternalUrlAsync() 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 and emailWebhookTransport.js
|
||||
* use for outbound HTTP) 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();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,109 @@
|
||||
/**
|
||||
* Blind SSRF via the restore S3 download path (GHSA-vm2x-c628-3cx5).
|
||||
*
|
||||
* downloadFileFromS3() built a bare S3StorageAdapter and called .download()
|
||||
* directly, never running the DNS-resolving isHostAllowed() guard that
|
||||
* testConnection() applies elsewhere — so an admin with backup.restore could
|
||||
* point the request-supplied S3 endpoint at an internal/metadata address for
|
||||
* unauthenticated egress via the server. `s3Config` here is fully attacker
|
||||
* controlled (POST /api/admin/restore/validate and /restore/start take it
|
||||
* straight from the request body — see routes/adminRestore.js), unlike the
|
||||
* scheduled-backup S3 endpoint, which is vetted at settings-save time.
|
||||
*/
|
||||
|
||||
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-restores3ssrf-')), 'db.sqlite',
|
||||
);
|
||||
process.env.JWT_SECRET = process.env.JWT_SECRET || 'restores3ssrf-test-secret';
|
||||
|
||||
jest.mock('dns', () => {
|
||||
const actual = jest.requireActual('dns');
|
||||
return { ...actual, promises: { ...actual.promises, lookup: jest.fn() } };
|
||||
});
|
||||
|
||||
jest.mock('../../src/services/storage/s3Storage', () =>
|
||||
jest.fn().mockImplementation(() => ({
|
||||
download: jest.fn().mockResolvedValue(undefined),
|
||||
}))
|
||||
);
|
||||
|
||||
const dns = require('dns');
|
||||
const lookup = dns.promises.lookup;
|
||||
const S3StorageAdapter = require('../../src/services/storage/s3Storage');
|
||||
const { RestoreService } = require('../../src/services/restoreService');
|
||||
|
||||
describe('downloadFileFromS3 SSRF guard (GHSA-vm2x-c628-3cx5)', () => {
|
||||
let restoreService;
|
||||
let originalNodeEnv;
|
||||
|
||||
beforeEach(() => {
|
||||
restoreService = new RestoreService();
|
||||
lookup.mockReset();
|
||||
S3StorageAdapter.mockClear();
|
||||
originalNodeEnv = process.env.NODE_ENV;
|
||||
process.env.NODE_ENV = 'production';
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
process.env.NODE_ENV = originalNodeEnv;
|
||||
});
|
||||
|
||||
it('rejects an endpoint hostname that resolves to a private/internal address before any network call', async () => {
|
||||
lookup.mockResolvedValue([{ address: '10.0.0.5', family: 4 }]);
|
||||
|
||||
await expect(
|
||||
restoreService.downloadFileFromS3(
|
||||
's3://backups/manifest.json',
|
||||
'/tmp/whatever/manifest.json',
|
||||
{ endpoint: 'evil-rebind.example.com', accessKeyId: 'k', secretAccessKey: 's' }
|
||||
)
|
||||
).rejects.toThrow(/private or internal network address/i);
|
||||
|
||||
expect(S3StorageAdapter).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('rejects an endpoint hostname that resolves to the cloud metadata address', async () => {
|
||||
lookup.mockResolvedValue([{ address: '169.254.169.254', family: 4 }]);
|
||||
|
||||
await expect(
|
||||
restoreService.downloadFileFromS3(
|
||||
's3://backups/manifest.json',
|
||||
'/tmp/whatever/manifest.json',
|
||||
{ endpoint: 'metadata-rebind.example.com', accessKeyId: 'k', secretAccessKey: 's' }
|
||||
)
|
||||
).rejects.toThrow(/private or internal network address/i);
|
||||
|
||||
expect(S3StorageAdapter).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('allows a legitimate public S3 endpoint through to download()', async () => {
|
||||
lookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }]);
|
||||
|
||||
await restoreService.downloadFileFromS3(
|
||||
's3://backups/manifest.json',
|
||||
'/tmp/whatever/manifest.json',
|
||||
{ endpoint: 's3.example-cdn.com', accessKeyId: 'k', secretAccessKey: 's' }
|
||||
);
|
||||
|
||||
expect(S3StorageAdapter).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('does not require the guard outside production (dev MinIO stays usable), but still downloads', async () => {
|
||||
process.env.NODE_ENV = 'development';
|
||||
lookup.mockResolvedValue([{ address: '10.0.0.5', family: 4 }]); // would be rejected in prod
|
||||
|
||||
await restoreService.downloadFileFromS3(
|
||||
's3://backups/manifest.json',
|
||||
'/tmp/whatever/manifest.json',
|
||||
{ endpoint: 'localhost:9000', accessKeyId: 'k', secretAccessKey: 's' }
|
||||
);
|
||||
|
||||
expect(lookup).not.toHaveBeenCalled();
|
||||
expect(S3StorageAdapter).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
@@ -1628,10 +1628,44 @@ END $$;`
|
||||
throw new Error('Invalid S3 URL format');
|
||||
}
|
||||
|
||||
// SSRF guard: this method calls S3StorageAdapter.download() directly
|
||||
// rather than going through testConnection(), so it must re-run the same
|
||||
// DNS-resolving host check testConnection() applies — otherwise an
|
||||
// admin-configured S3 endpoint could point at a private/internal or
|
||||
// cloud-metadata address for unauthenticated egress via the server.
|
||||
// Prod-only, matching S3StorageAdapter's own gate (dev points at
|
||||
// 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.
|
||||
// validateExternalUrlAsync's resolved addresses get pinned into the
|
||||
// S3Client's requestHandler via pinnedRequestOptions — the same
|
||||
// primitive webhookDeliveryWorker.js/emailWebhookTransport.js use for
|
||||
// outbound HTTP — so the connection can only land on an address that
|
||||
// was actually vetted.
|
||||
let pinnedAgents = {};
|
||||
if (process.env.NODE_ENV === 'production' && s3Config && s3Config.endpoint) {
|
||||
const { validateExternalUrlAsync } = require('../utils/networkValidation');
|
||||
const { pinnedRequestOptions } = require('../utils/pinnedRequest');
|
||||
const endpointUrl = /^https?:\/\//.test(s3Config.endpoint)
|
||||
? s3Config.endpoint
|
||||
: `https://${s3Config.endpoint}`;
|
||||
const urlCheck = await validateExternalUrlAsync(endpointUrl);
|
||||
if (!urlCheck.valid) {
|
||||
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 s3Client = new S3StorageAdapter({
|
||||
...s3Config,
|
||||
bucket
|
||||
bucket,
|
||||
...pinnedAgents
|
||||
});
|
||||
|
||||
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.connectionTimeout=120000] - Ms to acquire+establish a socket
|
||||
* @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) {
|
||||
super();
|
||||
@@ -90,7 +94,13 @@ class S3StorageAdapter extends stream.EventEmitter {
|
||||
// into a bounded failure, not to enforce latency targets.
|
||||
requestHandler: {
|
||||
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 })
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
Reference in New Issue
Block a user