hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-17 01:03:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #3408 from esbena/js/unsafe-html-expansion

Browse Source 
Approved by asgerf, mchammer01
This commit is contained in:
semmle-qlci
2020-05-15 08:24:12 +01:00
committed by GitHub
parent c06680a496 c6fa88af28
commit a536069059
9 changed files with 216 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
change-notes/1.25/analysis-javascript.md
View File

@@ -15,6 +15,7 @@
|---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| 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. |
| Incomplete HTML attribute sanitization (`js/incomplete-html-attribute-sanitization`) | security, external/cwe/cwe-20, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities due to incomplete sanitization of HTML meta-characters. Results are shown on LGTM by default. |
| Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. |
## Changes to existing queries

99
javascript/ql/src/Security/CWE-116/UnsafeHtmlExpansion.qhelp Normal file
View File

@@ -0,0 +1,99 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
Sanitizing untrusted input for HTML meta-characters is an
important technique for preventing cross-site scripting attacks. But
even a sanitized input can be dangerous to use if it is modified
further before a browser treats it as HTML.
A seemingly innocent transformation that expands a
self-closing HTML tag from <code>&lt;div attr="{sanitized}"/&gt;</code>
to <code>&lt;div attr="{sanitized}"&gt;&lt;/div&gt;</code> may
in fact cause cross-site scripting vulnerabilities.
</p>
</overview>
<recommendation>
<p>
Use a well-tested sanitization library if at all
possible, and avoid modifying sanitized values further before treating
them as HTML.
</p>
</recommendation>
<example>
<p>
The following function transforms a self-closing HTML tag
to a pair of open/close tags. It does so for all non-<code>img</code>
and non-<code>area</code> tags, by using a regular expression with two
capture groups. The first capture group corresponds to the name of the
tag, and the second capture group to the content of the tag.
</p>
<sample src="examples/UnsafeHtmlExpansion.js" />
<p>
While it is generally known regular expressions are
ill-suited for parsing HTML, variants of this particular transformation
pattern have long been considered safe.
</p>
<p>
However, the function is not safe. As an example, consider
the following string:
</p>
<sample src="examples/UnsafeHtmlExpansion-original.html" />
<p>
When the above function transforms the string, it becomes
a string that results in an alert when a browser treats it as HTML.
</p>
<sample src="examples/UnsafeHtmlExpansion-transformed.html" />
</example>
<references>
<li>jQuery:
<a href="https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/">Security fixes in jQuery 3.5.0</a>
</li>
<li>
OWASP:
<a href="https://cheatsheetseries.owasp.org/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.html">DOM based
XSS Prevention Cheat Sheet</a>.
</li>
<li>
OWASP:
<a href="https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html">XSS
(Cross Site Scripting) Prevention Cheat Sheet</a>.
</li>
<li>
OWASP
<a href="https://owasp.org/www-community/Types_of_Cross-Site_Scripting">Types of Cross-Site</a>.
</li>
<li>
Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
</li>
</references>
</qhelp>

58
javascript/ql/src/Security/CWE-116/UnsafeHtmlExpansion.ql Normal file
View File

@@ -0,0 +1,58 @@
/**
* @name Unsafe expansion of self-closing HTML tag
* @description Using regular expressions to expand self-closing HTML
* tags may lead to cross-site scripting vulnerabilities.
* @kind problem
* @problem.severity warning
* @precision very-high
* @id js/unsafe-html-expansion
* @tags correctness
* security
* external/cwe/cwe-079
* external/cwe/cwe-116
*/
import javascript
/**
* A regular expression that captures the name and content of a
* self-closing HTML tag such as `<div id='foo'/>`.
*/
class SelfClosingTagRecognizer extends DataFlow::RegExpCreationNode {
SelfClosingTagRecognizer() {
exists(RegExpSequence seq, RegExpGroup name, RegExpGroup content |
// `/.../g`
RegExp::isGlobal(this.getFlags()) and
this.getRoot() = seq.getRootTerm() and
// `/<.../`
seq.getChild(0).getConstantValue() = "<" and
// `/...\/>/`
seq.getLastChild().getPredecessor().getConstantValue() = "/" and
seq.getLastChild().getConstantValue() = ">" and
// `/...((...)...).../`
seq.getAChild() = content and
content.getNumber() = 1 and
name.getNumber() = 2 and
name = content.getChild(0).(RegExpSequence).getChild(0) and
// `/...(([a-z]+)...).../` or `/...(([a-z][...]*)...).../`
exists(RegExpQuantifier quant | name.getAChild*() = quant |
quant instanceof RegExpStar or
quant instanceof RegExpPlus
) and
// `/...((...)[^>]*).../`
exists(RegExpCharacterClass lazy |
name.getSuccessor().(RegExpStar).getChild(0) = lazy and
lazy.isInverted() and
lazy.getAChild().getConstantValue() = ">"
)
)
}
}
from SelfClosingTagRecognizer regexp, StringReplaceCall replace
where
regexp.getAReference().flowsTo(replace.getArgument(0)) and
replace.getRawReplacement().mayHaveStringValue("<$1></$2>")
select replace,
"This self-closing HTML tag expansion invalidates prior sanitization as $@ may match part of an attribute value.",
regexp, "this regular expression"

