From da14f428e225d1dde770049f4a16ad0cb96d37ec Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 13 Oct 2023 13:57:54 +0100 Subject: [PATCH 1/7] Swift: Remove now redundant additional taint step. from the XXE query. --- swift/ql/lib/codeql/swift/security/XXEExtensions.qll | 7 ------- 1 file changed, 7 deletions(-) diff --git a/swift/ql/lib/codeql/swift/security/XXEExtensions.qll b/swift/ql/lib/codeql/swift/security/XXEExtensions.qll index a1f7780a3b1..863a54098a4 100644 --- a/swift/ql/lib/codeql/swift/security/XXEExtensions.qll +++ b/swift/ql/lib/codeql/swift/security/XXEExtensions.qll @@ -188,13 +188,6 @@ private predicate lib2xmlOptionLocalTaintStep(DataFlow::Node source, DataFlow::N exists(MemberRefExpr rawValue | rawValue.getMember().(VarDecl).getName() = "rawValue" | source.asExpr() = rawValue.getBase() and sink.asExpr() = rawValue ) - or - exists(ApplyExpr int32Init | - int32Init.getStaticTarget().(Initializer).getEnclosingDecl().asNominalTypeDecl().getName() = - "SignedInteger" - | - source.asExpr() = int32Init.getAnArgument().getExpr() and sink.asExpr() = int32Init - ) } /** From 9e473ebda4a595609f050e5cc49b2d270714fabd Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 13 Oct 2023 14:02:15 +0100 Subject: [PATCH 2/7] Swift: Remove the 'rawValue' step as well. --- .../codeql/swift/security/XXEExtensions.qll | 4 ---- .../Security/CWE-611/XXETest.expected | 18 +++++++++++++++++- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/swift/ql/lib/codeql/swift/security/XXEExtensions.qll b/swift/ql/lib/codeql/swift/security/XXEExtensions.qll index 863a54098a4..5bef17d032f 100644 --- a/swift/ql/lib/codeql/swift/security/XXEExtensions.qll +++ b/swift/ql/lib/codeql/swift/security/XXEExtensions.qll @@ -184,10 +184,6 @@ private class Libxml2XxeSink extends XxeSink { */ private predicate lib2xmlOptionLocalTaintStep(DataFlow::Node source, DataFlow::Node sink) { TaintTracking::localTaintStep(source, sink) - or - exists(MemberRefExpr rawValue | rawValue.getMember().(VarDecl).getName() = "rawValue" | - source.asExpr() = rawValue.getBase() and sink.asExpr() = rawValue - ) } /** diff --git a/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected b/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected index 48de9172b36..8cbc8d2c929 100644 --- a/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected +++ b/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected @@ -1,2 +1,18 @@ -failures testFailures +| testLibxmlXXE.swift:101:78:102:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:102:80:103:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:103:107:104:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:104:82:105:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:106:78:107:1 | // $ hasXXE=95\n | Missing result:hasXXE=95 | +| testLibxmlXXE.swift:107:80:108:1 | // $ hasXXE=95\n | Missing result:hasXXE=95 | +| testLibxmlXXE.swift:109:87:110:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:110:89:111:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:112:99:113:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:113:97:114:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:115:87:116:1 | // $ hasXXE=95\n | Missing result:hasXXE=95 | +| testLibxmlXXE.swift:116:89:117:1 | // $ hasXXE=95\n | Missing result:hasXXE=95 | +| testLibxmlXXE.swift:118:89:119:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:119:91:120:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:121:98:122:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +| testLibxmlXXE.swift:122:100:123:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | +failures From 228aaee0bfefe1c65c3e9107befc38d7351fb802 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 13 Oct 2023 14:34:05 +0100 Subject: [PATCH 3/7] Swift: Add data flow tests for RawRepresentable, OptionSet. --- .../dataflow/taint/libraries/optionset.swift | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 swift/ql/test/library-tests/dataflow/taint/libraries/optionset.swift diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/optionset.swift b/swift/ql/test/library-tests/dataflow/taint/libraries/optionset.swift new file mode 100644 index 00000000000..262764dbca6 --- /dev/null +++ b/swift/ql/test/library-tests/dataflow/taint/libraries/optionset.swift @@ -0,0 +1,66 @@ + +// --- stubs --- + +// --- tests --- + +func sourceInt() -> Int { return 0 } +func sourceUInt() -> UInt { return 0 } +func sink(arg: Any) {} + +// --- + +enum MyRawRepresentable : RawRepresentable { + case valueOne + case valueTwo + + init?(rawValue: Int) { + switch rawValue { + case 1: self = .valueOne + case 2: self = .valueTwo + default: return nil + } + } + + var rawValue: Int { + switch self { + case .valueOne: return 1 + case .valueTwo: return 2 + } + } +} + +func testRawRepresentable() { + let rr1 = MyRawRepresentable.valueOne + let rr2 = MyRawRepresentable(rawValue: 1)! + let rr3 = MyRawRepresentable(rawValue: sourceInt())! + + sink(arg: rr1) + sink(arg: rr2) + sink(arg: rr3) // $ MISSING: tainted= + + sink(arg: rr1.rawValue) + sink(arg: rr2.rawValue) + sink(arg: rr3.rawValue) // $ MISSING: tainted= +} + +// --- + +struct MyOptionSet : OptionSet { + let rawValue: UInt + + static let red = MyOptionSet(rawValue: 1 << 0) + static let green = MyOptionSet(rawValue: 1 << 1) + static let blue = MyOptionSet(rawValue: 1 << 2) +} + +func testOptionSet() { + sink(arg: MyOptionSet.red) + sink(arg: MyOptionSet([.red, .green])) + sink(arg: MyOptionSet(rawValue: 0)) + sink(arg: MyOptionSet(rawValue: sourceUInt())) // $ MISSING: tainted= + + sink(arg: MyOptionSet.red.rawValue) + sink(arg: MyOptionSet([.red, .green]).rawValue) + sink(arg: MyOptionSet(rawValue: 0).rawValue) + sink(arg: MyOptionSet(rawValue: sourceUInt()).rawValue) // $ MISSING: tainted= +} From 4e29ed5ff07d7994343ac2165094a288407f575c Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 13 Oct 2023 14:44:49 +0100 Subject: [PATCH 4/7] Swift: Model RawRepresentable. --- .../StandardLibrary/RawRepresentable.qll | 35 +++++++++++++++++++ .../StandardLibrary/StandardLibrary.qll | 1 + .../dataflow/taint/libraries/optionset.swift | 8 ++--- 3 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 swift/ql/lib/codeql/swift/frameworks/StandardLibrary/RawRepresentable.qll diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/RawRepresentable.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/RawRepresentable.qll new file mode 100644 index 00000000000..e077d12fc8f --- /dev/null +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/RawRepresentable.qll @@ -0,0 +1,35 @@ +/** + * Provides models the `RawRepresentable` Swift class. + */ + +import swift +private import codeql.swift.dataflow.DataFlow +private import codeql.swift.dataflow.ExternalFlow +private import codeql.swift.dataflow.FlowSteps + +/** + * A model for `RawRepresentable` class members that permit taint flow. + */ +private class RawRepresentableSummaries extends SummaryModelCsv { + override predicate row(string row) { + row = ";RawRepresentable;true;init(rawValue:);;;Argument[0];ReturnValue;taint" + } +} + +/** + * A content implying that, if an `RawRepresentable` is tainted, then + * the `rawValue` field is tainted as well. + */ +private class RawRepresentableFieldsInheritTaint extends TaintInheritingContent, + DataFlow::Content::FieldContent +{ + RawRepresentableFieldsInheritTaint() { + exists(FieldDecl fieldDecl, Decl declaringDecl, TypeDecl namedTypeDecl | + namedTypeDecl.getFullName() = "RawRepresentable" and + fieldDecl.getName() = "rawValue" and + declaringDecl.getAMember() = fieldDecl and + declaringDecl.asNominalTypeDecl() = namedTypeDecl.getADerivedTypeDecl*() and + this.getField() = fieldDecl + ) + } +} diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/StandardLibrary.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/StandardLibrary.qll index 9d48ab6f5e2..11349367474 100644 --- a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/StandardLibrary.qll +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/StandardLibrary.qll @@ -17,6 +17,7 @@ private import NsObject private import NsString private import NsUrl private import Numeric +private import RawRepresentable private import PointerTypes private import Sequence private import Set diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/optionset.swift b/swift/ql/test/library-tests/dataflow/taint/libraries/optionset.swift index 262764dbca6..e4f704d8ef5 100644 --- a/swift/ql/test/library-tests/dataflow/taint/libraries/optionset.swift +++ b/swift/ql/test/library-tests/dataflow/taint/libraries/optionset.swift @@ -36,11 +36,11 @@ func testRawRepresentable() { sink(arg: rr1) sink(arg: rr2) - sink(arg: rr3) // $ MISSING: tainted= + sink(arg: rr3) // $ tainted=35 sink(arg: rr1.rawValue) sink(arg: rr2.rawValue) - sink(arg: rr3.rawValue) // $ MISSING: tainted= + sink(arg: rr3.rawValue) // $ tainted=35 } // --- @@ -57,10 +57,10 @@ func testOptionSet() { sink(arg: MyOptionSet.red) sink(arg: MyOptionSet([.red, .green])) sink(arg: MyOptionSet(rawValue: 0)) - sink(arg: MyOptionSet(rawValue: sourceUInt())) // $ MISSING: tainted= + sink(arg: MyOptionSet(rawValue: sourceUInt())) // $ tainted=60 sink(arg: MyOptionSet.red.rawValue) sink(arg: MyOptionSet([.red, .green]).rawValue) sink(arg: MyOptionSet(rawValue: 0).rawValue) - sink(arg: MyOptionSet(rawValue: sourceUInt()).rawValue) // $ MISSING: tainted= + sink(arg: MyOptionSet(rawValue: sourceUInt()).rawValue) // $ tainted=65 } From d0f214a9a700c26405d55c117e19c3214eb2a9f1 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 13 Oct 2023 17:18:52 +0100 Subject: [PATCH 5/7] Swift: Widen the model to include things that are not strictly RawRepresentable but which appear similar. This fixes the XXE test cases. Unclear whether xmlParserOption in the test should in fact extend RawRepresentable, or not. --- .../StandardLibrary/RawRepresentable.qll | 15 ++++----------- .../Security/CWE-611/XXETest.expected | 16 ---------------- 2 files changed, 4 insertions(+), 27 deletions(-) diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/RawRepresentable.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/RawRepresentable.qll index e077d12fc8f..8d56ffb4dfd 100644 --- a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/RawRepresentable.qll +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/RawRepresentable.qll @@ -17,19 +17,12 @@ private class RawRepresentableSummaries extends SummaryModelCsv { } /** - * A content implying that, if an `RawRepresentable` is tainted, then - * the `rawValue` field is tainted as well. + * A content implying that, if a `RawRepresentable` is tainted, then the + * `rawValue` field is tainted as well. This model has been extended to assume + * that any object's `rawValue` field also inherits taint. */ private class RawRepresentableFieldsInheritTaint extends TaintInheritingContent, DataFlow::Content::FieldContent { - RawRepresentableFieldsInheritTaint() { - exists(FieldDecl fieldDecl, Decl declaringDecl, TypeDecl namedTypeDecl | - namedTypeDecl.getFullName() = "RawRepresentable" and - fieldDecl.getName() = "rawValue" and - declaringDecl.getAMember() = fieldDecl and - declaringDecl.asNominalTypeDecl() = namedTypeDecl.getADerivedTypeDecl*() and - this.getField() = fieldDecl - ) - } + RawRepresentableFieldsInheritTaint() { this.getField().getName() = "rawValue" } } diff --git a/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected b/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected index 8cbc8d2c929..8ec8033d086 100644 --- a/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected +++ b/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected @@ -1,18 +1,2 @@ testFailures -| testLibxmlXXE.swift:101:78:102:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:102:80:103:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:103:107:104:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:104:82:105:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:106:78:107:1 | // $ hasXXE=95\n | Missing result:hasXXE=95 | -| testLibxmlXXE.swift:107:80:108:1 | // $ hasXXE=95\n | Missing result:hasXXE=95 | -| testLibxmlXXE.swift:109:87:110:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:110:89:111:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:112:99:113:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:113:97:114:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:115:87:116:1 | // $ hasXXE=95\n | Missing result:hasXXE=95 | -| testLibxmlXXE.swift:116:89:117:1 | // $ hasXXE=95\n | Missing result:hasXXE=95 | -| testLibxmlXXE.swift:118:89:119:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:119:91:120:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:121:98:122:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | -| testLibxmlXXE.swift:122:100:123:1 | // $ hasXXE=96\n | Missing result:hasXXE=96 | failures From aa0db1426dc6cbb2cafba516b659b9216edeaa77 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 13 Oct 2023 17:40:03 +0100 Subject: [PATCH 6/7] Swift: Simplify the QL a bit further. --- swift/ql/lib/codeql/swift/security/XXEExtensions.qll | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/swift/ql/lib/codeql/swift/security/XXEExtensions.qll b/swift/ql/lib/codeql/swift/security/XXEExtensions.qll index 5bef17d032f..b9c4dea1a87 100644 --- a/swift/ql/lib/codeql/swift/security/XXEExtensions.qll +++ b/swift/ql/lib/codeql/swift/security/XXEExtensions.qll @@ -172,20 +172,12 @@ private class Libxml2XxeSink extends XxeSink { Libxml2XxeSink() { exists(Libxml2ParseCall c, Libxml2BadOption opt | this.asExpr() = c.getXml() and - lib2xmlOptionLocalTaintStep*(DataFlow::exprNode(opt.getAnAccess()), + TaintTracking::localTaintStep*(DataFlow::exprNode(opt.getAnAccess()), DataFlow::exprNode(c.getOptions())) ) } } -/** - * Holds if taint can flow from `source` to `sink` in one local step, - * including bitwise operations, accesses to `.rawValue`, and casts to `Int32`. - */ -private predicate lib2xmlOptionLocalTaintStep(DataFlow::Node source, DataFlow::Node sink) { - TaintTracking::localTaintStep(source, sink) -} - /** * A sink defined in a CSV model. */ From e2ac3769bca4f0df7fc30460575c4dfad68c52ec Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 13 Oct 2023 16:31:53 +0100 Subject: [PATCH 7/7] Swift: Change note. --- swift/ql/lib/change-notes/2023-10-13-rawrepresentable.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 swift/ql/lib/change-notes/2023-10-13-rawrepresentable.md diff --git a/swift/ql/lib/change-notes/2023-10-13-rawrepresentable.md b/swift/ql/lib/change-notes/2023-10-13-rawrepresentable.md new file mode 100644 index 00000000000..114afd58ab8 --- /dev/null +++ b/swift/ql/lib/change-notes/2023-10-13-rawrepresentable.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- + +* Added taint flow models for `RawRepresentable`.