Skip to content

Commit 51ddb55

Browse files
erik-krogherik-krogh
committed
use tainted-object to precisely model that plain object are fine, but their properties are not
1 parent 09794fa commit 51ddb55

File tree

4 files changed

+133
-17
lines changed

4 files changed

+133
-17
lines changed

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -182,16 +182,17 @@ module UnsafeHtmlConstruction {
182182
}
183183

184184
/** A test for the value of `typeof x`, restricting the potential types of `x`. */
185-
class TypeTestGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
185+
class TypeTestGuard extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode {
186186
override EqualityTest astNode;
187187
Expr operand;
188188
boolean polarity;
189189

190190
TypeTestGuard() { TaintTracking::isStringTypeGuard(astNode, operand, polarity) }
191191

192-
override predicate sanitizes(boolean outcome, Expr e) {
192+
override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
193193
polarity = outcome and
194-
e = operand
194+
e = operand and
195+
lbl.isTaint()
195196
}
196197
}
197198
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,23 @@ import javascript
77
private import semmle.javascript.security.dataflow.DomBasedXssCustomizations::DomBasedXss as DomBasedXss
88
private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQueryPlugin
99
import UnsafeHtmlConstructionCustomizations::UnsafeHtmlConstruction
10+
import semmle.javascript.security.TaintedObject
1011

1112
/**
1213
* A taint-tracking configuration for reasoning about unsafe HTML constructed from library input vulnerabilities.
1314
*/
1415
class Configration extends TaintTracking::Configuration {
1516
Configration() { this = "UnsafeHtmlConstruction" }
1617

17-
override predicate isSource(DataFlow::Node source) { source instanceof Source }
18+
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
19+
source instanceof Source and
20+
label = [TaintedObject::label(), DataFlow::FlowLabel::taint(), DataFlow::FlowLabel::data()]
21+
}
1822

19-
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
23+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
24+
sink instanceof Sink and
25+
label = DataFlow::FlowLabel::taint()
26+
}
2027

