hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-05-02 04:05:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #4206 from erik-krogh/consistentJquery

Browse Source 
Approved by esbena
This commit is contained in:
CodeQL CI
2020-09-07 11:23:23 +01:00
committed by GitHub
parent 68f421f9c3 4175637631
commit 85f6388a19
6 changed files with 79 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
javascript/ql/src/Security/CWE-079/UnsafeJQueryPlugin.ql
View File

@@ -20,7 +20,6 @@ from
JQuery::JQueryPluginMethod plugin
where
cfg.hasFlowPath(source, sink) and
source.getNode().(Source).getPlugin() = plugin and
not isLikelyIntentionalHtmlSink(plugin, sink.getNode())
source.getNode().(Source).getPlugin() = plugin
select sink.getNode(), source, sink, "Potential XSS vulnerability in the $@.", plugin,
"'$.fn." + plugin.getPluginName() + "' plugin"

19
javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPlugin.qll
View File

@@ -29,7 +29,8 @@ module UnsafeJQueryPlugin {
override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node sink) {
// jQuery plugins tend to be implemented as classes that store data in fields initialized by the constructor.
DataFlow::localFieldStep(src, sink)
DataFlow::localFieldStep(src, sink) or
aliasPropertyPresenceStep(src, sink)
}
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
@@ -51,4 +52,20 @@ module UnsafeJQueryPlugin {
node instanceof PropertyPresenceSanitizer
}
}
/**
* Holds if there is a taint-step from `src` to `sink`,
* where `src` is a property read that acts as a sanitizer for the base,
* and `sink` is that same property read from the same base.
*
* For an condition like `if(foo.bar) {...}`, the base `foo` is sanitized but the property `foo.bar` is not.
* With this taint-step we regain that `foo.bar` is tainted, because `PropertyPresenceSanitizer` could remove it.
*/
private predicate aliasPropertyPresenceStep(DataFlow::Node src, DataFlow::Node sink) {
exists(PropertyPresenceSanitizer sanitizer, DataFlow::PropRead read | read = src |
read = sanitizer.getPropRead() and
sink = AccessPath::getAnAliasedSourceNode(read) and
read.getBasicBlock().(ReachableBasicBlock).strictlyDominates(sink.getBasicBlock())
)
}
}

21
javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll
View File

