Skip to content

Commit 50aa5e0

Browse files
erik-krogherik-krogh
authored
Merge pull request github#12177 from erik-krogh/alias-html
JS: More precise type-test sanitizer guards in unsafe-html-construction
2 parents 7f607fb + 0e60fc5 commit 50aa5e0

File tree

4 files changed

+133
-27
lines changed

4 files changed

+133
-27
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 & 14 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,12 +43,22 @@ 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) {
44-
guard instanceof PrefixStringSanitizer or
4562
guard instanceof QuoteGuard or
4663
guard instanceof ContainsHtmlGuard or
4764
guard instanceof TypeTestGuard
@@ -50,15 +67,6 @@ class Configration extends TaintTracking::Configuration {
5067

5168
private import semmle.javascript.security.dataflow.Xss::Shared as Shared
5269

53-
private class PrefixStringSanitizer extends TaintTracking::SanitizerGuardNode,
54-
DomBasedXss::PrefixStringSanitizer {
55-
PrefixStringSanitizer() { this = this }
56-
}
57-
58-
private class PrefixString extends DataFlow::FlowLabel, DomBasedXss::PrefixString {
59-
PrefixString() { this = this }
60-
}
61-
6270
private class QuoteGuard extends TaintTracking::SanitizerGuardNode, Shared::QuoteGuard {
6371
QuoteGuard() { this = this }
6472
}

‎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
| lib2/src/MyNode.ts:1:28:1:28 | s |
1634
| lib2/src/MyNode.ts:1:28:1:28 | s |
1735
| lib2/src/MyNode.ts:2:29:2:29 | s |
@@ -45,13 +63,31 @@ nodes
4563
| main.js:53:20:53:20 | s |
4664
| main.js:56:28:56:34 | options |
4765
| main.js:56:28:56:34 | options |
66+
| main.js:56:28:56:34 | options |
67+
| main.js:56:28:56:34 | options |
68+
| main.js:57:11:59:5 | defaults |
4869
| main.js:57:11:59:5 | defaults |
70+
| main.js:57:11:59:5 | defaults |
71+
| main.js:57:22:59:5 | {\\n ... "\\n } |
4972
| main.js:57:22:59:5 | {\\n ... "\\n } |
73+
| main.js:57:22:59:5 | {\\n ... "\\n } |
74+
| main.js:60:11:60:48 | settings |
75+
| main.js:60:11:60:48 | settings |
5076
| main.js:60:11:60:48 | settings |
5177
| main.js:60:22:60:48 | $.exten ... ptions) |
78+
| main.js:60:22:60:48 | $.exten ... ptions) |
79+
| main.js:60:22:60:48 | $.exten ... ptions) |
80+
| main.js:60:31:60:38 | defaults |
81+
| main.js:60:31:60:38 | defaults |
5282
| main.js:60:31:60:38 | defaults |
5383
| main.js:60:41:60:47 | options |
84+
| main.js:60:41:60:47 | options |
85+
| main.js:60:41:60:47 | options |
86+
| main.js:62:19:62:26 | settings |
5487
| main.js:62:19:62:26 | settings |
88+
| main.js:62:19:62:26 | settings |
89+
| main.js:62:19:62:31 | settings.name |
90+
| main.js:62:19:62:31 | settings.name |
5591
| main.js:62:19:62:31 | settings.name |
5692
| main.js:62:19:62:31 | settings.name |
5793
| main.js:66:35:66:41 | attrVal |
@@ -106,12 +142,32 @@ edges
106142
| jquery-plugin.js:11:27:11:31 | stuff | jquery-plugin.js:14:31:14:35 | stuff |
107143
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
108144
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
145+
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
146+
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
147+
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
148+
| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options |
149+
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
150+
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
151+
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
109152
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
110153
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
111-
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s |
112-
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s |
113-
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s |
114-
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s |
154+
| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo |
155+
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s |
156+
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s |
157+
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s |
158+
| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s |
159+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings |
160+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings |
161+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings |
162+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings |
163+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:13:16:13:23 | settings |
164+
| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:13:16:13:23 | settings |
165+
| lib2/index.ts:13:9:13:41 | name | lib2/index.ts:18:62:18:65 | name |
166+
| lib2/index.ts:13:9:13:41 | name | lib2/index.ts:18:62:18:65 | name |
167+
| lib2/index.ts:13:16:13:23 | settings | lib2/index.ts:13:16:13:33 | settings.mySetting |
168+
| lib2/index.ts:13:16:13:33 | settings.mySetting | lib2/index.ts:13:16:13:36 | setting ... ting[i] |
169+
| lib2/index.ts:13:16:13:36 | setting ... ting[i] | lib2/index.ts:13:16:13:41 | setting ... i].name |
170+
| lib2/index.ts:13:16:13:41 | setting ... i].name | lib2/index.ts:13:9:13:41 | name |
115171
| lib2/src/MyNode.ts:1:28:1:28 | s | lib2/src/MyNode.ts:2:29:2:29 | s |
116172
| lib2/src/MyNode.ts:1:28:1:28 | s | lib2/src/MyNode.ts:2:29:2:29 | s |
117173
| lib2/src/MyNode.ts:1:28:1:28 | s | lib2/src/MyNode.ts:2:29:2:29 | s |
@@ -144,13 +200,35 @@ edges
144200
| main.js:53:20:53:20 | s | main.js:41:17:41:17 | s |
145201
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
146202
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
203+
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
204+
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
205+
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
206+
| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
207+
| main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults |
208+
| main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults |
147209
| main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults |
148210
| main.js:57:22:59:5 | {\\n ... "\\n } | main.js:57:11:59:5 | defaults |
211+
| main.js:57:22:59:5 | {\\n ... "\\n } | main.js:57:11:59:5 | defaults |
212+
| main.js:57:22:59:5 | {\\n ... "\\n } | main.js:57:11:59:5 | defaults |
213+
| main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings |
149214
| main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings |
215+
| main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings |
216+
| main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings |
150217
| main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings |
218+
| main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings |
219+
| main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) |
151220
| main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) |
221+
| main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) |
222+
| main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n ... "\\n } |
223+
| main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n ... "\\n } |
152224
| main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n ... "\\n } |
153225
| main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) |
226+
| main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) |
227+
| main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) |
228+
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
229+
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
230+
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
231+
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
154232
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
155233
| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
156234
| main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal |
@@ -207,7 +285,9 @@ edges
207285
#select
208286
| 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 |
209287
| 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 |
210-
| 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 |
288+
| 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 |
289+
| 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 |
290+
| 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 |
211291
| lib2/src/MyNode.ts:2:29:2:29 | s | lib2/src/MyNode.ts:1:28:1:28 | s | lib2/src/MyNode.ts:2:29:2:29 | s | This HTML construction which depends on $@ might later allow $@. | lib2/src/MyNode.ts:1:28:1:28 | s | library input | lib2/src/MyNode.ts:3:49:3:52 | html | cross-site scripting |
212292
| 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 |
213293
| 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 |

‎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)