Skip to content

Commit 629efb8

Browse files
author
Nati Pesaresi
committed
ternary operator
1 parent 0b5c890 commit 629efb8

File tree

2 files changed

+242
-1
lines changed

2 files changed

+242
-1
lines changed

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

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,66 @@ class Configuration extends TaintTracking::Configuration {
2020
}
2121
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) {
2222
nd instanceof IntegerCheck or
23-
nd instanceof ValidatorCheck
23+
nd instanceof ValidatorCheck or
24+
nd instanceof TernaryOperatorSanitizerGuard
25+
}
26+
}
27+
28+
/** TODO add comment */
29+
class TernaryOperatorSanitizerGuard extends TaintTracking::SanitizerGuardNode {
30+
TaintTracking::SanitizerGuardNode originalGuard;
31+
32+
TernaryOperatorSanitizerGuard() {
33+
exists(DataFlow::Node falseNode |
34+
this.getAPredecessor+() = falseNode and
35+
falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false)
36+
) and
37+
this.getAPredecessor+() = originalGuard and
38+
not this.asExpr() instanceof LogicalBinaryExpr
39+
}
40+
41+
override predicate sanitizes(boolean outcome, Expr e) {
42+
not this.asExpr() instanceof LogNotExpr and
43+
originalGuard.sanitizes(outcome, e)
44+
or
45+
exists(boolean originalOutcome |
46+
this.asExpr() instanceof LogNotExpr and
47+
originalGuard.sanitizes(originalOutcome, e) and
48+
(
49+
originalOutcome = true and outcome = false
50+
or
51+
originalOutcome = false and outcome = true
52+
)
53+
)
54+
}
55+
}
56+
57+
/** TODO add comment */
58+
class TernaryOperatorSanitizer extends RequestForgery::Sanitizer {
59+
TernaryOperatorSanitizer() {
60+
exists(
61+
TaintTracking::SanitizerGuardNode guard, IfStmt ifStmt, DataFlow::Node taintedInput, boolean outcome,
62+
Stmt r, DataFlow::Node falseNode
63+
|
64+
ifStmt.getCondition().flow().getAPredecessor+() = guard and
65+
ifStmt.getCondition().flow().getAPredecessor+() = falseNode and
66+
falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and
67+
not ifStmt.getCondition() instanceof LogicalBinaryExpr and
68+
guard.sanitizes(outcome, taintedInput.asExpr()) and
69+
(
70+
outcome = true and r = ifStmt.getThen() and not ifStmt.getCondition() instanceof LogNotExpr
71+
or
72+
outcome = false and r = ifStmt.getElse() and not ifStmt.getCondition() instanceof LogNotExpr
73+
or
74+
outcome = false and r = ifStmt.getThen() and ifStmt.getCondition() instanceof LogNotExpr
75+
or
76+
outcome = true and r = ifStmt.getElse() and ifStmt.getCondition() instanceof LogNotExpr
77+
) and
78+
r.getFirstControlFlowNode()
79+
.getBasicBlock()
80+
.(ReachableBasicBlock)
81+
.dominates(this.getBasicBlock())
82+
)
2483
}
2584
}
2685

