Adds load json ssrf mitigation #2467

ncordon · 2022-01-18T11:15:15Z

What

Uses Neo4j internal option unsupported.dbms.cypher_ip_blocklist, which consists on a list of ip addresses in CDIR notation, i.e. ip + mask for subnets, e.g. 192.167.2.3/8, 192.234.3.123/16. When using the apoc.load.json(url) command, the url will be looked up in a DNS resolver and we will fail if it is on the list of blocked ip addresses.

Why

Users can try to load data from any url (including internal ones), which could expose files the user does not have permissions to see from outside.

ncordon · 2022-01-18T11:15:49Z

build.gradle

@@ -109,7 +109,7 @@ subprojects {

 ext {
    // NB: due to version.json generation by parsing this file, the next line must not have any if/then/else logic
-    neo4jVersion = "4.4.1"
+    neo4jVersion = "4.4.4-SNAPSHOT"


A new 4.4 version of the database needs to be released and substituted here.

@ncordon If we don't use a non snapshot version we aren't able to test the PR in GH can you please change it to 4.4.3?

The code we need is in 4.4.4, and to release 4.4.4 we are going to need this to be merged on red. The CI of the database will compile the APOC code overriding the version.

So even bumping this, don't expect the CI to pass.

After the database has been released, we can bump here to 4.4.4 and release a new version of APOC.

This was done without the need to depend on 4.4.4 in the end

ncordon · 2022-01-26T15:50:04Z

Configuration setting for IP block/allow list

jakewins · 2022-01-28T09:52:50Z

core/src/main/java/apoc/util/JsonUtil.java

    }

-    public static Stream<Object> loadJson(Object urlOrBinary, Map<String,Object> headers, String payload, String path, boolean failOnError, String compressionAlgo, List<String> options) {
+    public static Stream<Object> loadJson(Object urlOrBinary, Map<String,Object> headers, String payload, String path, boolean failOnError, String compressionAlgo, List<String> options, ApocConfig apocConfig) {
        try {
            if (urlOrBinary instanceof String) {
                String url = (String) urlOrBinary;


Shouldn't this check be done in checkReadAllowed, where the other access rules as processed? With the check here, you can still make calls that violate the blocklist using apoc.load.xml, apoc.xml.import, apoc.load.xmlSimple, apoc.load.xls, apoc.cypher.runFiles and so on.

Similarly, we should process the blocklist in checkWriteAllowed, otherwise people can violate the blocklist by sending POST requests and other output network connections?

yes we should not pass the full apoc config through, either just what we need or better like Jake said handle it in the single place where we control the loading.

https://github.com/neo4j-contrib/neo4j-apoc-procedures/blob/4.3/core/src/main/java/apoc/util/FileUtils.java#L142-L241

Also you don't need to pass apocConfig through but use ApocConfig.getConfig().methodCall()

there are already some methods for checking, we can handle the address as part of that.
so you would rather do something like ApocConfig#isIpAllowed(ip) or ApocConfig#isHostNameAllowed(hostname)
need to make sure though to resolve the final hostname properly to ip-address (after all the http-redirects are handled).

https://github.com/neo4j-contrib/neo4j-apoc-procedures/blob/4.3/core/src/main/java/apoc/ApocConfig.java#L250-L262

@ncordon please look at the comments above

@jexp I have addressed the concern for redirects in a pending PR: https://github.com/neo-technology/neo4j/pull/14171. The logic for the blocking lives in the kernel of the database and here I am reusing the same logic. So once that's merged we should be safe with respect to redirects (I hope).

@conker84 I get the need for checkReadAllowed, but checkWriteAllowed works only with filenames, which this blocklist doesn't include, judging by its signature?

Do we have any situation where we can do a POST to a path in a remote server?

This has been addressed.

With respect to checkWriteAllowed, we've deemed that change is not necessary as it only affects files. POST requests (load json ones for example) go through the checkReadAllowed.

write list also works with urls e.g. for s3 URLs

checkWriteAllowed is only used with things that require a physical file I believe:

…om a blocked ip range

conker84

this should be not necessary
edit. github seems that cutted my review, lemme do that again

core/src/main/java/apoc/ApocConfig.java

conker84 · 2022-01-28T13:09:26Z

core/src/main/java/apoc/util/JsonUtil.java

    }

-    public static Stream<Object> loadJson(Object urlOrBinary, Map<String,Object> headers, String payload, String path, boolean failOnError, String compressionAlgo, List<String> options) {
+    public static Stream<Object> loadJson(Object urlOrBinary, Map<String,Object> headers, String payload, String path, boolean failOnError, String compressionAlgo, List<String> options, ApocConfig apocConfig) {
        try {
            if (urlOrBinary instanceof String) {
                String url = (String) urlOrBinary;


@ncordon please look at the comments above

core/src/main/java/apoc/util/FileUtils.java

…igation

ncordon · 2022-02-01T15:32:38Z

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'


This is needed now if we want to avoid transitive dependencies that come from the database in 4.4.4

but then we need to take it out when 4.4.4 is out?

@jexp My idea is to keep in 4.4.x so we can cover also older Neo4j versions, and remove it from 5.0

conker84

Thank you so much @ncordon

Restricts load json in APOC

a122db1

ncordon commented Jan 18, 2022

View reviewed changes

jakewins reviewed Jan 28, 2022

View reviewed changes

adding tests to verify that load csv fails when attempting to load fr…

a00abfc

…om a blocked ip range

ncordon force-pushed the 4.4-load-json-ssrf-mitigation branch from bcc6103 to a00abfc Compare January 28, 2022 11:30

Fixes the rest of the code

d866d25

conker84 requested changes Jan 28, 2022

View reviewed changes

Addresses review comments

c5e8596

ncordon force-pushed the 4.4-load-json-ssrf-mitigation branch 2 times, most recently from 61ae529 to 9ca37c7 Compare January 31, 2022 12:31

Reverts build version

2670ade

ncordon force-pushed the 4.4-load-json-ssrf-mitigation branch from 9ca37c7 to 2670ade Compare January 31, 2022 12:32

ncordon commented Jan 31, 2022

View reviewed changes

core/src/main/java/apoc/util/FileUtils.java Show resolved Hide resolved

Finds a working solution for redirects

889df72

ncordon force-pushed the 4.4-load-json-ssrf-mitigation branch 5 times, most recently from c9cdfb9 to fba2b90 Compare February 1, 2022 12:25

Adds necessary deps to avoid binary incompatibilities

13814da

ncordon force-pushed the 4.4-load-json-ssrf-mitigation branch from fba2b90 to 13814da Compare February 1, 2022 12:27

Merge remote-tracking branch 'origin/4.4' into 4.4-load-json-ssrf-mit…

da47f85

…igation

ncordon force-pushed the 4.4-load-json-ssrf-mitigation branch 2 times, most recently from 5ac2f68 to 2c06679 Compare February 1, 2022 15:24

ncordon commented Feb 1, 2022

View reviewed changes

Fixes the problem with the kernel not finding the setting

c5a8ca8

ncordon force-pushed the 4.4-load-json-ssrf-mitigation branch from 432ff3d to c5a8ca8 Compare February 1, 2022 16:06

conker84 approved these changes Feb 2, 2022

View reviewed changes

conker84 merged commit ebd1dd5 into neo4j-contrib:4.4 Feb 2, 2022

vga91 added a commit to vga91/neo4j-apoc-procedures that referenced this pull request Feb 23, 2022

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

8b7063e

vga91 added a commit to vga91/neo4j-apoc-procedures that referenced this pull request Feb 23, 2022

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

6cbd4d7

ncordon added a commit that referenced this pull request Mar 18, 2022

Adds load json ssrf mitigation (#2467)

2cf10e4

ncordon added a commit that referenced this pull request Mar 18, 2022

Adds load json ssrf mitigation (#2467)

8ac1427

ncordon mentioned this pull request Mar 18, 2022

Adds load json ssrf mitigation. No AUTO cherry-pick #2649

Merged

ncordon deleted the 4.4-load-json-ssrf-mitigation branch August 8, 2022 09:50

This was referenced Aug 8, 2022

NO AUTO Adds blocklist for load methods #3116

Merged

NO AUTO Adds blocklist for load methods #3117

Merged

NO AUTO Adds blocklist for load methods #3118

Merged

NO AUTO Adds blocklist for load methods #3119

Merged

jexp mentioned this pull request Jan 19, 2023

add IP whitelist checking also to apoc.spatial.geoCode with custom provider config #3416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds load json ssrf mitigation #2467

Adds load json ssrf mitigation #2467

ncordon commented Jan 18, 2022 •

edited

Loading

ncordon Jan 18, 2022

conker84 Jan 28, 2022

ncordon Jan 31, 2022 •

edited

Loading

ncordon Feb 2, 2022

ncordon commented Jan 26, 2022

jakewins Jan 28, 2022

jexp Jan 28, 2022

jexp Jan 28, 2022

jexp Jan 28, 2022

conker84 Jan 28, 2022

ncordon Jan 31, 2022

ncordon Feb 1, 2022

jexp Feb 4, 2022

ncordon Feb 4, 2022

conker84 left a comment •

edited

Loading

conker84 Jan 28, 2022

ncordon Feb 1, 2022 •

edited

Loading

jexp Feb 4, 2022

conker84 Feb 4, 2022

conker84 left a comment

Adds load json ssrf mitigation #2467

Adds load json ssrf mitigation #2467

Conversation

ncordon commented Jan 18, 2022 • edited Loading

What

Why

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncordon Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncordon commented Jan 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conker84 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncordon Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conker84 left a comment

Choose a reason for hiding this comment

ncordon commented Jan 18, 2022 •

edited

Loading

ncordon Jan 31, 2022 •

edited

Loading

conker84 left a comment •

edited

Loading

ncordon Feb 1, 2022 •

edited

Loading