diff --git a/package-lock.json b/package-lock.json index fef2be20353..3d3604c546a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,7 +9,7 @@ "version": "1.0.1", "license": "Apache-2.0", "dependencies": { - "adm-zip": "0.4.7", + "adm-zip": "0.4.16", "body-parser": "1.9.0", "cfenv": "^1.0.4", "consolidate": "0.14.5", @@ -332,9 +332,9 @@ } }, "node_modules/adm-zip": { - "version": "0.4.7", - "resolved": "https://registry.npmjs.org/adm-zip/-/adm-zip-0.4.7.tgz", - "integrity": "sha1-hgbCy/HEJs6MjsABdER/1Jtur8E=", + "version": "0.4.16", + "resolved": "https://registry.npmjs.org/adm-zip/-/adm-zip-0.4.16.tgz", + "integrity": "sha512-TFi4HBKSGfIKsK5YCkKaaFG2m4PEDyViZmEwof3MTIgzimHLto6muaHVpbrljdIvIrFZzEq/p4nafOeLcYegrg==", "engines": { "node": ">=0.3.0" } @@ -12832,9 +12832,9 @@ "dev": true }, "adm-zip": { - "version": "0.4.7", - "resolved": "https://registry.npmjs.org/adm-zip/-/adm-zip-0.4.7.tgz", - "integrity": "sha1-hgbCy/HEJs6MjsABdER/1Jtur8E=" + "version": "0.4.16", + "resolved": "https://registry.npmjs.org/adm-zip/-/adm-zip-0.4.16.tgz", + "integrity": "sha512-TFi4HBKSGfIKsK5YCkKaaFG2m4PEDyViZmEwof3MTIgzimHLto6muaHVpbrljdIvIrFZzEq/p4nafOeLcYegrg==" }, "agent-base": { "version": "4.3.0", diff --git a/package.json b/package.json index d5f9362a36d..16cff26d9f2 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,7 @@ "test": "snyk test" }, "dependencies": { - "adm-zip": "0.4.7", + "adm-zip": "0.4.16", "body-parser": "1.9.0", "cfenv": "^1.0.4", "consolidate": "0.14.5", diff --git a/routes/index.js b/routes/index.js index 6b5455f03e4..24c6ab82189 100644 --- a/routes/index.js +++ b/routes/index.js @@ -254,7 +254,7 @@ exports.import = function (req, res, next) { if (importedFileType["mime"] === zipFileExt["mime"]) { var zip = AdmZip(importFile.data); var extracted_path = "/tmp/extracted_files"; - zip.extractAllTo(extracted_path, true); + utils.safe_extract_zip(zip, extracted_path); data = "No backup.txt file found"; fs.readFile('backup.txt', 'ascii', function (err, data) { if (!err) { diff --git a/tests/zip-slip.spec.js b/tests/zip-slip.spec.js new file mode 100644 index 00000000000..c968a72d2f7 --- /dev/null +++ b/tests/zip-slip.spec.js @@ -0,0 +1,44 @@ +var tap = require('tap'); +var fs = require('fs'); +var os = require('os'); +var path = require('path'); +var AdmZip = require('adm-zip'); +var utils = require('../utils'); + +// entries: ['../../usr/src/goof/public/about.html', 'backup.txt'] +var maliciousZip = path.join(__dirname, '..', 'exploits', 'zip-slip', 'malicious_backup.zip'); +var traversalTarget = path.join('usr', 'src', 'goof', 'public', 'about.html'); + +function tempDest() { + var base = fs.mkdtempSync(path.join(os.tmpdir(), 'zip-slip-')); + var dest = path.join(base, 'a', 'b', 'extracted_files'); + // where the '../../' entry of malicious_backup.zip lands when unchecked + return { base: base, dest: dest, escaped: path.resolve(dest, '..', '..', traversalTarget) }; +} + +tap.test('safe_extract_path rejects entries escaping the destination', function (t) { + var root = '/tmp/extracted_files'; + + t.equal(utils.safe_extract_path(root, '../../../../etc/cron.d/x'), null); + t.equal(utils.safe_extract_path(root, 'a/../../outside.txt'), null); + t.equal(utils.safe_extract_path(root, '/etc/passwd'), null); + t.equal(utils.safe_extract_path(root, '..\\..\\outside.txt'), null); + t.equal(utils.safe_extract_path(root, ''), null); + t.equal(utils.safe_extract_path(root, 'backup.txt'), path.join(root, 'backup.txt')); + t.equal(utils.safe_extract_path(root, 'dir/backup.txt'), path.join(root, 'dir', 'backup.txt')); + t.end(); +}); + +tap.test('safe_extract_zip writes safe entries and skips traversal entries', function (t) { + var tmp = tempDest(); + var escaped = tmp.escaped; + + var written = utils.safe_extract_zip(new AdmZip(maliciousZip), tmp.dest); + + t.ok(fs.existsSync(path.join(tmp.dest, 'backup.txt')), 'safe entry extracted'); + t.notOk(fs.existsSync(escaped), 'traversal entry must not be written outside the destination'); + t.equal(written.length, 1); + + fs.rmSync(tmp.base, { recursive: true, force: true }); + t.end(); +}); diff --git a/utils.js b/utils.js index 4ecf7d9aefa..20c9aff113f 100644 --- a/utils.js +++ b/utils.js @@ -1,3 +1,6 @@ +var fs = require( 'fs' ); +var path = require( 'path' ); + module.exports = { ran_no : function ( min, max ){ @@ -17,6 +20,50 @@ module.exports = { return str; }, + // Resolves a zip entry name inside root, or null when the entry escapes it + // (Zip Slip: '../' segments, absolute paths, drive-relative names). + safe_extract_path : function ( root, entry_name ){ + if ( typeof entry_name !== 'string' || entry_name === '' ){ + return null; + } + + var resolved_root = path.resolve( root ); + var target = path.resolve( resolved_root, entry_name.replace( /\\/g, '/' )); + + if ( target !== resolved_root && target.indexOf( resolved_root + path.sep ) !== 0 ){ + return null; + } + + return target; + }, + + // Extracts every entry of an AdmZip instance below dest, skipping entries + // whose name would write outside of it. Returns the written paths. + safe_extract_zip : function ( zip, dest ){ + var self = this; + var extracted = []; + + zip.getEntries().forEach( function ( entry ){ + var target = self.safe_extract_path( dest, entry.entryName ); + + if ( target === null ){ + console.error( 'skipping unsafe zip entry: ' + entry.entryName ); + return; + } + + if ( entry.isDirectory ){ + fs.mkdirSync( target, { recursive : true }); + return; + } + + fs.mkdirSync( path.dirname( target ), { recursive : true }); + fs.writeFileSync( target, entry.getData()); + extracted.push( target ); + }); + + return extracted; + }, + forbidden : function ( res ){ var body = 'Forbidden'; res.statusCode = 403;