Skip to content

Commit da32926

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3191 from erik-krogh/XssDom
Approved by esbena, mchammer01
2 parents f696594 + ac26741 commit da32926

File tree

12 files changed

+389
-6
lines changed

12 files changed

+389
-6
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
| **Query** | **Tags** | **Purpose** |
99
|---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
10-
10+
| Cross-site scripting through DOM (`js/xss-through-dom`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities where existing text from the DOM is used as HTML. Results are not shown on LGTM by default. |
1111

1212
## Changes to existing queries
1313

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Extracting text from a DOM node and interpreting it as HTML can lead to a cross-site scripting vulnerability.
9+
</p>
10+
<p>
11+
A webpage with this vulnerability reads text from the DOM, and afterwards adds the text as HTML to the DOM.
12+
Using text from the DOM as HTML effectively unescapes the text, and thereby invalidates any escaping done on the text.
13+
If an attacker is able to control the safe sanitized text, then this vulnerability can be exploited to perform a cross-site scripting attack.
14+
</p>
15+
</overview>
16+
17+
<recommendation>
18+
<p>
19+
To guard against cross-site scripting, consider using contextual output encoding/escaping before
20+
writing text to the page, or one of the other solutions that are mentioned in the References section below.
21+
</p>
22+
</recommendation>
23+
24+
<example>
25+
<p>
26+
The following example shows a webpage using a <code>data-target</code> attribute
27+
to select and manipulate a DOM element using the JQuery library. In the example, the
28+
<code>data-target</code> attribute is read into the <code>target</code> variable, and the
29+
<code>$</code> function is then supposed to use the <code>target</code> variable as a CSS
30+
selector to determine which element should be manipulated.
31+
</p>
32+
<sample src="examples/XssThroughDom.js" />
33+
<p>
34+
However, if an attacker can control the <code>data-target</code> attribute,
35+
then the value of <code>target</code> can be used to cause the <code>$</code> function
36+
to execute arbitary JavaScript.
37+
</p>
38+
<p>
39+
The above vulnerability can be fixed by using <code>$.find</code> instead of <code>$</code>.
40+
The <code>$.find</code> function will only interpret <code>target</code> as a CSS selector
41+
and never as HTML, thereby preventing an XSS attack.
42+
</p>
43+
<sample src="examples/XssThroughDomFixed.js" />
44+
</example>
45+
46+
<references>
47+
<li>
48+
OWASP:
49+
<a href="https://cheatsheetseries.owasp.org/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.html">DOM based
50+
XSS Prevention Cheat Sheet</a>.
51+
</li>
52+
<li>
53+
OWASP:
54+
<a href="https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html">XSS
55+
(Cross Site Scripting) Prevention Cheat Sheet</a>.
56+
</li>
57+
<li>
58+
OWASP
59+
<a href="https://owasp.org/www-community/attacks/DOM_Based_XSS">DOM Based XSS</a>.
60+
</li>
61+
<li>
62+
OWASP
63+
<a href="https://owasp.org/www-community/Types_of_Cross-Site_Scripting">Types of Cross-Site
64+
Scripting</a>.
65+
</li>
66+
<li>
67+
Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
68+
</li>
69+
</references>
70+
</qhelp>

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* @name Cross-site scripting through DOM
3+
* @description Writing user-controlled DOM to HTML can allow for
4+
* a cross-site scripting vulnerability.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision medium
8+
* @id js/xss-through-dom
9+
* @tags security
10+
* external/cwe/cwe-079
11+
* external/cwe/cwe-116
12+
*/
13+
14+
import javascript
15+
import semmle.javascript.security.dataflow.XssThroughDom::XssThroughDom
16+
import DataFlow::PathGraph
17+
18+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
19+
where cfg.hasFlowPath(source, sink)
20+
select sink.getNode(), source, sink, "Cross-site scripting vulnerability due to $@.",
21+
source.getNode(), "DOM text"

‎javascript/ql/src/Security/CWE-079/examples/XssThroughDom.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
$("button").click(function () {
2+
var target = this.attr("data-target");
3+
$(target).hide();
4+
});

‎javascript/ql/src/Security/CWE-079/examples/XssThroughDomFixed.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
$("button").click(function () {
2+
var target = this.attr("data-target");
3+
$.find(target).hide();
4+
});

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

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import javascript
66
import semmle.javascript.frameworks.Templating
7+
private import semmle.javascript.dataflow.InferredTypes
78

89
module DOM {
910
/**
@@ -292,10 +293,18 @@ module DOM {
292293

293294
private class DefaultRange extends Range {
294295
DefaultRange() {
295-
this.asExpr().(VarAccess).getVariable() instanceof DOMGlobalVariable or
296-
this = domValueRef().getAPropertyRead() or
297-
this = domElementCreationOrQuery() or
296+
this.asExpr().(VarAccess).getVariable() instanceof DOMGlobalVariable
297+
or
298+
this = domValueRef().getAPropertyRead()
299+
or
300+
this = domElementCreationOrQuery()
301+
or
298302
this = domElementCollection()
303+
or
304+
exists(JQuery::MethodCall call | this = call and call.getMethodName() = "get" |
305+
call.getNumArgument() = 1 and
306+
forex(InferredType t | t = call.getArgument(0).analyze().getAType() | t = TTNumber())
307+
)
299308
}
300309
}
301310
}

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -538,9 +538,13 @@ module JQuery {
538538
MethodCall() {
539539
this = dollarCall() and name = "$"
540540
or
541-
this = dollar().getAMemberCall(name)
541+
this = ([dollar(), objectRef()]).getAMemberCall(name)
542542
or
543-
this = objectRef().getAMethodCall(name)
543+
// Handle basic dynamic method dispatch (e.g. `$element[html ? 'html' : 'text'](content)`)
544+
exists(DataFlow::PropRead read | read = this.getCalleeNode() |
545+
read.getBase().getALocalSource() = [dollar(), objectRef()] and
546+
read.getPropertyNameExpr().flow().mayHaveStringValue(name)
547+
)
544548
or
545549
// Handle contributed JQuery objects that aren't source nodes (usually parameter uses)
546550
getReceiver() = legacyObjectSource() and

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -413,3 +413,9 @@ module StoredXss {
413413

414414
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
415415
}
416+
417+
/** Provides classes and predicates for the XSS through DOM query. */
418+
module XssThroughDom {
419+
/** A data flow source for XSS through DOM vulnerabilities. */
420+
abstract class Source extends Shared::Source { }
421+
}

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

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about
3+
* cross-site scripting vulnerabilities through the DOM.
4+
*/
5+
6+
import javascript
7+
8+
/**
9+
* Classes and predicates for the XSS through DOM query.
10+
*/
11+
module XssThroughDom {
12+
import Xss::XssThroughDom
13+
private import semmle.javascript.security.dataflow.Xss::DomBasedXss as DomBasedXss
14+
private import semmle.javascript.dataflow.InferredTypes
15+
private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQuery
16+
17+
/**
18+
* A taint-tracking configuration for reasoning about XSS through the DOM.
19+
*/
20+
class Configuration extends TaintTracking::Configuration {
21+
Configuration() { this = "XssThroughDOM" }
22+
23+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
24+
25+
override predicate isSink(DataFlow::Node sink) { sink instanceof DomBasedXss::Sink }
26+
27+
override predicate isSanitizer(DataFlow::Node node) {
28+
super.isSanitizer(node) or
29+
node instanceof DomBasedXss::Sanitizer
30+
}
31+
32+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
33+
guard instanceof TypeTestGuard or
34+
guard instanceof UnsafeJQuery::PropertyPresenceSanitizer
35+
}
36+
}
37+
38+
/**
39+
* Gets an attribute name that could store user-controlled data.
40+
*
41+
* Attributes such as "id", "href", and "src" are often used as input to HTML.
42+
* However, they are either rarely controlable by a user, or already a sink for other XSS vulnerabilities.
43+
* Such attributes are therefore ignored.
44+
*/
45+
bindingset[result]
46+
string unsafeAttributeName() {
47+
result.regexpMatch("data-.*") or
48+
result.regexpMatch("aria-.*") or
49+
result = ["name", "value", "title", "alt"]
50+
}
51+
52+
/**
53+
* A source for text from the DOM from a JQuery method call.
54+
*/
55+
class JQueryTextSource extends Source, JQuery::MethodCall {
56+
JQueryTextSource() {
57+
(
58+
this.getMethodName() = ["text", "val"] and this.getNumArgument() = 0
59+
or
60+
this.getMethodName() = "attr" and
61+
this.getNumArgument() = 1 and
62+
forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and
63+
this.getArgument(0).mayHaveStringValue(unsafeAttributeName())
64+
) and
65+
// looks like a $("<p>" + ... ) source, which is benign for this query.
66+
not exists(DataFlow::Node prefix |
67+
DomBasedXss::isPrefixOfJQueryHtmlString(this
68+
.getReceiver()
69+
.(DataFlow::CallNode)
70+
.getAnArgument(), prefix)
71+
|
72+
prefix.getStringValue().regexpMatch("\\s*<.*")
73+
)
74+
}
75+
}
76+
77+
/**
78+
* A source for text from the DOM from a DOM property read or call to `getAttribute()`.
79+
*/
80+
class DOMTextSource extends Source {
81+
DOMTextSource() {
82+
exists(DataFlow::PropRead read | read = this |
83+
read.getBase().getALocalSource() = DOM::domValueRef() and
84+
exists(string propName | propName = ["innerText", "textContent", "value", "name"] |
85+
read.getPropertyName() = propName or
86+
read.getPropertyNameExpr().flow().mayHaveStringValue(propName)
87+
)
88+
)
89+
or
90+
exists(DataFlow::MethodCallNode mcn | mcn = this |
91+
mcn.getReceiver().getALocalSource() = DOM::domValueRef() and
92+
mcn.getMethodName() = "getAttribute" and
93+
mcn.getArgument(0).mayHaveStringValue(unsafeAttributeName())
94+
)
95+
}
96+
}
97+
98+
/**
99+
* A test of form `typeof x === "something"`, preventing `x` from being a string in some cases.
100+
*
101+
* This sanitizer helps prune infeasible paths in type-overloaded functions.
102+
*/
103+
class TypeTestGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
104+
override EqualityTest astNode;
105+
TypeofExpr typeof;
106+
boolean polarity;
107+
108+
TypeTestGuard() {
109+
astNode.getAnOperand() = typeof and
110+
(
111+
// typeof x === "string" sanitizes `x` when it evaluates to false
112+
astNode.getAnOperand().getStringValue() = "string" and
113+
polarity = astNode.getPolarity().booleanNot()
114+
or
115+
// typeof x === "object" sanitizes `x` when it evaluates to true
116+
astNode.getAnOperand().getStringValue() != "string" and
117+
polarity = astNode.getPolarity()
118+
)
119+
}
120+
121+
override predicate sanitizes(boolean outcome, Expr e) {
122+
polarity = outcome and
123+
e = typeof.getOperand()
124+
}
125+
}
126+
}

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

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
nodes
2+
| xss-through-dom.js:2:16:2:34 | $("textarea").val() |
3+
| xss-through-dom.js:2:16:2:34 | $("textarea").val() |
4+
| xss-through-dom.js:2:16:2:34 | $("textarea").val() |
5+
| xss-through-dom.js:4:16:4:40 | $(".som ... .text() |
6+
| xss-through-dom.js:4:16:4:40 | $(".som ... .text() |
7+
| xss-through-dom.js:4:16:4:40 | $(".som ... .text() |
8+
| xss-through-dom.js:8:16:8:53 | $(".som ... arget") |
9+
| xss-through-dom.js:8:16:8:53 | $(".som ... arget") |
10+
| xss-through-dom.js:8:16:8:53 | $(".som ... arget") |
11+
| xss-through-dom.js:11:3:11:42 | documen ... nerText |
12+
| xss-through-dom.js:11:3:11:42 | documen ... nerText |
13+
| xss-through-dom.js:11:3:11:42 | documen ... nerText |
14+
| xss-through-dom.js:19:3:19:44 | documen ... Content |
15+
| xss-through-dom.js:19:3:19:44 | documen ... Content |
16+
| xss-through-dom.js:19:3:19:44 | documen ... Content |
17+
| xss-through-dom.js:23:3:23:48 | documen ... ].value |
18+
| xss-through-dom.js:23:3:23:48 | documen ... ].value |
19+
| xss-through-dom.js:23:3:23:48 | documen ... ].value |
20+
| xss-through-dom.js:27:3:27:61 | documen ... arget') |
21+
| xss-through-dom.js:27:3:27:61 | documen ... arget') |
22+
| xss-through-dom.js:27:3:27:61 | documen ... arget') |
23+
| xss-through-dom.js:51:30:51:48 | $("textarea").val() |
24+
| xss-through-dom.js:51:30:51:48 | $("textarea").val() |
25+
| xss-through-dom.js:51:30:51:48 | $("textarea").val() |
26+
| xss-through-dom.js:54:31:54:49 | $("textarea").val() |
27+
| xss-through-dom.js:54:31:54:49 | $("textarea").val() |
28+
| xss-through-dom.js:54:31:54:49 | $("textarea").val() |
29+
| xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name |
30+
| xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name |
31+
| xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name |
32+
| xss-through-dom.js:57:30:57:67 | $("inpu ... "name") |
33+
| xss-through-dom.js:57:30:57:67 | $("inpu ... "name") |
34+
| xss-through-dom.js:57:30:57:67 | $("inpu ... "name") |
35+
| xss-through-dom.js:61:30:61:69 | $(docum ... value") |
36+
| xss-through-dom.js:61:30:61:69 | $(docum ... value") |
37+
| xss-through-dom.js:61:30:61:69 | $(docum ... value") |
38+
| xss-through-dom.js:64:30:64:40 | valMethod() |
39+
| xss-through-dom.js:64:30:64:40 | valMethod() |
40+
| xss-through-dom.js:64:30:64:40 | valMethod() |
41+
edges
42+
| xss-through-dom.js:2:16:2:34 | $("textarea").val() | xss-through-dom.js:2:16:2:34 | $("textarea").val() |
43+
| xss-through-dom.js:4:16:4:40 | $(".som ... .text() | xss-through-dom.js:4:16:4:40 | $(".som ... .text() |
44+
| xss-through-dom.js:8:16:8:53 | $(".som ... arget") | xss-through-dom.js:8:16:8:53 | $(".som ... arget") |
45+
| xss-through-dom.js:11:3:11:42 | documen ... nerText | xss-through-dom.js:11:3:11:42 | documen ... nerText |
46+
| xss-through-dom.js:19:3:19:44 | documen ... Content | xss-through-dom.js:19:3:19:44 | documen ... Content |
47+
| xss-through-dom.js:23:3:23:48 | documen ... ].value | xss-through-dom.js:23:3:23:48 | documen ... ].value |
48+
| xss-through-dom.js:27:3:27:61 | documen ... arget') | xss-through-dom.js:27:3:27:61 | documen ... arget') |
49+
| xss-through-dom.js:51:30:51:48 | $("textarea").val() | xss-through-dom.js:51:30:51:48 | $("textarea").val() |
50+
| xss-through-dom.js:54:31:54:49 | $("textarea").val() | xss-through-dom.js:54:31:54:49 | $("textarea").val() |
51+
| xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name | xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name |
52+
| xss-through-dom.js:57:30:57:67 | $("inpu ... "name") | xss-through-dom.js:57:30:57:67 | $("inpu ... "name") |
53+
| xss-through-dom.js:61:30:61:69 | $(docum ... value") | xss-through-dom.js:61:30:61:69 | $(docum ... value") |
54+
| xss-through-dom.js:64:30:64:40 | valMethod() | xss-through-dom.js:64:30:64:40 | valMethod() |
55+
#select
56+
| 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 |
57+
| 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 |
58+
| xss-through-dom.js:8:16:8:53 | $(".som ... arget") | xss-through-dom.js:8:16:8:53 | $(".som ... arget") | xss-through-dom.js:8:16:8:53 | $(".som ... arget") | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:8:16:8:53 | $(".som ... arget") | DOM text |
59+
| xss-through-dom.js:11:3:11:42 | documen ... nerText | xss-through-dom.js:11:3:11:42 | documen ... nerText | xss-through-dom.js:11:3:11:42 | documen ... nerText | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:11:3:11:42 | documen ... nerText | DOM text |
60+
| xss-through-dom.js:19:3:19:44 | documen ... Content | xss-through-dom.js:19:3:19:44 | documen ... Content | xss-through-dom.js:19:3:19:44 | documen ... Content | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:19:3:19:44 | documen ... Content | DOM text |
61+
| xss-through-dom.js:23:3:23:48 | documen ... ].value | xss-through-dom.js:23:3:23:48 | documen ... ].value | xss-through-dom.js:23:3:23:48 | documen ... ].value | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:23:3:23:48 | documen ... ].value | DOM text |
62+
| xss-through-dom.js:27:3:27:61 | documen ... arget') | xss-through-dom.js:27:3:27:61 | documen ... arget') | xss-through-dom.js:27:3:27:61 | documen ... arget') | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:27:3:27:61 | documen ... arget') | DOM text |
63+
| xss-through-dom.js:51:30:51:48 | $("textarea").val() | xss-through-dom.js:51:30:51:48 | $("textarea").val() | xss-through-dom.js:51:30:51:48 | $("textarea").val() | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:51:30:51:48 | $("textarea").val() | DOM text |
64+
| xss-through-dom.js:54:31:54:49 | $("textarea").val() | xss-through-dom.js:54:31:54:49 | $("textarea").val() | xss-through-dom.js:54:31:54:49 | $("textarea").val() | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:54:31:54:49 | $("textarea").val() | DOM text |
65+
| xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name | xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name | xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name | DOM text |
66+
| xss-through-dom.js:57:30:57:67 | $("inpu ... "name") | xss-through-dom.js:57:30:57:67 | $("inpu ... "name") | xss-through-dom.js:57:30:57:67 | $("inpu ... "name") | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:57:30:57:67 | $("inpu ... "name") | DOM text |
67+
| 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 |
68+
| 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 |

0 commit comments

Comments
 (0)