diff --git a/scm-core/src/main/java/sonia/scm/plugin/SmpArchive.java b/scm-core/src/main/java/sonia/scm/plugin/SmpArchive.java index bb750fdfd7..c49252aab8 100644 --- a/scm-core/src/main/java/sonia/scm/plugin/SmpArchive.java +++ b/scm-core/src/main/java/sonia/scm/plugin/SmpArchive.java @@ -21,35 +21,32 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.plugin; //~--- non-JDK imports -------------------------------------------------------- -import com.google.common.base.Charsets; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.common.io.ByteSource; import com.google.common.io.ByteStreams; import com.google.common.io.Files; import com.google.common.io.Resources; - import sonia.scm.util.IOUtil; -//~--- JDK imports ------------------------------------------------------------ - import java.io.File; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; - import java.net.URL; - import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.nio.file.Path; - import java.util.zip.ZipEntry; import java.util.zip.ZipInputStream; +//~--- JDK imports ------------------------------------------------------------ + /** * Smp plugin archive. * @@ -65,9 +62,14 @@ public final class SmpArchive * * @param archive */ - public SmpArchive(ByteSource archive) - { + public SmpArchive(ByteSource archive) { + this(archive, source -> new ZipInputStream(archive.openStream(), StandardCharsets.UTF_8)); + } + + @VisibleForTesting + SmpArchive(ByteSource archive, ZipInputStreamFactory zipInputStreamFactory) { this.archive = archive; + this.zipInputStreamFactory = zipInputStreamFactory; } //~--- methods -------------------------------------------------------------- @@ -167,6 +169,9 @@ public final class SmpArchive String fileName = ze.getName(); File file = new File(target, fileName); + if (!IOUtil.isChild(target, file)) { + throw new PluginException("smp contains illegal path, which tries to escape from the plugin directory: " + fileName); + } IOUtil.mkdirs(file.getParentFile()); @@ -285,9 +290,8 @@ public final class SmpArchive * * @throws IOException */ - private ZipInputStream open() throws IOException - { - return new ZipInputStream(archive.openStream(), Charsets.UTF_8); + private ZipInputStream open() throws IOException { + return zipInputStreamFactory.open(archive); } /** @@ -300,7 +304,7 @@ public final class SmpArchive */ private NonClosingZipInputStream openNonClosing() throws IOException { - return new NonClosingZipInputStream(archive.openStream(), Charsets.UTF_8); + return new NonClosingZipInputStream(archive.openStream(), StandardCharsets.UTF_8); } //~--- inner classes -------------------------------------------------------- @@ -398,12 +402,20 @@ public final class SmpArchive } } + @FunctionalInterface + interface ZipInputStreamFactory { + + ZipInputStream open(ByteSource source) throws IOException; + + } + //~--- fields --------------------------------------------------------------- - /** Field description */ private final ByteSource archive; + private final ZipInputStreamFactory zipInputStreamFactory; + /** Field description */ private InstalledPluginDescriptor plugin; } diff --git a/scm-core/src/test/java/sonia/scm/plugin/SmpArchiveTest.java b/scm-core/src/test/java/sonia/scm/plugin/SmpArchiveTest.java index c1c4bb80ec..158cd9c502 100644 --- a/scm-core/src/test/java/sonia/scm/plugin/SmpArchiveTest.java +++ b/scm-core/src/test/java/sonia/scm/plugin/SmpArchiveTest.java @@ -28,6 +28,7 @@ package sonia.scm.plugin; import com.google.common.base.Charsets; import com.google.common.base.Strings; +import com.google.common.io.ByteSource; import com.google.common.io.Files; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -46,10 +47,13 @@ import java.io.FileOutputStream; import java.io.IOException; import java.nio.file.Path; import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; import java.util.zip.ZipOutputStream; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; //~--- JDK imports ------------------------------------------------------------ @@ -106,6 +110,18 @@ class SmpArchiveTest { assertThrows(PluginException.class, smp::getPlugin); } + @Test + void shouldFailOnZipEntriesWhichCreateFilesOutsideOfThePluginFolder(@TempDir Path tempDir) throws IOException { + ZipInputStream zis = mock(ZipInputStream.class); + ZipEntry entry = mock(ZipEntry.class); + when(zis.getNextEntry()).thenReturn(entry); + when(entry.getName()).thenReturn("../../plugin.xml"); + SmpArchive smp = new SmpArchive(ByteSource.empty(), source -> zis); + File directory = tempDir.resolve("one").resolve("two").resolve("three").toFile(); + assertThat(directory.mkdirs()).isTrue(); + assertThrows(PluginException.class, () -> smp.extract(directory)); + } + private File createArchive(Path tempDir, String name, String version) throws IOException, XMLStreamException { File descriptor = tempDir.resolve("descriptor.xml").toFile(); diff --git a/scm-test/src/main/java/sonia/scm/repository/spi/ZippedRepositoryTestBase.java b/scm-test/src/main/java/sonia/scm/repository/spi/ZippedRepositoryTestBase.java index 0ea8887ff6..6994667056 100644 --- a/scm-test/src/main/java/sonia/scm/repository/spi/ZippedRepositoryTestBase.java +++ b/scm-test/src/main/java/sonia/scm/repository/spi/ZippedRepositoryTestBase.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.repository.spi; //~--- non-JDK imports -------------------------------------------------------- @@ -150,18 +150,18 @@ public abstract class ZippedRepositoryTestBase extends AbstractTestBase public static void extract(File targetFolder, String zippedRepositoryResource) throws IOException { URL url = Resources.getResource(zippedRepositoryResource); - ZipInputStream zip = null; - try + try (ZipInputStream zip = new ZipInputStream(url.openStream());) { - zip = new ZipInputStream(url.openStream()); - ZipEntry entry = zip.getNextEntry(); while (entry != null) { File file = new File(targetFolder, entry.getName()); File parent = file.getParentFile(); + if (!IOUtil.isChild(parent, file)) { + throw new IOException("invalid zip entry name"); + } if (!parent.exists()) { @@ -174,27 +174,16 @@ public abstract class ZippedRepositoryTestBase extends AbstractTestBase } else { - OutputStream output = null; - - try + try (OutputStream output = new FileOutputStream(file)) { - output = new FileOutputStream(file); IOUtil.copy(zip, output); } - finally - { - IOUtil.close(output); - } } zip.closeEntry(); entry = zip.getNextEntry(); } } - finally - { - IOUtil.close(zip); - } } //~--- fields ---------------------------------------------------------------