diff --git a/python/ql/lib/semmle/python/security/dataflow/ServerSideRequestForgery.qll b/python/ql/lib/semmle/python/security/dataflow/ServerSideRequestForgery.qll index 70239f42ff5..ba19be68713 100644 --- 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 index 8ec3eb5e063..e9ca51c406c 100644 --- 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 index 34dd9676501..59abd68643d 100644 --- 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 index f44e3ad3c27..2cdec0a5996 100644 --- 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 index cf3eade5da1..43e24d280c2 100644 --- 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 |