Skip to content

Commit cb934e1

Browse files
RasmusWLRasmusWL
committed
Python: Adjust SSRF location to request call
Since that might not be the same place where the vulnerable URL part is.
1 parent b1bca85 commit cb934e1

File tree

5 files changed

+72
-51
lines changed

5 files changed

+72
-51
lines changed

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

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,12 @@ module ServerSideRequestForgery {
2424
/**
2525
* A data flow sink for "Server-side request forgery" vulnerabilities.
2626
*/
27-
abstract class Sink extends DataFlow::Node { }
27+
abstract class Sink extends DataFlow::Node {
28+
/**
29+
* Gets the request this sink belongs to.
30+
*/
31+
abstract HTTP::Client::Request getRequest();
32+
}
2833

2934
/**
3035
* A sanitizer for "Server-side request forgery" vulnerabilities.
@@ -50,12 +55,24 @@ module ServerSideRequestForgery {
5055

5156
/** The URL of an HTTP request, considered as a sink. */
5257
class HttpRequestUrlAsSink extends Sink {
58+
HTTP::Client::Request req;
59+
5360
HttpRequestUrlAsSink() {
54-
exists(HTTP::Client::Request req | req.getAUrlPart() = this) and
55-
// Since we find sinks inside stdlib, we need to exclude them manually. See
56-
// comment for command injection sinks for more details.
57-
not this.getScope().getEnclosingModule().getName() in ["http.client", "httplib"]
61+
req.getAUrlPart() = this and
62+
// if we extract the stdlib code for HTTPConnection, we will also find calls that
63+
// make requests within the HTTPConnection implementation -- for example the
64+
// `request` method calls the `_send_request` method internally. So without this
65+
// extra bit of code, we would give alerts within the HTTPConnection
66+
// implementation as well, which is just annoying.
67+
//
68+
// Notice that we're excluding based on the request location, and not the URL part
69+
// location, since the URL part would be in user code for the scenario above.
70+
//
71+
// See comment for command injection sinks for more details.
72+
not req.getScope().getEnclosingModule().getName() in ["http.client", "httplib"]
5873
}
74+
75+
override HTTP::Client::Request getRequest() { result = req }
5976
}
6077

6178
/**

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ import DataFlow::PathGraph
1616

1717
from
1818
FullServerSideRequestForgery::Configuration config, DataFlow::PathNode source,
19-
DataFlow::PathNode sink
20-
where config.hasFlowPath(source, sink)
21-
select sink.getNode(), source, sink, "The full URL of this request depends on $@.",
22-
source.getNode(), "a user-provided value"
19+
DataFlow::PathNode sink, HTTP::Client::Request request
20+
where
21+
request = sink.getNode().(FullServerSideRequestForgery::Sink).getRequest() and
22+
config.hasFlowPath(source, sink)
23+
select request, source, sink, "The full URL of this request depends on $@.", source.getNode(),
24+
"a user-provided value"

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,10 @@ import DataFlow::PathGraph
1717
from
1818
FullServerSideRequestForgery::Configuration fullConfig,
1919
PartialServerSideRequestForgery::Configuration partialConfig, DataFlow::PathNode source,
20-
DataFlow::PathNode sink
20+
DataFlow::PathNode sink, HTTP::Client::Request request
2121
where
22+
request = sink.getNode().(PartialServerSideRequestForgery::Sink).getRequest() and
2223
partialConfig.hasFlowPath(source, sink) and
2324
not fullConfig.hasFlow(source.getNode(), sink.getNode())
24-
select sink.getNode(), source, sink, "Part of the URL of this request depends on $@.",
25-
source.getNode(), "a user-provided value"
25+
select request, source, sink, "Part of the URL of this request depends on $@.", source.getNode(),
26+
"a user-provided value"

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

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -145,14 +145,15 @@ nodes
145145
| test_requests.py:8:18:8:27 | ControlFlowNode for user_input | semmle.label | ControlFlowNode for user_input |
146146
subpaths
147147
#select
148-
| full_partial_test.py:10:18:10:27 | ControlFlowNode for user_input | full_partial_test.py:7:18:7:24 | ControlFlowNode for request | full_partial_test.py:10:18:10:27 | ControlFlowNode for user_input | The full URL of this request depends on $@. | full_partial_test.py:7:18:7:24 | ControlFlowNode for request | a user-provided value |
149-
| test_http_client.py:13:27:13:37 | ControlFlowNode for unsafe_host | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:13:27:13: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 |
150-
| test_http_client.py:14:25:14:35 | ControlFlowNode for unsafe_path | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:14:25:14: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 |
151-
| test_http_client.py:14:25:14:35 | ControlFlowNode for unsafe_path | test_http_client.py:10:19:10:25 | ControlFlowNode for request | test_http_client.py:14:25:14: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 |
152-
| test_http_client.py:18:27:18:37 | ControlFlowNode for unsafe_host | 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 |
153-
| test_http_client.py:19:25:19:35 | ControlFlowNode for unsafe_path | 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 |
154-
| test_http_client.py:19:25:19:35 | ControlFlowNode for unsafe_path | 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:25:27:25:37 | ControlFlowNode for unsafe_host | 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 |
156-
| test_http_client.py:29:25:29:35 | ControlFlowNode for unsafe_path | 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 |
157-
| test_http_client.py:29:25:29:35 | ControlFlowNode for unsafe_path | 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 |
158-
| test_requests.py:8:18:8:27 | ControlFlowNode for user_input | 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 |
148+
| full_partial_test.py:10:5:10:28 | ControlFlowNode for Attribute() | full_partial_test.py:7:18:7:24 | ControlFlowNode for request | full_partial_test.py:10:18:10:27 | ControlFlowNode for user_input | The full URL of this request depends on $@. | full_partial_test.py:7:18:7:24 | ControlFlowNode for request | a user-provided value |
149+
| test_http_client.py:14:5:14:36 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:13:27:13: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 |
150+
| test_http_client.py:14:5:14:36 | ControlFlowNode for Attribute() | test_http_client.py:9:19:9:25 | ControlFlowNode for request | test_http_client.py:14:25:14: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 |
151+
| test_http_client.py:14:5:14:36 | ControlFlowNode for Attribute() | test_http_client.py:10:19:10:25 | ControlFlowNode for request | test_http_client.py:14:25:14: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 |
152+
| 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 |
153+
| 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 |
154+
| 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 |
159+
| 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 |

0 commit comments

Comments
 (0)