From c6ca7a53ab06821e2ddd963aa9715b94d3923ad1 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Fri, 5 Mar 2021 10:12:31 +0000 Subject: [PATCH] Polish "Prevent extracting zip entries outside of destination path" See gh-25505 --- .../jarmode/layertools/ExtractCommand.java | 22 ++++++++------- .../layertools/ExtractCommandTests.java | 28 ++++++++++++++++++- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/spring-boot-project/spring-boot-tools/spring-boot-jarmode-layertools/src/main/java/org/springframework/boot/jarmode/layertools/ExtractCommand.java b/spring-boot-project/spring-boot-tools/spring-boot-jarmode-layertools/src/main/java/org/springframework/boot/jarmode/layertools/ExtractCommand.java index 20ae04af7f1..82f4f4f8aa4 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-jarmode-layertools/src/main/java/org/springframework/boot/jarmode/layertools/ExtractCommand.java +++ b/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 java.util.zip.ZipInputStream; import org.springframework.util.Assert; import org.springframework.util.StreamUtils; -import org.springframework.util.StringUtils; /** * The {@code 'extract'} tools command. @@ -86,15 +85,18 @@ class ExtractCommand extends Command { } 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.getCanonicalPath().startsWith(destination.getCanonicalPath() + File.separator)) { - 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 { diff --git a/spring-boot-project/spring-boot-tools/spring-boot-jarmode-layertools/src/test/java/org/springframework/boot/jarmode/layertools/ExtractCommandTests.java b/spring-boot-project/spring-boot-tools/spring-boot-jarmode-layertools/src/test/java/org/springframework/boot/jarmode/layertools/ExtractCommandTests.java index 2846eefcbda..fe3b9140339 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-jarmode-layertools/src/test/java/org/springframework/boot/jarmode/layertools/ExtractCommandTests.java +++ b/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.io.IOException; 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 @@ import static org.mockito.BDDMockito.given; * Tests for {@link ExtractCommand}. * * @author Phillip Webb + * @author Andy Wilkinson */ class ExtractCommandTests { @@ -76,6 +78,7 @@ class ExtractCommandTests { 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 @@ class ExtractCommandTests { 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 @@ class ExtractCommandTests { .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 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 @@ class ExtractCommandTests { out.closeEntry(); out.putNextEntry(new ZipEntry("d/")); out.closeEntry(); + streamHandler.accept(out); } return file; }