fix(security): unauth share_token leak (HIGH) + restore path-traversal, logo file-read, branding path keys (#946)
* fix(security): stop unauth share_token leak + block restore path-traversal, logo-path file read, branding path keys * test: update resolveLogoFile for the c7x5 containment (reject outside-storage absolute paths, keep inside) * fix(security): codex round-1 — escape LIKE wildcards in share-link resolve, keep in-storage absolute logos, guard restore verification - shareLinkService: escape %/_ in the link_partial LIKE fallback so an anonymous /resolve/____… wildcard can't match an arbitrary share_link and leak its bearer token (reopened GHSA-rh8r). Explicit ESCAPE for SQLite. - resolveLogoFile: re-add the raw absolute candidate but keep it subject to the storage-root containment filter (GHSA-c7x5) so legit in-storage absolute logos resolve while /etc/passwd stays rejected. - restoreService: apply the same pathEscapes guard in post-restore verification so a skipped traversal entry isn't fs.access'd/hashed. --------- Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
8cbb37310b
commit
9050affd8d
@@ -39,7 +39,14 @@ const getStoragePath = () => process.env.STORAGE_PATH || path.join(__dirname, '.
|
||||
// oidc_client_secret is reserved too: it is AES-encrypted at rest and only
|
||||
// writable through PUT /sso below — a generic upsert would store plaintext
|
||||
// and break decryption (#798).
|
||||
const RESERVED_SETTING_KEYS = ['setup_wizard_completed', 'setup_token'];
|
||||
// Branding *path* keys (GHSA-665x) are server-computed by the logo-upload
|
||||
// flow and feed a filesystem logo resolver; letting the general settings PUT
|
||||
// set them to arbitrary strings makes them an input to path resolution.
|
||||
// Reserve them here — the dedicated upload endpoints still write them.
|
||||
const RESERVED_SETTING_KEYS = [
|
||||
'setup_wizard_completed', 'setup_token',
|
||||
'branding_logo_path', 'branding_logo_path_dark', 'branding_watermark_logo_path',
|
||||
];
|
||||
// EVERY oidc_* key is reserved (#798 phase 2): the client secret would be
|
||||
// clobbered with plaintext, and the policy/mapping keys carry invariants
|
||||
// (role targets exist, break-glass account present) that only the dedicated
|
||||
|
||||
@@ -153,9 +153,20 @@ router.get('/resolve/:identifier', handleAsync(async (req, res) => {
|
||||
}
|
||||
|
||||
const { event, matchType, shareToken } = result;
|
||||
const linkVariants = await buildShareLinkVariants({ slug: event.slug, shareToken });
|
||||
const requiresPassword = !(event.require_password === false || event.require_password === 0 || event.require_password === '0');
|
||||
|
||||
// The share_token is a bearer secret. Only return it (and the share
|
||||
// links/URLs that embed it) when the caller already proved they hold it —
|
||||
// i.e. they resolved via the token or the full share link. A bare *slug*
|
||||
// lookup (slugs appear in gallery URLs and are guessable) must NOT hand
|
||||
// back the secret, or an anonymous caller could turn a known slug into
|
||||
// share-link access to a no-password gallery (GHSA-rh8r).
|
||||
const callerHasToken = matchType !== 'slug';
|
||||
if (!callerHasToken) {
|
||||
return res.json({ slug: event.slug, matchType, requires_password: requiresPassword });
|
||||
}
|
||||
|
||||
const linkVariants = await buildShareLinkVariants({ slug: event.slug, shareToken });
|
||||
res.json({
|
||||
slug: event.slug,
|
||||
token: shareToken,
|
||||
|
||||
@@ -12,6 +12,15 @@ const backupManifest = require('./backupManifest');
|
||||
const S3StorageAdapter = require('./storage/s3Storage');
|
||||
const { queueEmail } = require('./emailProcessor');
|
||||
const { formatBoolean } = require('../utils/dbCompat');
|
||||
|
||||
// A manifest is attacker-influenceable (hand-crafted backup). Reject any
|
||||
// entry path that would resolve OUTSIDE its intended base directory
|
||||
// (traversal / absolute path) before any fs write. The target may not exist
|
||||
// yet, so resolve rather than realpath (GHSA-fm58).
|
||||
function pathEscapes(baseDir, candidate) {
|
||||
const rel = path.relative(path.resolve(baseDir), path.resolve(candidate));
|
||||
return !rel || rel === '..' || rel.startsWith('..' + path.sep) || path.isAbsolute(rel);
|
||||
}
|
||||
const { formatBytes } = require('../utils/formatBytes');
|
||||
const os = require('os');
|
||||
|
||||
@@ -771,7 +780,14 @@ class RestoreService {
|
||||
for (const file of filesToDownload) {
|
||||
const s3Key = path.posix.join(prefix, file.path);
|
||||
const localFilePath = path.join(localPath, file.path);
|
||||
|
||||
|
||||
// Containment guard (GHSA-fm58): reject a manifest path that would
|
||||
// write outside the download staging dir.
|
||||
if (pathEscapes(localPath, localFilePath)) {
|
||||
this.log('error', `Refusing unsafe manifest path on download: ${file.path}`);
|
||||
continue;
|
||||
}
|
||||
|
||||
await fs.mkdir(path.dirname(localFilePath), { recursive: true });
|
||||
|
||||
try {
|
||||
@@ -1204,6 +1220,14 @@ END $$;`
|
||||
const sourcePath = path.join(backupPath, file.path);
|
||||
const targetPath = path.join(storagePath, file.path);
|
||||
|
||||
// Containment guard (GHSA-fm58): a crafted manifest path like
|
||||
// `../../etc/cron.d/x` would otherwise escape the storage root and
|
||||
// overwrite arbitrary files. Skip any entry that escapes.
|
||||
if (pathEscapes(backupPath, sourcePath) || pathEscapes(storagePath, targetPath)) {
|
||||
errors.push(`Refusing unsafe manifest path: ${file.path}`);
|
||||
continue;
|
||||
}
|
||||
|
||||
// Check if source file exists
|
||||
try {
|
||||
await fs.access(sourcePath);
|
||||
@@ -1375,6 +1399,15 @@ END $$;`
|
||||
|
||||
for (const file of filesToVerify) {
|
||||
const filePath = path.join(storagePath, file.path);
|
||||
// Same containment guard as performFilesRestore: a traversal
|
||||
// manifest entry (e.g. `../../etc/passwd`) was skipped during the
|
||||
// restore, so it must not be fs.access'd/hashed here either —
|
||||
// otherwise an existing outside file makes the skipped entry look
|
||||
// "verified" (and we'd read an arbitrary file off disk).
|
||||
if (pathEscapes(storagePath, filePath)) {
|
||||
verification.errors.push(`Refusing unsafe manifest path on verification: ${file.path}`);
|
||||
continue;
|
||||
}
|
||||
try {
|
||||
await fs.access(filePath);
|
||||
|
||||
|
||||
@@ -159,7 +159,16 @@ const resolveShareIdentifier = async (identifier) => {
|
||||
return { event, matchType: 'link', shareToken: getEventShareToken(event) };
|
||||
}
|
||||
|
||||
event = await baseQuery.clone().where('share_link', 'like', `%/${trimmed}`).first();
|
||||
// GHSA-rh8r hardening: `trimmed` is attacker-controlled, so escape LIKE
|
||||
// wildcards (`%`, `_`, and the escape char itself) before embedding it.
|
||||
// Otherwise an anonymous `/resolve/________…________` (32 underscores)
|
||||
// matches ANY share_link via single-char wildcards, resolves as
|
||||
// matchType 'link_partial', and the /resolve route hands back the
|
||||
// gallery's bearer token — reopening the very hole the token-withholding
|
||||
// fix closed. Explicit ESCAPE clause because SQLite has no default LIKE
|
||||
// escape character (Postgres defaults to backslash, but we set it for both).
|
||||
const likeTail = `%/${trimmed.replace(/[\\%_]/g, (c) => `\\${c}`)}`;
|
||||
event = await baseQuery.clone().whereRaw('share_link LIKE ? ESCAPE \'\\\'', [likeTail]).first();
|
||||
if (event) {
|
||||
return { event, matchType: 'link_partial', shareToken: getEventShareToken(event) };
|
||||
}
|
||||
|
||||
@@ -57,18 +57,34 @@ function generateCandidates(raw, storageRoot) {
|
||||
if (!value) return [];
|
||||
const stripped = value.replace(/^\/+/, '');
|
||||
const baseName = path.basename(value);
|
||||
const cwdStorage = path.join(process.cwd(), 'storage');
|
||||
// Build candidate set; dedup at the end so we don't stat the same
|
||||
// file twice when the inputs overlap.
|
||||
const candidates = [
|
||||
path.isAbsolute(value) ? value : null,
|
||||
// Keep the raw absolute value as a candidate so a legitimate multer path
|
||||
// (branding_logo_path is stored absolute) or an absolute logo inside a
|
||||
// non-standard storage subdir still resolves. The containment filter
|
||||
// below is what enforces safety — it drops this candidate when it points
|
||||
// outside the storage roots, so `/etc/passwd` is still rejected.
|
||||
...(path.isAbsolute(value) ? [value] : []),
|
||||
path.join(storageRoot, stripped),
|
||||
path.join(storageRoot, 'uploads', 'logos', baseName),
|
||||
path.join(storageRoot, 'branding', baseName),
|
||||
path.join(process.cwd(), 'storage', stripped),
|
||||
path.join(process.cwd(), 'storage', 'uploads', 'logos', baseName),
|
||||
path.join(process.cwd(), 'storage', 'branding', baseName),
|
||||
].filter(Boolean);
|
||||
return [...new Set(candidates)];
|
||||
path.join(cwdStorage, stripped),
|
||||
path.join(cwdStorage, 'uploads', 'logos', baseName),
|
||||
path.join(cwdStorage, 'branding', baseName),
|
||||
];
|
||||
// GHSA-c7x5: only read logo files INSIDE the storage roots. An admin-set
|
||||
// logo_path of `/etc/passwd` was previously rasterised into a PDF; the
|
||||
// filter below drops any candidate (including the raw absolute one and any
|
||||
// `..`-escaping stripped path) that resolves outside the roots. baseName-
|
||||
// based candidates are inherently contained.
|
||||
const roots = [path.resolve(storageRoot), path.resolve(cwdStorage)];
|
||||
const contained = candidates.filter((c) => {
|
||||
const r = path.resolve(c);
|
||||
return roots.some((root) => r === root || r.startsWith(root + path.sep));
|
||||
});
|
||||
return [...new Set(contained)];
|
||||
}
|
||||
|
||||
function pickExisting(candidates) {
|
||||
|
||||
Reference in New Issue
Block a user