Skip to content

Commit a5bde53

Browse files
erik-krogherik-krogh
committed
use the TaintedObject library in js/template-object-injection 
1 parent c6a2284 commit a5bde53

File tree

3 files changed

+65
-21
lines changed

3 files changed

+65
-21
lines changed

‎javascript/ql/src/experimental/Security/CWE-073/TemplateObjectInjection.ql‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@
1111
*/
1212

1313
import javascript
14-
import DataFlow
15-
import PathGraph
14+
import DataFlow::PathGraph
15+
import semmle.javascript.security.TaintedObject
1616

1717
predicate isUsingHbsEngine() {
1818
Express::appCreation().getAMethodCall("set").getArgument(1).mayHaveStringValue("hbs")
@@ -21,19 +21,34 @@ predicate isUsingHbsEngine() {
2121
class HbsLFRTaint extends TaintTracking::Configuration {
2222
HbsLFRTaint() { this = "HbsLFRTaint" }
2323

24-
override predicate isSource(Node node) { node instanceof RemoteFlowSource }
24+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
2525

26-
override predicate isSink(Node node) {
26+
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
27+
TaintedObject::isSource(source, label)
28+
}
29+
30+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
31+
label = TaintedObject::label() and
2732
exists(MethodCallExpr mc |
2833
Express::isResponse(mc.getReceiver()) and
2934
mc.getMethodName() = "render" and
30-
node.asExpr() = mc.getArgument(1) and
35+
sink.asExpr() = mc.getArgument(1) and
3136
isUsingHbsEngine()
3237
)
3338
}
39+
40+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
41+
guard instanceof TaintedObject::SanitizerGuard
42+
}
43+
44+
override predicate isAdditionalFlowStep(
45+
DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
46+
) {
47+
TaintedObject::step(src, trg, inlbl, outlbl)
48+
}
3449
}
3550

36-
from HbsLFRTaint cfg, PathNode source, PathNode sink
51+
from HbsLFRTaint cfg, DataFlow::PathNode source, DataFlow::PathNode sink
3752
where cfg.hasFlowPath(source, sink)
3853
select sink.getNode(), source, sink, "Template object injection due to $@.", source.getNode(),
3954
"user-provided value"

