From 3b88036fda871b3a1ca2e933c39fa96e37950fe6 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sun, 2 Aug 2026 21:18:43 +0200 Subject: [PATCH] =?UTF-8?q?fix(security):=20backup/restore=20hardening=20?= =?UTF-8?q?=E2=80=94=20public-dir=20DB=20dump,=20restore=20path=20allowlis?= =?UTF-8?q?t,=20gunzip=20bound,=20manifest=20keying=20(stable)=20(#962)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(security): stop caller-chosen database backup destination (GHSA-jw8m) POST /api/admin/database-backup/backup forwarded req.body straight into databaseBackupService.backup(), which merges options over config: const { destinationPath = '/backup/database', ... } = { ...config, ...options } destinationPath is not a persistable setting — the /config allowlist only accepts database_backup_* keys — so the request body was its only source. The built-in `admin` role holds backup.create but neither settings.edit nor backup.restore, so it could aim a full DB dump (admin bcrypt hashes, gallery password hashes, encrypted SMTP creds) at the PUBLIC /uploads static mount (server.js mounts it with no auth middleware) and then fetch it unauthenticated. Filed low; it is a privilege escalation to unauthenticated disclosure. Forward only the real knobs, and only when present so absent keys can't override config defaults via spread. * fix(security): backup/restore hardening — restore path allowlist, gunzip bound, manifest checksum keying (GHSA-fw4c, h652, hgp8) - adminRestore /validate + /start: constrain caller-supplied source and manifestPath to the operator-configured backup roots — the SAME set the restore wizard discovers from — so disaster recovery from a rescued mount still works, with RESTORE_ALLOWED_ROOTS as an escape hatch (GHSA-fw4c). - restoreService.decompressFile: bound the EXPANDED size and abort the pipeline when exceeded; default 50 GB, RESTORE_MAX_DECOMPRESSED_BYTES overrides (GHSA-h652). - backupManifest: BACKUP_MANIFEST_KEY upgrades new manifests to a keyed HMAC (GHSA-hgp8). Deliberately opt-in and verify-if-present — the key cannot live in the database because the database is inside the backup, so a mandatory HMAC would lock operators out of the exact disaster-recovery case this exists for. Also fixes a pre-existing bug found while testing hgp8: the checksum passed Object.keys().sort() as JSON.stringify's second argument, which is an array REPLACER (a property allowlist applied at every depth), not a key sorter. All nested keys — path, size, per-file checksum — were dropped before hashing, so the file list sat outside the integrity check entirely and a manifest path could be rewritten to ../../etc/passwd without disturbing the digest. Now hashes a recursively-canonicalized copy, with the legacy serialization accepted on validation so existing backups stay restorable. * fix(security): codex round 2 — unbreak the restore wizard, share checksum verification, guard downgrades - adminRestore: `source` is usually a SOURCE TYPE ('local'|'s3'|'upload'), not a path — restoreService branches on those literals. The containment check treated it as a path, so path.resolve('local') fell outside the backup roots and BOTH /validate and /start returned 400, blocking every normal restore. Type tokens are now excluded from the path check. - backupManifest: extracted verifyManifestChecksum() as the single source of truth for the legacy/keyed fallbacks. restoreService.performPreRestoreValidation recomputed the digest itself with the default canonical+keyed settings, which rejected EVERY backup written before this batch. It now delegates. - backupManifest: guard the algorithm downgrade — with a key configured, an attacker able to rewrite the backup store could strip checksum_algorithm, edit the manifest and recompute a plain SHA-256 that verified. Opt-in via BACKUP_MANIFEST_REQUIRE_KEYED so pre-key backups keep restoring by default. * fix(security): codex round 3 — close two manifest-verification fail-opens (GHSA-hgp8) verifyManifestChecksum returned valid for a manifest with no verification.total_checksum at all, and restoreService only called it when that field was present. Deleting the field was therefore a complete bypass of the keying work: no digest check, no downgrade guard, no BACKUP_MANIFEST_REQUIRE_KEYED. Every manifest this codebase writes stamps the field, so an absent one now fails validation, and the call site invokes the verifier unconditionally. Second fail-open: the strict-mode rejection of an unkeyed manifest was gated on `&& key`, so with BACKUP_MANIFEST_REQUIRE_KEYED=true and no BACKUP_MANIFEST_KEY configured a plain SHA-256 manifest sailed through. Strict mode is a statement about the operator's manifests, not about the host — it is exactly the fresh disaster-recovery box that lacks the secret. The rejection no longer depends on a key being present. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me (cherry picked from commit 292a5b784acd7f47099aa234c1c2ea00050fca97) --------- Co-authored-by: Paul Nothaft --- .../routes/databaseBackupDestination.test.js | 120 +++++++++ .../routes/restorePathAllowlist.test.js | 86 +++++++ .../services/backupRestoreHardening.test.js | 241 ++++++++++++++++++ backend/src/routes/adminDatabaseBackup.js | 16 +- backend/src/routes/adminRestore.js | 75 ++++++ backend/src/services/backupManifest.js | 177 ++++++++++++- backend/src/services/restoreService.js | 48 +++- 7 files changed, 744 insertions(+), 19 deletions(-) create mode 100644 backend/__tests__/routes/databaseBackupDestination.test.js create mode 100644 backend/__tests__/routes/restorePathAllowlist.test.js create mode 100644 backend/__tests__/services/backupRestoreHardening.test.js diff --git a/backend/__tests__/routes/databaseBackupDestination.test.js b/backend/__tests__/routes/databaseBackupDestination.test.js new file mode 100644 index 00000000..37573566 --- /dev/null +++ b/backend/__tests__/routes/databaseBackupDestination.test.js @@ -0,0 +1,120 @@ +/** + * Manual database backup must not honour a caller-supplied destination + * (GHSA-jw8m-43r2-jqrm). + * + * POST /api/admin/database-backup/backup forwarded req.body straight into + * databaseBackupService.backup(), which merges options over its config: + * const { destinationPath = '/backup/database', ... } = { ...config, ...options } + * `destinationPath` is not a persistable setting (the /config allowlist only + * accepts `database_backup_*` keys), so the request body was its ONLY source. + * + * The `admin` role holds backup.create but neither settings.edit nor + * backup.restore — so it could aim a full DB dump (bcrypt hashes, gallery + * password hashes, encrypted SMTP creds) at the PUBLIC /uploads static mount + * (server.js mounts it with no auth middleware) and fetch it unauthenticated. + * + * Pins that destinationPath from the body is ignored, while the legitimate + * knobs still pass through. + */ + +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-dbbackup-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'dbbackup-test-secret'; + +// Capture what the route hands the service; never run a real backup. +const mockBackup = jest.fn(async () => ({ success: true })); +jest.mock('../../src/services/databaseBackup', () => ({ + databaseBackupService: { + get isRunning() { return false; }, + backup: (...args) => mockBackup(...args), + }, +})); + +const request = require('supertest'); +const express = require('express'); +const bcrypt = require('bcrypt'); +const jwt = require('jsonwebtoken'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +describe('manual database backup destination (GHSA-jw8m)', () => { + let db; let cleanup; let app; let adminToken; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + const role = await db('roles').where({ name: 'admin' }).first(); + const r = await db('admin_users').insert({ + username: 'limited-admin', + email: 'limited-admin@example.com', + password_hash: await bcrypt.hash('Passw0rd!', 4), + role_id: role.id, + is_active: 1, + created_at: new Date(), + updated_at: new Date(), + }).returning('id'); + const id = r[0]?.id ?? r[0]; + adminToken = jwt.sign( + { id, username: 'limited-admin', type: 'admin', role: 'admin', loginTime: Date.now() }, + process.env.JWT_SECRET, + { expiresIn: '1h', issuer: 'picpeak-auth' }, + ); + + app = express(); + app.use(express.json()); + app.use('/api/admin/database-backup', require('../../src/routes/adminDatabaseBackup')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + beforeEach(() => mockBackup.mockClear()); + + it('ignores a caller-supplied destinationPath', async () => { + const res = await request(app) + .post('/api/admin/database-backup/backup') + .set('Authorization', `Bearer ${adminToken}`) + .send({ destinationPath: '/app/storage/uploads' }); + + expect(res.status).toBe(200); + // Give the fire-and-forget call a tick to land. + await new Promise((resolve) => setImmediate(resolve)); + expect(mockBackup).toHaveBeenCalled(); + const opts = mockBackup.mock.calls[0][0]; + expect(opts).not.toHaveProperty('destinationPath'); + expect(JSON.stringify(opts)).not.toContain('uploads'); + }); + + it('still forwards the legitimate backup knobs', async () => { + const res = await request(app) + .post('/api/admin/database-backup/backup') + .set('Authorization', `Bearer ${adminToken}`) + .send({ compress: false, validateIntegrity: false, destinationPath: '/tmp/evil' }); + + expect(res.status).toBe(200); + await new Promise((resolve) => setImmediate(resolve)); + const opts = mockBackup.mock.calls[0][0]; + expect(opts.compress).toBe(false); + expect(opts.validateIntegrity).toBe(false); + expect(opts).not.toHaveProperty('destinationPath'); + }); + + it('omits absent knobs entirely so service/config defaults still apply', async () => { + const res = await request(app) + .post('/api/admin/database-backup/backup') + .set('Authorization', `Bearer ${adminToken}`) + .send({}); + + expect(res.status).toBe(200); + await new Promise((resolve) => setImmediate(resolve)); + // An explicit `{compress: undefined}` would override config on spread — + // absent keys must simply not be present. + expect(mockBackup.mock.calls[0][0]).toEqual({}); + }); +}); diff --git a/backend/__tests__/routes/restorePathAllowlist.test.js b/backend/__tests__/routes/restorePathAllowlist.test.js new file mode 100644 index 00000000..fbfff4eb --- /dev/null +++ b/backend/__tests__/routes/restorePathAllowlist.test.js @@ -0,0 +1,86 @@ +/** + * Restore path containment must not break the normal restore wizard + * (GHSA-fw4c, codex round 2). + * + * `source` is usually a SOURCE TYPE, not a path: RestoreWizard posts + * 'local' | 's3' | 'upload', and restoreService.restore() branches on those + * literals before deriving a directory. The first version of the containment + * check treated `source` as a path, so path.resolve('local') landed outside + * the configured backup roots and BOTH /validate and /start returned 400 — + * blocking every normal restore. + */ + +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-restorepath-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'restorepath-test-secret'; + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +describe('restore path allowlist (GHSA-fw4c)', () => { + let db; let cleanup; let checkRestorePathsAllowed; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + // Configure a backup root so the allowlist is actually active. + for (const [key, value] of [['backup_destination_path', '/backup']]) { + const existing = await db('app_settings').where({ setting_key: key }).first(); + if (existing) { + await db('app_settings').where({ setting_key: key }).update({ setting_value: JSON.stringify(value) }); + } else { + await db('app_settings').insert({ + setting_key: key, setting_value: JSON.stringify(value), setting_type: 'backup', + }); + } + } + + ({ checkRestorePathsAllowed } = require('../../src/routes/adminRestore')._internal); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('allows the wizard\'s source TYPE tokens', async () => { + for (const source of ['local', 's3', 'upload']) { + const err = await checkRestorePathsAllowed({ + source, manifestPath: '/backup/manifests/backup-manifest-1.json', + }); + expect(err).toBeNull(); + } + }); + + it('allows an s3:// source URL', async () => { + const err = await checkRestorePathsAllowed({ + source: 's3://bucket/key/backup.tar.gz', + manifestPath: '/backup/manifests/backup-manifest-1.json', + }); + expect(err).toBeNull(); + }); + + it('still rejects a manifestPath outside the configured roots', async () => { + const err = await checkRestorePathsAllowed({ + source: 'local', manifestPath: '/etc/passwd', + }); + expect(err).toMatch(/inside a configured backup location/i); + }); + + it('still rejects a traversal manifestPath', async () => { + const err = await checkRestorePathsAllowed({ + source: 'local', manifestPath: '/backup/../etc/shadow', + }); + expect(err).toBeTruthy(); + }); + + it('accepts a real path source inside the roots', async () => { + const err = await checkRestorePathsAllowed({ + source: '/backup/run-1', manifestPath: '/backup/run-1/manifest.json', + }); + expect(err).toBeNull(); + }); +}); diff --git a/backend/__tests__/services/backupRestoreHardening.test.js b/backend/__tests__/services/backupRestoreHardening.test.js new file mode 100644 index 00000000..7f539493 --- /dev/null +++ b/backend/__tests__/services/backupRestoreHardening.test.js @@ -0,0 +1,241 @@ +/** + * Backup/restore hardening — GHSA-h652 (unbounded gunzip) and GHSA-hgp8 + * (unkeyed manifest checksum). + * + * h652: decompressFile() piped gunzip straight to disk with no expanded-size + * bound, so a small crafted .gz could fill the volume. + * + * hgp8: the manifest checksum is a plain SHA-256 — it proves the manifest was + * not corrupted, not that it is authentic. BACKUP_MANIFEST_KEY upgrades new + * manifests to a keyed HMAC. It is deliberately OPT-IN and verify-if-present: + * the key cannot live in the database (the database is inside the backup), so + * a mandatory HMAC would lock an operator out of the exact disaster-recovery + * case this system exists for. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); +const zlib = require('zlib'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-bkharden-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'bkharden-test-secret'; + +const { restoreService } = require('../../src/services/restoreService'); +const backupManifest = require('../../src/services/backupManifest'); + +describe('decompressFile expanded-size bound (GHSA-h652)', () => { + let dir; + + beforeAll(() => { dir = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-gz-')); }); + afterAll(() => { fs.rmSync(dir, { recursive: true, force: true }); }); + afterEach(() => { delete process.env.RESTORE_MAX_DECOMPRESSED_BYTES; }); + + it('aborts when the decompressed stream exceeds the limit', async () => { + // 5 MB of zeroes compresses to a few KB — the classic shape of the attack. + const gzPath = path.join(dir, 'bomb.gz'); + fs.writeFileSync(gzPath, zlib.gzipSync(Buffer.alloc(5 * 1024 * 1024, 0))); + + process.env.RESTORE_MAX_DECOMPRESSED_BYTES = String(64 * 1024); // 64 KB + await expect( + restoreService.decompressFile(gzPath, path.join(dir, 'out-bomb')) + ).rejects.toThrow(/exceeds limit/i); + }); + + it('still decompresses a normal file within the limit', async () => { + const payload = Buffer.from('SELECT 1;\n'.repeat(100)); + const gzPath = path.join(dir, 'ok.gz'); + fs.writeFileSync(gzPath, zlib.gzipSync(payload)); + + const outPath = path.join(dir, 'out-ok'); + await restoreService.decompressFile(gzPath, outPath); + expect(fs.readFileSync(outPath)).toEqual(payload); + }); +}); + +describe('manifest checksum keying (GHSA-hgp8)', () => { + // validateManifest requires all of these sections to be present. + const baseManifest = () => ({ + manifest: { version: '1.0', id: 'test' }, + backup: { type: 'full' }, + system: { platform: 'linux' }, + application: { version: '1.0.0' }, + files: { count: 1, manifest: [{ path: 'a.jpg', size: 1 }] }, + database: { type: 'sqlite' }, + verification: { total_checksum: null, checksum_algorithm: null }, + }); + + afterEach(() => { delete process.env.BACKUP_MANIFEST_KEY; }); + + it('produces a different digest when a key is set', () => { + const m = baseManifest(); + const unkeyed = backupManifest.calculateManifestChecksum(m, { keyed: false }); + const keyed = backupManifest.calculateManifestChecksum(m, { keyed: 'secret-key' }); + expect(keyed).not.toBe(unkeyed); + }); + + it('validates a legacy unkeyed manifest even when a key IS configured', () => { + // Disaster recovery: manifests written before keying must not become + // un-restorable the moment the operator sets a key. + const m = baseManifest(); + m.verification.checksum_algorithm = 'sha256'; + m.verification.total_checksum = backupManifest.calculateManifestChecksum(m, { keyed: false }); + + process.env.BACKUP_MANIFEST_KEY = 'secret-key'; + expect(() => backupManifest.validateManifest(m)).not.toThrow(); + }); + + it('accepts a keyed manifest when the matching key is configured', () => { + process.env.BACKUP_MANIFEST_KEY = 'secret-key'; + const m = baseManifest(); + m.verification.checksum_algorithm = 'hmac-sha256'; + m.verification.total_checksum = backupManifest.calculateManifestChecksum(m, { keyed: 'secret-key' }); + + expect(() => backupManifest.validateManifest(m)).not.toThrow(); + }); + + it('rejects a keyed manifest whose body was tampered with', () => { + process.env.BACKUP_MANIFEST_KEY = 'secret-key'; + const m = baseManifest(); + m.verification.checksum_algorithm = 'hmac-sha256'; + m.verification.total_checksum = backupManifest.calculateManifestChecksum(m, { keyed: 'secret-key' }); + + m.files.manifest[0].path = '../../etc/passwd'; + expect(() => backupManifest.validateManifest(m)).toThrow(/checksum verification failed/i); + }); + + it('does NOT brick restore when a keyed manifest meets a missing key', () => { + // Key lost with the host — the precise moment a restore is needed. + const m = baseManifest(); + m.verification.checksum_algorithm = 'hmac-sha256'; + m.verification.total_checksum = backupManifest.calculateManifestChecksum(m, { keyed: 'secret-key' }); + + delete process.env.BACKUP_MANIFEST_KEY; + expect(() => backupManifest.validateManifest(m)).not.toThrow(); + }); +}); + +describe('manifest checksum coverage (canonicalization)', () => { + const fullManifest = () => ({ + manifest: { version: '1.0', id: 'test' }, + backup: { type: 'full' }, + system: { platform: 'linux' }, + application: { version: '1.0.0' }, + files: { count: 1, manifest: [{ path: 'a.jpg', size: 1 }] }, + database: { type: 'sqlite' }, + verification: { total_checksum: null, checksum_algorithm: 'sha256' }, + }); + + afterEach(() => { delete process.env.BACKUP_MANIFEST_KEY; }); + + it('covers nested file entries (the old replacer dropped them)', () => { + const m = fullManifest(); + m.verification.total_checksum = backupManifest.calculateManifestChecksum(m, { keyed: false }); + // Tampering a file path must now change the digest. + m.files.manifest[0].path = '../../etc/passwd'; + expect(() => backupManifest.validateManifest(m)).toThrow(/checksum verification failed/i); + }); + + it('still accepts a manifest written with the legacy serialization', () => { + const m = fullManifest(); + m.verification.total_checksum = backupManifest.calculateManifestChecksum( + m, { keyed: false, legacy: true } + ); + expect(() => backupManifest.validateManifest(m)).not.toThrow(); + }); +}); + +describe('checksum verification is shared and downgrade-aware (codex round 2)', () => { + const fullManifest = () => ({ + manifest: { version: '1.0', id: 'test' }, + backup: { type: 'full' }, + system: { platform: 'linux' }, + application: { version: '1.0.0' }, + files: { count: 1, manifest: [{ path: 'a.jpg', size: 1 }] }, + database: { type: 'sqlite' }, + verification: { total_checksum: null, checksum_algorithm: 'sha256' }, + }); + + afterEach(() => { + delete process.env.BACKUP_MANIFEST_KEY; + delete process.env.BACKUP_MANIFEST_REQUIRE_KEYED; + }); + + it('accepts a legacy-serialized manifest through the SHARED verifier', () => { + // restoreService recomputed the digest itself with the canonical + // serializer, which rejected every pre-existing backup. + const m = fullManifest(); + m.verification.total_checksum = backupManifest.calculateManifestChecksum( + m, { keyed: false, legacy: true }, + ); + const res = backupManifest.verifyManifestChecksum(m); + expect(res.valid).toBe(true); + expect(res.warnings.join(' ')).toMatch(/legacy checksum serialization/i); + }); + + it('warns but accepts an unkeyed manifest when a key is configured', () => { + const m = fullManifest(); + m.verification.total_checksum = backupManifest.calculateManifestChecksum(m, { keyed: false }); + process.env.BACKUP_MANIFEST_KEY = 'secret-key'; + + const res = backupManifest.verifyManifestChecksum(m); + expect(res.valid).toBe(true); + expect(res.warnings.join(' ')).toMatch(/authenticity NOT established/i); + }); + + it('REJECTS the algorithm downgrade once REQUIRE_KEYED is on', () => { + // Attacker rewrites the manifest, strips checksum_algorithm and recomputes + // a plain SHA-256. With the strict flag set that must not verify. + const m = fullManifest(); + m.files.manifest[0].path = '../../etc/passwd'; + m.verification.total_checksum = backupManifest.calculateManifestChecksum(m, { keyed: false }); + + process.env.BACKUP_MANIFEST_KEY = 'secret-key'; + process.env.BACKUP_MANIFEST_REQUIRE_KEYED = 'true'; + + const res = backupManifest.verifyManifestChecksum(m); + expect(res.valid).toBe(false); + expect(res.error).toMatch(/downgrade/i); + }); + + it('rejects a keyed manifest with no key when REQUIRE_KEYED is on', () => { + const m = fullManifest(); + m.verification.checksum_algorithm = 'hmac-sha256'; + m.verification.total_checksum = backupManifest.calculateManifestChecksum(m, { keyed: 'k' }); + process.env.BACKUP_MANIFEST_REQUIRE_KEYED = 'true'; + + expect(backupManifest.verifyManifestChecksum(m).valid).toBe(false); + }); + + it('REJECTS a manifest whose checksum was stripped entirely', () => { + // The cheapest bypass of every rule above: delete the field instead of + // forging it. Both the helper's early return and restoreService's + // `if (…total_checksum)` guard used to wave that through. + const m = fullManifest(); + delete m.verification.total_checksum; + + const res = backupManifest.verifyManifestChecksum(m); + expect(res.valid).toBe(false); + expect(res.error).toMatch(/no checksum/i); + + delete m.verification; + expect(backupManifest.verifyManifestChecksum(m).valid).toBe(false); + }); + + it('REJECTS an unkeyed manifest under REQUIRE_KEYED even with no key configured', () => { + // Strict mode is a claim about the manifests, not about this host — so a + // fresh disaster-recovery box that lost BACKUP_MANIFEST_KEY must not + // silently start accepting plain SHA-256 manifests again. + const m = fullManifest(); + m.verification.total_checksum = backupManifest.calculateManifestChecksum(m, { keyed: false }); + process.env.BACKUP_MANIFEST_REQUIRE_KEYED = 'true'; + delete process.env.BACKUP_MANIFEST_KEY; + + const res = backupManifest.verifyManifestChecksum(m); + expect(res.valid).toBe(false); + expect(res.error).toMatch(/downgrade/i); + }); +}); diff --git a/backend/src/routes/adminDatabaseBackup.js b/backend/src/routes/adminDatabaseBackup.js index 35658240..b4114091 100644 --- a/backend/src/routes/adminDatabaseBackup.js +++ b/backend/src/routes/adminDatabaseBackup.js @@ -123,8 +123,20 @@ router.post('/backup', requirePermission('backup.create'), async (req, res) => { trackingUrl: '/api/admin/database-backup/progress' }); - // Run backup in background - databaseBackupService.backup(req.body).catch(error => { + // Forward ONLY the real backup knobs (GHSA-jw8m). Passing req.body + // straight through let the caller set `destinationPath`, which the + // service merges over its config — so a backup.create holder (the + // `admin` role, which has neither settings.edit nor backup.restore) + // could dump the whole database into the PUBLIC /uploads static mount + // and fetch it unauthenticated, hashes and encrypted SMTP creds included. + // destinationPath is not a persistable setting; the request body was its + // only source, so dropping it here costs no legitimate behaviour. + const body = req.body || {}; + const options = {}; + for (const key of ['compress', 'validateIntegrity', 'includeChecksums']) { + if (body[key] !== undefined) options[key] = body[key]; + } + databaseBackupService.backup(options).catch(error => { logger.error('Manual database backup failed:', error); }); } catch (error) { diff --git a/backend/src/routes/adminRestore.js b/backend/src/routes/adminRestore.js index 775b7c18..5b69de34 100644 --- a/backend/src/routes/adminRestore.js +++ b/backend/src/routes/adminRestore.js @@ -91,6 +91,12 @@ router.post('/validate', requirePermission('backup.restore'), [ } try { + // Constrain the caller-supplied paths to configured backup roots (GHSA-fw4c) + const pathError = await checkRestorePathsAllowed(req.body); + if (pathError) { + return res.status(400).json({ success: false, error: pathError }); + } + // Transform S3 config from frontend format const s3Config = transformS3Config(req.body); @@ -165,6 +171,12 @@ router.post('/start', requirePermission('backup.restore'), [ }); } + // Constrain the caller-supplied paths to configured backup roots (GHSA-fw4c) + const pathError = await checkRestorePathsAllowed(req.body); + if (pathError) { + return res.status(400).json({ success: false, error: pathError }); + } + // Check permissions for dangerous options const settings = await getRestoreSettings(); if (req.body.force && !settings.restore_allow_force) { @@ -759,4 +771,67 @@ async function getBackupConfig() { return config; } +/** + * GHSA-fw4c: `source` and `manifestPath` were validated only as "not empty" + * before being handed to the privileged restore engine, which reads them, + * parses the manifest and executes the referenced SQL against the live + * database. Constrain them to the operator-configured backup locations. + * + * The allowlist is the SAME set the restore wizard discovers from + * (`backup_destination_path` + `backup_manifest_path`), so the disaster- + * recovery flow is untouched: an operator restoring from a rescued mount + * already has to point those settings at it for the backup to be listed. + * RESTORE_ALLOWED_ROOTS (colon-separated) is an escape hatch for unusual + * layouts. S3 sources are URLs, not paths, and are validated elsewhere. + * + * @returns {Promise} an error message, or null when acceptable + */ +// `source` is usually a SOURCE TYPE, not a path: the restore wizard posts +// 'local' | 's3' | 'upload' and restoreService.restore() branches on those +// literals before deriving an actual directory (see its comment at the +// `options.source === 'local'` branch). Treating them as paths resolved +// 'local' to /local, failed containment, and 400'd the entire normal +// restore workflow — so type tokens are excluded from the path check. +const SOURCE_TYPE_TOKENS = ['local', 's3', 'upload']; + +async function checkRestorePathsAllowed({ source, manifestPath }) { + const isS3 = (v) => typeof v === 'string' && v.startsWith('s3://'); + const isTypeToken = (v) => typeof v === 'string' + && SOURCE_TYPE_TOKENS.includes(v.trim().toLowerCase()); + const candidates = [source, manifestPath] + .filter((v) => v && !isS3(v) && !isTypeToken(v)); + if (candidates.length === 0) return null; + + const config = await getBackupConfig(); + const roots = []; + if (config.backup_destination_path) roots.push(config.backup_destination_path); + if (config.backup_manifest_path) roots.push(config.backup_manifest_path); + for (const extra of (process.env.RESTORE_ALLOWED_ROOTS || '').split(':')) { + if (extra.trim()) roots.push(extra.trim()); + } + if (roots.length === 0) { + // Nothing configured to compare against — a restore can't be scoped, so + // don't pretend to enforce. Discovery would find nothing either. + return null; + } + + const resolvedRoots = roots.map((r) => path.resolve(r)); + for (const candidate of candidates) { + const resolved = path.resolve(candidate); + const inside = resolvedRoots.some( + (root) => resolved === root || resolved.startsWith(root + path.sep) + ); + if (!inside) { + logger.warn('Refusing restore path outside the configured backup roots', { + candidate, roots, + }); + return 'Backup source and manifest path must be inside a configured backup location'; + } + } + return null; +} + module.exports = router; +// Exposed for tests: the source/manifestPath containment rules (GHSA-fw4c) are +// worth pinning directly, especially the source-TYPE-token carve-out. +module.exports._internal = { checkRestorePathsAllowed, SOURCE_TYPE_TOKENS }; diff --git a/backend/src/services/backupManifest.js b/backend/src/services/backupManifest.js index 41bf0370..ba8a00bb 100644 --- a/backend/src/services/backupManifest.js +++ b/backend/src/services/backupManifest.js @@ -134,7 +134,10 @@ class BackupManifestGenerator { } }; - // Calculate total checksum of the manifest + // Calculate total checksum of the manifest. Records WHICH algorithm was + // used so validation can tell a keyed manifest from a legacy unkeyed one + // (GHSA-hgp8). + manifest.verification.checksum_algorithm = this.getManifestKey() ? 'hmac-sha256' : 'sha256'; manifest.verification.total_checksum = this.calculateManifestChecksum(manifest); return manifest; @@ -227,11 +230,15 @@ class BackupManifestGenerator { throw new Error('File count mismatch'); } - // Validate total checksum - const calculatedChecksum = this.calculateManifestChecksum(manifest); - if (manifest.verification.total_checksum !== calculatedChecksum) { - throw new Error('Manifest checksum verification failed'); + // Validate total checksum (GHSA-hgp8) — delegated so every caller shares + // the same fallback rules. restoreService.performPreRestoreValidation() + // used to recompute the digest itself with the default (canonical, keyed) + // settings, which silently rejected every pre-existing backup. + const checksumResult = this.verifyManifestChecksum(manifest); + if (!checksumResult.valid) { + throw new Error(checksumResult.error || 'Manifest checksum verification failed'); } + checksumResult.warnings.forEach((w) => logger.warn(w)); logger.info('Manifest validation passed'); return true; @@ -327,6 +334,7 @@ class BackupManifestGenerator { // otherwise validateManifest() rejects the loaded manifest because // generateManifest() stamped a checksum that did NOT include this // section. + fullManifest.verification.checksum_algorithm = this.getManifestKey() ? 'hmac-sha256' : 'sha256'; fullManifest.verification.total_checksum = this.calculateManifestChecksum(fullManifest); return fullManifest; @@ -439,16 +447,167 @@ class BackupManifestGenerator { } } - calculateManifestChecksum(manifest) { + /** + * GHSA-hgp8: the plain SHA-256 below proves the manifest wasn't CORRUPTED, + * not that it is AUTHENTIC — anyone who can rewrite the file can recompute + * it. Setting BACKUP_MANIFEST_KEY upgrades new manifests to a keyed HMAC, + * which matters when the backup store is a different trust domain from the + * host (S3 bucket creds != host creds). + * + * Deliberately OPT-IN and verify-if-present: the key cannot live in the + * database (the database is inside the backup), so a mandatory HMAC would + * lock an operator out of the exact disaster-recovery case this system + * exists for — total host loss, fresh install, only the backup survives. + * Unkeyed manifests therefore still validate, and a keyed manifest is only + * held to the keyed check when a key is configured. + */ + getManifestKey() { + const key = process.env.BACKUP_MANIFEST_KEY; + return typeof key === 'string' && key.trim() ? key.trim() : null; + } + + /** + * Single source of truth for "does this manifest's checksum verify?" + * (GHSA-hgp8). Returns a result object rather than throwing so callers can + * surface warnings without duplicating the fallback rules — a duplicated + * check in restoreService recomputed the digest with the default canonical + * serializer and rejected every manifest written before that change. + * + * Rules, in order: + * - keyed manifest + no key configured → cannot verify; accept with a + * loud warning (refusing would brick recovery when the key was lost with + * the host, which is exactly when a restore is needed), UNLESS + * BACKUP_MANIFEST_REQUIRE_KEYED is set. + * - unkeyed manifest + key configured → possible downgrade. Accepted with + * a warning by default for backward compatibility; rejected when + * BACKUP_MANIFEST_REQUIRE_KEYED is set, which is the setting an operator + * turns on once all their backups are keyed. + * - digest mismatch → retry with the legacy (pre-canonicalization) + * serialization so old backups stay restorable, then fail. + * - no checksum at all → reject. Every manifest this codebase has ever + * written stamps `verification.total_checksum` (generateManifest and + * the incremental path both do), so an absent one means the manifest + * was rewritten — and accepting it would let an attacker strip the + * field to skip verification entirely, walking straight past both the + * downgrade guard and BACKUP_MANIFEST_REQUIRE_KEYED. + * + * @returns {{valid: boolean, error?: string, warnings: string[]}} + */ + verifyManifestChecksum(manifest) { + const warnings = []; + if (!manifest?.verification?.total_checksum) { + return { + valid: false, + error: 'Manifest carries no checksum — refusing to treat an unverifiable manifest as authentic', + warnings, + }; + } + + const declaredAlgorithm = manifest.verification.checksum_algorithm || 'sha256'; + const key = this.getManifestKey(); + const requireKeyed = /^(1|true|yes)$/i.test(String(process.env.BACKUP_MANIFEST_REQUIRE_KEYED || '')); + + if (declaredAlgorithm === 'hmac-sha256' && !key) { + if (requireKeyed) { + return { + valid: false, + error: 'Manifest is keyed but BACKUP_MANIFEST_KEY is not set (BACKUP_MANIFEST_REQUIRE_KEYED is on)', + warnings, + }; + } + warnings.push( + 'Manifest declares a keyed checksum but BACKUP_MANIFEST_KEY is not set — ' + + 'authenticity cannot be verified. Set the key to enable verification.' + ); + return { valid: true, warnings }; + } + + // Downgrade guard: with a key configured, an attacker who can rewrite the + // backup store could otherwise strip checksum_algorithm, edit the manifest + // and recompute a plain SHA-256 that we would happily accept. Rejecting + // that by default would break every pre-key backup, so it is opt-in. + // + // The strict rejection must NOT be conditional on a key being configured: + // strict mode is a statement about the manifests ("all mine are keyed"), + // not about this host. Gating it on `key` made the flag fail open on + // exactly the fresh disaster-recovery host that is missing the secret. + if (declaredAlgorithm !== 'hmac-sha256') { + if (requireKeyed) { + return { + valid: false, + error: 'Manifest is not keyed but BACKUP_MANIFEST_REQUIRE_KEYED is on — refusing a possible checksum downgrade', + warnings, + }; + } + if (key) { + warnings.push( + 'Manifest uses an unkeyed checksum while BACKUP_MANIFEST_KEY is set — integrity verified, ' + + 'authenticity NOT established (a rewritten manifest could have downgraded the algorithm). ' + + 'Set BACKUP_MANIFEST_REQUIRE_KEYED=true once all backups are keyed.' + ); + } + } + + const keyedArg = declaredAlgorithm === 'hmac-sha256' ? key : false; + const expected = manifest.verification.total_checksum; + + if (expected === this.calculateManifestChecksum(manifest, { keyed: keyedArg })) { + return { valid: true, warnings }; + } + // Pre-canonicalization manifests hashed a serialization that omitted + // nested fields; accept those so existing backups stay restorable. + if (expected === this.calculateManifestChecksum(manifest, { keyed: keyedArg, legacy: true })) { + warnings.push( + 'Manifest uses the legacy checksum serialization, which did not cover the file list — ' + + 'integrity of file paths/sizes is unverified. Re-run a backup to upgrade it.' + ); + return { valid: true, warnings }; + } + return { valid: false, error: 'Manifest checksum verification failed', warnings }; + } + + /** + * Canonical JSON: object keys sorted recursively so the digest is stable + * regardless of property insertion order, and — critically — so NESTED + * values are actually covered. + * + * The previous implementation passed `Object.keys(manifest).sort()` as + * JSON.stringify's second argument. That parameter is an array *replacer* + * (a property allowlist applied at every depth), not a key sorter, so every + * nested key absent from that top-level list — `path`, `size`, per-file + * `checksum` — was dropped before hashing. The file list was therefore + * outside the "integrity" check entirely: a manifest path could be rewritten + * to `../../etc/passwd` without disturbing the checksum. + */ + canonicalize(value) { + if (Array.isArray(value)) return value.map((v) => this.canonicalize(v)); + if (value && typeof value === 'object') { + return Object.keys(value).sort().reduce((acc, k) => { + acc[k] = this.canonicalize(value[k]); + return acc; + }, {}); + } + return value; + } + + calculateManifestChecksum(manifest, { keyed = null, legacy = false } = {}) { // Create a copy without the checksum field const manifestCopy = JSON.parse(JSON.stringify(manifest)); if (manifestCopy.verification) { delete manifestCopy.verification.total_checksum; + delete manifestCopy.verification.checksum_algorithm; } - // Calculate SHA256 of the sorted JSON - const content = JSON.stringify(manifestCopy, Object.keys(manifestCopy).sort()); - return crypto.createHash('sha256').update(content).digest('hex'); + // `legacy` reproduces the old (under-covering) serialization so manifests + // written by earlier versions still validate — see validateManifest. + const content = legacy + ? JSON.stringify(manifestCopy, Object.keys(manifestCopy).sort()) + : JSON.stringify(this.canonicalize(manifestCopy)); + + const key = keyed === null ? this.getManifestKey() : keyed; + return key + ? crypto.createHmac('sha256', key).update(content).digest('hex') + : crypto.createHash('sha256').update(content).digest('hex'); } /** diff --git a/backend/src/services/restoreService.js b/backend/src/services/restoreService.js index f85035c6..70e7909c 100644 --- a/backend/src/services/restoreService.js +++ b/backend/src/services/restoreService.js @@ -3,6 +3,7 @@ const path = require('path'); const crypto = require('crypto'); const zlib = require('zlib'); const { pipeline } = require('stream/promises'); +const { Transform } = require('stream'); const { createReadStream, createWriteStream } = require('fs'); const { spawnAsync, spawnToFile, spawnFromFile } = require('../utils/safeExec'); const { db } = require('../database/db'); @@ -519,13 +520,21 @@ class RestoreService { }; try { - // Check backup integrity - if (manifest.verification && manifest.verification.total_checksum) { - const calculatedChecksum = backupManifest.calculateManifestChecksum(manifest); - if (calculatedChecksum !== manifest.verification.total_checksum) { - validation.errors.push('Manifest checksum verification failed'); - validation.isValid = false; - } + // Check backup integrity. MUST delegate to verifyManifestChecksum rather + // than recomputing here — that helper owns the legacy-serialization and + // keyed/unkeyed fallbacks (GHSA-hgp8). Recomputing with the default + // canonical+keyed settings rejected every backup written before those + // changes, i.e. every existing one. + // + // Called UNCONDITIONALLY: the old `if (…total_checksum)` guard meant an + // attacker who could rewrite the backup store simply deleted the field + // to skip verification altogether. The helper owns that case now and + // rejects it. + const checksumResult = backupManifest.verifyManifestChecksum(manifest); + checksumResult.warnings.forEach((w) => this.log('warn', w)); + if (!checksumResult.valid) { + validation.errors.push(checksumResult.error || 'Manifest checksum verification failed'); + validation.isValid = false; } // Check backup age @@ -1512,10 +1521,33 @@ END $$;` * Decompress gzip file */ async decompressFile(inputPath, outputPath) { + // Bound the EXPANDED size (GHSA-h652). gunzip happily inflates a small + // crafted .gz into an unbounded stream, filling the disk before any later + // validation runs. Cap it and fail the pipeline the moment the limit is + // crossed. The default is deliberately generous — real database dumps are + // large — and overridable for installs with genuinely bigger data. + const configured = Number(process.env.RESTORE_MAX_DECOMPRESSED_BYTES); + const maxBytes = Number.isFinite(configured) && configured > 0 + ? configured + : 50 * 1024 * 1024 * 1024; // 50 GB + + let written = 0; + const limiter = new Transform({ + transform(chunk, _enc, cb) { + written += chunk.length; + if (written > maxBytes) { + return cb(new Error( + `Decompressed size exceeds limit of ${maxBytes} bytes — refusing to continue` + )); + } + cb(null, chunk); + }, + }); + const gunzip = zlib.createGunzip(); const source = createReadStream(inputPath); const destination = createWriteStream(outputPath); - await pipeline(source, gunzip, destination); + await pipeline(source, gunzip, limiter, destination); } /**