3
javascript/ql/src/Security/CWE-116/examples/UnsafeHtmlExpansion-original.html Normal file
View File

@@ -0,0 +1,3 @@
<div alt="
<x" title="/>
<img src=url404 onerror=alert(1)>"/>

3
javascript/ql/src/Security/CWE-116/examples/UnsafeHtmlExpansion-transformed.html Normal file
View File

@@ -0,0 +1,3 @@
<img alt="
<x" title="></x" >
<img src=url404 onerror=alert(1)>"/>

4
javascript/ql/src/Security/CWE-116/examples/UnsafeHtmlExpansion.js Normal file
View File

@@ -0,0 +1,4 @@
function expandSelfClosingTags(html) {
var rxhtmlTag = /<(?!img|area)(([a-z][^\w\/>]*)[^>]*)\/>/gi;
return html.replace(rxhtmlTag, "<$1></$2>"); // BAD
}

8
javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/UnsafeHtmlExpansion.expected Normal file
View File

@@ -0,0 +1,8 @@
| UnsafeHtmlExpansion.js:6:2:9:2 | html.re ... nded\\n\\t) | This self-closing HTML tag expansion invalidates prior sanitization as $@ may match part of an attribute value. | UnsafeHtmlExpansion.js:7:3:7:95 | /<(?!ar ... )\\/>/gi | this regular expression |
| UnsafeHtmlExpansion.js:10:2:10:68 | html.re ... panded) | This self-closing HTML tag expansion invalidates prior sanitization as $@ may match part of an attribute value. | UnsafeHtmlExpansion.js:10:15:10:57 | /<(([a- ... )\\/>/gi | this regular expression |
| UnsafeHtmlExpansion.js:13:2:16:2 | html.re ... nded\\n\\t) | This self-closing HTML tag expansion invalidates prior sanitization as $@ may match part of an attribute value. | UnsafeHtmlExpansion.js:14:3:14:75 | /<(?!ar ... )\\/>/gi | this regular expression |
| UnsafeHtmlExpansion.js:17:2:17:48 | html.re ... panded) | This self-closing HTML tag expansion invalidates prior sanitization as $@ may match part of an attribute value. | UnsafeHtmlExpansion.js:17:15:17:37 | /<(([\\w ... )\\/>/gi | this regular expression |
| UnsafeHtmlExpansion.js:20:2:23:2 | html.re ... nded\\n\\t) | This self-closing HTML tag expansion invalidates prior sanitization as $@ may match part of an attribute value. | UnsafeHtmlExpansion.js:21:3:21:76 | /<(?!ar ... )\\/>/gi | this regular expression |
| UnsafeHtmlExpansion.js:24:2:24:49 | html.re ... panded) | This self-closing HTML tag expansion invalidates prior sanitization as $@ may match part of an attribute value. | UnsafeHtmlExpansion.js:24:15:24:38 | /<(([\\w ... )\\/>/gi | this regular expression |
| UnsafeHtmlExpansion.js:26:2:26:39 | html.re ... panded) | This self-closing HTML tag expansion invalidates prior sanitization as $@ may match part of an attribute value. | UnsafeHtmlExpansion.js:2:23:2:45 | /<(([\\w ... )\\/>/gi | this regular expression |
| UnsafeHtmlExpansion.js:30:2:30:37 | html.re ... panded) | This self-closing HTML tag expansion invalidates prior sanitization as $@ may match part of an attribute value. | UnsafeHtmlExpansion.js:2:23:2:45 | /<(([\\w ... )\\/>/gi | this regular expression |

39
javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/UnsafeHtmlExpansion.js Normal file
View File

@@ -0,0 +1,39 @@
(function(){
let defaultPattern = /<(([\w:]+)[^>]*)\/>/gi;
let expanded = "<$1></$2>";
// lib1
html.replace(
/<(?!area|br|col|embed|hr|img|input|link|meta|param)(([a-z][^\/\0>\x20\t\r\n\f]*)[^>]*)\/>/gi,
expanded
); // NOT OK
html.replace(/<(([a-z][^\/\0>\x20\t\r\n\f]*)[^>]*)\/>/gi, expanded); // NOT OK
// lib2
html.replace(
/<(?!area|br|col|embed|hr|img|input|link|meta|param)(([\w:]+)[^>]*)\/>/gi,
expanded
); // NOT OK
html.replace(/<(([\w:]+)[^>]*)\/>/gi, expanded); // NOT OK
// lib3
html.replace(
/<(?!area|br|col|embed|hr|img|input|link|meta|param)(([\w:-]+)[^>]*)\/>/gi,
expanded
); // NOT OK
html.replace(/<(([\w:-]+)[^>]*)\/>/gi, expanded); // NOT OK
html.replace(defaultPattern, expanded); // NOT OK
function getPattern() {
return defaultPattern;
}
html.replace(getPattern(), expanded); // NOT OK
function getExpanded() {
return expanded;
}
html.replace(defaultPattern, getExpanded()); // NOT OK (but not tracking the expansion string)
html.replace(defaultPattern, something); // OK (possibly)
defaultPattern.match(something); // OK (possibly)
getPattern().match(something); // OK (possibly)
});

1
javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/UnsafeHtmlExpansion.qlref Normal file
View File

@@ -0,0 +1 @@
Security/CWE-116/UnsafeHtmlExpansion.ql