[H10zCpAQ] Fix CWE-73: Added check to prevent reading from outside me…

…trics directory (#3245) * [H10zCpAQ] Fix CWE-73: Added check to prevent reading from outside metrics directory * removed println
neo4j-contrib · Oct 27, 2022 · e0c7958 · e0c7958
1 parent 88dfc14
commit e0c7958
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 2 deletions.
diff --git a/full/src/main/java/apoc/metrics/Metrics.java b/full/src/main/java/apoc/metrics/Metrics.java
@@ -13,6 +13,7 @@
 
 import java.io.File;
 import java.io.FilenameFilter;
+import java.io.IOException;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
@@ -28,6 +29,9 @@
  */
 @Extended
 public class Metrics {
+    public static final String OUTSIDE_DIR_ERR_MSG = "The path you are trying to access is outside the metrics directory and " +
+            "this procedure is only permitted to access files in it. " +
+            "This may occur if the path in question is a symlink or other link.";
     @Context
     public Log log;
 
@@ -163,14 +167,21 @@ public Stream<GenericMetric> loadCsvForMetric(String metricName, Map<String,Obje
         config.put("header", true);
 
         File metricsDir = FileUtils.getMetricsDirectory();
-
         if (metricsDir == null) {
             throw new RuntimeException("Metrics directory either does not exist or is not readable.  " +
                     "To use this procedure please ensure CSV metrics are configured " +
                     "https://neo4j.com/docs/operations-manual/current/monitoring/metrics/expose/#metrics-csv");
         }
 
-        String url = new File(metricsDir, metricName + ".csv").getAbsolutePath();
+        final File file = new File(metricsDir, metricName + ".csv");
+        try {
+            if (!file.getCanonicalPath().startsWith(metricsDir.getAbsolutePath())) {
+                throw new RuntimeException(OUTSIDE_DIR_ERR_MSG);
+            }
+        } catch (IOException ioe) {
+            throw new RuntimeException("Unable to resolve basic metric file canonical path", ioe);
+        }
+        String url = file.getAbsolutePath();
         CountingReader reader = null;
         try {
             reader = FileUtils.SupportedProtocols.file

diff --git a/full/src/test/java/apoc/metrics/MetricsTest.java b/full/src/test/java/apoc/metrics/MetricsTest.java
@@ -3,6 +3,7 @@
 import apoc.util.Neo4jContainerExtension;
 import apoc.util.TestUtil;
 import org.junit.AfterClass;
+import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.neo4j.driver.Session;
@@ -15,6 +16,7 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import static apoc.metrics.Metrics.OUTSIDE_DIR_ERR_MSG;
 import static apoc.util.FileUtils.NEO4J_DIRECTORY_CONFIGURATION_SETTING_NAMES;
 import static apoc.util.TestContainerUtil.createEnterpriseDB;
 import static apoc.util.TestContainerUtil.testCall;
@@ -59,6 +61,16 @@ public static void afterAll() {
             neo4jContainer.close();
         }
     }
+
+    @Test
+    public void shouldNotGetFileOutsideMetricsDir() {
+        try {
+            testCall(session, "CALL apoc.metrics.get('../external')",
+                    (r) -> Assert.fail("Should fail because the path is outside the dir "));
+        } catch (RuntimeException e) {
+            assertEquals("Failed to invoke procedure `apoc.metrics.get`: Caused by: java.lang.RuntimeException: " + OUTSIDE_DIR_ERR_MSG, e.getMessage());
+        }
+    }
 
     @Test
     public void shouldGetMetrics() {