SCANJLIB-229 Improve tar.gz extraction

Author: henryju

lib/src/main/java/org/sonarsource/scanner/lib/internal/util/CompressionUtils.java

@@ -85,7 +85,7 @@ public static Path unzip(Path zip, Path toDir, Predicate<ZipEntry> filter) throw
         if (filter.test(entry)) {
           var target = toDir.resolve(entry.getName());
 
-          verifyInsideTargetDirectory(entry, target, targetDirNormalizedPath);
+          verifyInsideTargetDirectory(entry.getName(), target, targetDirNormalizedPath);
 
           if (entry.isDirectory()) {
             throwExceptionIfDirectoryIsNotCreatable(target);
@@ -100,10 +100,10 @@ public static Path unzip(Path zip, Path toDir, Predicate<ZipEntry> filter) throw
     }
   }
 
-  private static void verifyInsideTargetDirectory(ZipEntry entry, Path entryPath, Path targetDirNormalizedPath) {
+  private static void verifyInsideTargetDirectory(String entryName, Path entryPath, Path targetDirNormalizedPath) {
     if (!entryPath.normalize().startsWith(targetDirNormalizedPath)) {
       // vulnerability - trying to create a file outside the target directory
-      throw new IllegalStateException("Unzipping an entry outside the target directory is not allowed: " + entry.getName());
+      throw new IllegalStateException("Extracting an entry outside the target directory is not allowed: " + entryName);
     }
   }
 
@@ -123,6 +123,7 @@ private static void copy(ZipFile zipFile, ZipEntry entry, Path to) throws IOExce
   }
 
   public static void extractTarGz(Path compressedFile, Path targetDir) throws IOException {
+    Path targetDirNormalizedPath = targetDir.normalize();
     try (InputStream fis = Files.newInputStream(compressedFile);
       InputStream bis = new BufferedInputStream(fis);
       InputStream gzis = new GzipCompressorInputStream(bis);
@@ -132,18 +133,21 @@ public static void extractTarGz(Path compressedFile, Path targetDir) throws IOEx
         if (!tarArchiveInputStream.canReadEntryData(targzEntry)) {
           continue;
         }
-        var entry = targetDir.resolve(targzEntry.getName());
+        var target = targetDir.resolve(targzEntry.getName());
+
+        verifyInsideTargetDirectory(targzEntry.getName(), target, targetDirNormalizedPath);
+
         if (targzEntry.isDirectory()) {
-          Files.createDirectories(entry);
+          Files.createDirectories(target);
         } else {
-          if (!Files.isDirectory(entry.getParent())) {
-            Files.createDirectories(entry.getParent());
+          if (!Files.isDirectory(target.getParent())) {
+            Files.createDirectories(target.getParent());
           }
-          Files.copy(tarArchiveInputStream, entry, StandardCopyOption.REPLACE_EXISTING);
+          Files.copy(tarArchiveInputStream, target, StandardCopyOption.REPLACE_EXISTING);
           int mode = targzEntry.getMode();
           if (mode != 0 && !IS_OS_WINDOWS) {
             Set<PosixFilePermission> permissions = fromFileMode(mode);
-            Files.setPosixFilePermissions(entry, permissions);
+            Files.setPosixFilePermissions(target, permissions);
           }
         }
       }

lib/src/test/java/org/sonarsource/scanner/lib/internal/util/CompressionUtilsTest.java

@@ -67,7 +67,7 @@ void fail_if_unzipping_file_outside_target_directory() {
 
     assertThatThrownBy(() -> CompressionUtils.unzip(zip, toDir))
       .isInstanceOf(IllegalStateException.class)
-      .hasMessage("Unzipping an entry outside the target directory is not allowed: ../../../../../../../../../../../../../../../../" +
+      .hasMessage("Extracting an entry outside the target directory is not allowed: ../../../../../../../../../../../../../../../../" +
         "../../../../../../../../../../../../../../../../../../../../../../../../tmp/evil.txt");
   }
 
@@ -79,6 +79,19 @@ void extract_tar_gz() throws IOException {
     assertThat(toDir.toFile().list()).hasSize(3);
   }
 
+  @Test
+  void fail_if_extracting_targz_file_outside_target_directory() {
+    var targz = Paths.get("src/test/resources/slip.tar.gz");
+    var toDir = temp.resolve("dir");
+
+    assertThatThrownBy(() -> CompressionUtils.extractTarGz(targz, toDir))
+      .isInstanceOf(IllegalStateException.class)
+      .hasMessage("Extracting an entry outside the target directory is not allowed: ../../../../../../../../../../../../../../../../../"
+        + "../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../"
+        + "../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../"
+        + "../tmp/slipped.txt");
+  }
+
   @Test
   void fileMode_conversion() {
     assertThat(CompressionUtils.fromFileMode(Integer.parseInt("000", 8))).isEmpty();