Skip to content

Commit cbe417f

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3336 from erik-krogh/MoarJQuery
Approved by esbena
2 parents 4eea62c + 19c6092 commit cbe417f

File tree

11 files changed

+162
-69
lines changed

11 files changed

+162
-69
lines changed

‎change-notes/1.25/analysis-javascript.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
* Support for the following frameworks and libraries has been improved:
66
- [jGrowl](https://github.com/stanlemon/jGrowl)
7+
- [jQuery](https://jquery.com/)
78

89
## New queries
910

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ import semmle.javascript.security.dataflow.UnsafeJQueryPlugin::UnsafeJQueryPlugi
1616
import DataFlow::PathGraph
1717

1818
from
19-
Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, JQueryPluginMethod plugin
19+
Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink,
20+
JQuery::JQueryPluginMethod plugin
2021
where
2122
cfg.hasFlowPath(source, sink) and
2223
source.getNode().(Source).getPlugin() = plugin and

‎javascript/ql/src/semmle/javascript/DOM.qll

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,6 +305,13 @@ module DOM {
305305
call.getNumArgument() = 1 and
306306
forex(InferredType t | t = call.getArgument(0).analyze().getAType() | t = TTNumber())
307307
)
308+
or
309+
// A `this` node from a callback given to a `$().each(callback)` call.
310+
// purposely not using JQuery::MethodCall to avoid `jquery.each()`.
311+
exists(DataFlow::CallNode eachCall | eachCall = JQuery::objectRef().getAMethodCall("each") |
312+
this = DataFlow::thisNode(eachCall.getCallback(0).getFunction()) or
313+
this = eachCall.getABoundCallbackParameter(0, 1)
314+
)
308315
}
309316
}
310317
}

‎javascript/ql/src/semmle/javascript/frameworks/jQuery.qll

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -496,6 +496,15 @@ module JQuery {
496496
hasUnderlyingType("jQuery")
497497
}
498498
}
499+
500+
/**
501+
* A `this` node in a JQuery plugin function, which is a JQuery object.
502+
*/
503+
private class JQueryPluginThisObject extends Range {
504+
JQueryPluginThisObject() {
505+
this = DataFlow::thisNode(any(JQueryPluginMethod method).getFunction())
506+
}
507+
}
499508
}
500509

501510
/** A source of jQuery objects from the AST-based `JQueryObject` class. */
@@ -590,4 +599,64 @@ module JQuery {
590599
node = getArgument(0)
591600
}
592601
}
602+
603+
/**
604+
* Holds for jQuery plugin definitions of the form `$.fn.<pluginName> = <plugin>` or `$.extend($.fn, {<pluginName>, <plugin>})`.
605+
*/
606+
private predicate jQueryPluginDefinition(string pluginName, DataFlow::Node plugin) {
607+
exists(DataFlow::PropRead fn, DataFlow::PropWrite write |
608+
fn = jquery().getAPropertyRead("fn") and
609+
(
610+
write = fn.getAPropertyWrite()
611+
or
612+
exists(ExtendCall extend, DataFlow::SourceNode source |
613+
fn.flowsTo(extend.getDestinationOperand()) and
614+
source = extend.getASourceOperand() and
615+
write = source.getAPropertyWrite()
616+
)
617+
) and
618+
plugin = write.getRhs() and
619+
(
620+
pluginName = write.getPropertyName() or
621+
write.getPropertyNameExpr().flow().mayHaveStringValue(pluginName)
622+
)
623+
)
624+
}
625+
626+
/**
627+
* Gets a node that is registered as a jQuery plugin method at `def`.
628+
*/
629+
private DataFlow::SourceNode getAJQueryPluginMethod(
630+
DataFlow::TypeBackTracker t, DataFlow::Node def
631+
) {
632+
t.start() and jQueryPluginDefinition(_, def) and result.flowsTo(def)
633+
or
634+
exists(DataFlow::TypeBackTracker t2 | result = getAJQueryPluginMethod(t2, def).backtrack(t2, t))
635+
}
636+
637+
/**
638+
* Gets a function that is registered as a jQuery plugin method at `def`.
639+
*/
640+
private DataFlow::FunctionNode getAJQueryPluginMethod(DataFlow::Node def) {
641+
result = getAJQueryPluginMethod(DataFlow::TypeBackTracker::end(), def)
642+
}
643+
644+
/**
645+
* A function that is registered as a jQuery plugin method.
646+
*/
647+
class JQueryPluginMethod extends DataFlow::FunctionNode {
648+
string pluginName;
649+
650+
JQueryPluginMethod() {
651+
exists(DataFlow::Node def |
652+
jQueryPluginDefinition(pluginName, def) and
653+
this = getAJQueryPluginMethod(def)
654+
)
655+
}
656+
657+
/**
658+
* Gets the name of this plugin.
659+
*/
660+
string getPluginName() { result = pluginName }
661+
}
593662
}

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