@@ -152,6 +152,11 @@ module UnsafeJQueryPlugin {
)
}
/**
* Gets the property read that is used to sanitize the base value.
*/
DataFlow::PropRead getPropRead() { result = this }
override predicate sanitizes(boolean outcome, Expr e) {
outcome = polarity and
e = input.asExpr()
@@ -171,7 +176,9 @@ module UnsafeJQueryPlugin {
* An argument that may act as a HTML fragment rather than a CSS selector, as a sink for remote unsafe jQuery plugins.
*/
class AmbiguousHtmlOrSelectorArgumentAsSink extends Sink {
AmbiguousHtmlOrSelectorArgumentAsSink() { this instanceof AmbiguousHtmlOrSelectorArgument }
AmbiguousHtmlOrSelectorArgumentAsSink() {
this instanceof AmbiguousHtmlOrSelectorArgument and not isLikelyIntentionalHtmlSink(this)
}
}
/**
@@ -184,15 +191,19 @@ module UnsafeJQueryPlugin {
}
/**
* Holds if `plugin` likely expects `sink` to be treated as a HTML fragment.
* Holds if there exists a jQuery plugin that likely expects `sink` to be treated as a HTML fragment.
*/
predicate isLikelyIntentionalHtmlSink(JQuery::JQueryPluginMethod plugin, Sink sink) {
exists(DataFlow::PropWrite defaultDef, string default, DataFlow::PropRead finalRead |
predicate isLikelyIntentionalHtmlSink(DataFlow::Node sink) {
exists(
JQuery::JQueryPluginMethod plugin, DataFlow::PropWrite defaultDef, string default,
DataFlow::PropRead finalRead
|
hasDefaultOption(plugin, defaultDef) and
defaultDef.getPropertyName() = finalRead.getPropertyName() and
defaultDef.getRhs().mayHaveStringValue(default) and
default.regexpMatch("\\s*<.*") and
finalRead.flowsTo(sink)
finalRead.flowsTo(sink) and
sink.getTopLevel() = plugin.getTopLevel()
)
}
}

51
javascript/ql/test/query-tests/Security/CWE-079/UnsafeJQueryPlugin.expected
View File

@@ -6,6 +6,8 @@ nodes
| unsafe-jquery-plugin.js:5:5:5:11 | options |
| unsafe-jquery-plugin.js:5:5:5:18 | options.target |
| unsafe-jquery-plugin.js:5:5:5:18 | options.target |
| unsafe-jquery-plugin.js:7:17:7:23 | options |
| unsafe-jquery-plugin.js:7:17:7:30 | options.target |
| unsafe-jquery-plugin.js:11:7:11:29 | target |
| unsafe-jquery-plugin.js:11:16:11:22 | options |
| unsafe-jquery-plugin.js:11:16:11:29 | options.target |
@@ -23,6 +25,15 @@ nodes
| unsafe-jquery-plugin.js:52:6:52:11 | target |
| unsafe-jquery-plugin.js:60:6:60:11 | target |
| unsafe-jquery-plugin.js:60:6:60:11 | target |
| unsafe-jquery-plugin.js:65:47:65:53 | options |
| unsafe-jquery-plugin.js:65:47:65:53 | options |
| unsafe-jquery-plugin.js:67:24:67:44 | $.exten ... ptions) |
| unsafe-jquery-plugin.js:67:33:67:34 | {} |
| unsafe-jquery-plugin.js:67:37:67:43 | options |
| unsafe-jquery-plugin.js:68:7:68:18 | this.options |
| unsafe-jquery-plugin.js:68:7:68:25 | this.options.parent |
| unsafe-jquery-plugin.js:68:45:68:63 | this.options.parent |
| unsafe-jquery-plugin.js:68:45:68:63 | this.options.parent |
| unsafe-jquery-plugin.js:71:38:71:44 | options |
| unsafe-jquery-plugin.js:71:38:71:44 | options |
| unsafe-jquery-plugin.js:72:5:72:11 | options |
@@ -55,9 +66,6 @@ nodes
| unsafe-jquery-plugin.js:102:13:105:13 | $.exten ... ptions) |
| unsafe-jquery-plugin.js:102:22:105:3 | {\\n\\t\\t\\tme ... in'\\n\\t\\t} |
| unsafe-jquery-plugin.js:105:6:105:12 | options |
| unsafe-jquery-plugin.js:106:5:106:11 | options |
| unsafe-jquery-plugin.js:106:5:106:16 | options.menu |
| unsafe-jquery-plugin.js:106:5:106:16 | options.menu |
| unsafe-jquery-plugin.js:107:5:107:11 | options |
| unsafe-jquery-plugin.js:107:5:107:18 | options.target |
| unsafe-jquery-plugin.js:107:5:107:18 | options.target |
@@ -67,9 +75,6 @@ nodes
| unsafe-jquery-plugin.js:115:13:115:58 | $.exten ... ptions) |
| unsafe-jquery-plugin.js:115:22:115:23 | {} |
| unsafe-jquery-plugin.js:115:51:115:57 | options |
| unsafe-jquery-plugin.js:116:5:116:11 | options |
| unsafe-jquery-plugin.js:116:5:116:16 | options.menu |
| unsafe-jquery-plugin.js:116:5:116:16 | options.menu |
| unsafe-jquery-plugin.js:117:5:117:11 | options |
| unsafe-jquery-plugin.js:117:5:117:18 | options.target |
| unsafe-jquery-plugin.js:117:5:117:18 | options.target |
@@ -96,6 +101,10 @@ nodes
| unsafe-jquery-plugin.js:136:5:136:29 | options ... elector |
| unsafe-jquery-plugin.js:153:38:153:44 | options |
| unsafe-jquery-plugin.js:153:38:153:44 | options |
| unsafe-jquery-plugin.js:154:16:154:22 | options |
| unsafe-jquery-plugin.js:154:16:154:29 | options.target |
| unsafe-jquery-plugin.js:156:3:156:9 | options |
| unsafe-jquery-plugin.js:156:3:156:16 | options.target |
| unsafe-jquery-plugin.js:157:44:157:50 | options |
| unsafe-jquery-plugin.js:157:44:157:57 | options.target |
| unsafe-jquery-plugin.js:157:44:157:59 | options.target.a |
@@ -119,10 +128,15 @@ edges
| unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:3:5:3:11 | options |
| unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:5:5:5:11 | options |
| unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:5:5:5:11 | options |
| unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:7:17:7:23 | options |
| unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:7:17:7:23 | options |
| unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:11:16:11:22 | options |
| unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:11:16:11:22 | options |
| unsafe-jquery-plugin.js:5:5:5:11 | options | unsafe-jquery-plugin.js:5:5:5:18 | options.target |
| unsafe-jquery-plugin.js:5:5:5:11 | options | unsafe-jquery-plugin.js:5:5:5:18 | options.target |
| unsafe-jquery-plugin.js:5:5:5:18 | options.target | unsafe-jquery-plugin.js:11:16:11:29 | options.target |
| unsafe-jquery-plugin.js:7:17:7:23 | options | unsafe-jquery-plugin.js:7:17:7:30 | options.target |
| unsafe-jquery-plugin.js:7:17:7:30 | options.target | unsafe-jquery-plugin.js:11:16:11:29 | options.target |
| unsafe-jquery-plugin.js:11:7:11:29 | target | unsafe-jquery-plugin.js:22:6:22:11 | target |
| unsafe-jquery-plugin.js:11:7:11:29 | target | unsafe-jquery-plugin.js:22:6:22:11 | target |
| unsafe-jquery-plugin.js:11:7:11:29 | target | unsafe-jquery-plugin.js:30:6:30:11 | target |
@@ -139,6 +153,15 @@ edges
| unsafe-jquery-plugin.js:11:7:11:29 | target | unsafe-jquery-plugin.js:60:6:60:11 | target |
| unsafe-jquery-plugin.js:11:16:11:22 | options | unsafe-jquery-plugin.js:11:16:11:29 | options.target |
| unsafe-jquery-plugin.js:11:16:11:29 | options.target | unsafe-jquery-plugin.js:11:7:11:29 | target |
| unsafe-jquery-plugin.js:65:47:65:53 | options | unsafe-jquery-plugin.js:67:37:67:43 | options |
| unsafe-jquery-plugin.js:65:47:65:53 | options | unsafe-jquery-plugin.js:67:37:67:43 | options |
| unsafe-jquery-plugin.js:67:24:67:44 | $.exten ... ptions) | unsafe-jquery-plugin.js:68:7:68:18 | this.options |
| unsafe-jquery-plugin.js:67:33:67:34 | {} | unsafe-jquery-plugin.js:67:24:67:44 | $.exten ... ptions) |
| unsafe-jquery-plugin.js:67:37:67:43 | options | unsafe-jquery-plugin.js:67:24:67:44 | $.exten ... ptions) |
| unsafe-jquery-plugin.js:67:37:67:43 | options | unsafe-jquery-plugin.js:67:33:67:34 | {} |
| unsafe-jquery-plugin.js:68:7:68:18 | this.options | unsafe-jquery-plugin.js:68:7:68:25 | this.options.parent |
| unsafe-jquery-plugin.js:68:7:68:25 | this.options.parent | unsafe-jquery-plugin.js:68:45:68:63 | this.options.parent |
| unsafe-jquery-plugin.js:68:7:68:25 | this.options.parent | unsafe-jquery-plugin.js:68:45:68:63 | this.options.parent |
| unsafe-jquery-plugin.js:71:38:71:44 | options | unsafe-jquery-plugin.js:72:5:72:11 | options |
| unsafe-jquery-plugin.js:71:38:71:44 | options | unsafe-jquery-plugin.js:72:5:72:11 | options |
| unsafe-jquery-plugin.js:72:5:72:11 | options | unsafe-jquery-plugin.js:72:5:72:15 | options.foo |
@@ -165,26 +188,20 @@ edges
| unsafe-jquery-plugin.js:92:5:92:11 | options | unsafe-jquery-plugin.js:85:14:85:14 | o |
| unsafe-jquery-plugin.js:101:38:101:44 | options | unsafe-jquery-plugin.js:105:6:105:12 | options |
| unsafe-jquery-plugin.js:101:38:101:44 | options | unsafe-jquery-plugin.js:105:6:105:12 | options |
| unsafe-jquery-plugin.js:102:3:105:13 | options | unsafe-jquery-plugin.js:106:5:106:11 | options |
| unsafe-jquery-plugin.js:102:3:105:13 | options | unsafe-jquery-plugin.js:107:5:107:11 | options |
| unsafe-jquery-plugin.js:102:13:105:13 | $.exten ... ptions) | unsafe-jquery-plugin.js:102:3:105:13 | options |
| unsafe-jquery-plugin.js:102:22:105:3 | {\\n\\t\\t\\tme ... in'\\n\\t\\t} | unsafe-jquery-plugin.js:102:13:105:13 | $.exten ... ptions) |
| unsafe-jquery-plugin.js:105:6:105:12 | options | unsafe-jquery-plugin.js:102:13:105:13 | $.exten ... ptions) |
| unsafe-jquery-plugin.js:105:6:105:12 | options | unsafe-jquery-plugin.js:102:22:105:3 | {\\n\\t\\t\\tme ... in'\\n\\t\\t} |
| unsafe-jquery-plugin.js:106:5:106:11 | options | unsafe-jquery-plugin.js:106:5:106:16 | options.menu |
| unsafe-jquery-plugin.js:106:5:106:11 | options | unsafe-jquery-plugin.js:106:5:106:16 | options.menu |
| unsafe-jquery-plugin.js:107:5:107:11 | options | unsafe-jquery-plugin.js:107:5:107:18 | options.target |
| unsafe-jquery-plugin.js:107:5:107:11 | options | unsafe-jquery-plugin.js:107:5:107:18 | options.target |
| unsafe-jquery-plugin.js:114:38:114:44 | options | unsafe-jquery-plugin.js:115:51:115:57 | options |
| unsafe-jquery-plugin.js:114:38:114:44 | options | unsafe-jquery-plugin.js:115:51:115:57 | options |
| unsafe-jquery-plugin.js:115:3:115:58 | options | unsafe-jquery-plugin.js:116:5:116:11 | options |
| unsafe-jquery-plugin.js:115:3:115:58 | options | unsafe-jquery-plugin.js:117:5:117:11 | options |
| unsafe-jquery-plugin.js:115:13:115:58 | $.exten ... ptions) | unsafe-jquery-plugin.js:115:3:115:58 | options |
| unsafe-jquery-plugin.js:115:22:115:23 | {} | unsafe-jquery-plugin.js:115:13:115:58 | $.exten ... ptions) |
| unsafe-jquery-plugin.js:115:51:115:57 | options | unsafe-jquery-plugin.js:115:13:115:58 | $.exten ... ptions) |
| unsafe-jquery-plugin.js:115:51:115:57 | options | unsafe-jquery-plugin.js:115:22:115:23 | {} |
| unsafe-jquery-plugin.js:116:5:116:11 | options | unsafe-jquery-plugin.js:116:5:116:16 | options.menu |
| unsafe-jquery-plugin.js:116:5:116:11 | options | unsafe-jquery-plugin.js:116:5:116:16 | options.menu |
| unsafe-jquery-plugin.js:117:5:117:11 | options | unsafe-jquery-plugin.js:117:5:117:18 | options.target |
| unsafe-jquery-plugin.js:117:5:117:11 | options | unsafe-jquery-plugin.js:117:5:117:18 | options.target |
| unsafe-jquery-plugin.js:121:40:121:46 | options | unsafe-jquery-plugin.js:122:5:122:11 | options |
@@ -204,8 +221,17 @@ edges
| unsafe-jquery-plugin.js:136:5:136:11 | options | unsafe-jquery-plugin.js:136:5:136:20 | options.viewport |
| unsafe-jquery-plugin.js:136:5:136:20 | options.viewport | unsafe-jquery-plugin.js:136:5:136:29 | options ... elector |
| unsafe-jquery-plugin.js:136:5:136:20 | options.viewport | unsafe-jquery-plugin.js:136:5:136:29 | options ... elector |
| unsafe-jquery-plugin.js:153:38:153:44 | options | unsafe-jquery-plugin.js:154:16:154:22 | options |
| unsafe-jquery-plugin.js:153:38:153:44 | options | unsafe-jquery-plugin.js:154:16:154:22 | options |
| unsafe-jquery-plugin.js:153:38:153:44 | options | unsafe-jquery-plugin.js:156:3:156:9 | options |
| unsafe-jquery-plugin.js:153:38:153:44 | options | unsafe-jquery-plugin.js:156:3:156:9 | options |
| unsafe-jquery-plugin.js:153:38:153:44 | options | unsafe-jquery-plugin.js:157:44:157:50 | options |
| unsafe-jquery-plugin.js:153:38:153:44 | options | unsafe-jquery-plugin.js:157:44:157:50 | options |
| unsafe-jquery-plugin.js:154:16:154:22 | options | unsafe-jquery-plugin.js:154:16:154:29 | options.target |
| unsafe-jquery-plugin.js:154:16:154:29 | options.target | unsafe-jquery-plugin.js:156:3:156:16 | options.target |
| unsafe-jquery-plugin.js:154:16:154:29 | options.target | unsafe-jquery-plugin.js:157:44:157:57 | options.target |
| unsafe-jquery-plugin.js:156:3:156:9 | options | unsafe-jquery-plugin.js:156:3:156:16 | options.target |
| unsafe-jquery-plugin.js:156:3:156:16 | options.target | unsafe-jquery-plugin.js:157:44:157:57 | options.target |
| unsafe-jquery-plugin.js:157:44:157:50 | options | unsafe-jquery-plugin.js:157:44:157:57 | options.target |
| unsafe-jquery-plugin.js:157:44:157:57 | options.target | unsafe-jquery-plugin.js:157:44:157:59 | options.target.a |
| unsafe-jquery-plugin.js:157:44:157:57 | options.target | unsafe-jquery-plugin.js:157:44:157:59 | options.target.a |
@@ -229,6 +255,7 @@ edges
| unsafe-jquery-plugin.js:48:6:48:11 | target | unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:48:6:48:11 | target | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:2:19:63:2 | functio ... \\t\\t}\\n\\n\\t} | '$.fn.my_plugin' plugin |
| unsafe-jquery-plugin.js:52:6:52:11 | target | unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:52:6:52:11 | target | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:2:19:63:2 | functio ... \\t\\t}\\n\\n\\t} | '$.fn.my_plugin' plugin |
| unsafe-jquery-plugin.js:60:6:60:11 | target | unsafe-jquery-plugin.js:2:38:2:44 | options | unsafe-jquery-plugin.js:60:6:60:11 | target | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:2:19:63:2 | functio ... \\t\\t}\\n\\n\\t} | '$.fn.my_plugin' plugin |
| unsafe-jquery-plugin.js:68:45:68:63 | this.options.parent | unsafe-jquery-plugin.js:65:47:65:53 | options | unsafe-jquery-plugin.js:68:45:68:63 | this.options.parent | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:65:19:69:2 | functio ... T OK\\n\\t} | '$.fn.my_plugin' plugin |
| unsafe-jquery-plugin.js:72:5:72:23 | options.foo.bar.baz | unsafe-jquery-plugin.js:71:38:71:44 | options | unsafe-jquery-plugin.js:72:5:72:23 | options.foo.bar.baz | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:71:19:74:2 | functio ... / OK\\n\\t} | '$.fn.my_plugin' plugin |
| unsafe-jquery-plugin.js:77:17:77:35 | options.foo.bar.baz | unsafe-jquery-plugin.js:76:38:76:44 | options | unsafe-jquery-plugin.js:77:17:77:35 | options.foo.bar.baz | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:76:19:78:2 | functio ... T OK\\n\\t} | '$.fn.my_plugin' plugin |
| unsafe-jquery-plugin.js:90:6:90:6 | t | unsafe-jquery-plugin.js:84:38:84:44 | options | unsafe-jquery-plugin.js:90:6:90:6 | t | Potential XSS vulnerability in the $@. | unsafe-jquery-plugin.js:84:19:93:2 | functio ... ns);\\n\\t} | '$.fn.my_plugin' plugin |

