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 archiver = require('archiver');
|
||||||
const StreamZip = require('node-stream-zip');
|
const StreamZip = require('node-stream-zip');
|
||||||
const { requireEventOwnership } = require('../middleware/ownership');
|
const { requireEventOwnership } = require('../middleware/ownership');
|
||||||
|
const { assertZipEntriesWithin } = require('../utils/safePath');
|
||||||
const logger = require('../utils/logger');
|
const logger = require('../utils/logger');
|
||||||
const { getPagination } = require('../utils/routeHelpers');
|
const { getPagination } = require('../utils/routeHelpers');
|
||||||
const router = express.Router();
|
const router = express.Router();
|
||||||
@@ -183,6 +184,16 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re
|
|||||||
const entries = Object.values(await zip.entries());
|
const entries = Object.values(await zip.entries());
|
||||||
logger.info(`Archive contains ${entries.length} 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
|
// Stream-extract everything to disk
|
||||||
await zip.extract(null, eventDir);
|
await zip.extract(null, eventDir);
|
||||||
await zip.close();
|
await zip.close();
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ const fsp = require('fs').promises;
|
|||||||
const path = require('path');
|
const path = require('path');
|
||||||
const os = require('os');
|
const os = require('os');
|
||||||
const StreamZip = require('node-stream-zip');
|
const StreamZip = require('node-stream-zip');
|
||||||
|
const { assertZipEntriesWithin } = require('../utils/safePath');
|
||||||
const { db } = require('../database/db');
|
const { db } = require('../database/db');
|
||||||
const knexConfig = require('../../knexfile');
|
const knexConfig = require('../../knexfile');
|
||||||
const { getStoragePath } = require('../config/storage');
|
const { getStoragePath } = require('../config/storage');
|
||||||
@@ -232,6 +233,10 @@ async function importFromPicpeak({ picpeakPath, currentAdminId }) {
|
|||||||
try {
|
try {
|
||||||
const zip = new StreamZip.async({ file: picpeakPath });
|
const zip = new StreamZip.async({ file: picpeakPath });
|
||||||
try {
|
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);
|
await zip.extract(null, staging);
|
||||||
} finally {
|
} finally {
|
||||||
await zip.close();
|
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 = {
|
module.exports = {
|
||||||
assertPathInside,
|
assertPathInside,
|
||||||
assertContractPdfPath,
|
assertContractPdfPath,
|
||||||
|
assertZipEntriesWithin,
|
||||||
};
|
};
|
||||||
|
|||||||
Reference in New Issue
Block a user