Skip to content

Commit 2934aa1

Browse files
author
Stephan Brandauer
committed
rewrite docs, improve error messages, etc
1 parent d2335b6 commit 2934aa1

File tree

5 files changed

+104
-93
lines changed

5 files changed

+104
-93
lines changed

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

Lines changed: 42 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -4,62 +4,67 @@
44
<qhelp>
55
<overview>
66
<p>
7+
Including a resource from an untrusted source or using an untrusted channel may
8+
allow an attacker to include arbitrary code in the response.
9+
When including an external resource (eg., a <code>script</code> element or an
10+
<code>iframe</code> element) on a page, it is important to ensure that the received
11+
data is not malicious.
12+
</p>
713

8-
Including functionality from an external source via an <code>http</code> URL may
9-
allow an attacker to inject malicious code via a MITM (man-in-the-middle) attack.
14+
<p>
15+
When including external resources, it is possible to verify that the origin (the server
16+
that responds to the request) is the intended one by using an <code>https</code> URL.
17+
This prevents a MITM (man-in-the-middle) attack where an attacker might have been able
18+
to spoof a server response.
19+
</p>
1020

21+
<p>
22+
Even when <code>https</code> is used, an attacker might still compromise the origin server.
23+
When using a <code>script</code> element, checking for <em>subresource integrity</em>
24+
(checking the contents of the data received by supplying a cryptographic digest of the
25+
expected sources to the script element) is possible. The script will only load sources
26+
that match the digest and an attacker will be unable to modify the script even when the
27+
server is compromised.
1128
</p>
1229

30+
<p>
31+
Subresource integrity checking is commonly recommended when importing a fixed version of
32+
a library, eg., from a CDN (content-delivery network). Then, the fixed digest of that
33+
version of the library can easily be added to the <code>script</code> element's
34+
<code>integrity</code> attribute.
35+
</p>
1336
</overview>
1437

1538
<recommendation>
1639
<p>
17-
18-
When including external pages or behaviour, use <code>https</code> URLs
19-
to make sure you're getting the intended data, or users will be vulnerable
20-
to MITM attacks.
21-
40+
When an <code>iframe</code> element is used to embed a page, it is important to use a
41+
<code>https</code> URL.
2242
</p>
2343

2444
<p>
25-
26-
When including external behaviour in iframe or script elements, using
27-
<code>http</code> URLs is unsafe because the request sent may be intercepted
28-
by an attacker, and malicious data may be sent back in reply.
29-
45+
When using a <code>script</code> element to load a script, it is important to use a
46+
<code>https</code> URL and to consider checking subresource integrity.
3047
</p>
48+
</recommendation>
3149

32-
50+
<example>
3351
<p>
52+
The following sample loads the jQuery library from the jQuery CDN without using <code>https</code>
53+
and without checking subresource integrity.
54+
</p>
3455

35-
Even when <code>https</code> is used, an attacker might still compromise the
36-
server the page is receving data from.
37-
38-
When including scripts from a CDN (content-delivery network), it is therefore recommended
39-
to set the integrity-attribute on the script tag to the hash of the script you're expecting
40-
to receive.
41-
42-
This makes it impossible for an attacker to inject any code into the page, because the
43-
integrity check would fail &mdash; even when the CDN is compromised.
44-
45-
See the reference on Subresource Integrity for more information.
56+
<sample src="jquery-http-nocheck.html" />
4657

58+
<p>
59+
Instead, loading jQuery from the same domain using <code>https</code> and checking
60+
subresource integrity is recommended, as in the next sample.
4761
</p>
4862

49-
</recommendation>
63+
<sample src="jquery-https-check.html" />
64+
</example>
5065

5166
<references>
52-
53-
<li>
54-
55-
MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity">Subresource Integrity</a>
56-
57-
</li>
58-
59-
<li>
60-
61-
cwe.mitre.org: <a href="https://cwe.mitre.org/data/definitions/830.html">CWE 830</a>
62-
63-
</li>
67+
<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity">Subresource Integrity</a></li>
68+
<li>Smashing Magazine: <a href="https://www.smashingmagazine.com/2019/04/understanding-subresource-integrity/">Understanding Subresource Integrity</a></li>
6469
</references>
6570
</qhelp>

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

Lines changed: 39 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,26 @@
11
/**
2-
* @name Inclusion of untrusted functionality by a HTML element.
3-
* @description Including untrusted functionality by a HTML element
4-
* opens up for potential man-in-the-middle attacks.
2+
* @name Inclusion of functionality from untrusted source.
3+
* @description Including functionality from an untrusted source may allow
4+
* an attacker to control the functionality and execute arbitrary code.
55
* @kind problem
66
* @problem.severity warning
7-
* @security-severity 8.1
7+
* @security-severity 6.0
88
* @precision high
99
* @id js/functionality-from-untrusted-source
1010
* @tags security
1111
* external/cwe/cwe-830
1212
*/
1313

