Skip to content

Commit a3d17a1

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#5769 from erik-krogh/libXss
Approved by esbena
2 parents 097b6e5 + d913668 commit a3d17a1

20 files changed

+727
-125
lines changed

‎javascript/change-notes/2021-04-26-unsafe-html-construction.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
lgtm,codescanning
2+
* A new query, `js/html-constructed-from-input`, has been added to the query suite,
3+
highlighting libraries that may leave clients vulnerable to cross-site-scripting attacks.

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

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
When a library function dynamically constructs HTML in a potentially unsafe
8+
way, then it's important to document to clients of the library that the function
9+
should only be used with trusted inputs.
10+
11+
If the function is not documented as being potentially unsafe, then a client
12+
may inadvertently use inputs containing unsafe HTML fragments, and thereby leave
13+
the client vulnerable to cross-site scripting attacks.
14+
</p>
15+
</overview>
16+
<recommendation>
17+
18+
<p>
19+
Document all library functions that can lead to cross-site scripting
20+
attacks, and guard against unsafe inputs where dynamic HTML
21+
construction is not intended.
22+
</p>
23+
</recommendation>
24+
<example>
25+
26+
<p>
27+
The following example has a library function that renders a boldface name
28+
by writing to the <code>innerHTML</code> property of an element.
29+
</p>
30+
31+
<sample src="examples/unsafe-html-construction.js" />
32+
33+
<p>
34+
This library function, however, does not escape unsafe HTML, and a client
35+
that calls the function with user-supplied input may be vulnerable to
36+
cross-site scripting attacks.
37+
</p>
38+
39+
<p>
40+
The library could either document that this function should not be used
41+
with unsafe inputs, or use safe APIs such as <code>innerText</code>.
42+
</p>
43+
44+
<sample src="examples/unsafe-html-construction_safe.js" />
45+
46+
<p>
47+
Alternatively, a HTML sanitizer can be used to remove unsafe content.
48+
</p>
49+
50+
<sample src="examples/unsafe-html-construction_sanitizer.js" />
51+
52+
</example>
53+
<references>
54+
<li>
55+
OWASP:
56+
<a href="https://www.owasp.org/index.php/DOM_based_XSS_Prevention_Cheat_Sheet">DOM based
57+
XSS Prevention Cheat Sheet</a>.
58+
</li>
59+
<li>
60+
OWASP:
61+
<a href="https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet">XSS
62+
(Cross Site Scripting) Prevention Cheat Sheet</a>.
63+
</li>
64+
<li>
65+
OWASP
66+
<a href="https://www.owasp.org/index.php/DOM_Based_XSS">DOM Based XSS</a>.
67+
</li>
68+
<li>
69+
OWASP
70+
<a href="https://www.owasp.org/index.php/Types_of_Cross-Site_Scripting">Types of Cross-Site
71+
Scripting</a>.
72+
</li>
73+
<li>
74+
Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
75+
</li>
76+
</references>
77+
</qhelp>

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name Unsafe HTML constructed from library input
3+
* @description Using externally controlled strings to construct HTML might allow a malicious
4+
* user to perform a cross-site scripting attack.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id js/html-constructed-from-input
9+
* @tags security
10+
* external/cwe/cwe-079
11+
* external/cwe/cwe-116
12+
*/
13+
14+
import javascript
15+
import DataFlow::PathGraph
16+
import semmle.javascript.security.dataflow.UnsafeHtmlConstruction::UnsafeHtmlConstruction
17+
18+
from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
19+
where cfg.hasFlowPath(source, sink) and sink.getNode() = sinkNode
20+
select sinkNode, source, sink, "$@ based on $@ might later cause $@.", sinkNode,
21+
sinkNode.describe(), source.getNode(), "library input", sinkNode.getSink(),
22+
sinkNode.getVulnerabilityKind().toLowerCase()

‎javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
module.exports = function showBoldName(name) {
2+
document.getElementById('name').innerHTML = "<b>" + name + "</b>";
3+
}

‎javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_safe.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
module.exports = function showBoldName(name) {
2+
const bold = document.createElement('b');
3+
bold.innerText = name;
4+
document.getElementById('name').appendChild(bold);
5+
}

‎javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_sanitizer.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
2+
const striptags = require('striptags');
3+
module.exports = function showBoldName(name) {
4+
document.getElementById('name').innerHTML = "<b>" + striptags(name) + "</b>";
5+
}

‎javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2068,3 +2068,14 @@ class VarAccessBarrier extends DataFlow::Node {
20682068
)
20692069
}
20702070
}
2071+
2072+
/**
2073+
* Holds if there is a path without unmatched return steps from `source` to `sink`.
2074+
*/
2075+
predicate hasPathWithoutUnmatchedReturn(SourcePathNode source, SinkPathNode sink) {
2076+
exists(MidPathNode mid |
2077+
source.getASuccessor*() = mid and
2078+
sink = mid.getASuccessor() and
2079+
mid.getPathSummary().hasReturn() = false
2080+
)
2081+
}

0 commit comments

Comments
 (0)