Skip to content

Commit c70997f

Browse files
esbenaesbena
committed
JS: address review comments for js/unsafe-jquery-plugin
1 parent eaff78b commit c70997f

File tree

5 files changed

+48
-11
lines changed

5 files changed

+48
-11
lines changed

‎javascript/ql/src/Security/CWE-079/UnsafeJQueryPlugin.qhelp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,17 @@
1818
Otherwise, the plugin may write user input (for example, a URL query
1919
parameter) to a web page without properly sanitizing the input first,
2020
which allows for a cross-site scripting vulnerability in the client
21-
application.
21+
application through dynamic HTML construction.
2222

2323
</p>
2424
</overview>
2525

2626
<recommendation>
2727
<p>
2828

29-
Document all options that can lead to cross-site scripting attacks.
29+
Document all options that can lead to cross-site scripting
30+
attacks, and guard against unsafe inputs where dynamic HTML
31+
construction is not intended.
3032

3133
</p>
3234
</recommendation>

‎javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPlugin.qll‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ module UnsafeJQueryPlugin {
3636
// prefixing prevents forced html/css confusion:
3737

3838
// prefixing through concatenation:
39-
succ.asExpr().(AddExpr).getRightOperand().flow() = pred
39+
StringConcatenation::getFirstOperand(succ) != pred
4040
or
4141
// prefixing through a poor-mans templating system:
4242
exists(DataFlow::MethodCallNode replace |
@@ -49,7 +49,7 @@ module UnsafeJQueryPlugin {
4949
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode node) {
5050
super.isSanitizerGuard(node) or
5151
node instanceof IsElementSanitizer or
52-
node instanceof PropertyPrecenseSanitizer
52+
node instanceof PropertyPresenceSanitizer
5353
}
5454
}
5555
}

‎javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,9 @@ module UnsafeJQueryPlugin {
7373
}
7474

