Skip to content

Commit 4b5599f

Browse files
RasmusWLRasmusWL
committed
Python: Improve full/partial SSRF split
Now full-ssrf will only alert if **all** URL parts are fully user-controlled.
1 parent cb934e1 commit 4b5599f

5 files changed

Lines changed: 23 additions & 8 deletions

File tree

‎python/ql/lib/semmle/python/security/dataflow/ServerSideRequestForgery.qll‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,16 @@
99
private import python
1010
import semmle.python.dataflow.new.DataFlow
1111
import semmle.python.dataflow.new.TaintTracking
12+
import semmle.python.Concepts
1213

1314
/**
1415
* Provides a taint-tracking configuration for detecting "Server-side request forgery" vulnerabilities.
1516
*
1617
* This configuration has a sanitizer to limit results to cases where attacker has full control of URL.
1718
* See `PartialServerSideRequestForgery` for a variant without this requirement.
19+
*
20+
* You should use the `partOfFullyControlledRequest` to only select results where all
21+
* URL parts are fully controlled.
1822
*/
1923
module FullServerSideRequestForgery {
2024
import ServerSideRequestForgeryCustomizations::ServerSideRequestForgery
@@ -41,6 +45,17 @@ module FullServerSideRequestForgery {
4145
}
4246
}
4347

48+
/**
49+
* Holds if all URL parts of `request` is fully user controlled.
50+
*/
51+
predicate fullyControlledRequest(HTTP::Client::Request request) {
52+
exists(FullServerSideRequestForgery::Configuration fullConfig |
53+
forall(DataFlow::Node urlPart | urlPart = request.getAUrlPart() |
54+
fullConfig.hasFlow(_, urlPart)
55+
)
56+
)
57+
}
58+
4459
/**
4560
* Provides a taint-tracking configuration for detecting "Server-side request forgery" vulnerabilities.
4661
*

‎python/ql/src/Security/CWE-918/FullServerSideRequestForgery.ql‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,11 @@ import semmle.python.security.dataflow.ServerSideRequestForgery
1515
import DataFlow::PathGraph
1616

1717
from
18-
FullServerSideRequestForgery::Configuration config, DataFlow::PathNode source,
18+
FullServerSideRequestForgery::Configuration fullConfig, DataFlow::PathNode source,
1919
DataFlow::PathNode sink, HTTP::Client::Request request
2020
where
2121
request = sink.getNode().(FullServerSideRequestForgery::Sink).getRequest() and
22-
config.hasFlowPath(source, sink)
22+
fullConfig.hasFlowPath(source, sink) and
23+
fullyControlledRequest(request)
2324
select request, source, sink, "The full URL of this request depends on $@.", source.getNode(),
2425
"a user-provided value"

‎python/ql/src/Security/CWE-918/PartialServerSideRequestForgery.ql‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,11 @@ import semmle.python.security.dataflow.ServerSideRequestForgery
1515
import DataFlow::PathGraph
1616

1717
from
18-
FullServerSideRequestForgery::Configuration fullConfig,
1918
PartialServerSideRequestForgery::Configuration partialConfig, DataFlow::PathNode source,
2019
DataFlow::PathNode sink, HTTP::Client::Request request
2120
where
2221
request = sink.getNode().(PartialServerSideRequestForgery::Sink).getRequest() and
2322
partialConfig.hasFlowPath(source, sink) and
24-
not fullConfig.hasFlow(source.getNode(), sink.getNode())
23+
not fullyControlledRequest(request)
2524
select request, source, sink, "Part of the URL of this request depends on $@.", source.getNode(),
2625
"a user-provided value"

‎python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/FullServerSideRequestForgery.expected‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,4 @@ subpaths
152152
| 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 |
153153
| 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 |
154154
| 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 |
155-
| 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 |
156-
| 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 |
157-
| 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 |
158-
| 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 |
159155
| 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 |

‎python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/PartialServerSideRequestForgery.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,10 @@ subpaths
167167
| 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 |
168168
| 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 |
169169
| 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 |
170+
| 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 |
171+
| 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 |
172+
| 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 |
173+
| 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 |
170174
| 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 |
171175
| 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 |
172176
| 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 |

0 commit comments

Comments
 (0)