Skip to content

Commit fd77e27

Browse files
author
Stephan Brandauer
committed
replace taint tracking by type tracking and merge remaining queries for CWE-830
1 parent 8cafa6d commit fd77e27

9 files changed

Lines changed: 163 additions & 166 deletions

javascript/ql/src/Security/CWE-830/DynamicCreationOfUntrustedSourceUse.ql

Lines changed: 0 additions & 58 deletions
This file was deleted.

javascript/ql/src/Security/CWE-830/FunctionalityFromUntrustedSource.ql

Lines changed: 148 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -13,73 +13,169 @@
1313

1414
import javascript
1515
import semmle.javascript.HTML
16+
import semmle.javascript.dataflow.TaintTracking
1617

17-
bindingset[host]
18-
predicate isLocalhostPrefix(string host) {
19-
host.toLowerCase()
20-
.regexpMatch([
21-
"localhost(:[0-9]+)?/.*", "127.0.0.1(:[0-9]+)?/.*", "::1/.*", "\\[::1\\]:[0-9]+/.*"
22-
])
23-
}
18+
module Generic {
19+
/** A `CallNode` that creates an element of kind `name`. */
20+
predicate isCreateElementNode(DataFlow::CallNode call, string name) {
21+
call = DataFlow::globalVarRef("document").getAMethodCall("createElement") and
22+
call.getArgument(0).getStringValue().toLowerCase() = name
23+
}
2424

25-
/** A path that is vulnerable to a MITM attack. */
26-
bindingset[url]
27-
predicate isUntrustedSourceUrl(string url) {
28-
url.substring(0, 2) = "//"
29-
or
30-
exists(string hostPath | hostPath = url.regexpCapture("http://(.*)", 1) |
31-
not isLocalhostPrefix(hostPath)
32-
)
33-
}
25+
/**
26+
* True if `rhs` is being assigned to the `propName` property of an HTML
27+
* element created by `createElementCall`.
28+
*/
29+
predicate isPropWrite(DataFlow::CallNode createElementCall, string propName, DataFlow::Node rhs) {
30+
exists(DataFlow::PropWrite assignment |
31+
isCreateElementNode(createElementCall, _) and
32+
assignment.writes(createElementCall.getALocalUse(), propName, rhs)
33+
)
34+
}
3435

35-
/** A path that needs an integrity check — even with https. */
36-
bindingset[url]
37-
predicate isCdnUrlWithCheckingRequired(string url) {
38-
// Some CDN URLs are required to have an integrity attribute. We only add CDNs to that list
39-
// that recommend integrity-checking.
40-
url.regexpMatch([
41-
"^https?://code\\.jquery\\.com/.*\\.js$", "^https?://cdnjs\\.cloudflare\\.com/.*\\.js$",
42-
"^https?://cdnjs\\.com/.*\\.js$"
43-
])
44-
}
36+
/**
37+
* A `createElement` call that creates a `<script ../>` element which never
38+
* has its `integrity` attribute set locally.
39+
*/
40+
predicate isCreateScriptNodeWoIntegrityCheck(DataFlow::CallNode createCall) {
41+
isCreateElementNode(createCall, "script") and
42+
not exists(DataFlow::Node rhs | isPropWrite(createCall, "integrity", rhs))
43+
}
4544

46-
abstract class IncludesUntrustedContent extends HTML::Element {
47-
/** Gets an explanation why this source is untrusted. */
48-
abstract string getProblem();
45+
/** A location that adds a reference to an untrusted source. */
46+
abstract class AddsUntrustedUrl extends Locatable {
47+
/** Gets an explanation why this source is untrusted. */
48+
abstract string getProblem();
49+
}
4950
}
5051

51-
/** A script element that refers to untrusted content. */
52-
class ScriptElementWithUntrustedContent extends IncludesUntrustedContent, HTML::ScriptElement {
53-
ScriptElementWithUntrustedContent() {
54-
not exists(string digest | not digest = "" | this.getIntegrityDigest() = digest) and
55-
isUntrustedSourceUrl(this.getSourcePath())
52+
module StaticCreation {
53+
bindingset[host]
54+
predicate isLocalhostPrefix(string host) {
55+
host.toLowerCase()
56+
.regexpMatch([
57+
"localhost(:[0-9]+)?/.*", "127.0.0.1(:[0-9]+)?/.*", "::1/.*", "\\[::1\\]:[0-9]+/.*"
58+
])
5659
}
5760

58-
override string getProblem() {
59-
result = "script elements should use an HTTPS url and/or use the integrity attribute"
61+
/** A path that is vulnerable to a MITM attack. */
62+
bindingset[url]
63+
predicate isUntrustedSourceUrl(string url) {
64+
exists(string hostPath | hostPath = url.regexpCapture("http://(.*)", 1) |
65+
not isLocalhostPrefix(hostPath)
66+
)
67+
}
68+
69+
/** A path that needs an integrity check — even with https. */
70+
bindingset[url]
71+
predicate isCdnUrlWithCheckingRequired(string url) {
72+
// Some CDN URLs are required to have an integrity attribute. We only add CDNs to that list
73+
// that recommend integrity-checking.
74+
url.regexpMatch([
75+
"^https?://code\\.jquery\\.com/.*\\.js$", "^https?://cdnjs\\.cloudflare\\.com/.*\\.js$",
76+
"^https?://cdnjs\\.com/.*\\.js$"
77+
])
78+
}
79+
80+
/** A script element that refers to untrusted content. */
81+
class ScriptElementWithUntrustedContent extends Generic::AddsUntrustedUrl, HTML::ScriptElement {
82+
ScriptElementWithUntrustedContent() {
83+
not exists(string digest | not digest = "" | this.getIntegrityDigest() = digest) and
84+
isUntrustedSourceUrl(this.getSourcePath())
85+
}
86+
87+
override string getProblem() {
88+
result = "script elements should use an HTTPS url and/or use the integrity attribute"
89+
}
6090
}
61-
}
6291

63-
/** A script element that refers to untrusted content. */
64-
class CDNScriptElementWithUntrustedContent extends IncludesUntrustedContent, HTML::ScriptElement {
65-
CDNScriptElementWithUntrustedContent() {
66-
not exists(string digest | not digest = "" | this.getIntegrityDigest() = digest) and
67-
isCdnUrlWithCheckingRequired(this.getSourcePath())
92+
/** A script element that refers to untrusted content. */
93+
class CDNScriptElementWithUntrustedContent extends Generic::AddsUntrustedUrl, HTML::ScriptElement {
94+
CDNScriptElementWithUntrustedContent() {
95+
not exists(string digest | not digest = "" | this.getIntegrityDigest() = digest) and
96+
isCdnUrlWithCheckingRequired(this.getSourcePath())
97+
}
98+
99+
override string getProblem() {
100+
result =
101+
"script elements that depend on this CDN should use an HTTPS url and use the integrity attribute"
102+
}
68103
}
69104

70-
override string getProblem() {
71-
result =
72-
"script elements that depend on this CDN should use an HTTPS url and use the integrity attribute"
105+
/** An iframe element that includes untrusted content. */
106+
class IframeElementWithUntrustedContent extends HTML::IframeElement, Generic::AddsUntrustedUrl {
107+
IframeElementWithUntrustedContent() { isUntrustedSourceUrl(this.getSourcePath()) }
108+
109+
override string getProblem() { result = "iframe elements should use an HTTPS url" }
73110
}
74111
}
75112

76-
/** An iframe element that includes untrusted content. */
77-
class IframeElementWithUntrustedContent extends HTML::IframeElement, IncludesUntrustedContent {
78-
IframeElementWithUntrustedContent() { isUntrustedSourceUrl(this.getSourcePath()) }
113+
module DynamicCreation {
114+
import DataFlow::TypeTracker
115+
116+
predicate isUnsafeSourceLiteral(DataFlow::Node source) {
117+
exists(StringLiteral s | source = s.flow() |
118+
s.getValue().toLowerCase() = "http:" + any(string rest)
119+
)
120+
}
79121

80-
override string getProblem() { result = "iframe elements should use an HTTPS url" }
122+
DataFlow::Node urlTrackedFromUnsafeSourceLiteral(DataFlow::TypeTracker t) {
123+
t.start() and isUnsafeSourceLiteral(result)
124+
or
125+
exists(DataFlow::TypeTracker t2, DataFlow::Node prev |
126+
prev = urlTrackedFromUnsafeSourceLiteral(t2)
127+
|
128+
not exists(string httpsUrl | httpsUrl.toLowerCase() = "https:" + any(string rest) |
129+
// when the result may have a string value starting with https,
130+
// we're most likely with an assignment like:
131+
// e.src = ('https:' == document.location.protocol ? 'https://ssl' : 'http://www') + '.google-analytics.com/ga.js'
132+
// these assignments, we don't want to fix — once the browser is using http,
133+
// MITM attacks are possible anyway.
134+
result.mayHaveStringValue(httpsUrl)
135+
) and
136+
(
137+
t2 = t.smallstep(prev, result)
138+
or
139+
TaintTracking::sharedTaintStep(prev, result) and
140+
t = t2
141+
)
142+
)
143+
}
144+
145+
DataFlow::Node urlTrackedFromUnsafeSourceLiteral() {
146+
result = urlTrackedFromUnsafeSourceLiteral(DataFlow::TypeTracker::end())
147+
}
148+
149+
predicate isAssignedToSrcAttribute(string name, DataFlow::Node sink) {
150+
exists(DataFlow::CallNode createElementCall |
151+
name = "script" and
152+
Generic::isCreateScriptNodeWoIntegrityCheck(createElementCall) and
153+
Generic::isPropWrite(createElementCall, "src", sink)
154+
or
155+
name = "iframe" and
156+
Generic::isCreateElementNode(createElementCall, "iframe") and
157+
Generic::isPropWrite(createElementCall, "src", sink)
158+
)
159+
}
160+
161+
class IframeOrScriptSrcAssignment extends Expr, Generic::AddsUntrustedUrl {
162+
string name;
163+
164+
IframeOrScriptSrcAssignment() {
165+
exists(DataFlow::Node n | n.asExpr() = this |
166+
DynamicCreation::isAssignedToSrcAttribute(name, n) and
167+
n = DynamicCreation::urlTrackedFromUnsafeSourceLiteral()
168+
)
169+
}
170+
171+
override string getProblem() {
172+
name = "script" and
173+
result = "script elements should use an HTTPS url and/or use the integrity attribute"
174+
or
175+
name = "iframe" and result = "iframe elements should use an HTTPS url"
176+
}
177+
}
81178
}
82179

83-
from IncludesUntrustedContent s, string problem
84-
where problem = s.getProblem()
85-
select s, "HTML-element uses untrusted content (" + problem + ")"
180+
from Generic::AddsUntrustedUrl s
181+
select s, "HTML-element uses untrusted content (" + s.getProblem() + ")"

javascript/ql/src/change-notes/2022-02-14-dynamic-creation-of-untrusted-source-use.md

Lines changed: 0 additions & 6 deletions
This file was deleted.
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
---
22
category: newQuery
33
---
4-
* A new query, `js/functionality-from-untrusted-source`, has been added to the query suite. It finds HTML elements
4+
* A new query, `js/functionality-from-untrusted-source`, has been added to the query suite. It finds DOM elements
55
that load functionality from untrusted sources, like a `script`- or `iframe`-element using http-links.
66
The query is run by default.

javascript/ql/test/query-tests/Security/CWE-830/DynamicCreationOfUntrustedSourceUse.expected

Lines changed: 0 additions & 20 deletions
This file was deleted.

javascript/ql/test/query-tests/Security/CWE-830/DynamicCreationOfUntrustedSourceUse.html

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,15 @@
22
<head>
33
<script type="text/javascript">
44
(function() {
5-
// NOT OK (no integrity attribute)
5+
// OK (we accept this, as a http document location is vulnerable anyway)
66
var scrpt = document.createElement('script');
77
scrpt.type = 'text/javascript';
88
scrpt.src = ('https:' == document.location.protocol ? 'https://ssl' : 'http://www') + '.cdn.local/ga.js';
99

1010
// OK (integrity digest present)
1111
var scrpt2 = document.createElement('script');
1212
scrpt2.type = 'text/javascript';
13-
scrpt2.src = ('https:' == document.location.protocol ? 'https://ssl' : 'http://www') + '.cdn.local/ga.js';
13+
scrpt2.src = 'http://www.cdn.local/ga.js';
1414
scrpt2.integrity = 'sha256-h0UuK3mE9taiYlB5u9vT9A0s/XDgkfVd+F4VhN/sky=';
1515

1616
// NOT OK (http URL)
@@ -20,6 +20,13 @@
2020
// OK (https URL)
2121
var ifrm2 = document.createElement('iframe');
2222
ifrm2.src = 'https://www.example.com/';
23+
24+
// NOT OK (http URL tracked through calls)
25+
function getUrl(version) {
26+
return 'http://www.cdn.local/'+version+'/ga.js';
27+
}
28+
var ifrm3 = document.createElement('iframe');
29+
ifrm3.src = getUrl('v123');
2330
})();
2431
</script>
2532
</head>

javascript/ql/test/query-tests/Security/CWE-830/DynamicCreationOfUntrustedSourceUse.qlref

Lines changed: 0 additions & 1 deletion
This file was deleted.
Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
| FunctionalityFromUntrustedSource.html:6:9:6:56 | <script>...</> | HTML-element uses untrusted content (script elements should use an HTTPS url and/or use the integrity attribute) |
2-
| FunctionalityFromUntrustedSource.html:9:9:9:58 | <iframe>...</> | HTML-element uses untrusted content (iframe elements should use an HTTPS url) |
3-
| FunctionalityFromUntrustedSource.html:11:9:11:53 | <iframe>...</> | HTML-element uses untrusted content (iframe elements should use an HTTPS url) |
4-
| FunctionalityFromUntrustedSource.html:20:9:20:155 | <script>...</> | HTML-element uses untrusted content (script elements that depend on this CDN should use an HTTPS url and use the integrity attribute) |
1+
| DynamicCreationOfUntrustedSourceUse.html:18:26:18:50 | 'http:/ ... e.com/' | HTML-element uses untrusted content (iframe elements should use an HTTPS url) |
2+
| DynamicCreationOfUntrustedSourceUse.html:29:27:29:40 | getUrl('v123') | HTML-element uses untrusted content (iframe elements should use an HTTPS url) |
3+
| StaticCreationOfUntrustedSourceUse.html:6:9:6:56 | <script>...</> | HTML-element uses untrusted content (script elements should use an HTTPS url and/or use the integrity attribute) |
4+
| StaticCreationOfUntrustedSourceUse.html:9:9:9:58 | <iframe>...</> | HTML-element uses untrusted content (iframe elements should use an HTTPS url) |
5+
| StaticCreationOfUntrustedSourceUse.html:21:9:21:155 | <script>...</> | HTML-element uses untrusted content (script elements that depend on this CDN should use an HTTPS url and use the integrity attribute) |

javascript/ql/test/query-tests/Security/CWE-830/FunctionalityFromUntrustedSource.html

Lines changed: 0 additions & 22 deletions
This file was deleted.

0 commit comments

Comments
 (0)