fix(backend): validate the S3 endpoint host before the restore download (stable) (#1391)

* 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.

Backport of 89c6a673bf from main
(GHSA-vm2x-c628-3cx5) to stable; cherry-picked cleanly, no adaptation
needed.

* 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.

---------

Co-authored-by: Paul Nothaft <[email protected]>

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();
+  });
+});

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);
+  });
+});

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);

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 })
       }
     };
     

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,
+};

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 };