fix(security): backup/restore hardening — public-dir DB dump, restore path allowlist, gunzip bound, manifest keying (#956)
* 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
---------
Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
acd6b453d1
commit
0d4c30884e
@@ -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');
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user