diff --git a/core/src/main/java/apoc/export/csv/ExportCSV.java b/core/src/main/java/apoc/export/csv/ExportCSV.java index bb0560832e..e8debedd4d 100644 --- a/core/src/main/java/apoc/export/csv/ExportCSV.java +++ b/core/src/main/java/apoc/export/csv/ExportCSV.java @@ -116,8 +116,8 @@ private Stream exportCsv(@Name("file") String fileName, String sou private void dump(Object data, ExportConfig c, ProgressReporter reporter, ExportFileManager printWriter, CsvFormat exporter) { if (data instanceof SubGraph) - exporter.dump((SubGraph)data,printWriter,reporter,c); + exporter.dump((SubGraph) data, printWriter, reporter, c); if (data instanceof Result) - exporter.dump((Result)data,printWriter,reporter,c); + exporter.dump((Result) data, printWriter, reporter, c); } } diff --git a/core/src/main/java/apoc/util/FileUtils.java b/core/src/main/java/apoc/util/FileUtils.java index 8e93a00227..6c3e0b34f8 100644 --- a/core/src/main/java/apoc/util/FileUtils.java +++ b/core/src/main/java/apoc/util/FileUtils.java @@ -230,7 +230,7 @@ private static Path getPath(String url) { Path urlPath; URL toURL = null; try { - final URI uri = URI.create(url.trim()); + final URI uri = URI.create(url.trim()).normalize(); toURL = uri.toURL(); urlPath = Paths.get(uri); } catch (Exception e) { @@ -245,10 +245,10 @@ private static Path getPath(String url) { private static boolean pathStartsWithOther(Path resolvedPath, Path basePath) throws IOException { try { - return resolvedPath.toRealPath().startsWith(basePath.toRealPath()); + return resolvedPath.toFile().getCanonicalFile().toPath().startsWith(basePath.toRealPath()); } catch (Exception e) { - if (e instanceof NoSuchFileException) { // If we're about to creating a file this exception has been thrown - return resolvedPath.normalize().startsWith(basePath); + if (e instanceof NoSuchFileException) { // If we're about to create a file this exception has been thrown + return resolvedPath.toFile().getCanonicalFile().toPath().startsWith(basePath); } return false; } diff --git a/core/src/test/java/apoc/export/ExportCoreSecurityTest.java b/core/src/test/java/apoc/export/ExportCoreSecurityTest.java index af26f37b6b..23cbe997f7 100644 --- a/core/src/test/java/apoc/export/ExportCoreSecurityTest.java +++ b/core/src/test/java/apoc/export/ExportCoreSecurityTest.java @@ -8,9 +8,9 @@ import apoc.export.json.ExportJson; import apoc.util.FileUtils; import apoc.util.TestUtil; +import junit.framework.TestCase; import org.apache.commons.lang3.exception.ExceptionUtils; -import org.junit.After; -import org.junit.Before; +import org.junit.Assert; import org.junit.BeforeClass; import org.junit.ClassRule; import org.junit.Test; @@ -18,6 +18,7 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.neo4j.configuration.GraphDatabaseSettings; +import org.neo4j.graphdb.QueryExecutionException; import org.neo4j.graphdb.Result; import org.neo4j.test.rule.DbmsRule; import org.neo4j.test.rule.ImpermanentDbmsRule; @@ -38,10 +39,17 @@ public class ExportCoreSecurityTest { private static final File directory = new File("target/import"); + private static final File directoryWithSamePrefix = new File("target/imported"); + private static final File subDirectory = new File("target/import/tests"); private static final List APOC_EXPORT_PROCEDURE_NAME = Arrays.asList("csv", "json", "graphml", "cypher"); static { + //noinspection ResultOfMethodCallIgnored directory.mkdirs(); + //noinspection ResultOfMethodCallIgnored + subDirectory.mkdirs(); + //noinspection ResultOfMethodCallIgnored + directoryWithSamePrefix.mkdirs(); } @ClassRule @@ -52,16 +60,11 @@ public class ExportCoreSecurityTest { @BeforeClass public static void setUp() throws Exception { TestUtil.registerProcedure(db, ExportCSV.class, ExportJson.class, ExportGraphML.class, ExportCypher.class); + setFileExport(false); } - @Before - public void before() { - ApocConfig.apocConfig().setProperty(ApocSettings.apoc_export_file_enabled, false); - } - - @After - public void after() { - ApocConfig.apocConfig().setProperty(ApocSettings.apoc_export_file_enabled, false); + private static void setFileExport(boolean allowed) { + ApocConfig.apocConfig().setProperty(ApocConfig.APOC_EXPORT_FILE_ENABLED, allowed); } private static Collection data(Map> apocProcedureArguments) { @@ -114,16 +117,12 @@ public static Collection data() { @Test public void testIllegalFSAccessExport() { - final String message = apocProcedure + " should throw an exception"; - try { - db.executeTransactionally("CALL " + apocProcedure, Map.of(), - Result::resultAsString); - fail(message); - } catch (Exception e) { - assertError(e, ApocConfig.EXPORT_TO_FILE_ERROR, RuntimeException.class, apocProcedure); - } - } + QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class, + () -> TestUtil.testCall(db, "CALL " + apocProcedure, (r) -> {}) + ); + assertError(e, ApocConfig.EXPORT_TO_FILE_ERROR, RuntimeException.class, apocProcedure); + } } @RunWith(Parameterized.class) @@ -162,45 +161,310 @@ public static Collection data() { public void testIllegalExternalFSAccessExport() { final String message = apocProcedure + " should throw an exception"; try { - ApocConfig.apocConfig().setProperty(ApocSettings.apoc_export_file_enabled, true); + setFileExport(true); db.executeTransactionally("CALL " + apocProcedure, Map.of(), Result::resultAsString); fail(message); } catch (Exception e) { assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure); } finally { - ApocConfig.apocConfig().setProperty(ApocSettings.apoc_export_file_enabled, false); + setFileExport(false); } } } + @RunWith(Parameterized.class) + public static class TestPathTraversalAccess { + private final String apocProcedure; + + public TestPathTraversalAccess(String exportMethod, String exportMethodType, String exportMethodArguments) { + this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")"; + } + + private static final String case1 = "'file:///%2e%2e%2f%2f%2e%2e%2f%2f%2e%2e%2f%2f%2e%2e%2f%2fapoc/test.txt'"; + private static final String case2 = "'file:///%2e%2e%2f%2ftest.txt'"; + private static final String case3 = "'../test.txt'"; + private static final String case4 = "'tests/../../test.txt'"; + private static final String case5 = "'tests/..//..//test.txt'"; + + private static final List cases = Arrays.asList(case1, case2, case3, case4, case5); + + private static final Map> METHOD_ARGUMENTS = Map.of( + "query", cases.stream().map( + filePath -> "\"RETURN 1\", " + filePath + ", {}" + ).collect(Collectors.toList()), + "all", cases.stream().map( + filePath -> filePath + ", {}" + ).collect(Collectors.toList()), + "data", cases.stream().map( + filePath -> "[], [], " + filePath + ", {}" + ).collect(Collectors.toList()), + "graph", cases.stream().map( + filePath -> "{nodes: [], relationships: []}, " + filePath + ", {}" + ).collect(Collectors.toList()) + ); + + @Parameterized.Parameters + public static Collection data() { + return ExportCoreSecurityTest.data(METHOD_ARGUMENTS); + } + + @Test + public void testPathTraversal() { + setFileExport(true); + + QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class, + () -> TestUtil.testCall(db, "CALL " + apocProcedure, (r) -> {}) + ); + + assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure); + setFileExport(false); + } + } + + /** + * All of these will resolve to a local path after normalization which will point to + * a non-existing directory in our import folder: /apoc. Causing them to error that is + * not found. They all attempt to exit the import folder back to the apoc folder: + * Directory Layout: .../apoc/core/target/import + */ + @RunWith(Parameterized.class) + public static class TestPathTraversalIsNormalised { + private final String apocProcedure; + + public TestPathTraversalIsNormalised(String exportMethod, String exportMethodType, String exportMethodArguments) { + this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")"; + } + + private static final String case1 = "'file://%2e%2e%2f%2e%2e%2f%2e%2e%2f%2e%2e%2f/apoc/test.txt'"; + private static final String case2 = "'file://../../../../apoc/test.txt'"; + private static final String case3 = "'file:///..//..//..//..//apoc//core//..//test.txt'"; + private static final String case4 = "'file:///..//..//..//..//apoc/test.txt'"; + private static final String case5 = "'file://" + directory.getAbsolutePath() + "//..//..//..//..//apoc/test.txt'"; + private static final String case6 = "'file:///%252e%252e%252f%252e%252e%252f%252e%252e%252f%252e%252e%252f/apoc/test.txt'"; + + private static final List cases = Arrays.asList(case1, case2, case3, case4, case5, case6); + + private static final Map> METHOD_ARGUMENTS = Map.of( + "query", cases.stream().map( + filePath -> "\"RETURN 1\", " + filePath + ", {}" + ).collect(Collectors.toList()), + "all", cases.stream().map( + filePath -> filePath + ", {}" + ).collect(Collectors.toList()), + "data", cases.stream().map( + filePath -> "[], [], " + filePath + ", {}" + ).collect(Collectors.toList()), + "graph", cases.stream().map( + filePath -> "{nodes: [], relationships: []}, " + filePath + ", {}" + ).collect(Collectors.toList()) + ); + + @Parameterized.Parameters + public static Collection data() { + return ExportCoreSecurityTest.data(METHOD_ARGUMENTS); + } + + @Test + public void testPathTraversal() { + setFileExport(true); + + QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class, + () -> TestUtil.testCall(db, "CALL " + apocProcedure, (r) -> {}) + ); + + TestCase.assertTrue(e.getMessage().contains("apoc/test.txt (No such file or directory)")); + setFileExport(false); + } + } + + /** + * These tests normalize the path to be within the import directory and make the file there. + * Some attempt to exit the directory. + * They result in a file name test.txt being created (and deleted after). + */ + @RunWith(Parameterized.class) + public static class TestPathTraversalIsNormalisedWithinDirectory { + private final String apocProcedure; + + public TestPathTraversalIsNormalisedWithinDirectory(String exportMethod, String exportMethodType, String exportMethodArguments) { + this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")"; + } + + private static final String case1 = "'file:///..//..//..//..//apoc//..//..//..//..//test.txt'"; + private static final String case2 = "'file:///..//..//..//..//apoc//..//test.txt'"; + private static final String case3 = "'file:///../import/../import//..//test.txt'"; + private static final String case4 = "'file://test.txt'"; + private static final String case5 = "'file://tests/../test.txt'"; + private static final String case6 = "'file:///tests//..//test.txt'"; + private static final String case7 = "'test.txt'"; + private static final String case8 = "'file:///..//..//..//..//test.txt'"; + + private static final List cases = Arrays.asList(case1, case2, case3, case4, case5, case6, case7, case8); + + private static final Map> METHOD_ARGUMENTS = Map.of( + "query", cases.stream().map( + filePath -> "\"RETURN 1\", " + filePath + ", {}" + ).collect(Collectors.toList()), + "all", cases.stream().map( + filePath -> filePath + ", {}" + ).collect(Collectors.toList()), + "data", cases.stream().map( + filePath -> "[], [], " + filePath + ", {}" + ).collect(Collectors.toList()), + "graph", cases.stream().map( + filePath -> "{nodes: [], relationships: []}, " + filePath + ", {}" + ).collect(Collectors.toList()) + ); + + @Parameterized.Parameters + public static Collection data() { + return ExportCoreSecurityTest.data(METHOD_ARGUMENTS); + } + + @Test + public void testPathTraversal() { + setFileExport(true); + + TestUtil.testCall(db, "CALL " + apocProcedure, + (r) -> assertTrue(((String) r.get("file")).contains("test.txt")) + ); + + File f = new File(directory.getAbsolutePath() + "/test.txt"); + TestCase.assertTrue(f.exists()); + TestCase.assertTrue(f.delete()); + } + } + + /* + * These test cases attempt to access a directory with the same prefix as the import directory. This is design to + * test "directoryName.startsWith" logic which is a common path traversal bug. + * + * All these tests should fail because they access a directory which isn't the configured directory + */ + @RunWith(Parameterized.class) + public static class TestPathTraversalIsWithSimilarDirectoryName { + private final String apocProcedure; + + public TestPathTraversalIsWithSimilarDirectoryName(String exportMethod, String exportMethodType, String exportMethodArguments) { + this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")"; + } + + private static final String case1 = "'../imported/test.txt'"; + private static final String case2 = "'tests/../../imported/test.txt'"; + + private static final List cases = Arrays.asList(case1, case2); + + private static final Map> METHOD_ARGUMENTS = Map.of( + "query", cases.stream().map( + filePath -> "\"RETURN 1\", " + filePath + ", {}" + ).collect(Collectors.toList()), + "all", cases.stream().map( + filePath -> filePath + ", {}" + ).collect(Collectors.toList()), + "data", cases.stream().map( + filePath -> "[], [], " + filePath + ", {}" + ).collect(Collectors.toList()), + "graph", cases.stream().map( + filePath -> "{nodes: [], relationships: []}, " + filePath + ", {}" + ).collect(Collectors.toList()) + ); + + @Parameterized.Parameters + public static Collection data() { + return ExportCoreSecurityTest.data(METHOD_ARGUMENTS); + } + + @Test + public void testPathTraversal() { + setFileExport(true); + + QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class, + () -> TestUtil.testCall(db, "CALL " + apocProcedure, (r) -> {}) + ); + + assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure); + + setFileExport(false); + } + } + + + /** + * These tests normalize the path to be within the import directory and step into a subdirectory + * to make the file there. + * Some attempt to exit the directory. + * They result in a file name test.txt in the directory /tests being created (and deleted after). + */ + @RunWith(Parameterized.class) + public static class TestPathTraversAllowedWithinDirectory { + private final String apocProcedure; + + public TestPathTraversAllowedWithinDirectory(String exportMethod, String exportMethodType, String exportMethodArguments) { + this.apocProcedure = "apoc.export." + exportMethod + "." + exportMethodType + "(" + exportMethodArguments + ")"; + } + + private static final String case1 = "'file:///../import/../import//..//tests/test.txt'"; + private static final String case2 = "'file:///..//..//..//..//apoc//..//tests/test.txt'"; + private static final String case3 = "'file:///../import/../import//..//tests/../tests/test.txt'"; + private static final String case4 = "'file:///tests/test.txt'"; + private static final String case5 = "'tests/test.txt'"; + + private static final List cases = Arrays.asList(case1, case2, case3, case4, case5); + + private static final Map> METHOD_ARGUMENTS = Map.of( + "query", cases.stream().map( + filePath -> "\"RETURN 1\", " + filePath + ", {}" + ).collect(Collectors.toList()), + "all", cases.stream().map( + filePath -> filePath + ", {}" + ).collect(Collectors.toList()), + "data", cases.stream().map( + filePath -> "[], [], " + filePath + ", {}" + ).collect(Collectors.toList()), + "graph", cases.stream().map( + filePath -> "{nodes: [], relationships: []}, " + filePath + ", {}" + ).collect(Collectors.toList()) + ); + + @Parameterized.Parameters + public static Collection data() { + return ExportCoreSecurityTest.data(METHOD_ARGUMENTS); + } + + @Test + public void testPathTraversal() { + setFileExport(true); + + TestUtil.testCall(db, "CALL " + apocProcedure, + (r) -> assertTrue(((String) r.get("file")).contains("tests/test.txt")) + ); + + File f = new File(subDirectory.getAbsolutePath() + "/test.txt"); + TestCase.assertTrue(f.exists()); + TestCase.assertTrue(f.delete()); + } + } + public static class TestCypherSchema { private final String apocProcedure = "apoc.export.cypher.schema(%s)"; - private final String message = apocProcedure + " should throw an exception"; @Test public void testIllegalFSAccessExportCypherSchema() { - try { - db.executeTransactionally(String.format("CALL " + apocProcedure, "'./hello', {}"), Map.of(), - Result::resultAsString); - fail(message); - } catch (Exception e) { - assertError(e, ApocConfig.EXPORT_TO_FILE_ERROR, RuntimeException.class, apocProcedure); - } + QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class, + () -> TestUtil.testCall(db, String.format("CALL " + apocProcedure, "'./hello', {}"), (r) -> {}) + ); + assertError(e, ApocConfig.EXPORT_TO_FILE_ERROR, RuntimeException.class, apocProcedure); } @Test public void testIllegalExternalFSAccessExportCypherSchema() { - try { - ApocConfig.apocConfig().setProperty(ApocSettings.apoc_export_file_enabled, true); - db.executeTransactionally(String.format("CALL " + apocProcedure, "'../hello', {}"), Map.of(), - Result::resultAsString); - fail(message); - } catch (Exception e) { - assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure); - } finally { - ApocConfig.apocConfig().setProperty(ApocSettings.apoc_export_file_enabled, false); - } + setFileExport(true); + QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class, + () -> TestUtil.testCall(db, String.format("CALL " + apocProcedure, "'../hello', {}"), (r) -> {}) + ); + assertError(e, FileUtils.ACCESS_OUTSIDE_DIR_ERROR, IOException.class, apocProcedure); + setFileExport(false); } } diff --git a/core/src/test/java/apoc/export/csv/ExportCsvTest.java b/core/src/test/java/apoc/export/csv/ExportCsvTest.java index 2bb329646b..f6800386a4 100644 --- a/core/src/test/java/apoc/export/csv/ExportCsvTest.java +++ b/core/src/test/java/apoc/export/csv/ExportCsvTest.java @@ -114,8 +114,9 @@ public class ExportCsvTest { ",,,,,,,,0,1,KNOWS%n" + ",,,,,,,,3,4,NEXT_DELIVERY%n"); - private static File directory = new File("target/import"); - static { //noinspection ResultOfMethodCallIgnored + private static final File directory = new File("target/import"); + static { + //noinspection ResultOfMethodCallIgnored directory.mkdirs(); }