Skip to content

Commit aa3482c

Browse files
erik-krogherik-krogh
committed
improve detection of duplicate results with js/code-injection 
1 parent 5142670 commit aa3482c

3 files changed

Lines changed: 54 additions & 11 deletions

File tree

‎javascript/ql/src/Security/CWE-094/ImproperCodeSanitization.ql‎

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
* external/cwe/cwe-116
1212
*/
1313

14-
// TODO: Proper customizations module, Source class Sink class etc.
14+
// TODO: Proper customizations module, Source class Sink class etc. and qldoc.
1515
import javascript
1616
import DataFlow::PathGraph
1717
private import semmle.javascript.heuristics.HeuristicSinks
@@ -33,7 +33,7 @@ class Configuration extends TaintTracking::Configuration {
3333
}
3434
}
3535

36-
private DataFlow::Node source() {
36+
private DataFlow::CallNode source() {
3737
result instanceof HtmlSanitizerCall
3838
or
3939
result = DataFlow::globalVarRef("JSON").getAMemberCall("stringify")
@@ -42,8 +42,7 @@ private DataFlow::Node source() {
4242
private StringOps::ConcatenationLeaf sink() {
4343
exists(StringOps::ConcatenationRoot root, int i |
4444
root.getOperand(i) = result and
45-
not exists(result.getStringValue()) and
46-
not root = endsInCodeInjectionSink()
45+
not exists(result.getStringValue())
4746
|
4847
exists(StringOps::ConcatenationLeaf functionLeaf |
4948
functionLeaf = root.getOperand(any(int j | j < i))
@@ -56,19 +55,39 @@ private StringOps::ConcatenationLeaf sink() {
5655
)
5756
}
5857

58+
DataFlow::SourceNode remoteFlow(DataFlow::TypeTracker t) {
59+
t.start() and
60+
result instanceof RemoteFlowSource
61+
or
62+
exists(DataFlow::TypeTracker t2 | result = remoteFlow(t2).track(t2, t))
63+
}
64+
65+
DataFlow::SourceNode remoteFlow() { result = remoteFlow(DataFlow::TypeTracker::end()) }
66+
5967
private DataFlow::Node endsInCodeInjectionSink(DataFlow::TypeBackTracker t) {
6068
t.start() and
61-
(result instanceof CodeInjection::Sink or result instanceof HeuristicCodeInjectionSink) and
62-
not result instanceof StringOps::ConcatenationRoot // the heuristic CodeInjection sink looks for string-concats, we are not interrested in those here.
69+
(
70+
result instanceof CodeInjection::Sink
71+
or
72+
result instanceof HeuristicCodeInjectionSink and
73+
not result instanceof StringOps::ConcatenationRoot // the heuristic CodeInjection sink looks for string-concats, we are not interrested in those here.
74+
)
6375
or
6476
exists(DataFlow::TypeBackTracker t2 | t = t2.smallstep(result, endsInCodeInjectionSink(t2)))
6577
}
6678

67-
private DataFlow::Node endsInCodeInjectionSink() {
68-
result = endsInCodeInjectionSink(DataFlow::TypeBackTracker::end())
79+
DataFlow::Node endsInCodeInjectionSink() {
80+
result = endsInCodeInjectionSink(DataFlow::TypeBackTracker::end()) and
81+
result.getFile().getBaseName() = "bad-code-sanitization.js" // TODO: TMp
6982
}
7083

7184
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
72-
where cfg.hasFlowPath(source, sink)
85+
where
86+
cfg.hasFlowPath(source, sink) and
87+
// Basic detection of duplicate results with `js/code-injection`.
88+
not (
89+
sink.getNode().(StringOps::ConcatenationLeaf).getRoot() = endsInCodeInjectionSink() and
90+
remoteFlow().flowsTo(source.getNode().(DataFlow::InvokeNode).getAnArgument())
91+
)
7392
select sink.getNode(), source, sink, "$@ flows to here and is used to construct code.",
7493
source.getNode(), "Improperly sanitized value"

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.expected‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,21 @@ nodes
1616
| bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) |
1717
| bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) |
1818
| bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) |
19+
| bad-code-sanitization.js:31:30:31:50 | JSON.st ... (input) |
20+
| bad-code-sanitization.js:31:30:31:50 | JSON.st ... (input) |
21+
| bad-code-sanitization.js:31:30:31:50 | JSON.st ... (input) |
1922
| bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) |
2023
| bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) |
2124
| bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) |
2225
| bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) |
2326
| bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) |
2427
| bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) |
28+
| bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) |
29+
| bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) |
30+
| bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) |
31+
| bad-code-sanitization.js:54:29:54:63 | JSON.st ... bble")) |
32+
| bad-code-sanitization.js:54:29:54:63 | JSON.st ... bble")) |
33+
| bad-code-sanitization.js:54:29:54:63 | JSON.st ... bble")) |
2534
edges
2635
| bad-code-sanitization.js:2:12:2:90 | /^[_$a- ... key)}]` | bad-code-sanitization.js:7:31:7:43 | safeProp(key) |
2736
| bad-code-sanitization.js:2:65:2:90 | `[${JSO ... key)}]` | bad-code-sanitization.js:2:12:2:90 | /^[_$a- ... key)}]` |
@@ -35,11 +44,16 @@ edges
3544
| bad-code-sanitization.js:8:27:8:36 | statements | bad-code-sanitization.js:8:27:8:46 | statements.join(';') |
3645
| bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) |
3746
| bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) | bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) |
47+
| bad-code-sanitization.js:31:30:31:50 | JSON.st ... (input) | bad-code-sanitization.js:31:30:31:50 | JSON.st ... (input) |
3848
| bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) | bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) |
3949
| bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) |
50+
| bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) | bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) |
51+
| bad-code-sanitization.js:54:29:54:63 | JSON.st ... bble")) | bad-code-sanitization.js:54:29:54:63 | JSON.st ... bble")) |
4052
#select
4153
| bad-code-sanitization.js:8:27:8:46 | statements.join(';') | bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) | bad-code-sanitization.js:8:27:8:46 | statements.join(';') | $@ flows to here and is used to construct code. | bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) | Improperly sanitized value |
4254
| bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | Improperly sanitized value |
4355
| bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) | bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) | bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) | Improperly sanitized value |
56+
| bad-code-sanitization.js:31:30:31:50 | JSON.st ... (input) | bad-code-sanitization.js:31:30:31:50 | JSON.st ... (input) | bad-code-sanitization.js:31:30:31:50 | JSON.st ... (input) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:31:30:31:50 | JSON.st ... (input) | Improperly sanitized value |
4457
| bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) | bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) | bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) | Improperly sanitized value |
4558
| bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | Improperly sanitized value |
59+
| bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) | bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) | bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) | Improperly sanitized value |

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/bad-code-sanitization.js‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ function test4(input) {
2828
}
2929

3030
function test4(input) {
31-
var foo = `(function(){${JSON.stringify(input)}))` // OK - for this query - we can type-track to a code-injection sink.
31+
var foo = `(function(){${JSON.stringify(input)}))` // NOT OK - we can type-track to a code-injection sink, the source is not remote flow.
3232
setTimeout(foo);
3333
}
3434

@@ -42,4 +42,14 @@ function test6(input) {
4242

4343
function test7(input) {
4444
return `() => {${JSON.stringify(input)}` // NOT OK
45-
}
45+
}
46+
47+
var express = require('express');
48+
49+
var app = express();
50+
51+
app.get('/some/path', function(req, res) {
52+
var foo = `(function(){${JSON.stringify(req.param("wobble"))}))` // NOT - the source is remote-flow, but we know of no sink.
53+
54+
setTimeout(`(function(){${JSON.stringify(req.param("wobble"))}))`); // OK - the source is remote-flow, and the sink is code-injection.
55+
});

0 commit comments

Comments
 (0)