Skip to content

Commit 2895428

Browse files
codeql-cicodeql-ci
authored
Merge pull request #6714 from valeria-meli/javascript/ssrf
Approved by asgerf
2 parents 5515256 + e509385 commit 2895428

File tree

13 files changed

+879
-0
lines changed

13 files changed

+879
-0
lines changed

‎javascript/ql/src/experimental/Security/CWE-918/SSRF.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
const axios = require('axios');
2+
3+
export const handler = async (req, res, next) => {
4+
const { target } = req.body;
5+
6+
try {
7+
// BAD: `target` is controlled by the attacker
8+
const response = await axios.get('https://example.com/current_api/' + target);
9+
10+
// process request response
11+
use(response);
12+
} catch (err) {
13+
// process error
14+
}
15+
};

‎javascript/ql/src/experimental/Security/CWE-918/SSRF.qhelp

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Directly incorporating user input into an HTTP request without validating the input can facilitate
9+
server side request forgery attacks, where the attacker essentially controls the request.
10+
</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>
15+
To guard against server side request forgery, it is advisable to avoid putting user input directly into a
16+
network request. If using user input is necessary, then is mandatory to validate them. Only allow numeric and alphanumeric values.
17+
URL encoding is not a solution in certain scenarios, such as, an architecture build over NGINX proxies.
18+
</p>
19+
</recommendation>
20+
21+
<example>
22+
<p>
23+
The following example shows an HTTP request parameter being used directly in a URL request without
24+
validating the input, which facilitates an SSRF attack. The request <code>axios.get("https://example.com/current_api/"+target)</code> is
25+
vulnerable since attackers can choose the value of <code>target</code> to be anything they want. For
26+
instance, the attacker can choose <code>"../super_secret_api"</code> as the target, causing the
27+
URL to become <code>"https://example.com/super_secret_api"</code>.
28+
</p>
29+
30+
<p>
31+
A request to <code>https://example.com/super_secret_api</code> may be problematic if that api is not
32+
meant to be directly accessible from the attacker's machine.
33+
</p>
34+
35+
<sample src="SSRF.js"/>
36+
37+
<p>
38+
One way to remedy the problem is to validate the user input to only allow alphanumeric values:
39+
</p>
40+
41+
<sample src="SSRFGood.js"/>
42+
</example>
43+
44+
<references>
45+
46+
<li>OWASP: <a href="https://www.owasp.org/www-community/attacks/Server_Side_Request_Forgery">SSRF</a></li>
47+
48+
</references>
49+
</qhelp>

‎javascript/ql/src/experimental/Security/CWE-918/SSRF.ql

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
/**
2+
* @id javascript/ssrf
3+
* @kind path-problem
4+
* @name Uncontrolled data used in network request
5+
* @description Sending network requests with user-controlled data as part of the URL allows for request forgery attacks.
6+
* @problem.severity error
7+
* @precision medium
8+
* @tags security
9+
* external/cwe/cwe-918
10+
*/
11+
12+
import javascript
13+
import SSRF
14+
import DataFlow::PathGraph
15+
16+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node request
17+
where
18+
cfg.hasFlowPath(source, sink) and request = sink.getNode().(RequestForgery::Sink).getARequest()
19+
select sink, source, sink, "The URL of this request depends on a user-provided value"