7575
/**
76-
* Gets an function that is registered as a jQuery plugin method at `def`.
76+
* Gets a node that is registered as a jQuery plugin method at `def`.
7777
*/
78-
private DataFlow::FunctionNode getAJQueryPluginMethod(
78+
private DataFlow::SourceNode getAJQueryPluginMethod(
7979
DataFlow::TypeBackTracker t, DataFlow::Node def
8080
) {
8181
t.start() and
@@ -85,6 +85,13 @@ module UnsafeJQueryPlugin {
8585
exists(DataFlow::TypeBackTracker t2 | result = getAJQueryPluginMethod(t2, def).backtrack(t2, t))
8686
}
8787

88+
/**
89+
* Gets a function that is registered as a jQuery plugin method at `def`.
90+
*/
91+
private DataFlow::FunctionNode getAJQueryPluginMethod(DataFlow::Node def) {
92+
result = getAJQueryPluginMethod(DataFlow::TypeBackTracker::end(), def)
93+
}
94+
8895
/**
8996
* Gets an operand to `extend`.
9097
*/
@@ -95,6 +102,13 @@ module UnsafeJQueryPlugin {
95102
exists(DataFlow::TypeBackTracker t2 | result = getAnExtendOperand(t2, extend).backtrack(t2, t))
96103
}
97104

105+
/**
106+
* Gets an operand to `extend`.
107+
*/
108+
private DataFlow::SourceNode getAnExtendOperand(ExtendCall extend) {
109+
result = getAnExtendOperand(DataFlow::TypeBackTracker::end(), extend)
110+
}
111+
98112
/**
99113
* A function that is registered as a jQuery plugin method.
100114
*/
@@ -104,7 +118,7 @@ module UnsafeJQueryPlugin {
104118
JQueryPluginMethod() {
105119
exists(DataFlow::Node def |
106120
jQueryPluginDefinition(pluginName, def) and
107-
this = getAJQueryPluginMethod(DataFlow::TypeBackTracker::end(), def)
121+
this = getAJQueryPluginMethod(def)
108122
)
109123
}
110124

@@ -120,8 +134,8 @@ module UnsafeJQueryPlugin {
120134
private predicate hasDefaultOption(JQueryPluginMethod plugin, DataFlow::PropWrite def) {
121135
exists(ExtendCall extend, JQueryPluginOptions options, DataFlow::SourceNode default |
122136
options.getPlugin() = plugin and
123-
options = getAnExtendOperand(DataFlow::TypeBackTracker::end(), extend) and
124-
default = getAnExtendOperand(DataFlow::TypeBackTracker::end(), extend) and
137+
options = getAnExtendOperand(extend) and
138+
default = getAnExtendOperand(extend) and
125139
default.getAPropertyWrite() = def
126140
)
127141
}
@@ -173,11 +187,11 @@ module UnsafeJQueryPlugin {
173187
/**
174188
* Expression like `typeof x.<?> !== "undefined"` or `x.<?>`, which sanitizes `x`, as it is unlikely to be a string afterwards.
175189
*/
176-
class PropertyPrecenseSanitizer extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
190+
class PropertyPresenceSanitizer extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
177191
DataFlow::Node input;
178192
boolean polarity;
179193

180-
PropertyPrecenseSanitizer() {
194+
PropertyPresenceSanitizer() {
181195
exists(DataFlow::PropRead read, string name |
182196
not name = "length" and read.accesses(input, name)
183197
|

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,11 @@ nodes
115115
| unsafe-jquery-plugin.js:165:16:165:29 | options.target |
116116
| unsafe-jquery-plugin.js:170:6:170:11 | target |
117117
| unsafe-jquery-plugin.js:170:6:170:11 | target |
118+
| unsafe-jquery-plugin.js:178:27:178:33 | options |
119+
| unsafe-jquery-plugin.js:178:27:178:33 | options |
120+
| unsafe-jquery-plugin.js:179:5:179:11 | options |
121+
| unsafe-jquery-plugin.js:179:5:179:18 | options.target |
122+
| unsafe-jquery-plugin.js:179:5:179:18 | options.target |
118123
edges
119124
| unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:3:5:3:11 | options |
120125
| unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:3:5:3:11 | options |
@@ -228,6 +233,10 @@ edges
228233
| unsafe-jquery-plugin.js:165:7:165:29 | target | unsafe-jquery-plugin.js:170:6:170:11 | target |
229234
| unsafe-jquery-plugin.js:165:16:165:22 | options | unsafe-jquery-plugin.js:165:16:165:29 | options.target |
230235
| unsafe-jquery-plugin.js:165:16:165:29 | options.target | unsafe-jquery-plugin.js:165:7:165:29 | target |
236+
| unsafe-jquery-plugin.js:178:27:178:33 | options | unsafe-jquery-plugin.js:179:5:179:11 | options |
237+
| unsafe-jquery-plugin.js:178:27:178:33 | options | unsafe-jquery-plugin.js:179:5:179:11 | options |
238+
| unsafe-jquery-plugin.js:179:5:179:11 | options | unsafe-jquery-plugin.js:179:5:179:18 | options.target |
239+
| unsafe-jquery-plugin.js:179:5:179:11 | options | unsafe-jquery-plugin.js:179:5:179:18 | options.target |
231240
#select
232241
| unsafe-jquery-plugin.js:3:5:3:11 | options | unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:3:5:3:11 | options | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:2:19:63:2 | functio ... \\t\\t}\\n\\n\\t} | '$.fn.my_plugin' plugin |
233242
| unsafe-jquery-plugin.js:5:5:5:18 | options.target | unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:5:5:5:18 | options.target | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:2:19:63:2 | functio ... \\t\\t}\\n\\n\\t} | '$.fn.my_plugin' plugin |
@@ -251,3 +260,4 @@ edges
251260
| unsafe-jquery-plugin.js:156:41:156:54 | options.target | unsafe-jquery-plugin.js:153:38:153:44 | options | unsafe-jquery-plugin.js:156:41:156:54 | options.target | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:153:19:158:2 | functio ... gged\\n\\t} | '$.fn.my_plugin' plugin |
252261
| unsafe-jquery-plugin.js:157:44:157:59 | options.target.a | unsafe-jquery-plugin.js:153:38:153:44 | options | unsafe-jquery-plugin.js:157:44:157:59 | options.target.a | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:153:19:158:2 | functio ... gged\\n\\t} | '$.fn.my_plugin' plugin |
253262
| unsafe-jquery-plugin.js:170:6:170:11 | target | unsafe-jquery-plugin.js:160:38:160:44 | options | unsafe-jquery-plugin.js:170:6:170:11 | target | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:160:19:173:2 | functio ... \\t\\t}\\n\\n\\t} | '$.fn.my_plugin' plugin |
263+
| unsafe-jquery-plugin.js:179:5:179:18 | options.target | unsafe-jquery-plugin.js:178:27:178:33 | options | unsafe-jquery-plugin.js:179:5:179:18 | options.target | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:178:18:180:2 | functio ... T OK\\n\\t} | '$.fn.my_plugin' plugin |

‎javascript/ql/test/query-tests/Security/CWE-079/unsafe-jquery-plugin.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,4 +171,15 @@
171171
}
172172

173173
}
174+
175+
function setupPlugin(o) {
176+
$.fn.my_plugin = o.f
177+
}
178+
setupPlugin({f: function(options) {
179+
$(options.target); // NOT OK
180+
}});
181+
setupPlugin({f:function(options) {
182+
$(document).find(options.target); // OK
183+
}});
184+
174185
});

0 commit comments

Comments
 (0)