Fix tar/untar to never add current dir entry and avoid traversal attacks (#283)

Author: jonesbusy

src/main/java/land/oras/utils/ArchiveUtils.java

@@ -92,7 +92,13 @@ public static LocalPath tar(LocalPath sourceDir) {
                 paths.forEach(path -> {
                     LOG.trace("Visiting path: {}", path);
                     try {
-                        String entryName = sourceDir.getPath().relativize(path).toString();
+
+                        Path relativePath = sourceDir.getPath().relativize(path);
+                        if (relativePath.toString().isEmpty()) {
+                            LOG.trace("Skipping root directory: {}", path);
+                            return;
+                        }
+                        String entryName = relativePath.toString();
 
                         TarArchiveEntry entry = null;
                         BasicFileAttributes attrs = Files.readAttributes(path, BasicFileAttributes.class);
@@ -142,6 +148,21 @@ public static LocalPath tar(LocalPath sourceDir) {
         return LocalPath.of(tarFile, Const.DEFAULT_BLOB_MEDIA_TYPE);
     }
 
+    /**
+     * Ensure that the entry is safe to extract
+     * @param entry The tar entry
+     * @param target The target directory
+     * @throws IOException
+     */
+    static void ensureSafeEntry(TarArchiveEntry entry, Path target) throws IOException {
+        // Prevent path traversal attacks
+        Path outputPath = target.resolve(entry.getName()).normalize();
+        Path normalizedTarget = target.toAbsolutePath().normalize();
+        if (!outputPath.startsWith(normalizedTarget)) {
+            throw new IOException("Entry is outside of the target dir: " + target);
+        }
+    }
+
     /**
      * Extract a tar file to a target directory
      * @param fis The archive stream
@@ -161,6 +182,9 @@ public static void untar(InputStream fis, Path target) {
                     // Prevent path traversal attacks
                     Path outputPath = target.resolve(entry.getName()).normalize();
 
+                    // Check if the entry is outside the target directory
+                    ensureSafeEntry(entry, target);
+
                     LOG.trace("Extracting entry: {}", entry.getName());
 
                     if (entry.isDirectory()) {

src/test/java/land/oras/utils/ArchiveUtilsTest.java

@@ -20,13 +20,18 @@
 
 package land.oras.utils;
 
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
 
+import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.attribute.PosixFilePermission;
 import java.util.Set;
 import land.oras.LocalPath;
+import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.CleanupMode;
@@ -96,6 +101,28 @@ static void beforeAll() throws Exception {
                         PosixFilePermission.OTHERS_EXECUTE));
     }
 
+    @Test
+    void testEnsureSafeEntry() throws Exception {
+        TarArchiveEntry entry = mock(TarArchiveEntry.class);
+        doReturn("test").when(entry).getName();
+        ArchiveUtils.ensureSafeEntry(entry, archiveDir);
+    }
+
+    @Test
+    void throwOnUnsafeEntries() throws Exception {
+        TarArchiveEntry entry = mock(TarArchiveEntry.class);
+
+        // Simulate a path traversal attack
+        assertThrows(IOException.class, () -> {
+            doReturn("/").when(entry).getName();
+            ArchiveUtils.ensureSafeEntry(entry, archiveDir);
+        });
+        assertThrows(IOException.class, () -> {
+            doReturn("foo/bar/../../../test").when(entry).getName();
+            ArchiveUtils.ensureSafeEntry(entry, archiveDir);
+        });
+    }
+
     @Test
     void shouldCreateTarGzAndExtractIt() throws Exception {
         LocalPath directory = LocalPath.of(archiveDir);