Skip to content

Commit dc6f60a

Browse files
atorralbaatorralba
committed
Add new XXE query
Only XMLParser sinks for the time being
1 parent 83caf01 commit dc6f60a

File tree

12 files changed

+273
-0
lines changed

12 files changed

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

‎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, 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 Morgen:
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>.
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: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
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, "XML parser with enabled external entities depends on $@.",
24+
source.getNode(), "user input"

‎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-611/XXETest.expected‎

Whitespace-only changes.

0 commit comments

Comments
 (0)