diff --git a/backend/__tests__/services/restoreServiceS3Pinning.test.js b/backend/__tests__/services/restoreServiceS3Pinning.test.js new file mode 100644 index 00000000..a1621ffd --- /dev/null +++ b/backend/__tests__/services/restoreServiceS3Pinning.test.js @@ -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(); + }); +}); diff --git a/backend/__tests__/services/restoreServiceS3Ssrf.test.js b/backend/__tests__/services/restoreServiceS3Ssrf.test.js new file mode 100644 index 00000000..b8eef873 --- /dev/null +++ b/backend/__tests__/services/restoreServiceS3Ssrf.test.js @@ -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); + }); +}); diff --git a/backend/src/services/restoreService.js b/backend/src/services/restoreService.js index 70e7909c..71f8c15a 100644 --- a/backend/src/services/restoreService.js +++ b/backend/src/services/restoreService.js @@ -1559,10 +1559,42 @@ 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. + // 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) { + const { validateExternalUrlWithAddresses } = require('../utils/networkValidation'); + const { pinnedRequestOptions } = require('../utils/pinnedRequest'); + const endpointUrl = /^https?:\/\//.test(s3Config.endpoint) + ? s3Config.endpoint + : `https://${s3Config.endpoint}`; + const urlCheck = await validateExternalUrlWithAddresses(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); diff --git a/backend/src/services/storage/s3Storage.js b/backend/src/services/storage/s3Storage.js index d25215ab..cd71673a 100644 --- a/backend/src/services/storage/s3Storage.js +++ b/backend/src/services/storage/s3Storage.js @@ -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 }) } }; diff --git a/backend/src/utils/networkValidation.js b/backend/src/utils/networkValidation.js index 8ad21af7..1612b6dc 100644 --- a/backend/src/utils/networkValidation.js +++ b/backend/src/utils/networkValidation.js @@ -244,4 +244,52 @@ async function validateExternalUrlAsync(urlString) { 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, +}; diff --git a/backend/src/utils/pinnedRequest.js b/backend/src/utils/pinnedRequest.js new file mode 100644 index 00000000..2b3750d2 --- /dev/null +++ b/backend/src/utils/pinnedRequest.js @@ -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 };