diff --git a/backend/__tests__/utils/safePathZipEntries.test.js b/backend/__tests__/utils/safePathZipEntries.test.js new file mode 100644 index 00000000..97d00acc --- /dev/null +++ b/backend/__tests__/utils/safePathZipEntries.test.js @@ -0,0 +1,41 @@ +const path = require('path'); +const { assertZipEntriesWithin } = require('../../src/utils/safePath'); + +describe('assertZipEntriesWithin (ZIP-slip guard, GHSA-jfhw-fj23-fx6x)', () => { + const root = path.join('/tmp', 'picpeak-extract-root'); + + it('accepts entries that stay within the extraction root', () => { + const entries = [ + { name: 'photo.jpg' }, + { name: 'category/nested/photo.png' }, + { name: 'photos_manifest.json' }, + { name: 'subdir/' }, + ]; + expect(() => assertZipEntriesWithin(entries, root)).not.toThrow(); + }); + + it('rejects a parent-traversal entry', () => { + const entries = [{ name: '../../uploads/logos/evil.svg' }]; + expect(() => assertZipEntriesWithin(entries, root)).toThrow(/escapes the extraction directory/); + }); + + it('rejects an absolute-path entry', () => { + const entries = [{ name: '/etc/cron.d/evil' }]; + expect(() => assertZipEntriesWithin(entries, root)).toThrow(/escapes the extraction directory/); + }); + + it('rejects when a safe entry is mixed with a traversal entry', () => { + const entries = [{ name: 'ok.jpg' }, { name: '../escape.txt' }]; + expect(() => assertZipEntriesWithin(entries, root)).toThrow(/escapes the extraction directory/); + }); + + it('tolerates empty / nameless entries', () => { + expect(() => assertZipEntriesWithin([{}, { name: '' }, null], root)).not.toThrow(); + }); + + it('does not treat a sibling prefix directory as inside the root', () => { + // root is .../picpeak-extract-root; ../picpeak-extract-root-evil must not pass + const entries = [{ name: '../picpeak-extract-root-evil/x' }]; + expect(() => assertZipEntriesWithin(entries, root)).toThrow(/escapes the extraction directory/); + }); +}); diff --git a/backend/src/routes/adminArchives.js b/backend/src/routes/adminArchives.js index d81c8713..fcebccfa 100644 --- a/backend/src/routes/adminArchives.js +++ b/backend/src/routes/adminArchives.js @@ -9,6 +9,7 @@ const { requirePermission } = require('../middleware/permissions'); const archiver = require('archiver'); const StreamZip = require('node-stream-zip'); const { requireEventOwnership } = require('../middleware/ownership'); +const { assertZipEntriesWithin } = require('../utils/safePath'); const logger = require('../utils/logger'); const { getPagination } = require('../utils/routeHelpers'); const router = express.Router(); @@ -183,6 +184,16 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re const entries = Object.values(await zip.entries()); logger.info(`Archive contains ${entries.length} entries`); + // Reject ZIP-slip entries before writing anything to disk — extract() + // does not neutralise `../` in entry names (GHSA-jfhw-fj23-fx6x). + try { + assertZipEntriesWithin(entries, eventDir); + } catch (slipErr) { + await zip.close(); + logger.warn(`Refusing archive restore — unsafe entry path: ${slipErr.message}`); + return res.status(400).json({ error: 'Archive contains invalid entry paths' }); + } + // Stream-extract everything to disk await zip.extract(null, eventDir); await zip.close(); diff --git a/backend/src/services/picpeakImportService.js b/backend/src/services/picpeakImportService.js index b1a6b0f3..a7a6294b 100644 --- a/backend/src/services/picpeakImportService.js +++ b/backend/src/services/picpeakImportService.js @@ -18,6 +18,7 @@ const fsp = require('fs').promises; const path = require('path'); const os = require('os'); const StreamZip = require('node-stream-zip'); +const { assertZipEntriesWithin } = require('../utils/safePath'); const { db } = require('../database/db'); const knexConfig = require('../../knexfile'); const { getStoragePath } = require('../config/storage'); @@ -232,6 +233,10 @@ async function importFromPicpeak({ picpeakPath, currentAdminId }) { try { const zip = new StreamZip.async({ file: picpeakPath }); try { + // Reject ZIP-slip entries before extracting — a crafted .picpeak could + // otherwise write outside the staging dir via `../` entry names + // (same class as GHSA-jfhw-fj23-fx6x). + assertZipEntriesWithin(Object.values(await zip.entries()), staging); await zip.extract(null, staging); } finally { await zip.close(); diff --git a/backend/src/utils/safePath.js b/backend/src/utils/safePath.js index a1dca605..a90553bf 100644 --- a/backend/src/utils/safePath.js +++ b/backend/src/utils/safePath.js @@ -118,7 +118,41 @@ function assertContractPdfPath(filePath) { ]); } +/** + * ZIP-slip guard. `node-stream-zip`'s `extract(null, root)` writes each entry + * to `path.join(root, entry.name)` without neutralising `../` — a crafted + * archive with an entry named `../../uploads/logos/evil.svg` escapes `root` + * and overwrites arbitrary files (GHSA-jfhw-fj23-fx6x). Call this with the + * entry list BEFORE extract() to reject any entry that resolves outside the + * target directory. + * + * Purely lexical (path.resolve, no realpath) because the extraction target + * does not exist on disk yet. Absolute entry names (`/etc/passwd`) resolve + * away from `root` and are caught too. Throws AppError 400 on the first + * offending entry so the whole archive is refused. + * + * @param {Array<{name?: string}>} entries node-stream-zip entry objects + * @param {string} extractRoot directory extract() will write into + */ +function assertZipEntriesWithin(entries, extractRoot) { + const rootResolved = path.resolve(extractRoot); + const prefix = rootResolved.endsWith(path.sep) ? rootResolved : rootResolved + path.sep; + for (const entry of entries || []) { + const name = entry && entry.name; + if (!name) continue; + const target = path.resolve(rootResolved, name); + if (target !== rootResolved && !target.startsWith(prefix)) { + throw new AppError( + `Archive contains an entry that escapes the extraction directory: ${name}`, + 400, + 'ZIP_SLIP' + ); + } + } +} + module.exports = { assertPathInside, assertContractPdfPath, + assertZipEntriesWithin, };