8
javascript/ql/test/query-tests/Security/CWE-079/unsafe-jquery-plugin.js
View File

@@ -65,7 +65,7 @@
$.fn.my_plugin = function my_plugin(element, options) {
this.$element = $(element);
this.options = $.extend({}, options);
if (this.options.parent) this.$parent = $(this.options.parent) // NOT OK - but not flagged [INCONSISTENCY]
if (this.options.parent) this.$parent = $(this.options.parent) // NOT OK
};
$.fn.my_plugin = function my_plugin(options) {
@@ -103,7 +103,7 @@
menu: '<div></div>',
target: '.my_plugin'
}, options);
$(options.menu); // OK - but is flagged [INCONSISTENCY]
$(options.menu); // OK
$(options.target); // NOT OK
};
@@ -113,7 +113,7 @@
};
$.fn.my_plugin = function my_plugin(options) {
options = $.extend({}, $.fn.my_plugin.defaults, options);
$(options.menu); // OK - but is flagged [INCONSISTENCY]
$(options.menu); // OK
$(options.target); // NOT OK
};
@@ -154,7 +154,7 @@
let target = options.target;
target === DEFAULTS.target? $(target): $(document).find(target); // OK
options.target === DEFAULTS.target? $(options.target): $(document).find(options.target); // OK
options.targets.a === DEFAULTS.target? $(options.target.a): $(document).find(options.target.a); // OK - but still flagged [INCONSISTENCY]
options.targets.a === DEFAULTS.target? $(options.target.a): $(document).find(options.target.a); // OK - should be sanitized by `MembershipTestSanitizer` - but still flagged because `AccessPath` can't handle these deeply nested properties [INCONSISTENCY]
}
$.fn.my_plugin = function my_plugin(options) {