Fix potential vulnerability

Fixes a potential vulnerability that allows to overwrite files outside the plugin directory using a manipulated SMP archive.

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;
 }

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();
 

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 ---------------------------------------------------------------