Skip to content

Commit 65e508a

Browse files
author
Max Schaefer
authored
Merge pull request #1252 from esben-semmle/mb/1.20-master
Mergeback: rc/1.20 into Semmle/master
2 parents aeebc36 + c80ee3d commit 65e508a

File tree

11 files changed

+351
-24
lines changed

11 files changed

+351
-24
lines changed

‎javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,34 @@ predicate allBackslashesEscaped(DataFlow::Node nd) {
101101
allBackslashesEscaped(nd.getAPredecessor())
102102
}
103103

104+
/**
105+
* Holds if `repl` looks like a call to "String.prototype.replace" that deliberately removes the first occurrence of `str`.
106+
*/
107+
predicate removesFirstOccurence(DataFlow::MethodCallNode repl, string str) {
108+
repl.getMethodName() = "replace" and
109+
repl.getArgument(0).getStringValue() = str and
110+
repl.getArgument(1).getStringValue() = ""
111+
}
112+
113+
/**
114+
* Holds if `leftUnwrap` and `rightUnwrap` unwraps a string from a pair of surrounding delimiters.
115+
*/
116+
predicate isDelimiterUnwrapper(
117+
DataFlow::MethodCallNode leftUnwrap, DataFlow::MethodCallNode rightUnwrap
118+
) {
119+
exists(string left, string right |
120+
left = "[" and right = "]"
121+
or
122+
left = "{" and right = "}"
123+
or
124+
left = "(" and right = ")"
125+
|
126+
removesFirstOccurence(leftUnwrap, left) and
127+
removesFirstOccurence(rightUnwrap, right) and
128+
leftUnwrap.getAMethodCall() = rightUnwrap
129+
)
130+
}
131+
104132
from MethodCallExpr repl, Expr old, string msg
105133
where
106134
repl.getMethodName() = "replace" and
@@ -122,7 +150,10 @@ where
122150
)
123151
) and
124152
// don't flag replace operations in a loop
125-
not DataFlow::valueNode(repl.getReceiver()) = DataFlow::valueNode(repl).getASuccessor+()
153+
not DataFlow::valueNode(repl.getReceiver()) = DataFlow::valueNode(repl).getASuccessor+() and
154+
// dont' flag unwrapper
155+
not isDelimiterUnwrapper(repl.flow(), _) and
156+
not isDelimiterUnwrapper(_, repl.flow())
126157
or
127158
exists(RegExpLiteral rel |
128159
isBackslashEscape(repl, rel) and

‎javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteSanitization.expected‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,4 +15,14 @@
1515
| tst.js:61:10:61:18 | s.replace | This replaces only the first occurrence of "'" + "". |
1616
| tst.js:65:10:65:18 | s.replace | This replaces only the first occurrence of "'". |
1717
| tst.js:69:10:69:18 | s.replace | This replaces only the first occurrence of "'" + "". |
18-
| tst.js:169:9:169:17 | s.replace | This replaces only the first occurrence of /'/. |
18+
| tst.js:133:2:133:10 | s.replace | This replaces only the first occurrence of '<'. |
19+
| tst.js:133:2:133:27 | s.repla ... replace | This replaces only the first occurrence of '>'. |
20+
| tst.js:135:2:135:10 | s.replace | This replaces only the first occurrence of '['. |
21+
| tst.js:135:2:135:30 | s.repla ... replace | This replaces only the first occurrence of ']'. |
22+
| tst.js:136:2:136:10 | s.replace | This replaces only the first occurrence of '{'. |
23+
| tst.js:136:2:136:30 | s.repla ... replace | This replaces only the first occurrence of '}'. |
24+
| tst.js:140:2:140:10 | s.replace | This replaces only the first occurrence of /{/. |
25+
| tst.js:140:2:140:27 | s.repla ... replace | This replaces only the first occurrence of /}/. |
26+
| tst.js:141:2:141:10 | s.replace | This replaces only the first occurrence of ']'. |
27+
| tst.js:141:2:141:27 | s.repla ... replace | This replaces only the first occurrence of '['. |
28+
| tst.js:185:9:185:17 | s.replace | This replaces only the first occurrence of /'/. |

‎javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,21 @@ function good11(s) {
126126
return s.replace("%d", "42");
127127
}
128128

129+
function good12(s) {
130+
s.replace('[', '').replace(']', ''); // OK
131+
s.replace('(', '').replace(')', ''); // OK
132+
s.replace('{', '').replace('}', ''); // OK
133+
s.replace('<', '').replace('>', ''); // NOT OK: too common as a bad HTML sanitizer
134+
135+
s.replace('[', '\\[').replace(']', '\\]'); // NOT OK
136+
s.replace('{', '\\{').replace('}', '\\}'); // NOT OK
137+
138+
s = s.replace('[', ''); // OK
139+
s = s.replace(']', ''); // OK
140+
s.replace(/{/, '').replace(/}/, ''); // NOT OK: should have used a string literal if a single replacement was intended
141+
s.replace(']', '').replace('[', ''); // probably OK, but still flagged
142+
}
143+
129144
app.get('/some/path', function(req, res) {
130145
let untrusted = req.param("p");
131146

@@ -162,6 +177,7 @@ app.get('/some/path', function(req, res) {
162177
good10(untrusted);
163178
flowifyComments(untrusted);
164179
good11(untrusted);
180+
good12(untrusted);
165181
});
166182

167183
(function (s) {

‎python/ql/src/semmle/python/dataflow/DataFlow.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
import semmle.python.security.TaintTracking

‎python/ql/src/semmle/python/security/TaintTracking.qll‎

Lines changed: 7 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -326,8 +326,6 @@ abstract class Sanitizer extends string {
326326
private predicate valid_sanitizer(Sanitizer sanitizer) {
327327
not exists(TaintTracking::Configuration c)
328328
or
329-
exists(DataFlow::Configuration c | c.isSanitizer(sanitizer))
330-
or
331329
exists(TaintTracking::Configuration c | c.isSanitizer(sanitizer))
332330
}
333331

@@ -600,7 +598,7 @@ private newtype TTaintedNode =
600598
exists(DataFlow::Configuration config, TaintKind kind |
601599
taint = TaintFlowImplementation::TTrackedTaint(kind) and
602600
config.isSource(n) and context.getDepth() = 0 and
603-
kind instanceof GenericFlowType
601+
kind instanceof DataFlowType
604602
)
605603
or
606604
TaintFlowImplementation::step(_, taint, context, n) and
@@ -864,8 +862,6 @@ library module TaintFlowImplementation {
864862
(
865863
not exists(TaintTracking::Configuration c)
866864
or
867-
exists(DataFlow::Configuration c | c.isExtension(fromnodenode))
868-
or
869865
exists(TaintTracking::Configuration c | c.isExtension(fromnodenode))
870866
)
871867
|
@@ -1090,8 +1086,6 @@ library module TaintFlowImplementation {
10901086
(
10911087
not exists(TaintTracking::Configuration c)
10921088
or
1093-
exists(DataFlow::Configuration c | c.isExtension(originnode))
1094-
or
10951089
exists(TaintTracking::Configuration c | c.isExtension(originnode))
10961090
) and
10971091
originnode.getASuccessorVariable() = var and
@@ -1539,16 +1533,12 @@ class CallContext extends TCallContext {
15391533
*/
15401534
module DataFlow {
15411535

1542-
class FlowType = TaintKind;
1543-
15441536
/** Generic taint kind, source and sink classes for convenience and
15451537
* compatibility with other language libraries
15461538
*/
15471539

15481540
class Node = ControlFlowNode;
15491541

1550-
class PathNode = TaintedNode;
1551-
15521542
class Extension = DataFlowExtension::DataFlowNode;
15531543

15541544
abstract class Configuration extends string {
@@ -1560,19 +1550,14 @@ module DataFlow {
15601550

15611551
abstract predicate isSink(Node sink);
15621552

1563-
predicate isSanitizer(Sanitizer sanitizer) { none() }
1564-
1565-
predicate isExtension(Extension extension) { none() }
1566-
1567-
predicate hasFlowPath(PathNode source, PathNode sink) {
1553+
private predicate hasFlowPath(TaintedNode source, TaintedNode sink) {
15681554
this.isSource(source.getNode()) and
15691555
this.isSink(sink.getNode()) and
1570-
source.getTaintKind() instanceof GenericFlowType and
1571-
sink.getTaintKind() instanceof GenericFlowType
1556+
source.getASuccessor*() = sink
15721557
}
15731558

15741559
predicate hasFlow(Node source, Node sink) {
1575-
exists(PathNode psource, PathNode psink |
1560+
exists(TaintedNode psource, TaintedNode psink |
15761561
psource.getNode() = source and
15771562
psink.getNode() = sink and
15781563
this.isSource(source) and
@@ -1585,10 +1570,10 @@ module DataFlow {
15851570

15861571
}
15871572

1588-
private class GenericFlowType extends DataFlow::FlowType {
1573+
private class DataFlowType extends TaintKind {
15891574

1590-
GenericFlowType() {
1591-
this = "Generic taint kind" and
1575+
DataFlowType() {
1576+
this = "Data flow" and
15921577
exists(DataFlow::Configuration c)
15931578
}
15941579

‎python/ql/test/library-tests/taint/dataflow/Config.qll‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import python
2+
import semmle.python.dataflow.DataFlow
3+
4+
class TestConfiguration extends DataFlow::Configuration {
5+
6+
TestConfiguration() { this = "Test configuration" }
7+
8+
override predicate isSource(ControlFlowNode source) { source.(NameNode).getId() = "SOURCE" }
9+
10+
override predicate isSink(ControlFlowNode sink) {
11+
exists(CallNode call |
12+
call.getFunction().(NameNode).getId() = "SINK" and
13+
sink = call.getAnArg()
14+
)
15+
}
16+
17+
}

‎python/ql/test/library-tests/taint/dataflow/Dataflow.expected‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
| test.py:3:10:3:15 | ControlFlowNode for SOURCE | test.py:3:10:3:15 | ControlFlowNode for SOURCE |
2+
| test.py:6:9:6:14 | ControlFlowNode for SOURCE | test.py:7:10:7:10 | ControlFlowNode for s |
3+
| test.py:10:12:10:17 | ControlFlowNode for SOURCE | test.py:13:10:13:12 | ControlFlowNode for arg |
4+
| test.py:10:12:10:17 | ControlFlowNode for SOURCE | test.py:17:10:17:10 | ControlFlowNode for t |
5+
| test.py:20:9:20:14 | ControlFlowNode for SOURCE | test.py:13:10:13:12 | ControlFlowNode for arg |
6+
| test.py:37:13:37:18 | ControlFlowNode for SOURCE | test.py:41:14:41:14 | ControlFlowNode for t |
7+
| test.py:62:13:62:18 | ControlFlowNode for SOURCE | test.py:13:10:13:12 | ControlFlowNode for arg |
8+
| test.py:67:13:67:18 | ControlFlowNode for SOURCE | test.py:13:10:13:12 | ControlFlowNode for arg |
9+
| test.py:76:9:76:14 | ControlFlowNode for SOURCE | test.py:78:10:78:10 | ControlFlowNode for t |
10+
| test.py:108:13:108:18 | ControlFlowNode for SOURCE | test.py:112:14:112:14 | ControlFlowNode for t |
11+
| test.py:139:10:139:15 | ControlFlowNode for SOURCE | test.py:140:14:140:14 | ControlFlowNode for t |
12+
| test.py:143:9:143:14 | ControlFlowNode for SOURCE | test.py:145:10:145:10 | ControlFlowNode for s |
13+
| test.py:148:10:148:15 | ControlFlowNode for SOURCE | test.py:152:10:152:13 | ControlFlowNode for Subscript |
14+
| test.py:149:18:149:23 | ControlFlowNode for SOURCE | test.py:153:10:153:17 | ControlFlowNode for Subscript |
15+
| test.py:158:9:158:14 | ControlFlowNode for SOURCE | test.py:160:14:160:14 | ControlFlowNode for t |
16+
| test.py:158:9:158:14 | ControlFlowNode for SOURCE | test.py:166:14:166:14 | ControlFlowNode for t |

‎python/ql/test/library-tests/taint/dataflow/Dataflow.ql‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
2+
import python
3+
import Config
4+
5+
from TestConfiguration config, ControlFlowNode src, ControlFlowNode sink
6+
where config.hasFlow(src, sink)
7+
select src, sink

‎python/ql/test/library-tests/taint/dataflow/TestNode.expected‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
| Taint Data flow | test.py:3 | SOURCE | |
2+
| Taint Data flow | test.py:6 | SOURCE | |
3+
| Taint Data flow | test.py:7 | s | |
4+
| Taint Data flow | test.py:10 | SOURCE | |
5+
| Taint Data flow | test.py:12 | arg | test.py:21 |
6+
| Taint Data flow | test.py:12 | arg | test.py:25 |
7+
| Taint Data flow | test.py:12 | arg | test.py:47 from test.py:55 |
8+
| Taint Data flow | test.py:12 | arg | test.py:51 from test.py:63 |
9+
| Taint Data flow | test.py:12 | arg | test.py:51 from test.py:70 |
10+
| Taint Data flow | test.py:13 | arg | test.py:21 |
11+
| Taint Data flow | test.py:13 | arg | test.py:25 |
12+
| Taint Data flow | test.py:13 | arg | test.py:47 from test.py:55 |
13+
| Taint Data flow | test.py:13 | arg | test.py:51 from test.py:63 |
14+
| Taint Data flow | test.py:13 | arg | test.py:51 from test.py:70 |
15+
| Taint Data flow | test.py:16 | source() | |
16+
| Taint Data flow | test.py:17 | t | |
17+
| Taint Data flow | test.py:20 | SOURCE | |
18+
| Taint Data flow | test.py:21 | t | |
19+
| Taint Data flow | test.py:24 | source() | |
20+
| Taint Data flow | test.py:25 | t | |
21+
| Taint Data flow | test.py:31 | SOURCE | |
22+
| Taint Data flow | test.py:37 | SOURCE | |
23+
| Taint Data flow | test.py:41 | t | |
24+
| Taint Data flow | test.py:44 | source() | |
25+
| Taint Data flow | test.py:46 | arg | test.py:55 |
26+
| Taint Data flow | test.py:47 | arg | test.py:55 |
27+
| Taint Data flow | test.py:49 | arg | test.py:63 |
28+
| Taint Data flow | test.py:49 | arg | test.py:70 |
29+
| Taint Data flow | test.py:51 | arg | test.py:63 |
30+
| Taint Data flow | test.py:51 | arg | test.py:70 |
31+
| Taint Data flow | test.py:54 | source2() | |
32+
| Taint Data flow | test.py:55 | t | |
33+
| Taint Data flow | test.py:62 | SOURCE | |
34+
| Taint Data flow | test.py:63 | t | |
35+
| Taint Data flow | test.py:67 | SOURCE | |
36+
| Taint Data flow | test.py:70 | t | |
37+
| Taint Data flow | test.py:72 | arg | test.py:77 |
38+
| Taint Data flow | test.py:73 | arg | test.py:77 |
39+
| Taint Data flow | test.py:76 | SOURCE | |
40+
| Taint Data flow | test.py:77 | hub() | |
41+
| Taint Data flow | test.py:77 | t | |
42+
| Taint Data flow | test.py:78 | t | |
43+
| Taint Data flow | test.py:108 | SOURCE | |
44+
| Taint Data flow | test.py:112 | t | |
45+
| Taint Data flow | test.py:118 | SOURCE | |
46+
| Taint Data flow | test.py:120 | t | |
47+
| Taint Data flow | test.py:128 | SOURCE | |
48+
| Taint Data flow | test.py:129 | t | |
49+
| Taint Data flow | test.py:139 | SOURCE | |
50+
| Taint Data flow | test.py:140 | t | |
51+
| Taint Data flow | test.py:143 | SOURCE | |
52+
| Taint Data flow | test.py:144 | s | |
53+
| Taint Data flow | test.py:145 | s | |
54+
| Taint Data flow | test.py:148 | SOURCE | |
55+
| Taint Data flow | test.py:149 | SOURCE | |
56+
| Taint Data flow | test.py:152 | Subscript | |
57+
| Taint Data flow | test.py:153 | Subscript | |
58+
| Taint Data flow | test.py:158 | SOURCE | |
59+
| Taint Data flow | test.py:159 | t | |
60+
| Taint Data flow | test.py:160 | t | |
61+
| Taint Data flow | test.py:163 | t | |
62+
| Taint Data flow | test.py:166 | t | |
63+
| Taint [Data flow] | test.py:148 | List | |
64+
| Taint [Data flow] | test.py:150 | l | |
65+
| Taint [Data flow] | test.py:152 | x | |
66+
| Taint [Data flow] | test.py:154 | l | |
67+
| Taint [Data flow] | test.py:154 | list() | |
68+
| Taint {Data flow} | test.py:149 | Dict | |
69+
| Taint {Data flow} | test.py:151 | d | |
70+
| Taint {Data flow} | test.py:153 | y | |
71+
| Taint {Data flow} | test.py:155 | d | |
72+
| Taint {Data flow} | test.py:155 | dict() | |

‎python/ql/test/library-tests/taint/dataflow/TestNode.ql‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import python
2+
import Config
3+
4+
from TaintedNode n
5+
select n.getTrackedValue(), n.getLocation().toString(), n.getNode().getNode().toString(), n.getContext()

0 commit comments

Comments
 (0)