Merge pull request #25505 from trungPa

* gh-25505:
  Polish "Prevent extracting zip entries outside of destination path"
  Prevent extracting zip entries outside of destination path

Closes gh-25505

Author: wilkinsona

spring-boot-project/spring-boot-tools/spring-boot-jarmode-layertools/src/main/java/org/springframework/boot/jarmode/layertools/ExtractCommand.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2020 the original author or authors.
+ * Copyright 2012-2021 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -29,7 +29,6 @@
 
 import org.springframework.util.Assert;
 import org.springframework.util.StreamUtils;
-import org.springframework.util.StringUtils;
 
 /**
  * The {@code 'extract'} tools command.
@@ -86,15 +85,18 @@ protected void run(Map<Option, String> options, List<String> parameters) {
 	}
 
 	private void write(ZipInputStream zip, ZipEntry entry, File destination) throws IOException {
-		String path = StringUtils.cleanPath(entry.getName());
-		File file = new File(destination, path);
-		if (file.getAbsolutePath().startsWith(destination.getAbsolutePath())) {
-			mkParentDirs(file);
-			try (OutputStream out = new FileOutputStream(file)) {
-				StreamUtils.copy(zip, out);
-			}
-			Files.setAttribute(file.toPath(), "creationTime", entry.getCreationTime());
+		String canonicalOutputPath = destination.getCanonicalPath() + File.separator;
+		File file = new File(destination, entry.getName());
+		String canonicalEntryPath = file.getCanonicalPath();
+		Assert.state(canonicalEntryPath.startsWith(canonicalOutputPath),
+				() -> "Entry '" + entry.getName() + "' would be written to '" + canonicalEntryPath
+						+ "'. This is outside the output location of '" + canonicalOutputPath
+						+ "'. Verify the contents of your archive.");
+		mkParentDirs(file);
+		try (OutputStream out = new FileOutputStream(file)) {
+			StreamUtils.copy(zip, out);
 		}
+		Files.setAttribute(file.toPath(), "creationTime", entry.getCreationTime());
 	}
 
 	private void mkParentDirs(File file) throws IOException {

spring-boot-project/spring-boot-tools/spring-boot-jarmode-layertools/src/test/java/org/springframework/boot/jarmode/layertools/ExtractCommandTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2020 the original author or authors.
+ * Copyright 2012-2021 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -23,6 +23,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Iterator;
+import java.util.function.Consumer;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipOutputStream;
 
@@ -40,6 +41,7 @@
  * Tests for {@link ExtractCommand}.
  *
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 class ExtractCommandTests {
 
@@ -76,6 +78,7 @@ void runExtractsLayers() throws Exception {
 		assertThat(new File(this.extract, "b/b/b.jar")).exists();
 		assertThat(new File(this.extract, "c/c/c.jar")).exists();
 		assertThat(new File(this.extract, "d")).isDirectory();
+		assertThat(new File(this.extract.getParentFile(), "e.jar")).doesNotExist();
 	}
 
 	@Test
@@ -95,6 +98,7 @@ void runWhenHasLayerParamsExtractsLimitedLayers() {
 		assertThat(this.extract.list()).containsOnly("a", "c");
 		assertThat(new File(this.extract, "a/a/a.jar")).exists();
 		assertThat(new File(this.extract, "c/c/c.jar")).exists();
+		assertThat(new File(this.extract.getParentFile(), "e.jar")).doesNotExist();
 	}
 
 	@Test
@@ -110,7 +114,28 @@ void runWithJarFileContainingNoEntriesFails() throws IOException {
 				.withMessageContaining("not compatible with layertools");
 	}
 
+	@Test
+	void runWithJarFileThatWouldWriteEntriesOutsideDestinationFails() throws IOException {
+		this.jarFile = createJarFile("test.jar", (out) -> {
+			try {
+				out.putNextEntry(new ZipEntry("e/../../e.jar"));
+				out.closeEntry();
+			}
+			catch (IOException ex) {
+				throw new IllegalStateException(ex);
+			}
+		});
+		assertThatIllegalStateException()
+				.isThrownBy(() -> this.command.run(Collections.emptyMap(), Collections.emptyList()))
+				.withMessageContaining("Entry 'e/../../e.jar' would be written");
+	}
+
 	private File createJarFile(String name) throws IOException {
+		return createJarFile(name, (out) -> {
+		});
+	}
+
+	private File createJarFile(String name, Consumer<ZipOutputStream> streamHandler) throws IOException {
 		File file = new File(this.temp, name);
 		try (ZipOutputStream out = new ZipOutputStream(new FileOutputStream(file))) {
 			out.putNextEntry(new ZipEntry("a/"));
@@ -127,6 +152,7 @@ private File createJarFile(String name) throws IOException {
 			out.closeEntry();
 			out.putNextEntry(new ZipEntry("d/"));
 			out.closeEntry();
+			streamHandler.accept(out);
 		}
 		return file;
 	}