Skip to content

Commit a21db3b

Browse files
atorralbaatorralba
authored
Merge pull request github#11086 from atorralba/atorralba/swift/xxe-query
Swift: Add new query for XML External Entities (XML) vulnerabilities
2 parents 3afd895 + eef4fc3 commit a21db3b

File tree

14 files changed

+322
-1
lines changed

14 files changed

+322
-1
lines changed

‎swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,8 @@ private import internal.FlowSummaryImplSpecific
7979
*/
8080
private module Frameworks {
8181
private import codeql.swift.frameworks.StandardLibrary.CustomUrlSchemes
82+
private import codeql.swift.frameworks.StandardLibrary.Data
83+
private import codeql.swift.frameworks.StandardLibrary.InputStream
8284
private import codeql.swift.frameworks.StandardLibrary.String
8385
private import codeql.swift.frameworks.StandardLibrary.Url
8486
private import codeql.swift.frameworks.StandardLibrary.UrlSession

‎swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Data.qll

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import swift
2+
private import codeql.swift.dataflow.ExternalFlow
3+
4+
private class DataSummaries extends SummaryModelCsv {
5+
override predicate row(string row) { row = ";Data;true;init(_:);;;Argument[0];ReturnValue;taint" }
6+
}

‎swift/ql/lib/codeql/swift/frameworks/StandardLibrary/InputStream.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import swift
2+
private import codeql.swift.dataflow.ExternalFlow
3+
4+
private class InputStreamSummaries extends SummaryModelCsv {
5+
override predicate row(string row) {
6+
row = ";InputStream;true;init(data:);;;Argument[0];ReturnValue;taint"
7+
}
8+
}

‎swift/ql/lib/codeql/swift/security/XXE.qll

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
/** Provides classes and predicates to reason about XML external entities (XXE) vulnerabilities. */
2+
3+
import swift
4+
private import codeql.swift.dataflow.DataFlow
5+
6+
/** A data flow sink for XML external entities (XXE) vulnerabilities. */
7+
abstract class XxeSink extends DataFlow::Node { }
8+
9+
/** A sanitizer for XML external entities (XXE) vulnerabilities. */
10+
abstract class XxeSanitizer extends DataFlow::Node { }
11+
12+
/**
13+
* A unit class for adding additional taint steps.
14+
*
15+
* Extend this class to add additional taint steps that should apply to paths related to
16+
* XML external entities (XXE) vulnerabilities.
17+
*/
18+
class XxeAdditionalTaintStep extends Unit {
19+
abstract predicate step(DataFlow::Node n1, DataFlow::Node n2);
20+
}
21+
22+
/** The XML argument of a `XMLParser` vulnerable to XXE. */
23+
private class DefaultXxeSink extends XxeSink {
24+
DefaultXxeSink() {
25+
this.asExpr() = any(Argument a | a.getApplyExpr() instanceof VulnerableParser).getExpr()
26+
}
27+
}
28+
29+
/** The construction of a `XMLParser` that enables external entities. */
30+
private class VulnerableParser extends CallExpr {
31+
VulnerableParser() {
32+
resolvesExternalEntities(this) and this.getFunction() instanceof ConstructorRefCallExpr
33+
}
34+
}
35+
36+
/** Holds if there is an access of `ref` that sets `shouldResolveExternalEntities` to `true`. */
37+
private predicate resolvesExternalEntities(XmlParserRef ref) {
38+
exists(XmlParserRef base |
39+
DataFlow::localExprFlow(ref, base) or DataFlow::localExprFlow(base, ref)
40+
|
41+
exists(AssignExpr assign, ShouldResolveExternalEntities s, BooleanLiteralExpr b |
42+
s.getBase() = base and
43+
assign.getDest() = s and
44+
b.getValue() = true and
45+
DataFlow::localExprFlow(b, assign.getSource())
46+
)
47+
)
48+
}
49+
50+
/** A reference to the field `XMLParser.shouldResolveExternalEntities`. */
51+
private class ShouldResolveExternalEntities extends MemberRefExpr {
52+
ShouldResolveExternalEntities() {
53+
this.getMember().(FieldDecl).getName() = "shouldResolveExternalEntities" and
54+
this.getBase() instanceof XmlParserRef
55+
}
56+
}
57+
58+
/** An expression of type `XMLParser`. */
59+
private class XmlParserRef extends Expr {
60+
XmlParserRef() {
61+
this.getType() instanceof XmlParserType or
62+
this.getType() = any(OptionalType t | t.getBaseType() instanceof XmlParserType)
63+
}
64+
}
65+
66+
/** The type `XMLParser`. */
67+
private class XmlParserType extends NominalType {
68+
XmlParserType() { this.getFullName() = "XMLParser" }
69+
}

‎swift/ql/lib/codeql/swift/security/XXEQuery.qll

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about XML external entities
3+
* (XXE) vulnerabilities.
4+
*/
5+
6+
import swift
7+
import codeql.swift.dataflow.DataFlow
8+
import codeql.swift.dataflow.FlowSources
9+
import codeql.swift.dataflow.TaintTracking
10+
import codeql.swift.security.XXE
11+
12+
/**
13+
* A taint-tracking configuration for XML external entities (XXE) vulnerabilities.
14+
*/
15+
class XxeConfiguration extends TaintTracking::Configuration {
16+
XxeConfiguration() { this = "XxeConfiguration" }
17+
18+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
19+
20+
override predicate isSink(DataFlow::Node sink) { sink instanceof XxeSink }
21+
22+
override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof XxeSanitizer }
23+
24+
override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
25+
any(XxeAdditionalTaintStep s).step(n1, n2)
26+
}
27+
}

