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

…trics directory (#3245) (#3425)
neo4j-contrib · Jan 30, 2023 · b5069a5 · b5069a5
1 parent e8d2fca
commit b5069a5
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 1 deletion.
diff --git a/extended/src/main/java/apoc/metrics/Metrics.java b/extended/src/main/java/apoc/metrics/Metrics.java
@@ -15,6 +15,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;
@@ -30,6 +31,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;
 
@@ -172,7 +176,15 @@ public Stream<GenericMetric> loadCsvForMetric(String metricName, Map<String,Obje
                     "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.getStreamConnection(SupportedProtocols.file, url, null, null)

diff --git a/extended/src/test/java/apoc/metrics/MetricsTest.java b/extended/src/test/java/apoc/metrics/MetricsTest.java
@@ -3,6 +3,7 @@
 import apoc.util.Neo4jContainerExtension;
 import apoc.util.TestContainerUtil.ApocPackage;
 import org.junit.AfterClass;
+import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Ignore;
 import org.junit.Test;
@@ -16,9 +17,11 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import static apoc.metrics.Metrics.OUTSIDE_DIR_ERR_MSG;
 import static apoc.util.ExtendedFileUtils.NEO4J_DIRECTORY_CONFIGURATION_SETTING_NAMES;
 import static apoc.util.TestContainerUtil.*;
 import static apoc.util.Util.map;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.neo4j.test.assertion.Assert.assertEventually;
 
@@ -48,7 +51,18 @@ public static void beforeAll() throws InterruptedException {
     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());
+        }
+    }
+
     // TODO: Investigate broken test. It hangs for more than 30 seconds for no reason.
     @Test
     @Ignore