Python: Improve full/partial SSRF split

RasmusWL · RasmusWL · commit 4b5599fe175e · 2021-12-16T22:48:51.000+01:00
Now full-ssrf will only alert if **all** URL parts are fully
user-controlled.
diff --git a/python/ql/lib/semmle/python/security/dataflow/ServerSideRequestForgery.qll b/python/ql/lib/semmle/python/security/dataflow/ServerSideRequestForgery.qll
@@ -9,12 +9,16 @@
 private import python
 import semmle.python.dataflow.new.DataFlow
 import semmle.python.dataflow.new.TaintTracking
+import semmle.python.Concepts
 
 /**
  * Provides a taint-tracking configuration for detecting "Server-side request forgery" vulnerabilities.
  *
  * This configuration has a sanitizer to limit results to cases where attacker has full control of URL.
  * See `PartialServerSideRequestForgery` for a variant without this requirement.
+ *
+ * You should use the `partOfFullyControlledRequest` to only select results where all
+ * URL parts are fully controlled.
  */
 module FullServerSideRequestForgery {
   import ServerSideRequestForgeryCustomizations::ServerSideRequestForgery
@@ -41,6 +45,17 @@ module FullServerSideRequestForgery {
   }
 }
 
+/**
+ * Holds if all URL parts of `request` is fully user controlled.
+ */
+predicate fullyControlledRequest(HTTP::Client::Request request) {
+  exists(FullServerSideRequestForgery::Configuration fullConfig |
+    forall(DataFlow::Node urlPart | urlPart = request.getAUrlPart() |
+      fullConfig.hasFlow(_, urlPart)
+    )
+  )
+}
+
 /**
  * Provides a taint-tracking configuration for detecting "Server-side request forgery" vulnerabilities.
  *
diff --git a/python/ql/src/Security/CWE-918/FullServerSideRequestForgery.ql b/python/ql/src/Security/CWE-918/FullServerSideRequestForgery.ql
@@ -15,10 +15,11 @@ import semmle.python.security.dataflow.ServerSideRequestForgery
 import DataFlow::PathGraph
 
 from
-  FullServerSideRequestForgery::Configuration config, DataFlow::PathNode source,
+  FullServerSideRequestForgery::Configuration fullConfig, DataFlow::PathNode source,
   DataFlow::PathNode sink, HTTP::Client::Request request
 where
   request = sink.getNode().(FullServerSideRequestForgery::Sink).getRequest() and
-  config.hasFlowPath(source, sink)
+  fullConfig.hasFlowPath(source, sink) and
+  fullyControlledRequest(request)
 select request, source, sink, "The full URL of this request depends on $@.", source.getNode(),
   "a user-provided value"
diff --git a/python/ql/src/Security/CWE-918/PartialServerSideRequestForgery.ql b/python/ql/src/Security/CWE-918/PartialServerSideRequestForgery.ql
@@ -15,12 +15,11 @@ import semmle.python.security.dataflow.ServerSideRequestForgery
 import DataFlow::PathGraph
 
 from
-  FullServerSideRequestForgery::Configuration fullConfig,
   PartialServerSideRequestForgery::Configuration partialConfig, DataFlow::PathNode source,
   DataFlow::PathNode sink, HTTP::Client::Request request
 where
   request = sink.getNode().(PartialServerSideRequestForgery::Sink).getRequest() and
   partialConfig.hasFlowPath(source, sink) and
-  not fullConfig.hasFlow(source.getNode(), sink.getNode())
+  not fullyControlledRequest(request)
 select request, source, sink, "Part of the URL of this request depends on $@.", source.getNode(),
   "a user-provided value"
diff --git a/python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/FullServerSideRequestForgery.expected b/python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/FullServerSideRequestForgery.expected
@@ -152,8 +152,4 @@ subpaths
 | test_http_client.py:19:5:19:36 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:18:27:18:37 | ControlFlowNode for unsafe_host | The full URL of this request depends on $@. | test_http_client.py:9:19:9:25 | ControlFlowNode for request | a user-provided value |
 | test_http_client.py:19:5:19:36 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:19:25:19:35 | ControlFlowNode for unsafe_path | The full URL of this request depends on $@. | test_http_client.py:9:19:9:25 | ControlFlowNode for request | a user-provided value |
 | test_http_client.py:19:5:19:36 | ControlFlowNode for Attribute() | test_http_client.py:10:19:10:25 | ControlFlowNode for request | test_http_client.py:19:25:19:35 | ControlFlowNode for unsafe_path | The full URL of this request depends on $@. | test_http_client.py:10:19:10:25 | ControlFlowNode for request | a user-provided value |
-| test_http_client.py:22:5:22:31 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:18:27:18:37 | ControlFlowNode for unsafe_host | The full URL of this request depends on $@. | test_http_client.py:9:19:9:25 | ControlFlowNode for request | a user-provided value |
-| test_http_client.py:26:5:26:31 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:25:27:25:37 | ControlFlowNode for unsafe_host | The full URL of this request depends on $@. | test_http_client.py:9:19:9:25 | ControlFlowNode for request | a user-provided value |
-| test_http_client.py:29:5:29:36 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:29:25:29:35 | ControlFlowNode for unsafe_path | The full URL of this request depends on $@. | test_http_client.py:9:19:9:25 | ControlFlowNode for request | a user-provided value |
-| test_http_client.py:29:5:29:36 | ControlFlowNode for Attribute() | test_http_client.py:10:19:10:25 | ControlFlowNode for request | test_http_client.py:29:25:29:35 | ControlFlowNode for unsafe_path | The full URL of this request depends on $@. | test_http_client.py:10:19:10:25 | ControlFlowNode for request | a user-provided value |
 | test_requests.py:8:5:8:28 | ControlFlowNode for Attribute() | test_requests.py:6:18:6:24 | ControlFlowNode for request | test_requests.py:8:18:8:27 | ControlFlowNode for user_input | The full URL of this request depends on $@. | test_requests.py:6:18:6:24 | ControlFlowNode for request | a user-provided value |
diff --git a/python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/PartialServerSideRequestForgery.expected b/python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/PartialServerSideRequestForgery.expected
@@ -167,6 +167,10 @@ subpaths
 | full_partial_test.py:72:5:72:21 | ControlFlowNode for Attribute() | full_partial_test.py:60:18:60:24 | ControlFlowNode for request | full_partial_test.py:72:18:72:20 | ControlFlowNode for url | Part of the URL of this request depends on $@. | full_partial_test.py:60:18:60:24 | ControlFlowNode for request | a user-provided value |
 | full_partial_test.py:78:5:78:21 | ControlFlowNode for Attribute() | full_partial_test.py:60:18:60:24 | ControlFlowNode for request | full_partial_test.py:78:18:78:20 | ControlFlowNode for url | Part of the URL of this request depends on $@. | full_partial_test.py:60:18:60:24 | ControlFlowNode for request | a user-provided value |
 | full_partial_test.py:81:5:81:21 | ControlFlowNode for Attribute() | full_partial_test.py:60:18:60:24 | ControlFlowNode for request | full_partial_test.py:81:18:81:20 | ControlFlowNode for url | Part of the URL of this request depends on $@. | full_partial_test.py:60:18:60:24 | ControlFlowNode for request | a user-provided value |
+| test_http_client.py:22:5:22:31 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:18:27:18:37 | ControlFlowNode for unsafe_host | Part of the URL of this request depends on $@. | test_http_client.py:9:19:9:25 | ControlFlowNode for request | a user-provided value |
+| test_http_client.py:26:5:26:31 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:25:27:25:37 | ControlFlowNode for unsafe_host | Part of the URL of this request depends on $@. | test_http_client.py:9:19:9:25 | ControlFlowNode for request | a user-provided value |
+| test_http_client.py:29:5:29:36 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:29:25:29:35 | ControlFlowNode for unsafe_path | Part of the URL of this request depends on $@. | test_http_client.py:9:19:9:25 | ControlFlowNode for request | a user-provided value |
+| test_http_client.py:29:5:29:36 | ControlFlowNode for Attribute() | test_http_client.py:10:19:10:25 | ControlFlowNode for request | test_http_client.py:29:25:29:35 | ControlFlowNode for unsafe_path | Part of the URL of this request depends on $@. | test_http_client.py:10:19:10:25 | ControlFlowNode for request | a user-provided value |
 | test_http_client.py:33:5:33:29 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:33:25:33:28 | ControlFlowNode for path | Part of the URL of this request depends on $@. | test_http_client.py:9:19:9:25 | ControlFlowNode for request | a user-provided value |
 | test_http_client.py:33:5:33:29 | ControlFlowNode for Attribute() | test_http_client.py:10:19:10:25 | ControlFlowNode for request | test_http_client.py:33:25:33:28 | ControlFlowNode for path | Part of the URL of this request depends on $@. | test_http_client.py:10:19:10:25 | ControlFlowNode for request | a user-provided value |
 | test_http_client.py:33:5:33:29 | ControlFlowNode for Attribute() | test_http_client.py:11:18:11:24 | ControlFlowNode for request | test_http_client.py:33:25:33:28 | ControlFlowNode for path | Part of the URL of this request depends on $@. | test_http_client.py:11:18:11:24 | ControlFlowNode for request | a user-provided value |