‎javascript/ql/src/experimental/Security/CWE-918/SSRF.qll

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
import javascript
2+
import semmle.javascript.security.dataflow.RequestForgeryCustomizations
3+
import semmle.javascript.security.dataflow.UrlConcatenation
4+
5+
class Configuration extends TaintTracking::Configuration {
6+
Configuration() { this = "SSRF" }
7+
8+
override predicate isSource(DataFlow::Node source) { source instanceof RequestForgery::Source }
9+
10+
override predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgery::Sink }
11+
12+
override predicate isSanitizer(DataFlow::Node node) {
13+
super.isSanitizer(node) or
14+
node instanceof RequestForgery::Sanitizer
15+
}
16+
17+
private predicate hasSanitizingSubstring(DataFlow::Node nd) {
18+
nd.getStringValue().regexpMatch(".*[?#].*")
19+
or
20+
hasSanitizingSubstring(StringConcatenation::getAnOperand(nd))
21+
or
22+
hasSanitizingSubstring(nd.getAPredecessor())
23+
}
24+
25+
private predicate strictSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
26+
exists(DataFlow::Node operator, int n |
27+
StringConcatenation::taintStep(source, sink, operator, n) and
28+
hasSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1]))
29+
)
30+
}
31+
32+
override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) {
33+
strictSanitizingPrefixEdge(source, sink)
34+
}
35+
36+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) {
37+
nd instanceof IntegerCheck or
38+
nd instanceof ValidatorCheck or
39+
nd instanceof TernaryOperatorSanitizerGuard
40+
}
41+
}
42+
43+
/**
44+
* This sanitizers models the next example:
45+
* let valid = req.params.id ? Number.isInteger(req.params.id) : false
46+
* if (valid) { sink(req.params.id) }
47+
*
48+
* This sanitizer models this way of using ternary operators,
49+
* when the sanitizer guard is used as any of the branches
50+
* instead of being used as the condition.
51+
*
52+
* This sanitizer sanitize the corresponding if statement branch.
53+
*/
54+
class TernaryOperatorSanitizer extends RequestForgery::Sanitizer {
55+
TernaryOperatorSanitizer() {
56+
exists(
57+
TaintTracking::SanitizerGuardNode guard, IfStmt ifStmt, DataFlow::Node taintedInput,
58+
boolean outcome, Stmt r, DataFlow::Node falseNode
59+
|
60+
ifStmt.getCondition().flow().getAPredecessor+() = guard and
61+
ifStmt.getCondition().flow().getAPredecessor+() = falseNode and
62+
falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and
63+
not ifStmt.getCondition() instanceof LogicalBinaryExpr and
64+
guard.sanitizes(outcome, taintedInput.asExpr()) and
65+
(
66+
outcome = true and r = ifStmt.getThen() and not ifStmt.getCondition() instanceof LogNotExpr
67+
or
68+
outcome = false and r = ifStmt.getElse() and not ifStmt.getCondition() instanceof LogNotExpr
69+
or
70+
outcome = false and r = ifStmt.getThen() and ifStmt.getCondition() instanceof LogNotExpr
71+
or
72+
outcome = true and r = ifStmt.getElse() and ifStmt.getCondition() instanceof LogNotExpr
73+
) and
74+
r.getFirstControlFlowNode()
75+
.getBasicBlock()
76+
.(ReachableBasicBlock)
77+
.dominates(this.getBasicBlock())
78+
)
79+
}
80+
}
81+
82+
/**
83+
* This sanitizer guard is another way of modeling the example from above
84+
* In this case:
85+
* let valid = req.params.id ? Number.isInteger(req.params.id) : false
86+
* if (!valid) { return }
87+
* sink(req.params.id)
88+
*
89+
* The previous sanitizer is not enough,
90+
* because we are sanitizing the entire if statement branch
91+
* but we need to sanitize the use of this variable from now on.
92+
*
93+
* Thats why we model this sanitizer guard which says that
94+
* the result of the ternary operator execution is a sanitizer guard.
95+
*/
96+
class TernaryOperatorSanitizerGuard extends TaintTracking::SanitizerGuardNode {
97+
TaintTracking::SanitizerGuardNode originalGuard;
98+
99+
TernaryOperatorSanitizerGuard() {
100+
this.getAPredecessor+().asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and
101+
this.getAPredecessor+() = originalGuard and
102+
not this.asExpr() instanceof LogicalBinaryExpr
103+
}
104+
105+
override predicate sanitizes(boolean outcome, Expr e) {
106+
not this.asExpr() instanceof LogNotExpr and
107+
originalGuard.sanitizes(outcome, e)
108+
or
109+
exists(boolean originalOutcome |
110+
this.asExpr() instanceof LogNotExpr and
111+
originalGuard.sanitizes(originalOutcome, e) and
112+
(
113+
originalOutcome = true and outcome = false
114+
or
115+
originalOutcome = false and outcome = true
116+
)
117+
)
118+
}
119+
}
120+
121+
/**
122+
* Number.isInteger is a sanitizer guard because a number can't be used to exploit a SSRF.
123+
*/
124+
class IntegerCheck extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode {
125+
IntegerCheck() { this = DataFlow::globalVarRef("Number").getAMemberCall("isInteger") }
126+
127+
override predicate sanitizes(boolean outcome, Expr e) {
128+
outcome = true and
129+
e = getArgument(0).asExpr()
130+
}
131+
}
132+
133+
/**
134+
* ValidatorCheck identifies if exists a call to validator's library methods.
135+
* validator is a library which has a variety of input-validation functions. We are interesed in
136+
* checking that source is a number (any type of number) or an alphanumeric value.
137+
*/
138+
class ValidatorCheck extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode {
139+
ValidatorCheck() {
140+
exists(DataFlow::SourceNode mod, string method |
141+
mod = DataFlow::moduleImport("validator") and
142+
this = mod.getAChainedMethodCall(method) and
143+
method in [
144+
"isAlphanumeric", "isAlpha", "isDecimal", "isFloat", "isHexadecimal", "isHexColor",
145+
"isInt", "isNumeric", "isOctal", "isUUID"
146+
]
147+
)
148+
}
149+
150+
override predicate sanitizes(boolean outcome, Expr e) {
151+
outcome = true and
152+
e = getArgument(0).asExpr()
153+
}
154+
}

‎javascript/ql/src/experimental/Security/CWE-918/SSRFGood.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
const axios = require('axios');
2+
const validator = require('validator');
3+
4+
export const handler = async (req, res, next) => {
5+
const { target } = req.body;
6+
7+
if (!validator.isAlphanumeric(target)) {
8+
return next(new Error('Bad request'));
9+
}
10+
11+
try {
12+
// `target` is validated
13+
const response = await axios.get('https://example.com/current_api/' + target);
14+
15+
// process request response
16+
use(response);
17+
} catch (err) {
18+
// process error
19+
}
20+
};

0 commit comments

Comments
 (0)