‎swift/ql/src/queries/Security/CWE-611/XXE.qhelp

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>
6+
Parsing untrusted XML files with a weakly configured XML parser may lead to an XML External Entity (XXE) attack. This type of attack
7+
uses external entity references to access arbitrary files on a system, carry out denial-of-service attacks, or server-side
8+
request forgery. Even when the result of parsing is not returned to the user, out-of-band
9+
data retrieval techniques may allow attackers to steal sensitive data. Denial of services can also be
10+
carried out in this situation.
11+
</p>
12+
</overview>
13+
14+
<recommendation>
15+
<p>
16+
The easiest way to prevent XXE attacks is to disable external entity handling when
17+
parsing untrusted data. How this is done depends on the library being used. Note that some
18+
libraries, such as recent versions of <code>XMLParser</code>, disable entity expansion by default,
19+
so unless you have explicitly enabled entity expansion, no further action needs to be taken.
20+
</p>
21+
</recommendation>
22+
23+
<example>
24+
<p>
25+
The following example uses the <code>XMLParser</code> class to parse a string <code>data</code>.
26+
If that string is from an untrusted source, this code may be vulnerable to an XXE attack, since
27+
the parser is also setting its <code>shouldResolveExternalEntities</code> option to <code>true</code>:
28+
</p>
29+
<sample src="XXEBad.swift" />
30+
31+
<p>
32+
To guard against XXE attacks, the <code>shouldResolveExternalEntities</code> option should be
33+
left unset or explicitly set to <code>false</code>.
34+
</p>
35+
<sample src="XXEGood.swift" />
36+
37+
</example>
38+
39+
<references>
40+
<li>
41+
OWASP:
42+
<a href="https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing">XML External Entity (XXE) Processing</a>.
43+
</li>
44+
<li>
45+
OWASP:
46+
<a href="https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html">XML External Entity Prevention Cheat Sheet</a>.
47+
</li>
48+
<li>
49+
Timothy D. Morgan and Omar Al Ibrahim
50+
<a href="https://research.nccgroup.com/2014/05/19/xml-schema-dtd-and-entity-attacks-a-compendium-of-known-techniques/">XML Schema, DTD, and Entity Attacks: A Compendium of Known Techniques</a>.
51+
</li>
52+
<li>
53+
Timur Yunusov, Alexey Osipov:
54+
<a href="https://www.slideshare.net/qqlan/bh-ready-v4">XML Out-Of-Band Data Retrieval</a>.
55+
</li>
56+
</references>
57+
</qhelp>

‎swift/ql/src/queries/Security/CWE-611/XXE.ql

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/**
2+
* @name Resolving XML external entity in user-controlled data
3+
* @description Parsing user-controlled XML documents and allowing expansion of external entity
4+
* references may lead to disclosure of confidential data or denial of service.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 9.1
8+
* @precision high
9+
* @id swift/xxe
10+
* @tags security
11+
* external/cwe/cwe-611
12+
* external/cwe/cwe-776
13+
* external/cwe/cwe-827
14+
*/
15+
16+
import swift
17+
import codeql.swift.dataflow.DataFlow
18+
import codeql.swift.security.XXEQuery
19+
import DataFlow::PathGraph
20+
21+
from DataFlow::PathNode source, DataFlow::PathNode sink
22+
where any(XxeConfiguration c).hasFlowPath(source, sink)
23+
select sink.getNode(), source, sink,
24+
"XML parsing depends on a $@ without guarding against external entity expansion.",
25+
source.getNode(), "user-provided value"

‎swift/ql/src/queries/Security/CWE-611/XXEBad.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
let parser = XMLParser(data: remoteData) // BAD (parser explicitly enables external entities)
2+
parser.shouldResolveExternalEntities = true

