From 0a2f16471a09097e1d9cc90434904632f332482a Mon Sep 17 00:00:00 2001 From: Giuseppe Villani Date: Tue, 21 Feb 2023 10:38:40 +0100 Subject: [PATCH] [GE2QhC9X] Fix path traversal vulnerability (CVE-2022-23532) (#3457) * [GE2QhC9X] Fix path traversal vulnerability (CVE-2022-23532) * [GE2QhC9X] removed unused import --- apoc-core | 2 +- .../export/ExportExtendedSecurityTest.java | 316 +++++++++++++++++- 2 files changed, 299 insertions(+), 19 deletions(-) diff --git a/apoc-core b/apoc-core index 5ecd6f715a..f175f1f1f6 160000 --- a/apoc-core +++ b/apoc-core @@ -1 +1 @@ -Subproject commit 5ecd6f715a9e40c04e29ac4e3e81510f84491849 +Subproject commit f175f1f1f663d29fc151c297b56d154255eb7016 diff --git a/extended/src/test/java/apoc/export/ExportExtendedSecurityTest.java b/extended/src/test/java/apoc/export/ExportExtendedSecurityTest.java index d943285572..f5e64d1989 100644 --- a/extended/src/test/java/apoc/export/ExportExtendedSecurityTest.java +++ b/extended/src/test/java/apoc/export/ExportExtendedSecurityTest.java @@ -4,8 +4,9 @@ import apoc.export.xls.ExportXls; import apoc.util.FileUtils; import apoc.util.TestUtil; +import junit.framework.TestCase; import org.apache.commons.lang3.exception.ExceptionUtils; -import org.junit.AfterClass; +import org.junit.Assert; import org.junit.BeforeClass; import org.junit.ClassRule; import org.junit.Test; @@ -13,6 +14,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; @@ -25,7 +27,6 @@ import java.util.Map; import java.util.stream.Collectors; -import static apoc.ApocConfig.APOC_EXPORT_FILE_ENABLED; import static apoc.ApocConfig.apocConfig; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; @@ -39,10 +40,18 @@ public class ExportExtendedSecurityTest { 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("xls"); - + static { + //noinspection ResultOfMethodCallIgnored directory.mkdirs(); + //noinspection ResultOfMethodCallIgnored + subDirectory.mkdirs(); + //noinspection ResultOfMethodCallIgnored + directoryWithSamePrefix.mkdirs(); } @ClassRule @@ -50,14 +59,13 @@ public class ExportExtendedSecurityTest { .withSetting(GraphDatabaseSettings.load_csv_file_url_root, directory.toPath().toAbsolutePath()); @BeforeClass - public static void setUp() throws Exception { + public static void setUp() { TestUtil.registerProcedure(db, ExportXls.class); - apocConfig().setProperty(APOC_EXPORT_FILE_ENABLED, false); + setFileExport(false); } - @AfterClass - public static void tearDown() { - db.shutdown(); + private static void setFileExport(boolean allowed) { + apocConfig().setProperty(ApocConfig.APOC_EXPORT_FILE_ENABLED, allowed); } private static Collection data(Map> apocProcedureArguments) { @@ -110,14 +118,11 @@ 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); } } @@ -158,22 +163,297 @@ public static Collection data() { public void testIllegalExternalFSAccessExport() { final String message = apocProcedure + " should throw an exception"; try { - apocConfig().setProperty(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().setProperty(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 + ", {}" + ).toList(), + "all", cases.stream().map( + filePath -> filePath + ", {}" + ).toList(), + "data", cases.stream().map( + filePath -> "[], [], " + filePath + ", {}" + ).toList(), + "graph", cases.stream().map( + filePath -> "{nodes: [], relationships: []}, " + filePath + ", {}" + ).toList() + ); + + @Parameterized.Parameters + public static Collection data() { + return ExportExtendedSecurityTest.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 + ", {}" + ).toList(), + "all", cases.stream().map( + filePath -> filePath + ", {}" + ).toList(), + "data", cases.stream().map( + filePath -> "[], [], " + filePath + ", {}" + ).toList(), + "graph", cases.stream().map( + filePath -> "{nodes: [], relationships: []}, " + filePath + ", {}" + ).toList() + ); + + @Parameterized.Parameters + public static Collection data() { + return ExportExtendedSecurityTest.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 + ", {}" + ).toList(), + "all", cases.stream().map( + filePath -> filePath + ", {}" + ).toList(), + "data", cases.stream().map( + filePath -> "[], [], " + filePath + ", {}" + ).toList(), + "graph", cases.stream().map( + filePath -> "{nodes: [], relationships: []}, " + filePath + ", {}" + ).toList() + ); + + @Parameterized.Parameters + public static Collection data() { + return ExportExtendedSecurityTest.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 + ", {}" + ).toList(), + "all", cases.stream().map( + filePath -> filePath + ", {}" + ).toList(), + "data", cases.stream().map( + filePath -> "[], [], " + filePath + ", {}" + ).toList(), + "graph", cases.stream().map( + filePath -> "{nodes: [], relationships: []}, " + filePath + ", {}" + ).toList() + ); + + @Parameterized.Parameters + public static Collection data() { + return ExportExtendedSecurityTest.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 + ", {}" + ).toList(), + "all", cases.stream().map( + filePath -> filePath + ", {}" + ).toList(), + "data", cases.stream().map( + filePath -> "[], [], " + filePath + ", {}" + ).toList(), + "graph", cases.stream().map( + filePath -> "{nodes: [], relationships: []}, " + filePath + ", {}" + ).toList() + ); + + @Parameterized.Parameters + public static Collection data() { + return ExportExtendedSecurityTest.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()); + } + } + private static void assertError(Exception e, String errorMessage, Class exceptionType, String apocProcedure) { final Throwable rootCause = ExceptionUtils.getRootCause(e); assertTrue(apocProcedure + " should throw an instance of " + exceptionType.getSimpleName(), exceptionType.isInstance(rootCause)); assertEquals(apocProcedure + " should throw the following message", errorMessage, rootCause.getMessage()); } + } \ No newline at end of file