bug: fix Zip Slip in POST /import archive extraction

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",

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) {

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();
+});

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;