Lines changed: 8 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ module UnsafeJQueryPlugin {
1818
/**
1919
* Gets the plugin that this source is used in.
2020
*/
21-
abstract JQueryPluginMethod getPlugin();
21+
abstract JQuery::JQueryPluginMethod getPlugin();
2222
}
2323

2424
/**
@@ -49,49 +49,6 @@ module UnsafeJQueryPlugin {
4949
}
5050
}
5151

52-
/**
53-
* Holds for jQuery plugin definitions of the form `$.fn.<pluginName> = <plugin>`.
54-
*/
55-
private predicate jQueryPluginDefinition(string pluginName, DataFlow::Node plugin) {
56-
exists(DataFlow::PropRead fn, DataFlow::PropWrite write |
57-
fn = jquery().getAPropertyRead("fn") and
58-
(
59-
write = fn.getAPropertyWrite()
60-
or
61-
exists(ExtendCall extend, DataFlow::SourceNode source |
62-
fn.flowsTo(extend.getDestinationOperand()) and
63-
source = extend.getASourceOperand() and
64-
write = source.getAPropertyWrite()
65-
)
66-
) and
67-
plugin = write.getRhs() and
68-
(
69-
pluginName = write.getPropertyName() or
70-
write.getPropertyNameExpr().flow().mayHaveStringValue(pluginName)
71-
)
72-
)
73-
}
74-
75-
/**
76-
* Gets a node that is registered as a jQuery plugin method at `def`.
77-
*/
78-
private DataFlow::SourceNode getAJQueryPluginMethod(
79-
DataFlow::TypeBackTracker t, DataFlow::Node def
80-
) {
81-
t.start() and
82-
jQueryPluginDefinition(_, def) and
83-
result.flowsTo(def)
84-
or
85-
exists(DataFlow::TypeBackTracker t2 | result = getAJQueryPluginMethod(t2, def).backtrack(t2, t))
86-
}
87-
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-
9552
/**
9653
* Gets an operand to `extend`.
9754
*/
@@ -109,29 +66,10 @@ module UnsafeJQueryPlugin {
10966
result = getAnExtendOperand(DataFlow::TypeBackTracker::end(), extend)
11067
}
11168