1414
import javascript
15-
import semmle.javascript.HTML
16-
import semmle.javascript.dataflow.TaintTracking
1715

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-
}
24-
25-
/**
26-
* A `createElement` call that creates a `<script ../>` element which never
27-
* has its `integrity` attribute set locally.
28-
*/
29-
predicate isCreateScriptNodeWoIntegrityCheck(DataFlow::CallNode createCall) {
30-
isCreateElementNode(createCall, "script") and
31-
not exists(createCall.getAPropertyWrite("integrity"))
32-
}
33-
34-
/** A location that adds a reference to an untrusted source. */
35-
abstract class AddsUntrustedUrl extends Locatable {
36-
/** Gets an explanation why this source is untrusted. */
37-
abstract string getProblem();
38-
}
16+
/** A location that adds a reference to an untrusted source. */
17+
abstract class AddsUntrustedUrl extends Locatable {
18+
/** Gets an explanation why this source is untrusted. */
19+
abstract string getProblem();
3920
}
4021

4122
module StaticCreation {
23+
/** Holds if `host` is an alias of localhost. */
4224
bindingset[host]
4325
predicate isLocalhostPrefix(string host) {
4426
host.toLowerCase()
@@ -47,15 +29,15 @@ module StaticCreation {
4729
])
4830
}
4931

50-
/** A path that is vulnerable to a MITM attack. */
32+
/** Holds if `url` is a url that is vulnerable to a MITM attack. */
5133
bindingset[url]
5234
predicate isUntrustedSourceUrl(string url) {
5335
exists(string hostPath | hostPath = url.regexpCapture("(?i)http://(.*)", 1) |
5436
not isLocalhostPrefix(hostPath)
5537
)
5638
}
5739

