fix(security): reject ZIP-slip entries in archive/backup restore (GHSA-jfhw-fj23-fx6x)
node-stream-zip's extract(null, root) writes each entry to path.join(root, entry.name) without neutralising '../', so a crafted archive entry named '../../uploads/logos/evil.svg' escaped the target dir and overwrote arbitrary files (logos, .env, route files → RCE on source deploys). Requires admin with archives.restore. Adds assertZipEntriesWithin() to utils/safePath.js — a lexical containment check run on the entry list BEFORE extract() — and guards both extract sinks: adminArchives.js (the reported route) and picpeakImportService.js (the sibling .picpeak import, same sink). Adds unit tests for traversal, absolute-path, and sibling-prefix entries.
This commit is contained in:
@@ -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/);
|
||||
});
|
||||
});
|
||||
@@ -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();
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user