112-
/**
113-
* A function that is registered as a jQuery plugin method.
114-
*/
115-
class JQueryPluginMethod extends DataFlow::FunctionNode {
116-
string pluginName;
117-
118-
JQueryPluginMethod() {
119-
exists(DataFlow::Node def |
120-
jQueryPluginDefinition(pluginName, def) and
121-
this = getAJQueryPluginMethod(def)
122-
)
123-
}
124-
125-
/**
126-
* Gets the name of this plugin.
127-
*/
128-
string getPluginName() { result = pluginName }
129-
}
130-
13169
/**
13270
* Holds if `plugin` has a default option defined at `def`.
13371
*/
134-
private predicate hasDefaultOption(JQueryPluginMethod plugin, DataFlow::PropWrite def) {
72+
private predicate hasDefaultOption(JQuery::JQueryPluginMethod plugin, DataFlow::PropWrite def) {
13573
exists(ExtendCall extend, JQueryPluginOptions options, DataFlow::SourceNode default |
13674
options.getPlugin() = plugin and
13775
options = getAnExtendOperand(extend) and
@@ -144,7 +82,7 @@ module UnsafeJQueryPlugin {
14482
* The client-provided options object for a jQuery plugin.
14583
*/
14684
class JQueryPluginOptions extends DataFlow::ParameterNode {
147-
JQueryPluginMethod method;
85+
JQuery::JQueryPluginMethod method;
14886

14987
JQueryPluginOptions() {
15088
exists(string optionsPattern |
@@ -165,7 +103,7 @@ module UnsafeJQueryPlugin {
165103
/**
166104
* Gets the plugin method that these options are used in.
167105
*/
168-
JQueryPluginMethod getPlugin() { result = method }
106+
JQuery::JQueryPluginMethod getPlugin() { result = method }
169107
}
170108

171109
/**
@@ -224,7 +162,9 @@ module UnsafeJQueryPlugin {
224162
* The client-provided options object for a jQuery plugin, considered as a source for unsafe jQuery plugins.
225163
*/
226164
class JQueryPluginOptionsAsSource extends Source, JQueryPluginOptions {
227-
override JQueryPluginMethod getPlugin() { result = JQueryPluginOptions.super.getPlugin() }
165+
override JQuery::JQueryPluginMethod getPlugin() {
166+
result = JQueryPluginOptions.super.getPlugin()
167+
}
228168
}
229169

230170
/**
@@ -246,7 +186,7 @@ module UnsafeJQueryPlugin {
246186
/**
247187
* Holds if `plugin` likely expects `sink` to be treated as a HTML fragment.
248188
*/
249-
predicate isLikelyIntentionalHtmlSink(JQueryPluginMethod plugin, Sink sink) {
189+
predicate isLikelyIntentionalHtmlSink(JQuery::JQueryPluginMethod plugin, Sink sink) {
250190
exists(DataFlow::PropWrite defaultDef, string default, DataFlow::PropRead finalRead |
251191
hasDefaultOption(plugin, defaultDef) and
252192
defaultDef.getPropertyName() = finalRead.getPropertyName() and

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ module DomBasedXss {
8080
not exists(DataFlow::Node prefix, string strval |
8181
isPrefixOfJQueryHtmlString(this, prefix) and
8282
strval = prefix.getStringValue() and
83+
not strval = "" and
8384
not strval.regexpMatch("\\s*<.*")
8485
) and
8586
not DOM::locationRef().flowsTo(this)

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,16 @@ nodes
347347
| tst.js:354:16:354:39 | documen ... .search |
348348
| tst.js:355:12:355:17 | target |
349349
| tst.js:355:12:355:17 | target |
350+
| tst.js:361:10:361:42 | target |
351+
| tst.js:361:19:361:35 | document.___location |
352+
| tst.js:361:19:361:35 | document.___location |
353+
| tst.js:361:19:361:42 | documen ... .search |
354+
| tst.js:362:16:362:21 | target |
355+
| tst.js:362:16:362:21 | target |
356+
| tst.js:366:21:366:26 | target |
357+
| tst.js:366:21:366:26 | target |
358+
| tst.js:369:18:369:23 | target |
359+
| tst.js:369:18:369:23 | target |
350360
| typeahead.js:20:13:20:45 | target |
351361
| typeahead.js:20:22:20:38 | document.___location |
352362
| typeahead.js:20:22:20:38 | document.___location |
@@ -670,6 +680,15 @@ edges
670680
| tst.js:354:16:354:32 | document.___location | tst.js:354:16:354:39 | documen ... .search |
671681
| tst.js:354:16:354:32 | document.___location | tst.js:354:16:354:39 | documen ... .search |
672682
| tst.js:354:16:354:39 | documen ... .search | tst.js:354:7:354:39 | target |
683+
| tst.js:361:10:361:42 | target | tst.js:362:16:362:21 | target |
684+
| tst.js:361:10:361:42 | target | tst.js:362:16:362:21 | target |
685+
| tst.js:361:10:361:42 | target | tst.js:366:21:366:26 | target |
686+
| tst.js:361:10:361:42 | target | tst.js:366:21:366:26 | target |
687+
| tst.js:361:10:361:42 | target | tst.js:369:18:369:23 | target |
688+
| tst.js:361:10:361:42 | target | tst.js:369:18:369:23 | target |
689+
| tst.js:361:19:361:35 | document.___location | tst.js:361:19:361:42 | documen ... .search |
690+
| tst.js:361:19:361:35 | document.___location | tst.js:361:19:361:42 | documen ... .search |
691+
| tst.js:361:19:361:42 | documen ... .search | tst.js:361:10:361:42 | target |
673692
| typeahead.js:20:13:20:45 | target | typeahead.js:21:12:21:17 | target |
674693
| typeahead.js:20:22:20:38 | document.___location | typeahead.js:20:22:20:45 | documen ... .search |
675694
| typeahead.js:20:22:20:38 | document.___location | typeahead.js:20:22:20:45 | documen ... .search |
@@ -772,6 +791,9 @@ edges
772791
| tst.js:336:18:336:35 | params.get('name') | tst.js:330:18:330:34 | document.___location | tst.js:336:18:336:35 | params.get('name') | Cross-site scripting vulnerability due to $@. | tst.js:330:18:330:34 | document.___location | user-provided value |
773792
| tst.js:349:5:349:30 | getUrl( ... ring(1) | tst.js:347:20:347:36 | document.___location | tst.js:349:5:349:30 | getUrl( ... ring(1) | Cross-site scripting vulnerability due to $@. | tst.js:347:20:347:36 | document.___location | user-provided value |
774793
| tst.js:355:12:355:17 | target | tst.js:354:16:354:32 | document.___location | tst.js:355:12:355:17 | target | Cross-site scripting vulnerability due to $@. | tst.js:354:16:354:32 | document.___location | user-provided value |
794+
| tst.js:362:16:362:21 | target | tst.js:361:19:361:35 | document.___location | tst.js:362:16:362:21 | target | Cross-site scripting vulnerability due to $@. | tst.js:361:19:361:35 | document.___location | user-provided value |
795+
| tst.js:366:21:366:26 | target | tst.js:361:19:361:35 | document.___location | tst.js:366:21:366:26 | target | Cross-site scripting vulnerability due to $@. | tst.js:361:19:361:35 | document.___location | user-provided value |
796+
| tst.js:369:18:369:23 | target | tst.js:361:19:361:35 | document.___location | tst.js:369:18:369:23 | target | Cross-site scripting vulnerability due to $@. | tst.js:361:19:361:35 | document.___location | user-provided value |
775797
| typeahead.js:25:18:25:20 | val | typeahead.js:20:22:20:38 | document.___location | typeahead.js:25:18:25:20 | val | Cross-site scripting vulnerability due to $@. | typeahead.js:20:22:20:38 | document.___location | user-provided value |
776798
| v-html.vue:2:8:2:23 | v-html=tainted | v-html.vue:6:42:6:58 | document.___location | v-html.vue:2:8:2:23 | v-html=tainted | Cross-site scripting vulnerability due to $@. | v-html.vue:6:42:6:58 | document.___location | user-provided value |
777799
| winjs.js:3:43:3:49 | tainted | winjs.js:2:17:2:33 | document.___location | winjs.js:3:43:3:49 | tainted | Cross-site scripting vulnerability due to $@. | winjs.js:2:17:2:33 | document.___location | user-provided value |

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@ nodes
4141
| xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name |
4242
| xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name |
4343
| xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name |
44+
| xss-through-dom.js:73:9:73:41 | selector |
45+
| xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name |
46+
| xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name |
47+
| xss-through-dom.js:77:7:77:14 | selector |
48+
| xss-through-dom.js:77:7:77:14 | selector |
4449
edges
4550
| xss-through-dom.js:2:16:2:34 | $("textarea").val() | xss-through-dom.js:2:16:2:34 | $("textarea").val() |
4651
| xss-through-dom.js:4:16:4:40 | $(".som ... .text() | xss-through-dom.js:4:16:4:40 | $(".som ... .text() |
@@ -56,6 +61,10 @@ edges
5661
| xss-through-dom.js:61:30:61:69 | $(docum ... value") | xss-through-dom.js:61:30:61:69 | $(docum ... value") |
5762
| xss-through-dom.js:64:30:64:40 | valMethod() | xss-through-dom.js:64:30:64:40 | valMethod() |
5863
| xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name | xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name |
64+
| xss-through-dom.js:73:9:73:41 | selector | xss-through-dom.js:77:7:77:14 | selector |
65+
| xss-through-dom.js:73:9:73:41 | selector | xss-through-dom.js:77:7:77:14 | selector |
66+
| xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:73:9:73:41 | selector |
67+
| xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:73:9:73:41 | selector |
5968
#select
6069
| xss-through-dom.js:2:16:2:34 | $("textarea").val() | xss-through-dom.js:2:16:2:34 | $("textarea").val() | xss-through-dom.js:2:16:2:34 | $("textarea").val() | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:2:16:2:34 | $("textarea").val() | DOM text |
6170
| xss-through-dom.js:4:16:4:40 | $(".som ... .text() | xss-through-dom.js:4:16:4:40 | $(".som ... .text() | xss-through-dom.js:4:16:4:40 | $(".som ... .text() | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:4:16:4:40 | $(".som ... .text() | DOM text |
@@ -71,3 +80,4 @@ edges
7180
| xss-through-dom.js:61:30:61:69 | $(docum ... value") | xss-through-dom.js:61:30:61:69 | $(docum ... value") | xss-through-dom.js:61:30:61:69 | $(docum ... value") | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:61:30:61:69 | $(docum ... value") | DOM text |
7281
| xss-through-dom.js:64:30:64:40 | valMethod() | xss-through-dom.js:64:30:64:40 | valMethod() | xss-through-dom.js:64:30:64:40 | valMethod() | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:64:30:64:40 | valMethod() | DOM text |
7382
| xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name | xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name | xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name | DOM text |
83+
| xss-through-dom.js:77:7:77:14 | selector | xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:77:7:77:14 | selector | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | DOM text |

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,16 @@ nodes
347347
| tst.js:354:16:354:39 | documen ... .search |
348348
| tst.js:355:12:355:17 | target |
349349
| tst.js:355:12:355:17 | target |
350+
| tst.js:361:10:361:42 | target |
351+
| tst.js:361:19:361:35 | document.___location |
352+
| tst.js:361:19:361:35 | document.___location |
353+
| tst.js:361:19:361:42 | documen ... .search |
354+
| tst.js:362:16:362:21 | target |
355+
| tst.js:362:16:362:21 | target |
356+
| tst.js:366:21:366:26 | target |
357+
| tst.js:366:21:366:26 | target |
358+
| tst.js:369:18:369:23 | target |
359+
| tst.js:369:18:369:23 | target |
350360
| typeahead.js:9:28:9:30 | loc |
351361
| typeahead.js:9:28:9:30 | loc |
352362
| typeahead.js:10:16:10:18 | loc |
@@ -674,6 +684,15 @@ edges
674684
| tst.js:354:16:354:32 | document.___location | tst.js:354:16:354:39 | documen ... .search |
675685
| tst.js:354:16:354:32 | document.___location | tst.js:354:16:354:39 | documen ... .search |
676686
| tst.js:354:16:354:39 | documen ... .search | tst.js:354:7:354:39 | target |
687+
| tst.js:361:10:361:42 | target | tst.js:362:16:362:21 | target |
688+
| tst.js:361:10:361:42 | target | tst.js:362:16:362:21 | target |
689+
| tst.js:361:10:361:42 | target | tst.js:366:21:366:26 | target |
690+
| tst.js:361:10:361:42 | target | tst.js:366:21:366:26 | target |
691+
| tst.js:361:10:361:42 | target | tst.js:369:18:369:23 | target |
692+
| tst.js:361:10:361:42 | target | tst.js:369:18:369:23 | target |
693+
| tst.js:361:19:361:35 | document.___location | tst.js:361:19:361:42 | documen ... .search |
694+
| tst.js:361:19:361:35 | document.___location | tst.js:361:19:361:42 | documen ... .search |
695+
| tst.js:361:19:361:42 | documen ... .search | tst.js:361:10:361:42 | target |
677696
| typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |
678697
| typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |
679698
| typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,3 +355,20 @@ function growl() {
355355
$.jGrowl(target); // NOT OK
356356
}
357357

358+
function thisNodes() {
359+
var pluginName = "myFancyJQueryPlugin";
360+
var myPlugin = function () {
361+
var target = document.___location.search
362+
this.html(target); // NOT OK. (this is a jQuery object)
363+
this.innerHTML = target // OK. (this is a jQuery object)
364+
365+
this.each(function (i, e) {
366+
this.innerHTML = target; // NOT OK. (this is a DOM-node);
367+
this.html(target); // OK. (this is a DOM-node);
368+
369+
e.innerHTML = target; // NOT OK.
370+
});
371+
}
372+
$.fn[pluginName] = myPlugin;
373+
374+
}

0 commit comments

Comments
 (0)