Adds load json ssrf mitigation (#2467)

neo4j-contrib · Feb 2, 2022 · ebd1dd5 · ebd1dd5
1 parent 312920c
commit ebd1dd5
Show file tree

Hide file tree

Showing 8 changed files with 108 additions and 6 deletions.
diff --git a/core/build.gradle b/core/build.gradle
@@ -60,6 +60,7 @@ dependencies {
         exclude module: "snakeyaml"
     }
     compile group: 'org.yaml', name: 'snakeyaml', version: '1.26'
+    compile group: 'com.github.seancfoley', name: 'ipaddress', version: '5.3.3'
     testCompile group: 'com.github.stefanbirkner', name: 'system-rules', version: '1.19.0'
 
     testCompile '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
@@ -2,6 +2,7 @@
 
 import apoc.export.util.ExportConfig;
 import apoc.util.SimpleRateLimiter;
+import inet.ipaddr.IPAddressString;
 import org.apache.commons.configuration2.Configuration;
 import org.apache.commons.configuration2.PropertiesConfiguration;
 import org.apache.commons.configuration2.builder.combined.CombinedConfigurationBuilder;
@@ -20,7 +21,10 @@
 import org.neo4j.logging.NullLog;
 import org.neo4j.logging.internal.LogService;
 
+import java.io.IOException;
 import java.lang.reflect.Field;
+import java.net.InetAddress;
+import java.net.MalformedURLException;
 import java.net.URL;
 import java.time.Duration;
 import java.time.ZoneId;
@@ -43,8 +47,8 @@
 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 CYPHER_IP_BLOCKLIST = "unsupported.dbms.cypher_ip_blocklist";
     public static final String APOC_IMPORT_FILE_ENABLED = "apoc.import.file.enabled";
     public static final String APOC_EXPORT_FILE_ENABLED = "apoc.export.file.enabled";
     public static final String APOC_IMPORT_FILE_USE_NEO4J_CONFIG = "apoc.import.file.use_neo4j_config";
@@ -99,13 +103,17 @@ public class ApocConfig extends LifecycleAdapter {
     private LoggingType loggingType;
     private SimpleRateLimiter rateLimiter;
     private GraphDatabaseService systemDb;
+
+    private List<IPAddressString> blockedIpRanges = List.of();
+
     /**
      * keep track if this instance is already initialized so dependent class can wait if needed
      */
     private boolean initialized = false;
 
     public ApocConfig(Config neo4jConfig, LogService log, GlobalProcedures globalProceduresRegistry, DatabaseManagementService databaseManagementService) {
         this.neo4jConfig = neo4jConfig;
+        this.blockedIpRanges = neo4jConfig.get(ApocSettings.cypher_ip_blocklist);
         this.log = log.getInternalLog(ApocConfig.class);
         this.databaseManagementService = databaseManagementService;
         theInstance = this;
@@ -259,9 +267,37 @@ public void isImportFileEnabled() {
         }
     }
 
-    public void checkReadAllowed(String url) {
+    /*
+        TODO
+        This needs to be cleaned up in 5.0
+        WebURLAccessRule was added in the database in 4.4.5, so it would
+        break backwards compatibility with 4.4.xx previous versions
+    */
+    private void checkAllowedUrl(String url) throws IOException {
+        try {
+            if (blockedIpRanges != null && !blockedIpRanges.isEmpty()) {
+                URL parsedUrl = new URL(url);
+                InetAddress inetAddress = InetAddress.getByName(parsedUrl.getHost());
+
+                for (var blockedIpRange : blockedIpRanges)
+                {
+                    if (blockedIpRange.contains(new IPAddressString(inetAddress.getHostAddress())))
+                    {
+                        throw new IOException("access to " + inetAddress + " is blocked via the configuration property "
+                                               + ApocSettings.cypher_ip_blocklist.name());
+                    }
+                }
+            }
+        } catch (MalformedURLException e) {
+            throw new IOException(e);
+        }
+    }
+
+    public void checkReadAllowed(String url) throws IOException {
         if (isFile(url)) {
             isImportFileEnabled();
+        } else {
+            checkAllowedUrl(url);
         }
     }
 

diff --git a/core/src/main/java/apoc/ApocSettings.java b/core/src/main/java/apoc/ApocSettings.java
@@ -1,12 +1,16 @@
 package apoc;
 
+import inet.ipaddr.AddressStringException;
+import inet.ipaddr.IPAddressString;
+
 import org.neo4j.annotations.service.ServiceProvider;
 import org.neo4j.configuration.Description;
 import org.neo4j.configuration.SettingValueParser;
 import org.neo4j.configuration.SettingsDeclaration;
 import org.neo4j.graphdb.config.Setting;
 
 import java.time.Duration;
+import java.util.List;
 
 import static apoc.ApocConfig.*;
 import static org.neo4j.configuration.SettingImpl.newBuilder;
@@ -19,6 +23,43 @@
  */
 @ServiceProvider
 public class ApocSettings implements SettingsDeclaration {
+    /*
+        TODO
+        This needs to be cleaned up in 5.0, along with cypher_ip_blocklist
+        The option was added in the database in 4.4.5, so it would
+        break backwards compatibility with 4.4.xx previous versions
+     */
+    public static final SettingValueParser<IPAddressString> CIDR_IP = new SettingValueParser<>()
+    {
+        @Override
+        public IPAddressString parse( String value )
+        {
+            IPAddressString ipAddress = new IPAddressString( value.trim() );
+            try
+            {
+                ipAddress.validate();
+            }
+            catch ( AddressStringException e )
+            {
+                throw new IllegalArgumentException( String.format( "'%s' is not a valid CIDR ip", value ), e );
+            }
+            return ipAddress;
+        }
+
+        @Override
+        public String getDescription()
+        {
+            return "an ip with subnet in CDIR format. e.g. 127.168.0.1/8";
+        }
+
+        @Override
+        public Class<IPAddressString> getType()
+        {
+            return IPAddressString.class;
+        }
+    };
+
+    public static final Setting<List<IPAddressString>> cypher_ip_blocklist = newBuilder( CYPHER_IP_BLOCKLIST, listOf( CIDR_IP ), List.of() ).build();
 
     public static final Setting<Boolean> apoc_export_file_enabled = newBuilder( APOC_EXPORT_FILE_ENABLED, BOOL, false ).build();
 

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
@@ -43,6 +43,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"),
@@ -66,6 +67,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;
@@ -415,6 +416,7 @@ private static InputStream getFileStreamIntoCompressedFile(InputStream is, Strin
     }
 
     public static StreamConnection readHttpInputStream(String urlAddress, Map<String, Object> headers, String payload) throws IOException {
+        ApocConfig.apocConfig().checkReadAllowed(urlAddress);
         URLConnection con = openUrlConnection(urlAddress, headers);
         writePayload(con, payload);
         String newUrl = handleRedirect(con, urlAddress);

diff --git a/core/src/test/java/apoc/load/LoadJsonTest.java b/core/src/test/java/apoc/load/LoadJsonTest.java
@@ -1,14 +1,17 @@
 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.server.MockServerClient;
 import org.mockserver.integration.ClientAndServer;
 import org.mockserver.model.Header;
+
 import org.neo4j.graphdb.QueryExecutionException;
 import org.neo4j.graphdb.Result;
 import org.neo4j.internal.helpers.collection.Iterators;
@@ -56,7 +59,8 @@ public static void stopServer() {
     }
 
     @Rule
-    public DbmsRule db = new ImpermanentDbmsRule();
+    public DbmsRule db = new ImpermanentDbmsRule()
+            .withSetting(ApocSettings.cypher_ip_blocklist, List.of(new IPAddressString("127.168.0.0/8")));
 //            .withSetting(ApocSettings.apoc_import_file_enabled, true)
 //            .withSetting(ApocSettings.apoc_import_file_use__neo4j__config, false);
 
@@ -76,6 +80,21 @@ public static void stopServer() {
                 });
     }
 
+    @Test public void testLoadJsonFromBlockedIpRange() throws Exception {
+        var protocols = List.of("https", "http", "ftp");
+
+        for (var protocol: protocols) {
+            QueryExecutionException e = Assert.assertThrows(QueryExecutionException.class,
+                                            () -> testCall(db,
+                                                           "CALL apoc.load.json('" + protocol + "://127.168.0.0/test.csv')",
+                                                           map(),
+                                                           (r) -> {}
+                                            )
+            );
+            assertTrue(e.getMessage().contains("access to /127.168.0.0 is blocked via the configuration property unsupported.dbms.cypher_ip_blocklist"));
+        }
+    }
+
     @Test 
     public void testLoadMultiJsonWithBinary() {
         testResult(db, "CALL apoc.load.jsonParams($url, null, null, null, $config)",

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
 ----