Skip to content

Commit c17d8b1

Browse files
author
Stephan Brandauer
authored
Merge pull request #8054 from asgerf/js/split-request-forgery
JS: split request forgery query into server-side and client-side variants
2 parents 31a204a + 8194c04 commit c17d8b1

21 files changed

+498
-244
lines changed

‎csharp/ql/src/experimental/CWE-918/RequestForgery.ql

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
2-
* @name Uncontrolled data used in network request
3-
* @description Sending network requests with user-controlled data allows for request forgery attacks.
2+
* @name Server-side request forgery
3+
* @description Making a network request with user-controlled data in the URL allows for request forgery attacks.
44
* @kind path-problem
55
* @problem.severity error
66
* @precision high

‎javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideRequestForgeryQuery.qll

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about client-side
3+
* request forgery.
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* the `Configuration` class is needed, otherwise
7+
* `RequestForgeryCustomizations` should be imported instead.
8+
*/
9+
10+
import javascript
11+
import UrlConcatenation
12+
import RequestForgeryCustomizations::RequestForgery
13+
14+
/**
15+
* A taint tracking configuration for client-side request forgery.
16+
*/
17+
class Configuration extends TaintTracking::Configuration {
18+
Configuration() { this = "ClientSideRequestForgery" }
19+
20+
override predicate isSource(DataFlow::Node source) {
21+
exists(Source src |
22+
source = src and
23+
not src.isServerSide()
24+
)
25+
}
26+
27+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
28+
29+
override predicate isSanitizer(DataFlow::Node node) {
30+
super.isSanitizer(node) or
31+
node instanceof Sanitizer
32+
}
33+
34+
override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) {
35+
sanitizingPrefixEdge(source, sink)
36+
}
37+
38+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
39+
isAdditionalRequestForgeryStep(pred, succ)
40+
}
41+
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,15 @@ module RequestForgery {
99
/**
1010
* A data flow source for request forgery.
1111
*/
12-
abstract class Source extends DataFlow::Node { }
12+
abstract class Source extends DataFlow::Node {
13+
/**
14+
* Holds if this source is relevant for server-side request forgery (SSRF).
15+
*
16+
* Otherwise, it is considered to be a source for client-side request forgery, which is
17+
* considered less severe than the server-side variant.
18+
*/
19+
predicate isServerSide() { any() }
20+
}
1321

1422
/**
1523
* A data flow sink for request forgery.
@@ -31,15 +39,11 @@ module RequestForgery {
3139
*/
3240
abstract class Sanitizer extends DataFlow::Node { }
3341

34-
/** A source of remote user input, considered as a flow source for request forgery. */
35-
private class RemoteFlowSourceAsSource extends Source {
36-
RemoteFlowSourceAsSource() {
37-
// Reduce FPs by excluding sources from client-side path or URL
38-
exists(RemoteFlowSource src |
39-
this = src and
40-
not src.(ClientSideRemoteFlowSource).getKind().isPathOrUrl()
41-
)
42-
}
42+
/** A source of server-side remote user input, considered as a flow source for request forgery. */
43+
private class RemoteFlowSourceAsSource extends Source instanceof RemoteFlowSource {
44+
RemoteFlowSourceAsSource() { not this.(ClientSideRemoteFlowSource).getKind().isPathOrUrl() }
45+
46+
override predicate isServerSide() { not this instanceof ClientSideRemoteFlowSource }
4347
}
4448

4549
/**

‎javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryQuery.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import RequestForgeryCustomizations::RequestForgery
1717
class Configuration extends TaintTracking::Configuration {
1818
Configuration() { this = "RequestForgery" }
1919

20-
override predicate isSource(DataFlow::Node source) { source instanceof Source }
20+
override predicate isSource(DataFlow::Node source) { source.(Source).isServerSide() }
2121

2222
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
2323

‎javascript/ql/src/Security/CWE-918/ClientSideRequestForgery.qhelp

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
9+
Directly incorporating user input in the URL of an outgoing HTTP request
10+
can enable a request forgery attack, in which the request is altered to
11+
target an unintended API endpoint or resource.
12+
13+
A client-side forged request may perform an unwanted action affecting the victim's account,
14+
or may lead to cross-site scripting if the request response is handled in an unsafe way.
15+
16+
This is different from CSRF (cross-site request forgery), and will usually bypass CSRF protections.
17+
18+
This is usually less severe than SSRF (server-side request forgery), as it does not expose internal services.
19+
</p>
20+
</overview>
21+
22+
<include src="RequestForgeryRecommendation.inc.qhelp"/>
23+
24+
<example>
25+
26+
<p>
27+
28+
The following example shows an HTTP request used to fetch the pre-rendered
29+
HTML body of a message. It is using the endpoint <code>/api/messages/ID</code>, which
30+
is believed to respond with a safe HTML string, to be embedded in the page:
31+
32+
</p>
33+
34+
<sample src="examples/ClientSideRequestForgeryBad.js"/>
35+
36+
<p>
37+
38+
However, the format of the message ID is not checked, and an attacker can abuse this to
39+
alter the endpoint targeted by the request. If they can redirect it to an endpoint that returns
40+
an untrusted value, this leads to cross-site scripting.
41+
</p>
42+
43+
<p>
44+
For example, given the query string <code>message_id=../pastebin/123</code>, the request will
45+
end up targeting the <code>/api/pastebin</code> endpoint. Or if there is an open redirect on the login page,
46+
a query string like <code>message_id=../../login?redirect_url=https://evil.com</code> could give
47+
the attacker full control over the response as well.
48+
</p>
49+
50+
<p>
51+
In example below, the input has been restricted to a number so the endpoint cannot be altered:
52+
</p>
53+
54+
<sample src="examples/ClientSideRequestForgeryGood.js"/>
55+
56+
</example>
57+
58+
<references>
59+
60+
<li>OWASP: <a href="https://cwe.mitre.org/data/definitions/918.html">Server-side request forgery</a></li>
61+
<li>OWASP: <a href="https://cwe.mitre.org/data/definitions/352.html">Cross-site request forgery</a></li>
62+
63+
</references>
64+
</qhelp>

‎javascript/ql/src/Security/CWE-918/ClientSideRequestForgery.ql

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Client-side request forgery
3+
* @description Making a client-to-server request with user-controlled data in the URL allows a request forgery attack
4+
* against the client.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 5.0
8+
* @precision medium
9+
* @id js/client-side-request-forgery
10+
* @tags security
11+
* external/cwe/cwe-918
12+
*/
13+
14+
import javascript
15+
import semmle.javascript.security.dataflow.ClientSideRequestForgeryQuery
16+
import DataFlow::PathGraph
17+
18+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node request
19+
where
20+
cfg.hasFlowPath(source, sink) and
21+
request = sink.getNode().(Sink).getARequest()
22+
select request, source, sink, "The $@ of this request depends on $@.", sink.getNode(),
23+
sink.getNode().(Sink).getKind(), source, "a user-provided value"

‎javascript/ql/src/Security/CWE-918/RequestForgery.qhelp

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,40 +6,28 @@
66
<overview>
77
<p>
88

9-
Directly incorporating user input into an HTTP request
10-
without validating the input can facilitate different kinds of request
11-
forgery attacks, where the attacker essentially controls the request.
9+
Directly incorporating user input in the URL of an outgoing HTTP request
10+
can enable a request forgery attack, in which the request is altered to
11+
target an unintended API endpoint or resource.
1212

13-
If the vulnerable request is in server-side code, then security
14-
mechanisms, such as external firewalls, can be bypassed.
13+
If the server performing the request is connected to an internal network, this can give an attacker
14+
the means to bypass the network boundary and make requests against internal services.
1515

16-
If the vulnerable request is in client-side code, then unsuspecting
17-
users can send malicious requests to other servers, potentially
18-
resulting in a DDOS attack.
16+
A forged request may perform an unintended action on behalf of the attacker, or cause information
17+
leak if redirected to an external server or if the request response is fed back to the user.
18+
It may also compromise the server making the request, if the request response is handled in an unsafe way.
1919

2020
</p>
2121
</overview>
2222

23-
<recommendation>
24-
25-
<p>
26-
27-
To guard against request forgery, it is advisable to avoid
28-
putting user input directly into a network request. If a flexible
29-
network request mechanism is required, it is recommended to maintain a
30-
list of authorized request targets and choose from that list based on
31-
the user input provided.
32-
33-
</p>
34-
35-
</recommendation>
23+
<include src="RequestForgeryRecommendation.inc.qhelp"/>
3624

3725
<example>
3826

3927
<p>
4028

4129
The following example shows an HTTP request parameter
42-
being used directly in a URL request without validating the input,
30+
being used directly in the URL of a request without validating the input,
4331
which facilitates an SSRF attack. The request
4432
<code>http.get(...)</code> is vulnerable since attackers can choose
4533
the value of <code>target</code> to be anything they want. For

‎javascript/ql/src/Security/CWE-918/RequestForgery.ql

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
/**
2-
* @name Uncontrolled data used in network request
3-
* @description Sending network requests with user-controlled data allows for request forgery attacks.
2+
* @name Server-side request forgery
3+
* @description Making a network request with user-controlled data in the URL allows for request forgery attacks.
44
* @kind path-problem
55
* @problem.severity error
66
* @security-severity 9.1
7-
* @precision medium
7+
* @precision high
88
* @id js/request-forgery
99
* @tags security
1010
* external/cwe/cwe-918

‎javascript/ql/src/Security/CWE-918/RequestForgeryRecommendation.inc.qhelp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<recommendation>
7+
8+
<p>
9+
Restrict user inputs in the URL of an outgoing request, in particular:
10+
</p>
11+
<ul>
12+
<li>
13+
Avoid user input in the hostname of the URL.
14+
Pick the hostname from an allow-list instead of constructing it directly from user input.
15+
</li>
16+
<li>
17+
Take care when user input is part of the pathname of the URL.
18+
Restrict the input so that path traversal ("<code>../</code>")
19+
cannot be used to redirect the request to an unintended endpoint.
20+
</li>
21+
</ul>
22+
23+
</recommendation>
24+
25+
</qhelp>

‎javascript/ql/src/Security/CWE-918/examples/ClientSideRequestForgeryBad.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
async function loadMessage() {
2+
const query = new URLSearchParams(location.search);
3+
const url = '/api/messages/' + query.get('message_id');
4+
const data = await (await fetch(url)).json();
5+
document.getElementById('message').innerHTML = data.html;
6+
}

0 commit comments

Comments
 (0)