-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Swift: Add new query for XML External Entities (XML) vulnerabilities #11086
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
dc6f60a
Add new XXE query
atorralba f4047e0
Address QL-for-QL alert
atorralba 0c6957e
Adjust test expectations of a query affected by new summaries
atorralba fe138dc
Add explicitly safe test cases
atorralba 3e1819f
Model XMLParser constructor init(contentsOf:)
atorralba da67b10
Remove (now unnecessary) import
atorralba eef4fc3
Apply suggestions from code review
atorralba File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 changes: 6 additions & 0 deletions
6
swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Data.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| import swift | ||
| private import codeql.swift.dataflow.ExternalFlow | ||
|
|
||
| private class DataSummaries extends SummaryModelCsv { | ||
| override predicate row(string row) { row = ";Data;true;init(_:);;;Argument[0];ReturnValue;taint" } | ||
| } | ||
8 changes: 8 additions & 0 deletions
8
swift/ql/lib/codeql/swift/frameworks/StandardLibrary/InputStream.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import swift | ||
| private import codeql.swift.dataflow.ExternalFlow | ||
|
|
||
| private class InputStreamSummaries extends SummaryModelCsv { | ||
| override predicate row(string row) { | ||
| row = ";InputStream;true;init(data:);;;Argument[0];ReturnValue;taint" | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| /** Provides classes and predicates to reason about XML external entities (XXE) vulnerabilities. */ | ||
|
|
||
| import swift | ||
| private import codeql.swift.dataflow.DataFlow | ||
|
|
||
| /** A data flow sink for XML external entities (XXE) vulnerabilities. */ | ||
| abstract class XxeSink extends DataFlow::Node { } | ||
|
|
||
| /** A sanitizer for XML external entities (XXE) vulnerabilities. */ | ||
| abstract class XxeSanitizer extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A unit class for adding additional taint steps. | ||
| * | ||
| * Extend this class to add additional taint steps that should apply to paths related to | ||
| * XML external entities (XXE) vulnerabilities. | ||
| */ | ||
| class XxeAdditionalTaintStep extends Unit { | ||
| abstract predicate step(DataFlow::Node n1, DataFlow::Node n2); | ||
| } | ||
|
|
||
| /** The XML argument of a `XMLParser` vulnerable to XXE. */ | ||
| private class DefaultXxeSink extends XxeSink { | ||
| DefaultXxeSink() { | ||
| this.asExpr() = any(Argument a | a.getApplyExpr() instanceof VulnerableParser).getExpr() | ||
| } | ||
| } | ||
|
|
||
| /** The construction of a `XMLParser` that enables external entities. */ | ||
| private class VulnerableParser extends CallExpr { | ||
| VulnerableParser() { | ||
| resolvesExternalEntities(this) and this.getFunction() instanceof ConstructorRefCallExpr | ||
| } | ||
| } | ||
|
|
||
| /** Holds if there is an access of `ref` that sets `shouldResolveExternalEntities` to `true`. */ | ||
| private predicate resolvesExternalEntities(XmlParserRef ref) { | ||
| exists(XmlParserRef base | | ||
| DataFlow::localExprFlow(ref, base) or DataFlow::localExprFlow(base, ref) | ||
| | | ||
| exists(AssignExpr assign, ShouldResolveExternalEntities s, BooleanLiteralExpr b | | ||
| s.getBase() = base and | ||
| assign.getDest() = s and | ||
| b.getValue() = true and | ||
| DataFlow::localExprFlow(b, assign.getSource()) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| /** A reference to the field `XMLParser.shouldResolveExternalEntities`. */ | ||
| private class ShouldResolveExternalEntities extends MemberRefExpr { | ||
| ShouldResolveExternalEntities() { | ||
| this.getMember().(FieldDecl).getName() = "shouldResolveExternalEntities" and | ||
| this.getBase() instanceof XmlParserRef | ||
| } | ||
| } | ||
|
|
||
| /** An expression of type `XMLParser`. */ | ||
| private class XmlParserRef extends Expr { | ||
| XmlParserRef() { | ||
| this.getType() instanceof XmlParserType or | ||
| this.getType() = any(OptionalType t | t.getBaseType() instanceof XmlParserType) | ||
| } | ||
| } | ||
|
|
||
| /** The type `XMLParser`. */ | ||
| private class XmlParserType extends NominalType { | ||
| XmlParserType() { this.getFullName() = "XMLParser" } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| /** | ||
| * Provides a taint-tracking configuration for reasoning about XML external entities | ||
| * (XXE) vulnerabilities. | ||
| */ | ||
|
|
||
| import swift | ||
| import codeql.swift.dataflow.DataFlow | ||
| import codeql.swift.dataflow.FlowSources | ||
| import codeql.swift.dataflow.TaintTracking | ||
| import codeql.swift.security.XXE | ||
|
|
||
| /** | ||
| * A taint-tracking configuration for XML external entities (XXE) vulnerabilities. | ||
| */ | ||
| class XxeConfiguration extends TaintTracking::Configuration { | ||
| XxeConfiguration() { this = "XxeConfiguration" } | ||
|
|
||
| override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } | ||
|
|
||
| override predicate isSink(DataFlow::Node sink) { sink instanceof XxeSink } | ||
|
|
||
| override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof XxeSanitizer } | ||
|
|
||
| override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) { | ||
| any(XxeAdditionalTaintStep s).step(n1, n2) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| <!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
| <overview> | ||
| <p> | ||
| Parsing untrusted XML files with a weakly configured XML parser may lead to an XML External Entity (XXE) attack. This type of attack | ||
| uses external entity references to access arbitrary files on a system, carry out denial-of-service attacks, or server-side | ||
| request forgery. Even when the result of parsing is not returned to the user, out-of-band | ||
| data retrieval techniques may allow attackers to steal sensitive data. Denial of services can also be | ||
| carried out in this situation. | ||
| </p> | ||
| </overview> | ||
|
|
||
| <recommendation> | ||
| <p> | ||
| The easiest way to prevent XXE attacks is to disable external entity handling when | ||
| parsing untrusted data. How this is done depends on the library being used. Note that some | ||
| libraries, such as recent versions of <code>XMLParser</code>, disable entity expansion by default, | ||
| so unless you have explicitly enabled entity expansion, no further action needs to be taken. | ||
| </p> | ||
| </recommendation> | ||
|
|
||
| <example> | ||
| <p> | ||
| The following example uses the <code>XMLParser</code> class to parse a string <code>data</code>. | ||
| If that string is from an untrusted source, this code may be vulnerable to an XXE attack, since | ||
| the parser is also setting its <code>shouldResolveExternalEntities</code> option to <code>true</code>: | ||
| </p> | ||
| <sample src="XXEBad.swift" /> | ||
|
|
||
| <p> | ||
| To guard against XXE attacks, the <code>shouldResolveExternalEntities</code> option should be | ||
| left unset or explicitly set to <code>false</code>. | ||
| </p> | ||
| <sample src="XXEGood.swift" /> | ||
|
|
||
| </example> | ||
|
|
||
| <references> | ||
| <li> | ||
| OWASP: | ||
| <a href="https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing">XML External Entity (XXE) Processing</a>. | ||
| </li> | ||
| <li> | ||
| OWASP: | ||
| <a href="https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html">XML External Entity Prevention Cheat Sheet</a>. | ||
| </li> | ||
| <li> | ||
| Timothy D. Morgan and Omar Al Ibrahim | ||
| <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>. | ||
| </li> | ||
| <li> | ||
| Timur Yunusov, Alexey Osipov: | ||
| <a href="https://www.slideshare.net/qqlan/bh-ready-v4">XML Out-Of-Band Data Retrieval</a>. | ||
| </li> | ||
| </references> | ||
| </qhelp> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| /** | ||
| * @name Resolving XML external entity in user-controlled data | ||
| * @description Parsing user-controlled XML documents and allowing expansion of external entity | ||
| * references may lead to disclosure of confidential data or denial of service. | ||
| * @kind path-problem | ||
| * @problem.severity error | ||
| * @security-severity 9.1 | ||
| * @precision high | ||
| * @id swift/xxe | ||
| * @tags security | ||
| * external/cwe/cwe-611 | ||
| * external/cwe/cwe-776 | ||
| * external/cwe/cwe-827 | ||
| */ | ||
|
|
||
| import swift | ||
| import codeql.swift.dataflow.DataFlow | ||
| import codeql.swift.security.XXEQuery | ||
| import DataFlow::PathGraph | ||
|
|
||
| from DataFlow::PathNode source, DataFlow::PathNode sink | ||
| where any(XxeConfiguration c).hasFlowPath(source, sink) | ||
| select sink.getNode(), source, sink, | ||
| "XML parsing depends on a $@ without guarding against external entity expansion.", | ||
| source.getNode(), "user-provided value" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| let parser = XMLParser(data: remoteData) // BAD (parser explicitly enables external entities) | ||
| parser.shouldResolveExternalEntities = true |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| let parser = XMLParser(data: remoteData) // GOOD (parser explicitly disables external entities) | ||
| parser.shouldResolveExternalEntities = false |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import swift | ||
| import codeql.swift.security.XXEQuery | ||
| import TestUtilities.InlineExpectationsTest | ||
|
|
||
| class XxeTest extends InlineExpectationsTest { | ||
| XxeTest() { this = "XxeTest" } | ||
|
|
||
| override string getARelevantTag() { result = "hasXXE" } | ||
|
|
||
| override predicate hasActualResult(Location location, string element, string tag, string value) { | ||
| exists(XxeConfiguration config, DataFlow::Node source, DataFlow::Node sink, Expr sinkExpr | | ||
| config.hasFlow(source, sink) and | ||
| sinkExpr = sink.asExpr() and | ||
| location = sinkExpr.getLocation() and | ||
| element = sinkExpr.toString() and | ||
| tag = "hasXXE" and | ||
| value = source.asExpr().getLocation().getStartLine().toString() | ||
| ) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| // --- stubs --- | ||
|
|
||
| class Data { | ||
| init<S>(_ elements: S) {} | ||
| } | ||
|
|
||
| struct URL { | ||
| init?(string: String) {} | ||
| } | ||
|
|
||
| class InputStream { | ||
| init(data: Data) {} | ||
| } | ||
|
|
||
| extension String { | ||
| init(contentsOf: URL) { | ||
| let data = "" | ||
| self.init(data) | ||
| } | ||
| } | ||
|
|
||
| class XMLParser { | ||
| var shouldResolveExternalEntities: Bool { get { return false } set {} } | ||
| init?(contentsOf: URL) {} | ||
| init(data: Data) {} | ||
| init(stream: InputStream) {} | ||
| } | ||
|
|
||
| // --- tests --- | ||
|
|
||
| func testData() { | ||
| let remoteString = String(contentsOf: URL(string: "http://example.com/")!) | ||
| let remoteData = Data(remoteString) | ||
| let parser = XMLParser(data: remoteData) // $ hasXXE=32 | ||
| parser.shouldResolveExternalEntities = true | ||
| } | ||
atorralba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| func testInputStream() { | ||
| let remoteString = String(contentsOf: URL(string: "http://example.com/")!) | ||
| let remoteData = Data(remoteString) | ||
| let remoteStream = InputStream(data: remoteData) | ||
| let parser = XMLParser(stream: remoteStream) // $ hasXXE=39 | ||
| parser.shouldResolveExternalEntities = true | ||
| } | ||
|
|
||
| func testUrl() { | ||
| let remoteString = String(contentsOf: URL(string: "http://example.com/")!) | ||
| let remoteUrl = URL(string: remoteString)! | ||
| let parser = XMLParser(contentsOf: remoteUrl) // $ hasXXE=47 | ||
| parser?.shouldResolveExternalEntities = true | ||
| } | ||
|
|
||
| func testDataSafe() { | ||
| let remoteString = String(contentsOf: URL(string: "http://example.com/")!) | ||
| let remoteData = Data(remoteString) | ||
| let _ = XMLParser(data: remoteData) // NO XXE: parser doesn't enable external entities | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| func testDataSafeExplicit() { | ||
| let remoteString = String(contentsOf: URL(string: "http://example.com/")!) | ||
| let remoteData = Data(remoteString) | ||
| let parser = XMLParser(data: remoteData) // NO XXE: parser disables external entities | ||
| parser.shouldResolveExternalEntities = false | ||
| } | ||
|
|
||
| func testInputStreamSafe() { | ||
| let remoteString = String(contentsOf: URL(string: "http://example.com/")!) | ||
| let remoteData = Data(remoteString) | ||
| let remoteStream = InputStream(data: remoteData) | ||
| let _ = XMLParser(stream: remoteStream) // NO XXE: parser doesn't enable external entities | ||
| } | ||
|
|
||
| func testInputStreamSafeExplicit() { | ||
| let remoteString = String(contentsOf: URL(string: "http://example.com/")!) | ||
| let remoteData = Data(remoteString) | ||
| let remoteStream = InputStream(data: remoteData) | ||
| let parser = XMLParser(stream: remoteStream) // NO XXE: parser disables external entities | ||
| parser.shouldResolveExternalEntities = false | ||
| } | ||
|
|
||
| func testUrlSafe() { | ||
| let remoteString = String(contentsOf: URL(string: "http://example.com/")!) | ||
| let remoteUrl = URL(string: remoteString)! | ||
| let _ = XMLParser(contentsOf: remoteUrl) // NO XXE: parser doesn't enable external entities | ||
| } | ||
|
|
||
| func testUrlSafeExplicit() { | ||
| let remoteString = String(contentsOf: URL(string: "http://example.com/")!) | ||
| let remoteUrl = URL(string: remoteString)! | ||
| let parser = XMLParser(contentsOf: remoteUrl) // NO XXE: parser disables external entities | ||
| parser?.shouldResolveExternalEntities = false | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.