Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[H10zCpAQ] Fix CWE-73: Added check to prevent reading from outside metrics directory #3245

Merged
merged 2 commits into from
Oct 27, 2022
Merged

[H10zCpAQ] Fix CWE-73: Added check to prevent reading from outside metrics directory #3245

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c623e34
[H10zCpAQ] Fix CWE-73: Added check to prevent reading from outside me…
vga91 Oct 21, 2022
547f23a
removed println
vga91 Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions full/src/main/java/apoc/metrics/Metrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions full/src/test/java/apoc/metrics/MetricsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -59,6 +61,17 @@ public static void afterAll() {
neo4jContainer.close();
}
}

@Test
public void shouldNotGetFileOutsideMetricsDir() {
System.out.println("MetricsTest.shouldNotGetFileOutsideMetricsDir1");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to keep this System.out.println()?

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() {
Expand Down