Adds load json ssrf mitigation (neo4j-contrib#2467)

vga91 · Feb 23, 2022 · 8b7063e · 8b7063e
1 parent 711080b
commit 8b7063e
Show file tree

Hide file tree

Showing 11 changed files with 85 additions and 7 deletions.
diff --git a/core/build.gradle b/core/build.gradle
@@ -61,6 +61,7 @@ dependencies {
         exclude module: "snakeyaml"
     }
     implementation group: 'org.yaml', name: 'snakeyaml', version: '1.26'
+    testImplementation group: 'com.github.seancfoley', name: 'ipaddress', version: '5.3.3'
     testImplementation group: 'com.github.stefanbirkner', name: 'system-rules', version: '1.19.0'
 
     testImplementation 'net.sourceforge.jexcelapi:jxl:2.6.12'

diff --git a/core/src/main/java/apoc/ApocConfig.java b/core/src/main/java/apoc/ApocConfig.java
@@ -14,12 +14,15 @@
 import org.neo4j.graphdb.GraphDatabaseService;
 import org.neo4j.graphdb.Path;
 import org.neo4j.graphdb.config.Setting;
+import org.neo4j.graphdb.security.URLAccessRule;
 import org.neo4j.kernel.api.procedure.GlobalProcedures;
+import org.neo4j.kernel.internal.GraphDatabaseAPI;
 import org.neo4j.kernel.lifecycle.LifecycleAdapter;
 import org.neo4j.logging.Log;
 import org.neo4j.logging.NullLog;
 import org.neo4j.logging.internal.LogService;
 
+import java.io.IOException;
 import java.lang.reflect.Field;
 import java.net.URL;
 import java.time.Duration;
@@ -42,7 +45,6 @@
 import static org.neo4j.configuration.GraphDatabaseSettings.transaction_logs_root_path;
 
 public class ApocConfig extends LifecycleAdapter {
-
     public static final String SUN_JAVA_COMMAND = "sun.java.command";
     public static final String APOC_IMPORT_FILE_ENABLED = "apoc.import.file.enabled";
     public static final String APOC_EXPORT_FILE_ENABLED = "apoc.export.file.enabled";
@@ -260,6 +262,14 @@ public void isImportFileEnabled() {
     public void checkReadAllowed(String url) {
         if (isFile(url)) {
             isImportFileEnabled();
+        } else {
+            try {
+                ((GraphDatabaseAPI) systemDb).getDependencyResolver()
+                        .resolveDependency(URLAccessRule.class)
+                        .validate(neo4jConfig, new URL(url));
+            } catch (Exception e) {
+                throw new RuntimeException(e);
+            }
         }
     }
 

diff --git a/core/src/main/java/apoc/load/Xml.java b/core/src/main/java/apoc/load/Xml.java
@@ -8,6 +8,7 @@
 import apoc.util.CompressionAlgo;
 import apoc.util.CompressionConfig;
 import apoc.util.FileUtils;
+import apoc.util.Util;
 import org.apache.commons.lang3.BooleanUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.neo4j.graphdb.Label;
@@ -160,8 +161,8 @@ private XMLStreamReader getXMLStreamReader(Object urlOrBinary, XmlImportConfig c
             String url = (String) urlOrBinary;
             apocConfig.checkReadAllowed(url);
             url = FileUtils.changeFileUrlIfImportDirectoryConstrained(url);
-            URLConnection urlConnection = new URL(url).openConnection();
-            inputStream = urlConnection.getInputStream();
+            var sc = Util.openInputStream(url, null, null, null);
+            inputStream = sc.getStream();
         } else if (urlOrBinary instanceof byte[]) {
             inputStream = getInputStreamFromBinary((byte[]) urlOrBinary, config.getCompressionAlgo());
         } else {

diff --git a/core/src/main/java/apoc/util/FileUtils.java b/core/src/main/java/apoc/util/FileUtils.java
@@ -42,6 +42,7 @@ public class FileUtils {
     public enum SupportedProtocols {
         http(true, null),
         https(true, null),
+        ftp(true, null),
         s3(Util.classExists("com.amazonaws.services.s3.AmazonS3"),
                 Util.createInstanceOrNull("apoc.util.s3.S3UrlStreamHandlerFactory")),
         gs(Util.classExists("com.google.cloud.storage.Storage"),
@@ -65,6 +66,7 @@ public StreamConnection getStreamConnection(String urlAddress, Map<String, Objec
                     return FileUtils.openS3InputStream(new URL(urlAddress));
                 case hdfs:
                     return FileUtils.openHdfsInputStream(new URL(urlAddress));
+                case ftp:
                 case http:
                 case https:
                 case gs:

diff --git a/core/src/main/java/apoc/util/Util.java b/core/src/main/java/apoc/util/Util.java
@@ -1,5 +1,6 @@
 package apoc.util;
 
+import apoc.ApocConfig;
 import apoc.Pools;
 import apoc.convert.Convert;
 import apoc.export.util.CountingInputStream;

diff --git a/core/src/test/java/apoc/load/LoadCoreSecurityTest.java b/core/src/test/java/apoc/load/LoadCoreSecurityTest.java
@@ -6,6 +6,7 @@
 import apoc.util.SensitivePathGenerator;
 import apoc.util.TestUtil;
 import com.fasterxml.jackson.core.JsonParseException;
+import inet.ipaddr.IPAddressString;
 import org.apache.commons.lang3.exception.ExceptionUtils;
 import org.junit.After;
 import org.junit.Before;
@@ -15,7 +16,9 @@
 import org.junit.experimental.runners.Enclosed;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
+import org.neo4j.configuration.GraphDatabaseInternalSettings;
 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;
@@ -29,6 +32,8 @@
 import java.util.UUID;
 import java.util.stream.Collectors;
 
+import static apoc.security.WebSecurity.assertBlockedIpRange;
+import static apoc.util.TestUtil.testCall;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -37,7 +42,8 @@
 public class LoadCoreSecurityTest {
 
     public static Path import_folder;
-
+    private static final String BASE_ADDRESS = "127.168.0.1";
+
     static {
         try {
             import_folder = File.createTempFile(UUID.randomUUID().toString(), "tmp")
@@ -46,9 +52,10 @@ public class LoadCoreSecurityTest {
             throw new RuntimeException(e);
         }
     }
-
+    
     @ClassRule
     public static DbmsRule db = new ImpermanentDbmsRule()
+            .withSetting(GraphDatabaseInternalSettings.cypher_ip_blocklist, List.of(new IPAddressString(BASE_ADDRESS + "/8")))
             .withSetting(GraphDatabaseSettings.load_csv_file_url_root, import_folder)
             .withSetting(ApocSettings.apoc_import_file_enabled, false);
 
@@ -154,6 +161,11 @@ public void testReadSensitiveFileWorks() {
                 }
             }
         }
+
+        @Test
+        public void testLoadJsonFromBlockedIpRange() {
+            assertBlockedIpRange(db, apocProcedure, BASE_ADDRESS);
+        }
     }
 
     private static void assertError(Exception e, String errorMessage, Class<? extends Exception> exceptionType, String apocProcedure) {

diff --git a/core/src/test/java/apoc/load/LoadJsonTest.java b/core/src/test/java/apoc/load/LoadJsonTest.java
@@ -1,14 +1,18 @@
 package apoc.load;
 
+import apoc.ApocSettings;
 import apoc.util.CompressionAlgo;
 import apoc.util.JsonUtil;
 import apoc.util.TestUtil;
 import apoc.util.Util;
 import org.apache.commons.lang.exception.ExceptionUtils;
+import inet.ipaddr.IPAddressString;
 import org.junit.*;
 import org.mockserver.client.MockServerClient;
 import org.mockserver.integration.ClientAndServer;
 import org.mockserver.model.Header;
+
+import org.neo4j.configuration.GraphDatabaseInternalSettings;
 import org.neo4j.graphdb.QueryExecutionException;
 import org.neo4j.graphdb.Result;
 import org.neo4j.internal.helpers.collection.Iterators;
@@ -56,7 +60,8 @@ public static void stopServer() {
     }
 
     @Rule
-    public DbmsRule db = new ImpermanentDbmsRule();
+    public DbmsRule db = new ImpermanentDbmsRule()
+            .withSetting(GraphDatabaseInternalSettings.cypher_ip_blocklist, List.of(new IPAddressString("127.0.0.1/8")));
 //            .withSetting(ApocSettings.apoc_import_file_enabled, true)
 //            .withSetting(ApocSettings.apoc_import_file_use__neo4j__config, false);
 

diff --git a/full/src/test/java/apoc/load/LoadCsvTest.java b/full/src/test/java/apoc/load/LoadCsvTest.java
@@ -7,10 +7,12 @@
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.dataformat.csv.CsvMapper;
 import com.fasterxml.jackson.dataformat.csv.CsvSchema;
+import inet.ipaddr.IPAddressString;
 import org.junit.*;
 import org.mockserver.client.MockServerClient;
 import org.mockserver.integration.ClientAndServer;
 import org.mockserver.model.Header;
+import org.neo4j.configuration.GraphDatabaseInternalSettings;
 import org.neo4j.configuration.GraphDatabaseSettings;
 import org.neo4j.graphdb.QueryExecutionException;
 import org.neo4j.graphdb.Result;
@@ -31,6 +33,7 @@
 import static apoc.util.MapUtil.map;
 import static apoc.util.TestUtil.*;
 import static java.util.Arrays.asList;
+import static junit.framework.TestCase.assertTrue;
 import static org.junit.Assert.*;
 import static org.junit.Assert.assertEquals;
 import static org.mockserver.integration.ClientAndServer.startClientAndServer;
@@ -61,6 +64,7 @@ public static void stopServer() {
     @Rule
     public DbmsRule db = new ImpermanentDbmsRule()
                 .withSetting(ApocSettings.apoc_import_file_enabled, true)
+                .withSetting(GraphDatabaseInternalSettings.cypher_ip_blocklist, List.of(new IPAddressString("127.168.0.1/8")))
                 .withSetting(GraphDatabaseSettings.load_csv_file_url_root, Paths.get(getUrlFileName("test.csv").toURI()).getParent());
 
     private GenericContainer httpServer;

diff --git a/full/src/test/java/apoc/load/LoadFullSecurityTest.java b/full/src/test/java/apoc/load/LoadFullSecurityTest.java
@@ -5,6 +5,7 @@
 import apoc.util.FileUtils;
 import apoc.util.SensitivePathGenerator;
 import apoc.util.TestUtil;
+import inet.ipaddr.IPAddressString;
 import org.apache.commons.lang3.exception.ExceptionUtils;
 import org.apache.poi.openxml4j.exceptions.InvalidFormatException;
 import org.junit.After;
@@ -15,7 +16,9 @@
 import org.junit.experimental.runners.Enclosed;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
+import org.neo4j.configuration.GraphDatabaseInternalSettings;
 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;
@@ -29,6 +32,8 @@
 import java.util.UUID;
 import java.util.stream.Collectors;
 
+import static apoc.security.WebSecurity.assertBlockedIpRange;
+import static apoc.util.TestUtil.testCall;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -37,6 +42,7 @@
 public class LoadFullSecurityTest {
 
     public static Path import_folder;
+    private static final String BASE_ADDRESS = "127.168.0.1";
 
     static {
         try {
@@ -49,6 +55,7 @@ public class LoadFullSecurityTest {
 
     @ClassRule
     public static DbmsRule db = new ImpermanentDbmsRule()
+            .withSetting(GraphDatabaseInternalSettings.cypher_ip_blocklist, List.of(new IPAddressString(BASE_ADDRESS + "/8")))
             .withSetting(GraphDatabaseSettings.load_csv_file_url_root, import_folder);
 
     @BeforeClass
@@ -149,6 +156,11 @@ public void testReadSensitiveFileWorks() {
                 }
             }
         }
+
+        @Test
+        public void testLoadJsonFromBlockedIpRange() {
+            assertBlockedIpRange(db, apocProcedure, BASE_ADDRESS);
+        }
     }
 
     private static void assertError(Exception e, String errorMessage, Class<? extends Exception> exceptionType, String apocProcedure) {

diff --git a/readme.adoc b/readme.adoc
@@ -286,7 +286,7 @@ docker run \
 git clone http://github.com/neo4j-contrib/neo4j-apoc-procedures
 cd neo4j-apoc-procedures
 ./gradlew shadow
-cp build/libs/apoc-<version>-all.jar $NEO4J_HOME/plugins/
+cp build/full/libs/apoc-<version>-all.jar $NEO4J_HOME/plugins/
 $NEO4J_HOME/bin/neo4j restart
 ----
 

diff --git a/test-utils/src/main/java/apoc/security/WebSecurity.java b/test-utils/src/main/java/apoc/security/WebSecurity.java
@@ -0,0 +1,30 @@
+package apoc.security;
+
+import org.neo4j.configuration.GraphDatabaseInternalSettings;
+import org.neo4j.graphdb.QueryExecutionException;
+import org.neo4j.kernel.internal.GraphDatabaseAPI;
+
+import java.util.List;
+import java.util.Map;
+
+import static apoc.util.TestUtil.testCall;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+public class WebSecurity {
+
+    public static void assertBlockedIpRange(GraphDatabaseAPI db, String procedure, String baseAddress) {
+        List.of("https", "http", "ftp").forEach(protocol -> {
+            try {
+                testCall(db, "CALL " + procedure,
+                        Map.of("fileName", String.format("%s://%s/file.test", protocol, baseAddress)),
+                        r -> fail());
+            } catch (QueryExecutionException e) {
+                final String expectedMsg = String.format("access to /%s is blocked via the configuration property %s", 
+                        baseAddress, GraphDatabaseInternalSettings.cypher_ip_blocklist.name());
+
+                assertTrue(e.getMessage().contains(expectedMsg));
+            }
+        });
+    }
+}