Bug 535822 - Check for directory traversal entry paths

Change-Id: Ic989745f1115d3c23866957519f1d9fc8f315640
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>

4 files changed, 142 insertions, 22 deletions

diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleInstallUpdateTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleInstallUpdateTests.java
index 029f856a6..3faadc65d 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleInstallUpdateTests.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleInstallUpdateTests.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2009, 2016 IBM Corporation and others.
+ * Copyright (c) 2009, 2018 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -10,13 +10,28 @@
  *******************************************************************************/
 package org.eclipse.osgi.tests.bundles;
 
-import java.io.*;
-import java.net.*;
-import java.util.*;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.MalformedURLException;
+import java.net.URISyntaxException;
+import java.net.URL;
+import java.net.URLEncoder;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
 import junit.framework.Test;
 import junit.framework.TestSuite;
+import org.eclipse.osgi.service.urlconversion.URLConverter;
 import org.eclipse.osgi.tests.OSGiTestsActivator;
-import org.osgi.framework.*;
+import org.osgi.framework.Bundle;
+import org.osgi.framework.BundleException;
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.framework.ServiceRegistration;
 import org.osgi.framework.hooks.bundle.CollisionHook;
 
 public class BundleInstallUpdateTests extends AbstractBundleTests {
@@ -367,4 +382,49 @@ public class BundleInstallUpdateTests extends AbstractBundleTests {
 		is = encodedURL.openStream();
 		is.close();
 	}
+
+	public void testEscapeZipRoot() throws IOException, BundleException, InvalidSyntaxException, URISyntaxException {
+		String entry1 = "../../escapedZipRoot1.txt";
+		String entry2 = "dir1/../../../escapedZipRoot2.txt";
+		String cp1 = "../../cp.jar";
+		File bundlesDirectory = OSGiTestsActivator.getContext().getDataFile(getName());
+		bundlesDirectory.mkdirs();
+		Map<String, String> headers = new HashMap<String, String>();
+		headers.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+		headers.put(Constants.BUNDLE_SYMBOLICNAME, getName());
+		headers.put(Constants.BUNDLE_CLASSPATH, "., ../../cp.jar");
+		Map<String, String> entries = new HashMap<>();
+		entries.put(entry1, "value");
+		entries.put(entry2, "value");
+		entries.put(cp1, "value");
+
+		File testBundleJarFile = SystemBundleTests.createBundle(bundlesDirectory, getName(), headers, entries);
+		Bundle testBundle = getContext().installBundle(getName(), new FileInputStream(testBundleJarFile));
+		testBundle.start();
+		try {
+			testBundle.loadClass("does.not.exist.Test");
+		} catch (ClassNotFoundException e) {
+			// expected
+		}
+		URLConverter bundleURLConverter = getContext().getService(getContext().getServiceReferences(URLConverter.class, "(protocol=bundleentry)").iterator().next());
+
+		URL dir1 = bundleURLConverter.toFileURL(testBundle.getEntry("dir1/"));
+		File dir1File = new File(dir1.toExternalForm().substring(5));
+
+		File dir1EscapedFile2 = new File(dir1File, entry2.substring("dir1".length()));
+		assertFalse("File escaped zip root: " + dir1EscapedFile2.getCanonicalPath(), dir1EscapedFile2.exists());
+
+		URL root = bundleURLConverter.toFileURL(testBundle.getEntry("/"));
+		File rootFile = new File(root.toExternalForm().substring(5));
+
+		File rootEscapedFile1 = new File(rootFile, entry1);
+		assertFalse("File escaped zip root: " + rootEscapedFile1.getCanonicalPath(), rootEscapedFile1.exists());
+
+		File rootEscapedFile2 = new File(rootFile, entry2);
+		assertFalse("File escaped zip root: " + rootEscapedFile2.getCanonicalPath(), rootEscapedFile2.exists());
+
+		File rootEscapedFile3 = new File(rootFile, cp1);
+		assertFalse("File escaped zip root: " + rootEscapedFile3.getCanonicalPath(), rootEscapedFile3.exists());
+
+	}
 }
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/BundleInfo.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/BundleInfo.java
index e4ebfd822..e5ddf7161 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/BundleInfo.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/BundleInfo.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2012, 2017 IBM Corporation and others.
+ * Copyright (c) 2012, 2018 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -41,6 +41,7 @@ import org.eclipse.osgi.internal.framework.EquinoxContainer;
 import org.eclipse.osgi.internal.hookregistry.StorageHookFactory;
 import org.eclipse.osgi.internal.hookregistry.StorageHookFactory.StorageHook;
 import org.eclipse.osgi.internal.messages.Msg;
+import org.eclipse.osgi.storage.Storage.StorageException;
 import org.eclipse.osgi.storage.bundlefile.BundleEntry;
 import org.eclipse.osgi.storage.bundlefile.BundleFile;
 import org.eclipse.osgi.storage.url.BundleResourceHandler;
