diff --git a/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll b/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll index 28e783c80477..4d5635258301 100644 --- a/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll +++ b/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll @@ -79,6 +79,8 @@ private import internal.FlowSummaryImplSpecific */ private module Frameworks { private import codeql.swift.frameworks.StandardLibrary.CustomUrlSchemes + private import codeql.swift.frameworks.StandardLibrary.Data + private import codeql.swift.frameworks.StandardLibrary.InputStream private import codeql.swift.frameworks.StandardLibrary.String private import codeql.swift.frameworks.StandardLibrary.Url private import codeql.swift.frameworks.StandardLibrary.UrlSession diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Data.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Data.qll new file mode 100644 index 000000000000..740fc8874b6f --- /dev/null +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Data.qll @@ -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" } +} diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/InputStream.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/InputStream.qll new file mode 100644 index 000000000000..60b61bec294a --- /dev/null +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/InputStream.qll @@ -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" + } +} diff --git a/swift/ql/lib/codeql/swift/security/XXE.qll b/swift/ql/lib/codeql/swift/security/XXE.qll new file mode 100644 index 000000000000..506225f54a1b --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/XXE.qll @@ -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" } +} diff --git a/swift/ql/lib/codeql/swift/security/XXEQuery.qll b/swift/ql/lib/codeql/swift/security/XXEQuery.qll new file mode 100644 index 000000000000..2b5dc5ea3eaa --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/XXEQuery.qll @@ -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) + } +} diff --git a/swift/ql/src/queries/Security/CWE-611/XXE.qhelp b/swift/ql/src/queries/Security/CWE-611/XXE.qhelp new file mode 100644 index 000000000000..a4efe604c39b --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-611/XXE.qhelp @@ -0,0 +1,57 @@ + + + + +

+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. +

+ + + +

+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 XMLParser, disable entity expansion by default, +so unless you have explicitly enabled entity expansion, no further action needs to be taken. +

+ + + +

+The following example uses the XMLParser class to parse a string data. +If that string is from an untrusted source, this code may be vulnerable to an XXE attack, since +the parser is also setting its shouldResolveExternalEntities option to true: +

+ + +

+To guard against XXE attacks, the shouldResolveExternalEntities option should be +left unset or explicitly set to false. +

+ + + + + +
  • +OWASP: +XML External Entity (XXE) Processing. +
    • +
  • +OWASP: +XML External Entity Prevention Cheat Sheet. +
    • +
  • +Timothy D. Morgan and Omar Al Ibrahim +XML Schema, DTD, and Entity Attacks: A Compendium of Known Techniques. +
    • +
  • +Timur Yunusov, Alexey Osipov: +XML Out-Of-Band Data Retrieval. +
    • +     +     diff --git a/swift/ql/src/queries/Security/CWE-611/XXE.ql b/swift/ql/src/queries/Security/CWE-611/XXE.ql new file mode 100644 index 000000000000..04c27949c11b --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-611/XXE.ql @@ -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" diff --git a/swift/ql/src/queries/Security/CWE-611/XXEBad.swift b/swift/ql/src/queries/Security/CWE-611/XXEBad.swift new file mode 100644 index 000000000000..19ff4a7383d0 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-611/XXEBad.swift @@ -0,0 +1,2 @@ +let parser = XMLParser(data: remoteData) // BAD (parser explicitly enables external entities) +parser.shouldResolveExternalEntities = true diff --git a/swift/ql/src/queries/Security/CWE-611/XXEGood.swift b/swift/ql/src/queries/Security/CWE-611/XXEGood.swift new file mode 100644 index 000000000000..5552186c7591 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-611/XXEGood.swift @@ -0,0 +1,2 @@ +let parser = XMLParser(data: remoteData) // GOOD (parser explicitly disables external entities) +parser.shouldResolveExternalEntities = false diff --git a/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected b/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected index de02d0db461f..5580af94d229 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected +++ b/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected @@ -1,4 +1,8 @@ edges +| 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(_:) : | testSend.swift:37:19:37:19 | data2 | +| testSend.swift:33:19:33:19 | passwordPlain : | testSend.swift:5:5:5:29 | [summary param] 0 in init(_:) : | +| testSend.swift:33:19:33:19 | passwordPlain : | testSend.swift:33:14:33:32 | call to init(_:) : | | testSend.swift:41:10:41:18 | data : | testSend.swift:41:45:41:45 | data : | | testSend.swift:45:13:45:13 | password : | testSend.swift:52:27:52:27 | str1 | | testSend.swift:46:13:46:13 | password : | testSend.swift:53:27:53:27 | str2 | @@ -8,7 +12,12 @@ edges | testURL.swift:13:54:13:54 | passwd : | testURL.swift:13:22:13:54 | ... .+(_:_:) ... | | testURL.swift:16:55:16:55 | credit_card_no : | testURL.swift:16:22:16:55 | ... .+(_:_:) ... | nodes +| file://:0:0:0:0 | [summary] to write: return (return) in init(_:) : | semmle.label | [summary] to write: return (return) in init(_:) : | +| testSend.swift:5:5:5:29 | [summary param] 0 in init(_:) : | semmle.label | [summary param] 0 in init(_:) : | | testSend.swift:29:19:29:19 | passwordPlain | semmle.label | passwordPlain | +| testSend.swift:33:14:33:32 | call to init(_:) : | semmle.label | call to init(_:) : | +| testSend.swift:33:19:33:19 | passwordPlain : | semmle.label | passwordPlain : | +| testSend.swift:37:19:37:19 | data2 | semmle.label | data2 | | testSend.swift:41:10:41:18 | data : | semmle.label | data : | | testSend.swift:41:45:41:45 | data : | semmle.label | data : | | testSend.swift:45:13:45:13 | password : | semmle.label | password : | @@ -24,9 +33,11 @@ nodes | testURL.swift:16:55:16:55 | credit_card_no : | semmle.label | credit_card_no : | | testURL.swift:20:22:20:22 | passwd | semmle.label | passwd | subpaths +| 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(_:) : | | 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(_:) : | #select | 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 | +| 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 | | 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 | | 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 | | 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 | diff --git a/swift/ql/test/query-tests/Security/CWE-311/testSend.swift b/swift/ql/test/query-tests/Security/CWE-311/testSend.swift index 506b3a921e3b..aaf2e3487f24 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/testSend.swift +++ b/swift/ql/test/query-tests/Security/CWE-311/testSend.swift @@ -34,7 +34,7 @@ func test1(passwordPlain : String, passwordHash : String) { let data3 = Data(passwordHash) nw.send(content: data1, completion: .idempotent) // GOOD (not sensitive) - nw.send(content: data2, completion: .idempotent) // BAD [NOT DETECTED] + nw.send(content: data2, completion: .idempotent) // BAD nw.send(content: data3, completion: .idempotent) // GOOD (not sensitive) } diff --git a/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected b/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/swift/ql/test/query-tests/Security/CWE-611/XXETest.ql b/swift/ql/test/query-tests/Security/CWE-611/XXETest.ql new file mode 100644 index 000000000000..817f55678ad2 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-611/XXETest.ql @@ -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() + ) + } +} diff --git a/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift b/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift new file mode 100644 index 000000000000..75538f014f9f --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift @@ -0,0 +1,92 @@ +// --- stubs --- + +class Data { + init(_ 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 +} + +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 +} + +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 +} \ No newline at end of file