2128
override predicate isSanitizer(DataFlow::Node node) {
2229
super.isSanitizer(node)
@@ -36,8 +43,19 @@ class Configration extends TaintTracking::Configuration {
3643
DataFlow::hasPathWithoutUnmatchedReturn(source, sink)
3744
}
3845

39-
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
40-
DataFlow::localFieldStep(pred, succ)
46+
override predicate isAdditionalFlowStep(
47+
DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
48+
) {
49+
DataFlow::localFieldStep(pred, succ) and
50+
inlbl.isTaint() and
51+
outlbl.isTaint()
52+
or
53+
TaintedObject::step(pred, succ, inlbl, outlbl)
54+
or
55+
// property read from a tainted object is considered tainted
56+
succ.(DataFlow::PropRead).getBase() = pred and
57+
inlbl = TaintedObject::label() and
58+
outlbl = DataFlow::FlowLabel::taint()
4159
}
4260

4361
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {

‎javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected

Lines changed: 87 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,33 @@ nodes
33
| jquery-plugin.js:11:27:11:31 | stuff |
44
| jquery-plugin.js:11:34:11:40 | options |
55
| jquery-plugin.js:11:34:11:40 | options |
6+
| jquery-plugin.js:11:34:11:40 | options |
7+
| jquery-plugin.js:11:34:11:40 | options |
8+
| jquery-plugin.js:12:31:12:37 | options |
9+
| jquery-plugin.js:12:31:12:37 | options |
610
| jquery-plugin.js:12:31:12:37 | options |
711
| jquery-plugin.js:12:31:12:41 | options.foo |
812
| jquery-plugin.js:12:31:12:41 | options.foo |
13+
| jquery-plugin.js:12:31:12:41 | options.foo |
14+
| jquery-plugin.js:12:31:12:41 | options.foo |
915
| jquery-plugin.js:14:31:14:35 | stuff |
1016
| jquery-plugin.js:14:31:14:35 | stuff |
1117
| lib2/index.ts:1:28:1:28 | s |
1218
| lib2/index.ts:1:28:1:28 | s |
13-
| lib2/index.ts:2:29:2:29 | s |
14-
| lib2/index.ts:2:29:2:29 | s |
19+
| lib2/index.ts:2:27:2:27 | s |
20+
| lib2/index.ts:2:27:2:27 | s |
21+
| lib2/index.ts:6:29:6:36 | settings |
22+
| lib2/index.ts:6:29:6:36 | settings |
23+
| lib2/index.ts:6:29:6:36 | settings |
24+
| lib2/index.ts:7:58:7:65 | settings |
25+
| lib2/index.ts:7:58:7:65 | settings |
26+
| lib2/index.ts:13:9:13:41 | name |
27+
| lib2/index.ts:13:16:13:23 | settings |
28+
| lib2/index.ts:13:16:13:33 | settings.mySetting |
29+
| lib2/index.ts:13:16:13:36 | setting ... ting[i] |
30+
| lib2/index.ts:13:16:13:41 | setting ... i].name |
31+
| lib2/index.ts:18:62:18:65 | name |
32+
| lib2/index.ts:18:62:18:65 | name |
1533
| lib/src/MyNode.ts:1:28:1:28 | s |
1634
| lib/src/MyNode.ts:1:28:1:28 | s |
1735
| lib/src/MyNode.ts:2:29:2:29 | s |
@@ -41,13 +59,31 @@ nodes
4159
| main.js:53:20:53:20 | s |
4260
| main.js:56:28:56:34 | options |
4361
| main.js:56:28:56:34 | options |
62+
| main.js:56:28:56:34 | options |
63+
| main.js:56:28:56:34 | options |
64+
| main.js:57:11:59:5 | defaults |
4465
| main.js:57:11:59:5 | defaults |
66+
| main.js:57:11:59:5 | defaults |
67+
| main.js:57:22:59:5 | {\\n ... "\\n } |
4568
| main.js:57:22:59:5 | {\\n ... "\\n } |
69+
| main.js:57:22:59:5 | {\\n ... "\\n } |
70+
| main.js:60:11:60:48 | settings |
71+
| main.js:60:11:60:48 | settings |
4672
| main.js:60:11:60:48 | settings |
4773
| main.js:60:22:60:48 | $.exten ... ptions) |
74+
| main.js:60:22:60:48 | $.exten ... ptions) |
75+
| main.js:60:22:60:48 | $.exten ... ptions) |
76+
| main.js:60:31:60:38 | defaults |
77+
| main.js:60:31:60:38 | defaults |
4878
| main.js:60:31:60:38 | defaults |
4979
| main.js:60:41:60:47 | options |
80+
| main.js:60:41:60:47 | options |
81+
| main.js:60:41:60:47 | options |
82+
| main.js:62:19:62:26 | settings |
5083
| main.js:62:19:62:26 | settings |
84+
| main.js:62:19:62:26 | settings |
85+
| main.js:62:19:62:31 | settings.name |
86+
| main.js:62:19:62:31 | settings.name |
5187
| main.js:62:19:62:31 | settings.name |
5288
| main.js:62:19:62:31 | settings.name |
5389
| main.js:66:35:66:41 | attrVal |
@@ -102,12 +138,32 @@ edges
102138
| jquery-plugin.js:11:27:11:31 | stuff | jquery-plugin.js:14:31:14:35 | stuff |
103139
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
104140
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
141+
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
142+
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
143+
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
144+
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
145+
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
146+
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
147+
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
105148
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
106149
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
107-
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s |
108-
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s |
109-
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s |
110-
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s |
150+
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
151+
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s |
152+
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s |
153+
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s |
154+
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s |
155+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings |
156+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings |
157+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings |
158+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings |
159+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:13:16:13:23 | settings |
160+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:13:16:13:23 | settings |
161+
| lib2/index.ts:13:9:13:41 | name | lib2/index.ts:18:62:18:65 | name |
162+
| lib2/index.ts:13:9:13:41 | name | lib2/index.ts:18:62:18:65 | name |
163+
| lib2/index.ts:13:16:13:23 | settings | lib2/index.ts:13:16:13:33 | settings.mySetting |
164+
| lib2/index.ts:13:16:13:33 | settings.mySetting | lib2/index.ts:13:16:13:36 | setting ... ting[i] |
165+
| lib2/index.ts:13:16:13:36 | setting ... ting[i] | lib2/index.ts:13:16:13:41 | setting ... i].name |
166+
| lib2/index.ts:13:16:13:41 | setting ... i].name | lib2/index.ts:13:9:13:41 | name |
111167
| lib/src/MyNode.ts:1:28:1:28 | s | lib/src/MyNode.ts:2:29:2:29 | s |
112168
| lib/src/MyNode.ts:1:28:1:28 | s | lib/src/MyNode.ts:2:29:2:29 | s |
113169
| lib/src/MyNode.ts:1:28:1:28 | s | lib/src/MyNode.ts:2:29:2:29 | s |
@@ -136,13 +192,35 @@ edges
136192
| main.js:53:20:53:20 | s | main.js:41:17:41:17 | s |
137193
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
138194
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
195+
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
196+
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
197+
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
198+
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
199+
| main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults |
200+
| main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults |
139201
| main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults |
140202
| main.js:57:22:59:5 | {\\n ... "\\n } | main.js:57:11:59:5 | defaults |
203+
| main.js:57:22:59:5 | {\\n ... "\\n } | main.js:57:11:59:5 | defaults |
204+
| main.js:57:22:59:5 | {\\n ... "\\n } | main.js:57:11:59:5 | defaults |
205+
| main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings |
141206
| main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings |
207+
| main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings |
208+
| main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings |
142209
| main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings |
210+
| main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings |
211+
| main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) |
143212
| main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) |
213+
| main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) |
214+
| main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n ... "\\n } |
215+
| main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n ... "\\n } |
144216
| main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n ... "\\n } |
145217
| main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) |
218+
| main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) |
219+
| main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) |
220+
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
221+
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
222+
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
223+
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
146224
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
147225
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
148226
| main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal |
@@ -199,7 +277,9 @@ edges
199277
#select
200278
| jquery-plugin.js:12:31:12:41 | options.foo | jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:41 | options.foo | This HTML construction which depends on $@ might later allow $@. | jquery-plugin.js:11:34:11:40 | options | library input | jquery-plugin.js:12:20:12:53 | "<span> ... /span>" | cross-site scripting |
201279
| jquery-plugin.js:14:31:14:35 | stuff | jquery-plugin.js:11:27:11:31 | stuff | jquery-plugin.js:14:31:14:35 | stuff | This HTML construction which depends on $@ might later allow $@. | jquery-plugin.js:11:27:11:31 | stuff | library input | jquery-plugin.js:14:20:14:47 | "<span> ... /span>" | cross-site scripting |
202-
| lib2/index.ts:2:29:2:29 | s | lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s | This HTML construction which depends on $@ might later allow $@. | lib2/index.ts:1:28:1:28 | s | library input | lib2/index.ts:3:49:3:52 | html | cross-site scripting |
280+
| lib2/index.ts:2:27:2:27 | s | lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s | This HTML construction which depends on $@ might later allow $@. | lib2/index.ts:1:28:1:28 | s | library input | lib2/index.ts:3:47:3:50 | html | cross-site scripting |
281+
| lib2/index.ts:7:58:7:65 | settings | lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings | This HTML construction which depends on $@ might later allow $@. | lib2/index.ts:6:29:6:36 | settings | library input | lib2/index.ts:7:47:7:77 | "<span> ... /span>" | cross-site scripting |
282+
| lib2/index.ts:18:62:18:65 | name | lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:18:62:18:65 | name | This HTML construction which depends on $@ might later allow $@. | lib2/index.ts:6:29:6:36 | settings | library input | lib2/index.ts:18:51:18:77 | "<span> ... /span>" | cross-site scripting |
203283
| lib/src/MyNode.ts:2:29:2:29 | s | lib/src/MyNode.ts:1:28:1:28 | s | lib/src/MyNode.ts:2:29:2:29 | s | This HTML construction which depends on $@ might later allow $@. | lib/src/MyNode.ts:1:28:1:28 | s | library input | lib/src/MyNode.ts:3:49:3:52 | html | cross-site scripting |
204284
| main.js:2:29:2:29 | s | main.js:1:55:1:55 | s | main.js:2:29:2:29 | s | This HTML construction which depends on $@ might later allow $@. | main.js:1:55:1:55 | s | library input | main.js:3:49:3:52 | html | cross-site scripting |
205285
| main.js:7:49:7:49 | s | main.js:6:49:6:49 | s | main.js:7:49:7:49 | s | This XML parsing which depends on $@ might later allow $@. | main.js:6:49:6:49 | s | library input | main.js:8:48:8:66 | doc.documentElement | cross-site scripting |

‎javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/lib2/index.ts

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,21 @@
11
export function trivialXss(s: string) {
2-
const html = "<span>" + s + "</span>"; // NOT OK - this file is recognized as a main file.
3-
document.querySelector("#html").innerHTML = html;
4-
}
2+
const html = "<span>" + s + "</span>"; // NOT OK - this file is recognized as a main file.
3+
document.querySelector("#html").innerHTML = html;
4+
}
5+
6+
export function objectStuff(settings: any, i: number) {
7+
document.querySelector("#html").innerHTML = "<span>" + settings + "</span>"; // NOT OK
8+
var name;
9+
10+
if (settings.mySetting && settings.mySetting.length !== 0) {
11+
for (i = 0; i < settings.mySetting.length; ++i) {
12+
if (typeof settings.mySetting[i] === "object") {
13+
name = settings.mySetting[i].name; // `settings.mySetting[i]` is correctly sanitized, as it is an object. However, the `name` property is stil tainted.
14+
} else {
15+
name = "";
16+
}
17+
18+
document.querySelector("#html").innerHTML = "<span>" + name + "</span>"; // NOT OK
19+
}
20+
}
21+
}

0 commit comments

Comments
 (0)