Merge pull request from GHSA-5mp4-32rr-v3x5

Fix path traversal vulnerability in LocalFileLogServer

Author: aamine

digdag-core/src/main/java/io/digdag/core/log/LocalFileLogServerFactory.java

@@ -1,5 +1,6 @@
 package io.digdag.core.log;
 
+import java.nio.file.NoSuchFileException;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -122,11 +123,15 @@ protected void listFiles(String dateDir, String attemptDir, boolean enableDirect
         protected byte[] getFile(String dateDir, String attemptDir, String fileName)
             throws StorageFileNotFoundException
         {
-            Path path = getPrefixDir(dateDir, attemptDir).resolve(fileName);
+            Path prefixDir = getPrefixDir(dateDir, attemptDir);
+            Path path = prefixDir.resolve(fileName).normalize();
+            if (!path.startsWith(prefixDir)) {
+                throw new IllegalArgumentException("Invalid file name: " + fileName);
+            }
             try (InputStream in = Files.newInputStream(path)) {
                 return ByteStreams.toByteArray(in);
             }
-            catch (FileNotFoundException ex) {
+            catch (FileNotFoundException | NoSuchFileException ex) {
                 throw new StorageFileNotFoundException(ex);
             }
             catch (IOException ex) {

digdag-core/src/test/java/io/digdag/core/log/LocalFileLogServerFactoryTest.java

@@ -6,6 +6,7 @@
 import io.digdag.core.agent.AgentId;
 import io.digdag.core.config.PropertyUtils;
 import io.digdag.spi.LogFilePrefix;
+import io.digdag.spi.StorageFileNotFoundException;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -23,6 +24,7 @@
 import java.util.Properties;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.junit.Assert.assertThrows;
 
 import io.digdag.core.log.LocalFileLogServerFactory.LocalFileLogServer.LocalFileDirectTaskLogger;
 
@@ -142,4 +144,30 @@ private String repeatedString(String v, int num)
         }
         return b.toString();
     }
+
+    @Test
+    public void testGetFile() throws StorageFileNotFoundException
+    {
+        setUpTaskLogger(Optional.absent());
+        String fileName = localServer.putFile(prefix, "+task", Instant.now(), "agent", "foo".getBytes(UTF_8));
+        byte[] data = localServer.getFile(prefix, fileName);
+        assertThat(new String(data, UTF_8), is("foo"));
+    }
+
+    @Test
+    public void testGetFileNotFound()
+    {
+        setUpTaskLogger(Optional.absent());
+        assertThrows(StorageFileNotFoundException.class, () -> localServer.getFile(prefix, "foo"));
+    }
+
+    @Test
+    public void testGetFileInvalidFileName()
+    {
+        setUpTaskLogger(Optional.absent());
+        assertThrows(IllegalArgumentException.class, () -> localServer.getFile(prefix, ".."));
+        assertThrows(IllegalArgumentException.class, () -> localServer.getFile(prefix, "../foo"));
+        assertThrows(IllegalArgumentException.class, () -> localServer.getFile(prefix, "/foo"));
+        assertThrows(IllegalArgumentException.class, () -> localServer.getFile(prefix, "foo/../../bar"));
+    }
 }

digdag-tests/src/test/java/acceptance/LogIT.java

@@ -10,12 +10,15 @@
 import utils.CommandStatus;
 import utils.TemporaryDigdagServer;
 
+import javax.ws.rs.BadRequestException;
+import javax.ws.rs.NotFoundException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.regex.Pattern;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static utils.TestUtils.*;
 
@@ -91,5 +94,8 @@ public void verifyLogWithAttemptIdAndSessionId()
 
         final String regex = "\\[\\d+:\\w+:\\d+:\\d+]";
         assertTrue(Pattern.compile(regex, Pattern.DOTALL).matcher(logs).find());
+
+        assertThrows(NotFoundException.class, () -> client.getLogFile(attemptId, "foo"));
+        assertThrows(BadRequestException.class, () -> client.getLogFile(attemptId, "/foo"));
     }
 }