From 8b26c260d15107a16fc090e18da5190884b57bd6 Mon Sep 17 00:00:00 2001
From: Ryan Ernst <ryan@iernst.net>
Date: Fri, 11 Mar 2016 14:53:14 -0800
Subject: [PATCH] Plugins: Enforce plugin zip does not contain zip entries
 outside of the unzip dir

When unzipping a plugin zip, the zip entries are resolved relative to
the directory being unzipped into. However, there are currently no
checks that the entry name was not absolute, or relatively points
outside of the plugin dir. This change adds a check for those two cases.
---
 .../elasticsearch/plugins/InstallPluginCommand.java  |  9 ++++++++-
 .../plugins/InstallPluginCommandTests.java           | 12 ++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java b/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java
index 32c4bf18507..e72eb2100f6 100644
--- a/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java
+++ b/core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java
@@ -250,7 +250,14 @@ class InstallPluginCommand extends Command {
                 }
                 hasEsDir = true;
                 Path targetFile = target.resolve(entry.getName().substring("elasticsearch/".length()));
-                // TODO: handle name being an absolute path
+
+                // Using the entry name as a path can result in an entry outside of the plugin dir, either if the
+                // name starts with the root of the filesystem, or it is a relative entry like ../whatever.
+                // This check attempts to identify both cases by first normalizing the path (which removes foo/..)
+                // and ensuring the normalized entry is still rooted with the target plugin directory.
+                if (targetFile.normalize().startsWith(target) == false) {
+                    throw new IOException("Zip contains entry name '" + entry.getName() + "' resolving outside of plugin directory");
+                }
 
                 // be on the safe side: do not rely on that directories are always extracted
                 // before their children (although this makes sense, but is it guaranteed?)
diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java
index 514090d9869..fb69c817f3a 100644
--- a/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java
+++ b/qa/evil-tests/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java
@@ -466,6 +466,18 @@ public class InstallPluginCommandTests extends ESTestCase {
         assertInstallCleaned(env);
     }
 
+    public void testZipRelativeOutsideEntryName() throws Exception {
+        Path zip = createTempDir().resolve("broken.zip");
+        try (ZipOutputStream stream = new ZipOutputStream(Files.newOutputStream(zip))) {
+            stream.putNextEntry(new ZipEntry("elasticsearch/../blah"));
+        }
+        String pluginZip = zip.toUri().toURL().toString();
+        IOException e = expectThrows(IOException.class, () -> {
+            installPlugin(pluginZip, createEnv());
+        });
+        assertTrue(e.getMessage(), e.getMessage().contains("resolving outside of plugin directory"));
+    }
+
     // TODO: test batch flag?
     // TODO: test checksum (need maven/official below)
     // TODO: test maven, official, and staging install...need tests with fixtures...