‎javascript/ql/test/experimental/Security/CWE-918/ternary-operator.js

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
const express = require('express');
2+
const app = express();
3+
4+
app.use(express.json());
5+
6+
app.get('/direct-ternary-operator', function (req, res) {
7+
let taintedURL = req.params.url
8+
9+
let v = req.params.url ? req.params.url == "someURL" : false
10+
if (v) {
11+
req_frontend_restclient.get(req.params.url) // OK
12+
}
13+
14+
let v1 = taintedURL ? taintedURL == "someURL" : false
15+
if (v1) {
16+
req_frontend_restclient.get(taintedURL) // OK
17+
}
18+
19+
let v2 = taintedURL ? valid(taintedURL) : false
20+
if (v2) {
21+
req_frontend_restclient.get(taintedURL) // OK
22+
}
23+
24+
let v3 = req.params.url ? valid(req.params.url) : false
25+
if (v3) {
26+
req_frontend_restclient.get(req.params.url) // OK
27+
}
28+
29+
let v4 = req.params.url == undefined ? false : valid(req.params.url)
30+
if (v4) {
31+
req_frontend_restclient.get(req.params.url) // OK
32+
}
33+
34+
let v5 = req.params.url == undefined ? true : valid(req.params.url)
35+
if (v5) {
36+
req_frontend_restclient.get(req.params.url) // SSRF
37+
}
38+
39+
let v6 = req.params.url ? valid(req.params.url) : true
40+
if (v6) {
41+
req_frontend_restclient.get(req.params.url) // SSRF
42+
}
43+
44+
let f = false
45+
let v7 = req.params.url ? valid(req.params.url) : true
46+
if (v7) {
47+
req_frontend_restclient.get(req.params.url) // SSRF
48+
}
49+
50+
let v8 = req.params.url == undefined ? false : valid(req.params.url)
51+
if (!v8) {
52+
return
53+
}
54+
req_frontend_restclient.get(req.params.url) // OK
55+
})
56+
57+
app.get('/functions', function (req, res) {
58+
let taintedURL = req.params.url
59+
60+
if (valid2(taintedURL)) {
61+
req_frontend_restclient.get(taintedURL) // OK
62+
}
63+
64+
if (!invalid(taintedURL)) {
65+
req_frontend_restclient.get(taintedURL) // False positive
66+
}
67+
68+
if (valid2(req.params.url)){
69+
req_frontend_restclient.get(req.params.url) // OK
70+
}
71+
72+
if (!assertAlphanumeric(req.params.url)) {
73+
return
74+
}
75+
req_frontend_restclient.get(req.params.url); // OK
76+
})
77+
78+
app.get('/normal-use-of-ternary-operator', function (req, res) {
79+
let taintedURL = req.params.url
80+
81+
let url = valid(req.params.url) ? req.params.url : undefined
82+
req_frontend_restclient.get(url) // OK
83+
84+
let url = valid(taintedURL) ? taintedURL : undefined
85+
req_frontend_restclient.get(url) // OK
86+
87+
let url4 = req.params.url.match(/^[\w.-]+$/) ? req.params.url : undefined
88+
req_frontend_restclient.get(url4) // OK
89+
})
90+
91+
app.get('/throw-errors', function (req, res) {
92+
req_frontend_restclient.get(valid3(req.params.url)) // False positive
93+
94+
req_frontend_restclient.get(assertOther(req.params.url)); // False positive
95+
96+
req_frontend_restclient.get(assertOther2(req.params.url)); // False positive
97+
});
98+
99+
app.get('/bad-endpoint', function (req, res) {
100+
req_frontend_restclient.get(req.params.url); // SSRF
101+
102+
const valid = req.params.url ? req.params.url == "someURL" : false
103+
if (!valid) {
104+
throw new Error(`Invalid parameter: "${req.params.url}", must be alphanumeric`);
105+
}
106+
req_frontend_restclient.get(req.params.url); // OK
107+
})
108+
109+
110+
app.get('/bad-endpoint-variable', function (req, res) {
111+
let taintedURL = req.params.url
112+
req_frontend_restclient.get(taintedURL); // SSRF
113+
114+
const valid = taintedURL ? taintedURL == "someURL" : false
115+
if (!valid) {
116+
return
117+
}
118+
req_frontend_restclient.get(taintedURL); // False positive
119+
})
120+
121+
app.get('/not-invalid', function (req, res) {
122+
const invalidParam = req.params.url ? !Number.isInteger(req.params.url) : false
123+
if (invalidParam) {
124+
return
125+
}
126+
req_frontend_restclient.get(req.params.url); // False positive
127+
})
128+
129+
130+
app.get('/bad-endpoint-2', function (req, res) {
131+
other(req.params.url)
132+
})
133+
134+
function other(taintedURL) {
135+
req_frontend_restclient.get(taintedURL); // SSRF
136+
137+
const valid = taintedURL ? taintedURL == "someURL" : false
138+
if (!valid) {
139+
return
140+
}
141+
req_frontend_restclient.get(taintedURL); // False positive
142+
}
143+
144+
function assertAlphanumeric(value) {
145+
return value ? value.match(/^[\w.-]+$/) : false;
146+
}
147+
148+
function assertOther(value) {
149+
const valid = value ? !!value.match(/^[\w.-]+$/) : false;
150+
if (!valid) {
151+
throw new Error(`Invalid parameter: "${value}", must be alphanumeric`);
152+
}
153+
return value;
154+
}
155+
156+
function assertOther2(value) {
157+
const valid = value ? value.match(/^[\w.-]+$/) : false;
158+
if (!valid) {
159+
throw new Error(`Invalid parameter: "${value}", must be alphanumeric`);
160+
}
161+
return value;
162+
}
163+
164+
function invalid(value) {
165+
return value ? !Number.isInteger(value) : true
166+
}
167+
168+
function valid(value) {
169+
return value.match(/^[\w.-]+$/)
170+
}
171+
172+
function valid2(value) {
173+
return value ? value == "someURL" : false
174+
}
175+
176+
function valid3(value) {
177+
const valid = value ? value == "someURL" : false
178+
if (!valid) {
179+
throw new Error(`Invalid parameter: "${value}", must be alphanumeric`);
180+
}
181+
return value;
182+
}

0 commit comments

Comments
 (0)