‎javascript/ql/test/experimental/Security/CWE-073/TemplateObjectInjection.expected‎

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,34 +4,52 @@ nodes
44
| tst.js:5:25:5:32 | req.body |
55
| tst.js:5:25:5:46 | req.bod ... rameter |
66
| tst.js:6:9:6:49 | queryParameter |
7+
| tst.js:6:9:6:49 | queryParameter |
8+
| tst.js:6:26:6:49 | req.que ... rameter |
79
| tst.js:6:26:6:49 | req.que ... rameter |
810
| tst.js:6:26:6:49 | req.que ... rameter |
911
| tst.js:8:28:8:40 | bodyParameter |
1012
| tst.js:8:28:8:40 | bodyParameter |
1113
| tst.js:9:28:9:41 | queryParameter |
1214
| tst.js:9:28:9:41 | queryParameter |
13-
| tst.js:12:32:12:44 | bodyParameter |
14-
| tst.js:12:32:12:44 | bodyParameter |
15-
| tst.js:14:28:14:41 | queryParameter |
16-
| tst.js:14:28:14:46 | queryParameter + "" |
17-
| tst.js:14:28:14:46 | queryParameter + "" |
15+
| tst.js:18:19:18:32 | queryParameter |
16+
| tst.js:18:19:18:32 | queryParameter |
17+
| tst.js:21:24:21:26 | obj |
18+
| tst.js:21:24:21:26 | obj |
19+
| tst.js:22:28:22:30 | obj |
20+
| tst.js:22:28:22:30 | obj |
21+
| tst.js:24:11:24:24 | str |
22+
| tst.js:24:17:24:19 | obj |
23+
| tst.js:24:17:24:24 | obj + "" |
24+
| tst.js:27:28:27:42 | JSON.parse(str) |
25+
| tst.js:27:28:27:42 | JSON.parse(str) |
26+
| tst.js:27:39:27:41 | str |
1827
edges
1928
| tst.js:5:9:5:46 | bodyParameter | tst.js:8:28:8:40 | bodyParameter |
2029
| tst.js:5:9:5:46 | bodyParameter | tst.js:8:28:8:40 | bodyParameter |
21-
| tst.js:5:9:5:46 | bodyParameter | tst.js:12:32:12:44 | bodyParameter |
22-
| tst.js:5:9:5:46 | bodyParameter | tst.js:12:32:12:44 | bodyParameter |
2330
| tst.js:5:25:5:32 | req.body | tst.js:5:25:5:46 | req.bod ... rameter |
2431
| tst.js:5:25:5:32 | req.body | tst.js:5:25:5:46 | req.bod ... rameter |
2532
| tst.js:5:25:5:46 | req.bod ... rameter | tst.js:5:9:5:46 | bodyParameter |
2633
| tst.js:6:9:6:49 | queryParameter | tst.js:9:28:9:41 | queryParameter |
2734
| tst.js:6:9:6:49 | queryParameter | tst.js:9:28:9:41 | queryParameter |
28-
| tst.js:6:9:6:49 | queryParameter | tst.js:14:28:14:41 | queryParameter |
35+
| tst.js:6:9:6:49 | queryParameter | tst.js:18:19:18:32 | queryParameter |
36+
| tst.js:6:9:6:49 | queryParameter | tst.js:18:19:18:32 | queryParameter |
37+
| tst.js:6:26:6:49 | req.que ... rameter | tst.js:6:9:6:49 | queryParameter |
38+
| tst.js:6:26:6:49 | req.que ... rameter | tst.js:6:9:6:49 | queryParameter |
2939
| tst.js:6:26:6:49 | req.que ... rameter | tst.js:6:9:6:49 | queryParameter |
3040
| tst.js:6:26:6:49 | req.que ... rameter | tst.js:6:9:6:49 | queryParameter |
31-
| tst.js:14:28:14:41 | queryParameter | tst.js:14:28:14:46 | queryParameter + "" |
32-
| tst.js:14:28:14:41 | queryParameter | tst.js:14:28:14:46 | queryParameter + "" |
41+
| tst.js:18:19:18:32 | queryParameter | tst.js:21:24:21:26 | obj |
42+
| tst.js:18:19:18:32 | queryParameter | tst.js:21:24:21:26 | obj |
43+
| tst.js:21:24:21:26 | obj | tst.js:22:28:22:30 | obj |
44+
| tst.js:21:24:21:26 | obj | tst.js:22:28:22:30 | obj |
45+
| tst.js:21:24:21:26 | obj | tst.js:24:17:24:19 | obj |
46+
| tst.js:24:11:24:24 | str | tst.js:27:39:27:41 | str |
47+
| tst.js:24:17:24:19 | obj | tst.js:24:17:24:24 | obj + "" |
48+
| tst.js:24:17:24:24 | obj + "" | tst.js:24:11:24:24 | str |
49+
| tst.js:27:39:27:41 | str | tst.js:27:28:27:42 | JSON.parse(str) |
50+
| tst.js:27:39:27:41 | str | tst.js:27:28:27:42 | JSON.parse(str) |
3351
#select
3452
| tst.js:8:28:8:40 | bodyParameter | tst.js:5:25:5:32 | req.body | tst.js:8:28:8:40 | bodyParameter | Template object injection due to $@. | tst.js:5:25:5:32 | req.body | user-provided value |
3553
| tst.js:9:28:9:41 | queryParameter | tst.js:6:26:6:49 | req.que ... rameter | tst.js:9:28:9:41 | queryParameter | Template object injection due to $@. | tst.js:6:26:6:49 | req.que ... rameter | user-provided value |
36-
| tst.js:12:32:12:44 | bodyParameter | tst.js:5:25:5:32 | req.body | tst.js:12:32:12:44 | bodyParameter | Template object injection due to $@. | tst.js:5:25:5:32 | req.body | user-provided value |
37-
| tst.js:14:28:14:46 | queryParameter + "" | tst.js:6:26:6:49 | req.que ... rameter | tst.js:14:28:14:46 | queryParameter + "" | Template object injection due to $@. | tst.js:6:26:6:49 | req.que ... rameter | user-provided value |
54+
| tst.js:22:28:22:30 | obj | tst.js:6:26:6:49 | req.que ... rameter | tst.js:22:28:22:30 | obj | Template object injection due to $@. | tst.js:6:26:6:49 | req.que ... rameter | user-provided value |
55+
| tst.js:27:28:27:42 | JSON.parse(str) | tst.js:6:26:6:49 | req.que ... rameter | tst.js:27:28:27:42 | JSON.parse(str) | Template object injection due to $@. | tst.js:6:26:6:49 | req.que ... rameter | user-provided value |

‎javascript/ql/test/experimental/Security/CWE-073/tst.js‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,20 @@ app.post('/path', function(req, res) {
99
res.render('template', queryParameter); // NOT OK
1010

1111
if (typeof bodyParameter === "string") {
12-
res.render('template', bodyParameter); // OK - but still flagged [INCONSISTENCY]
12+
res.render('template', bodyParameter); // OK
1313
}
14-
res.render('template', queryParameter + ""); // OK - but still flagged [INCONSISTENCY]
14+
res.render('template', queryParameter + ""); // OK
1515

1616
res.render('template', {profile: bodyParameter}); // OK
17-
});
17+
18+
indirect(res, queryParameter);
19+
});
20+
21+
function indirect(res, obj) {
22+
res.render("template", obj); // NOT OK
23+
24+
const str = obj + "";
25+
res.render("template", str); // OK
26+
27+
res.render("template", JSON.parse(str)); // NOT OK
28+
}

0 commit comments

Comments
 (0)