@@ -308,15 +309,31 @@ public final class BundleInfo {
 		 * not exist if the content has not previously been stored.
 		 * @param path the path to the content to extract from the generation
 		 * @return a file object where content of the specified path may be stored.
+		 * @throws StorageException if the path will escape the persistent storage of the generation
 		 */
 		public File getExtractFile(String path) {
-			StringBuilder builder = new StringBuilder();
-			builder.append(getBundleId()).append('/').append(getGenerationId());
-			if (path.length() > 0 && path.charAt(0) != '/') {
-				builder.append('/');
+			return getExtractFile(null, path);
+		}
+
+		/**
+		 * Gets called by BundleFile during {@link BundleFile#getFile(String, boolean)}.  This method 
+		 * will allocate a File object where content of the specified path may be 
+		 * stored for this generation.  The returned File object may 
+		 * not exist if the content has not previously been stored.
+		 * @param path the path to the content to extract from the generation
+		 * @param base the base path that is prepended to the path, may be null
+		 * @return a file object where content of the specified path may be stored.
+		 * @throws StorageException if the path will escape the persistent storage of 
+		 * the generation starting at the specified base
+		 */
+		public File getExtractFile(String base, String path) {
+			StringBuilder baseBuilder = new StringBuilder();
+			baseBuilder.append(getBundleId()).append('/').append(getGenerationId());
+			if (base != null) {
+				baseBuilder.append('/').append(base);
 			}
-			builder.append(path);
-			return getStorage().getFile(builder.toString(), true);
+
+			return getStorage().getFile(baseBuilder.toString(), path, true);
 		}
 
 		public void storeContent(File destination, InputStream in, boolean nativeCode) throws IOException {
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
index 9b89de67b..578581f19 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
@@ -110,6 +110,27 @@ import org.osgi.resource.Namespace;
 import org.osgi.resource.Requirement;
 
 public class Storage {
+	public static class StorageException extends RuntimeException {
+		private static final long serialVersionUID = 1L;
+
+		public StorageException() {
+			super();
+		}
+
+		public StorageException(String message, Throwable cause) {
+			super(message, cause);
+		}
+
+		public StorageException(String message) {
+			super(message);
+		}
+
+		public StorageException(Throwable cause) {
+			super(cause);
+		}
+
+	}
+
 	public static final int VERSION = 4;
 	private static final int MR_JAR_VERSION = 4;
 	private static final int LOWEST_VERSION_SUPPORTED = 3;
@@ -931,15 +952,19 @@ public class Storage {
 		return bundleID + "/" + generationID + "/" + BUNDLE_FILE_NAME; //$NON-NLS-1$ //$NON-NLS-2$
 	}
 
-	public File getFile(String path, boolean checkParent) {
+	public File getFile(String path, boolean checkParent) throws StorageException {
+		return getFile(null, path, checkParent);
+	}
+
+	public File getFile(String base, String path, boolean checkParent) throws StorageException {
 		// first check the child location
-		File childPath = new File(childRoot, path);
+		File childPath = getFile(childRoot, base, path);
 		// now check the parent
 		if (checkParent && parentRoot != null) {
 			if (childPath.exists()) {
 				return childPath;
 			}
-			File parentPath = new File(parentRoot, path);
+			File parentPath = getFile(parentRoot, base, path);
 			if (parentPath.exists()) {
 				// only use the parent file only if it exists;
 				return parentPath;
@@ -949,6 +974,25 @@ public class Storage {
 		return childPath;
 	}
 
+	private static File getFile(File root, String base, String path) {
+		if (base != null) {
+			// if base is not null then move root to include the base
+			root = new File(root, base);
+		}
+		File result = new File(root, path);
+
+		try {
+			String resultCanonical = result.getCanonicalPath();
+			String rootCanonical = root.getCanonicalPath();
+			if (!resultCanonical.startsWith(rootCanonical + File.separator) && !resultCanonical.equals(rootCanonical)) {
+				throw new StorageException("Invalid path: " + path); //$NON-NLS-1$
+			}
+		} catch (IOException e) {
+			throw new StorageException("Invalid path: " + path, e); //$NON-NLS-1$
+		}
+		return result;
+	}
+
 	private File stageContent(final InputStream in, final URL sourceURL) throws BundleException {
 		if (System.getSecurityManager() == null)
 			return stageContent0(in, sourceURL);
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/bundlefile/ZipBundleFile.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/bundlefile/ZipBundleFile.java
index 915f5a47d..03bee5a59 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/bundlefile/ZipBundleFile.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/bundlefile/ZipBundleFile.java
@@ -31,6 +31,7 @@ import org.eclipse.osgi.internal.debug.Debug;
 import org.eclipse.osgi.internal.framework.EquinoxContainer;
 import org.eclipse.osgi.internal.messages.Msg;
 import org.eclipse.osgi.storage.BundleInfo;
+import org.eclipse.osgi.storage.Storage.StorageException;
 import org.eclipse.osgi.util.NLS;
 
 /**
@@ -190,13 +191,7 @@ public class ZipBundleFile extends BundleFile {
 	private File getExtractFile(String entryName) {
 		if (generation == null)
 			return null;
-		String path = ".cp"; /* put all these entries in this subdir *///$NON-NLS-1$
-		String name = entryName.replace('/', File.separatorChar);
-		if ((name.length() > 1) && (name.charAt(0) == File.separatorChar)) /* if name has a leading slash */
-			path = path.concat(name);
-		else
-			path = path + File.separator + name;
-		return generation.getExtractFile(path);
+		return generation.getExtractFile(".cp", entryName); //$NON-NLS-1$
 	}
 
 	public File getFile(String entry, boolean nativeCode) {
@@ -241,6 +236,10 @@ public class ZipBundleFile extends BundleFile {
 				if (debug.DEBUG_BUNDLE_FILE)
 					Debug.printStackTrace(e);
 				generation.getBundleInfo().getStorage().getLogServices().log(EquinoxContainer.NAME, FrameworkLogEntry.ERROR, "Unable to extract content: " + generation.getRevision() + ": " + entry, e); //$NON-NLS-1$ //$NON-NLS-2$
+			} catch (StorageException e) {
+				if (debug.DEBUG_BUNDLE_FILE)
+					Debug.printStackTrace(e);
+				generation.getBundleInfo().getStorage().getLogServices().log(EquinoxContainer.NAME, FrameworkLogEntry.ERROR, "Unable to extract content: " + generation.getRevision() + ": " + entry, e); //$NON-NLS-1$ //$NON-NLS-2$
 			}
 		} finally {
 			openLock.unlock();