58-
/** A path that needs an integrity check - even with https. */
40+
/** Holds if `url` refers to a CDN that needs an integrity check - even with https. */
5941
bindingset[url]
6042
predicate isCdnUrlWithCheckingRequired(string url) {
6143
// Some CDN URLs are required to have an integrity attribute. We only add CDNs to that list
@@ -68,47 +50,56 @@ module StaticCreation {
6850
}
6951

7052
/** A script element that refers to untrusted content. */
71-
class ScriptElementWithUntrustedContent extends Generic::AddsUntrustedUrl, HTML::ScriptElement {
53+
class ScriptElementWithUntrustedContent extends AddsUntrustedUrl instanceof HTML::ScriptElement {
7254
ScriptElementWithUntrustedContent() {
73-
not exists(string digest | not digest = "" | this.getIntegrityDigest() = digest) and
74-
isUntrustedSourceUrl(this.getSourcePath())
55+
not exists(string digest | not digest = "" | super.getIntegrityDigest() = digest) and
56+
isUntrustedSourceUrl(super.getSourcePath())
7557
}
7658

7759
override string getProblem() {
78-
result = "script elements should use an 'https:' URL and/or use the integrity attribute"
60+
result = "HTML script element loaded using unencrypted connection."
7961
}
8062
}
8163

8264
/** A script element that refers to untrusted content. */
83-
class CDNScriptElementWithUntrustedContent extends Generic::AddsUntrustedUrl, HTML::ScriptElement {
65+
class CDNScriptElementWithUntrustedContent extends AddsUntrustedUrl, HTML::ScriptElement {
8466
CDNScriptElementWithUntrustedContent() {
8567
not exists(string digest | not digest = "" | this.getIntegrityDigest() = digest) and
8668
isCdnUrlWithCheckingRequired(this.getSourcePath())
8769
}
8870

8971
override string getProblem() {
9072
result =
91-
"script elements that depend on this CDN should use an 'https:' URL and use the integrity attribute"
73+
"Script loaded from content delivery network with no integrity check."
9274
}
9375
}
9476

9577
/** An iframe element that includes untrusted content. */
96-
class IframeElementWithUntrustedContent extends HTML::IframeElement, Generic::AddsUntrustedUrl {
97-
IframeElementWithUntrustedContent() { isUntrustedSourceUrl(this.getSourcePath()) }
78+
class IframeElementWithUntrustedContent extends AddsUntrustedUrl instanceof HTML::IframeElement {
79+
IframeElementWithUntrustedContent() { isUntrustedSourceUrl(super.getSourcePath()) }
9880

99-
override string getProblem() { result = "iframe elements should use an 'https:' URL" }
81+
override string getProblem() { result = "HTML iframe element loaded using unencrypted connection." }
10082
}
10183
}
10284

10385
module DynamicCreation {
104-
import DataFlow::TypeTracker
86+
/** Holds if `call` creates a tag of kind `name`. */
87+
predicate isCreateElementNode(DataFlow::CallNode call, string name) {
88+
call = DataFlow::globalVarRef("document").getAMethodCall("createElement") and
89+
call.getArgument(0).getStringValue().toLowerCase() = name
90+
}
10591

106-
predicate isUnsafeSourceLiteral(DataFlow::Node source) {
107-
exists(StringLiteral s | source = s.flow() | s.getValue().regexpMatch("(?i)http:.*"))
92+
/**
93+
* Holds if `createCall` creates a `<script ../>` element which never
94+
* has its `integrity` attribute set locally.
95+
*/
96+
predicate isCreateScriptNodeWoIntegrityCheck(DataFlow::CallNode createCall) {
97+
isCreateElementNode(createCall, "script") and
98+
not exists(createCall.getAPropertyWrite("integrity"))
10899
}
109100

110101
DataFlow::Node urlTrackedFromUnsafeSourceLiteral(DataFlow::TypeTracker t) {
111-
t.start() and isUnsafeSourceLiteral(result)
102+
t.start() and result.getStringValue().regexpMatch("(?i)http:.*")
112103
or
113104
exists(DataFlow::TypeTracker t2, DataFlow::Node prev |
114105
prev = urlTrackedFromUnsafeSourceLiteral(t2)
@@ -137,16 +128,16 @@ module DynamicCreation {
137128
predicate isAssignedToSrcAttribute(string name, DataFlow::Node sink) {
138129
exists(DataFlow::CallNode createElementCall |
139130
name = "script" and
140-
Generic::isCreateScriptNodeWoIntegrityCheck(createElementCall) and
131+
isCreateScriptNodeWoIntegrityCheck(createElementCall) and
141132
sink = createElementCall.getAPropertyWrite("src").getRhs()
142133
or
143134
name = "iframe" and
144-
Generic::isCreateElementNode(createElementCall, "iframe") and
135+
isCreateElementNode(createElementCall, "iframe") and
145136
sink = createElementCall.getAPropertyWrite("src").getRhs()
146137
)
147138
}
148139

149-
class IframeOrScriptSrcAssignment extends Expr, Generic::AddsUntrustedUrl {
140+
class IframeOrScriptSrcAssignment extends AddsUntrustedUrl {
150141
string name;
151142

152143
IframeOrScriptSrcAssignment() {
@@ -157,13 +148,10 @@ module DynamicCreation {
157148
}
158149

159150
override string getProblem() {
160-
name = "script" and
161-
result = "script elements should use an 'https:' URL and/or use the integrity attribute"
162-
or
163-
name = "iframe" and result = "iframe elements should use an 'https:' URL"
151+
result = "HTML " + name + " element loaded using unencrypted connection."
164152
}
165153
}
166154
}
167155

168-
from Generic::AddsUntrustedUrl s
169-
select s, "HTML-element uses untrusted content (" + s.getProblem() + ")"
156+
from AddsUntrustedUrl s
157+
select s, s.getProblem()

‎javascript/ql/src/Security/CWE-830/jquery-http-nocheck.html

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<html>
2+
<head>
3+
<title>jQuery demo</title>
4+
<script src="http://code.jquery.com/jquery-3.6.0.slim.min.js" crossorigin="anonymous"></script>
5+
</head>
6+
<body>
7+
...
8+
</body>
9+
</html>

‎javascript/ql/src/Security/CWE-830/jquery-https-check.html

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<html>
2+
<head>
3+
<title>jQuery demo</title>
4+
<script src="https://code.jquery.com/jquery-3.6.0.slim.min.js" integrity="sha256-u7e5khyithlIdTpu22PHhENmPcRdFiHRjhAuHcs05RI=" crossorigin="anonymous"></script>
5+
</head>
6+
<body>
7+
...
8+
</body>
9+
</html>

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
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) |
1+
| DynamicCreationOfUntrustedSourceUse.html:18:26:18:50 | 'http:/ ... e.com/' | HTML iframe element loaded using unencrypted connection. |
2+
| DynamicCreationOfUntrustedSourceUse.html:29:27:29:40 | getUrl('v123') | HTML iframe element loaded using unencrypted connection. |
3+
| StaticCreationOfUntrustedSourceUse.html:6:9:6:56 | <script>...</> | HTML script element loaded using unencrypted connection. |
4+
| StaticCreationOfUntrustedSourceUse.html:9:9:9:58 | <iframe>...</> | HTML iframe element loaded using unencrypted connection. |
5+
| StaticCreationOfUntrustedSourceUse.html:21:9:21:155 | <script>...</> | Script loaded from content delivery network with no integrity check. |

0 commit comments

Comments
 (0)