‎swift/ql/src/queries/Security/CWE-611/XXEGood.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
let parser = XMLParser(data: remoteData) // GOOD (parser explicitly disables external entities)
2+
parser.shouldResolveExternalEntities = false

‎swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
edges
2+
| testSend.swift:5:5:5:29 | [summary param] 0 in init(_:) : | file://:0:0:0:0 | [summary] to write: return (return) in init(_:) : |
3+
| testSend.swift:33:14:33:32 | call to init(_:) : | testSend.swift:37:19:37:19 | data2 |
4+
| testSend.swift:33:19:33:19 | passwordPlain : | testSend.swift:5:5:5:29 | [summary param] 0 in init(_:) : |
5+
| testSend.swift:33:19:33:19 | passwordPlain : | testSend.swift:33:14:33:32 | call to init(_:) : |
26
| testSend.swift:41:10:41:18 | data : | testSend.swift:41:45:41:45 | data : |
37
| testSend.swift:45:13:45:13 | password : | testSend.swift:52:27:52:27 | str1 |
48
| testSend.swift:46:13:46:13 | password : | testSend.swift:53:27:53:27 | str2 |
@@ -8,7 +12,12 @@ edges
812
| testURL.swift:13:54:13:54 | passwd : | testURL.swift:13:22:13:54 | ... .+(_:_:) ... |
913
| testURL.swift:16:55:16:55 | credit_card_no : | testURL.swift:16:22:16:55 | ... .+(_:_:) ... |
1014
nodes
15+
| file://:0:0:0:0 | [summary] to write: return (return) in init(_:) : | semmle.label | [summary] to write: return (return) in init(_:) : |
16+
| testSend.swift:5:5:5:29 | [summary param] 0 in init(_:) : | semmle.label | [summary param] 0 in init(_:) : |
1117
| testSend.swift:29:19:29:19 | passwordPlain | semmle.label | passwordPlain |
18+
| testSend.swift:33:14:33:32 | call to init(_:) : | semmle.label | call to init(_:) : |
19+
| testSend.swift:33:19:33:19 | passwordPlain : | semmle.label | passwordPlain : |
20+
| testSend.swift:37:19:37:19 | data2 | semmle.label | data2 |
1221
| testSend.swift:41:10:41:18 | data : | semmle.label | data : |
1322
| testSend.swift:41:45:41:45 | data : | semmle.label | data : |
1423
| testSend.swift:45:13:45:13 | password : | semmle.label | password : |
@@ -24,9 +33,11 @@ nodes
2433
| testURL.swift:16:55:16:55 | credit_card_no : | semmle.label | credit_card_no : |
2534
| testURL.swift:20:22:20:22 | passwd | semmle.label | passwd |
2635
subpaths
36+
| testSend.swift:33:19:33:19 | passwordPlain : | testSend.swift:5:5:5:29 | [summary param] 0 in init(_:) : | file://:0:0:0:0 | [summary] to write: return (return) in init(_:) : | testSend.swift:33:14:33:32 | call to init(_:) : |
2737
| testSend.swift:47:17:47:17 | password : | testSend.swift:41:10:41:18 | data : | testSend.swift:41:45:41:45 | data : | testSend.swift:47:13:47:25 | call to pad(_:) : |
2838
#select
2939
| testSend.swift:29:19:29:19 | passwordPlain | testSend.swift:29:19:29:19 | passwordPlain | testSend.swift:29:19:29:19 | passwordPlain | This operation transmits 'passwordPlain', which may contain unencrypted sensitive data from $@. | testSend.swift:29:19:29:19 | passwordPlain | passwordPlain |
40+
| testSend.swift:37:19:37:19 | data2 | testSend.swift:33:19:33:19 | passwordPlain : | testSend.swift:37:19:37:19 | data2 | This operation transmits 'data2', which may contain unencrypted sensitive data from $@. | testSend.swift:33:19:33:19 | passwordPlain : | passwordPlain |
3041
| testSend.swift:52:27:52:27 | str1 | testSend.swift:45:13:45:13 | password : | testSend.swift:52:27:52:27 | str1 | This operation transmits 'str1', which may contain unencrypted sensitive data from $@. | testSend.swift:45:13:45:13 | password : | password |
3142
| testSend.swift:53:27:53:27 | str2 | testSend.swift:46:13:46:13 | password : | testSend.swift:53:27:53:27 | str2 | This operation transmits 'str2', which may contain unencrypted sensitive data from $@. | testSend.swift:46:13:46:13 | password : | password |
3243
| testSend.swift:54:27:54:27 | str3 | testSend.swift:47:17:47:17 | password : | testSend.swift:54:27:54:27 | str3 | This operation transmits 'str3', which may contain unencrypted sensitive data from $@. | testSend.swift:47:17:47:17 | password : | password |

0 commit comments

Comments
 (0)