Skip to content

Commit ffa3028

Browse files
MathiasVPMathiasVP
authored
Merge pull request #12896 from geoffw0/modernsec3
Swift: Fix member variable CSV sinks (swift/insecure-tls)
2 parents 65dea0b + 08b6755 commit ffa3028

File tree

16 files changed

+258
-71
lines changed

16 files changed

+258
-71
lines changed

‎swift/ql/.generated.list

Lines changed: 0 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎swift/ql/.gitattributes

Lines changed: 0 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎swift/ql/lib/codeql/swift/dataflow/internal/FlowSummaryImplSpecific.qll

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,22 @@ predicate interpretOutputSpecific(string c, InterpretNode mid, InterpretNode nod
197197
)
198198
}
199199

200-
predicate interpretInputSpecific(string c, InterpretNode mid, InterpretNode n) { none() }
200+
predicate interpretInputSpecific(string c, InterpretNode mid, InterpretNode node) {
201+
exists(Node n, AstNode ast, MemberRefExpr e |
202+
n = node.asNode() and
203+
ast = mid.asElement() and
204+
e.getMember() = ast
205+
|
206+
// Allow fields to be picked as input nodes.
207+
c = "" and
208+
e.getBase() = n.asExpr()
209+
or
210+
// Allow post update nodes to be picked as input nodes when the `input` column
211+
// of the row is `PostUpdate`.
212+
c = "PostUpdate" and
213+
e.getBase() = n.(PostUpdateNode).getPreUpdateNode().asExpr()
214+
)
215+
}
201216

202217
/** Gets the argument position obtained by parsing `X` in `Parameter[X]`. */
203218
bindingset[s]

‎swift/ql/lib/codeql/swift/dataflow/internal/TaintTrackingPublic.qll

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,14 @@ predicate localTaintStep = localTaintStepCached/2;
2626
* of `c` at sinks and inputs to additional taint steps.
2727
*/
2828
bindingset[node]
29-
predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c) { none() }
29+
predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet cs) {
30+
// If a `PostUpdateNode` is specified as a sink, there's (almost) always a store step preceding it.
31+
// So when the node is a `PostUpdateNode` we allow any sequence of implicit read steps of an appropriate
32+
// type to make sure we arrive at the sink with an empty access path.
33+
exists(NominalTypeDecl d, Decl cx |
34+
node.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr().getType() =
35+
d.getType().getABaseType*() and
36+
cx.asNominalTypeDecl() = d and
37+
cs.getAReadContent().(DataFlow::Content::FieldContent).getField() = cx.getAMember()
38+
)
39+
}

‎swift/ql/lib/codeql/swift/elements/type/DynamicSelfType.qll

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
1-
// generated by codegen/codegen.py, remove this comment if you wish to edit this file
21
private import codeql.swift.generated.type.DynamicSelfType
32

4-
class DynamicSelfType extends Generated::DynamicSelfType { }
3+
class DynamicSelfType extends Generated::DynamicSelfType {
4+
override Type getResolveStep() {
5+
// The type of qualifiers in a Swift constructor is assigned the type `Self` by the Swift compiler
6+
// This `getResolveStep` replaces that `Self` type with the type of the enclosing class.
7+
result = this.getImmediateStaticSelfType()
8+
}
9+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ private class EncryptionKeySinks extends SinkModelCsv {
7070
// Realm database library.
7171
";Realm.Configuration;true;init(fileURL:inMemoryIdentifier:syncConfiguration:encryptionKey:readOnly:schemaVersion:migrationBlock:deleteRealmIfMigrationNeeded:shouldCompactOnLaunch:objectTypes:);;;Argument[3];encryption-key",
7272
";Realm.Configuration;true;init(fileURL:inMemoryIdentifier:syncConfiguration:encryptionKey:readOnly:schemaVersion:migrationBlock:deleteRealmIfMigrationNeeded:shouldCompactOnLaunch:objectTypes:seedFilePath:);;;Argument[3];encryption-key",
73-
";Realm.Configuration;true;encryptionKey;;;;encryption-key",
73+
";Realm.Configuration;true;encryptionKey;;;PostUpdate;encryption-key",
7474
]
7575
}
7676
}

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

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -45,19 +45,16 @@ private class EnumInsecureTlsExtensionsSource extends InsecureTlsExtensionsSourc
4545
}
4646
}
4747

48-
/**
49-
* A sink for assignment of TLS-related properties of `NSURLSessionConfiguration`.
50-
*/
51-
private class NsUrlTlsExtensionsSink extends InsecureTlsExtensionsSink {
52-
NsUrlTlsExtensionsSink() {
53-
exists(AssignExpr assign |
54-
assign.getSource() = this.asExpr() and
55-
assign.getDest().(MemberRefExpr).getMember().(ConcreteVarDecl).getName() =
56-
[
57-
"tlsMinimumSupportedProtocolVersion", "tlsMinimumSupportedProtocol",
58-
"tlsMaximumSupportedProtocolVersion", "tlsMaximumSupportedProtocol"
59-
]
60-
)
48+
private class TlsExtensionsSinks extends SinkModelCsv {
49+
override predicate row(string row) {
50+
row =
51+
[
52+
// TLS-related properties of `URLSessionConfiguration`
53+
";URLSessionConfiguration;false;tlsMinimumSupportedProtocolVersion;;;PostUpdate;tls-protocol-version",
54+
";URLSessionConfiguration;false;tlsMinimumSupportedProtocol;;;PostUpdate;tls-protocol-version",
55+
";URLSessionConfiguration;false;tlsMaximumSupportedProtocolVersion;;;PostUpdate;tls-protocol-version",
56+
";URLSessionConfiguration;false;tlsMaximumSupportedProtocol;;;PostUpdate;tls-protocol-version",
57+
]
6158
}
6259
}
6360

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,8 @@ private class PathInjectionSinks extends SinkModelCsv {
129129
";Realm.Configuration;true;init(fileURL:inMemoryIdentifier:syncConfiguration:encryptionKey:readOnly:schemaVersion:migrationBlock:deleteRealmIfMigrationNeeded:shouldCompactOnLaunch:objectTypes:);;;Argument[0];path-injection",
130130
";Realm.Configuration;true;init(fileURL:inMemoryIdentifier:syncConfiguration:encryptionKey:readOnly:schemaVersion:migrationBlock:deleteRealmIfMigrationNeeded:shouldCompactOnLaunch:objectTypes:seedFilePath:);;;Argument[0];path-injection",
131131
";Realm.Configuration;true;init(fileURL:inMemoryIdentifier:syncConfiguration:encryptionKey:readOnly:schemaVersion:migrationBlock:deleteRealmIfMigrationNeeded:shouldCompactOnLaunch:objectTypes:seedFilePath:);;;Argument[10];path-injection",
132-
";Realm.Configuration;true;fileURL;;;;path-injection",
133-
";Realm.Configuration;true;seedFilePath;;;;path-injection",
132+
";Realm.Configuration;true;fileURL;;;PostUpdate;path-injection",
133+
";Realm.Configuration;true;seedFilePath;;;PostUpdate;path-injection",
134134
]
135135
}
136136
}

‎swift/ql/test/extractor-tests/generated/type/DynamicSelfType/DynamicSelfType.expected

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +0,0 @@
1-
| Self | getName: | Self | getCanonicalType: | Self | getStaticSelfType: | X |

‎swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ edges
235235
| test.swift:536:10:536:13 | s | test.swift:537:13:537:13 | s |
236236
| test.swift:537:7:537:7 | [post] self [str] | test.swift:536:5:538:5 | self[return] [str] |
237237
| test.swift:537:13:537:13 | s | test.swift:537:7:537:7 | [post] self [str] |
238-
| test.swift:542:17:545:5 | self[return] [str] | test.swift:550:13:550:41 | call to Self.init(contentsOfFile:) [str] |
238+
| test.swift:542:17:545:5 | self[return] [str] | test.swift:550:13:550:41 | call to MyClass.init(contentsOfFile:) [str] |
239239
| test.swift:543:7:543:7 | [post] self [str] | test.swift:542:17:545:5 | self[return] [str] |
240240
| test.swift:543:7:543:7 | [post] self [str] | test.swift:544:17:544:17 | self [str] |
241241
| test.swift:543:20:543:28 | call to source3() | test.swift:536:10:536:13 | s |
@@ -245,8 +245,8 @@ edges
245245
| test.swift:549:13:549:33 | call to MyClass.init(s:) [str] | test.swift:549:13:549:35 | .str |
246246
| test.swift:549:24:549:32 | call to source3() | test.swift:536:10:536:13 | s |
247247
| test.swift:549:24:549:32 | call to source3() | test.swift:549:13:549:33 | call to MyClass.init(s:) [str] |
248-
| test.swift:550:13:550:41 | call to Self.init(contentsOfFile:) [str] | test.swift:535:9:535:9 | self [str] |
249-
| test.swift:550:13:550:41 | call to Self.init(contentsOfFile:) [str] | test.swift:550:13:550:43 | .str |
248+
| test.swift:550:13:550:41 | call to MyClass.init(contentsOfFile:) [str] | test.swift:535:9:535:9 | self [str] |
249+
| test.swift:550:13:550:41 | call to MyClass.init(contentsOfFile:) [str] | test.swift:550:13:550:43 | .str |
250250
| test.swift:567:8:567:11 | x | test.swift:568:14:568:14 | x |
251251
| test.swift:568:5:568:5 | [post] self [x] | test.swift:567:3:569:3 | self[return] [x] |
252252
| test.swift:568:14:568:14 | x | test.swift:568:5:568:5 | [post] self [x] |
@@ -541,7 +541,7 @@ nodes
541541
| test.swift:549:13:549:33 | call to MyClass.init(s:) [str] | semmle.label | call to MyClass.init(s:) [str] |
542542
| test.swift:549:13:549:35 | .str | semmle.label | .str |
543543
| test.swift:549:24:549:32 | call to source3() | semmle.label | call to source3() |
544-
| test.swift:550:13:550:41 | call to Self.init(contentsOfFile:) [str] | semmle.label | call to Self.init(contentsOfFile:) [str] |
544+
| test.swift:550:13:550:41 | call to MyClass.init(contentsOfFile:) [str] | semmle.label | call to MyClass.init(contentsOfFile:) [str] |
545545
| test.swift:550:13:550:43 | .str | semmle.label | .str |
546546
| test.swift:567:3:569:3 | self[return] [x] | semmle.label | self[return] [x] |
547547
| test.swift:567:8:567:11 | x | semmle.label | x |
@@ -609,7 +609,7 @@ subpaths
609609
| test.swift:543:20:543:28 | call to source3() | test.swift:536:10:536:13 | s | test.swift:537:7:537:7 | [post] self [str] | test.swift:543:7:543:7 | [post] self [str] |
610610
| test.swift:549:13:549:33 | call to MyClass.init(s:) [str] | test.swift:535:9:535:9 | self [str] | file://:0:0:0:0 | .str | test.swift:549:13:549:35 | .str |
611611
| test.swift:549:24:549:32 | call to source3() | test.swift:536:10:536:13 | s | test.swift:536:5:538:5 | self[return] [str] | test.swift:549:13:549:33 | call to MyClass.init(s:) [str] |
612-
| test.swift:550:13:550:41 | call to Self.init(contentsOfFile:) [str] | test.swift:535:9:535:9 | self [str] | file://:0:0:0:0 | .str | test.swift:550:13:550:43 | .str |
612+
| test.swift:550:13:550:41 | call to MyClass.init(contentsOfFile:) [str] | test.swift:535:9:535:9 | self [str] | file://:0:0:0:0 | .str | test.swift:550:13:550:43 | .str |
613613
| test.swift:573:16:573:23 | call to source() | test.swift:567:8:567:11 | x | test.swift:567:3:569:3 | self[return] [x] | test.swift:573:11:573:24 | call to S.init(x:) [x] |
614614
| test.swift:575:13:575:13 | s [x] | test.swift:574:11:574:14 | enter #keyPath(...) [x] | test.swift:574:11:574:14 | exit #keyPath(...) | test.swift:575:13:575:25 | \\...[...] |
615615
| test.swift:578:13:578:13 | s [x] | test.swift:577:36:577:38 | enter #keyPath(...) [x] | test.swift:577:36:577:38 | exit #keyPath(...) | test.swift:578:13:578:32 | \\...[...] |

‎swift/ql/test/query-tests/Security/CWE-022/PathInjectionTest.ql

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,12 @@ class PathInjectionTest extends InlineExpectationsTest {
1010
override string getARelevantTag() { result = "hasPathInjection" }
1111

1212
override predicate hasActualResult(Location location, string element, string tag, string value) {
13-
exists(DataFlow::Node source, DataFlow::Node sink, Expr sinkExpr |
13+
exists(DataFlow::Node source, DataFlow::Node sink |
1414
PathInjectionFlow::flow(source, sink) and
15-
sinkExpr = sink.asExpr() and
16-
location = sinkExpr.getLocation() and
17-
element = sinkExpr.toString() and
15+
location = sink.getLocation() and
16+
element = sink.toString() and
1817
tag = "hasPathInjection" and
18+
location.getFile().getName() != "" and
1919
value = source.asExpr().getLocation().getStartLine().toString()
2020
)
2121
}

‎swift/ql/test/query-tests/Security/CWE-022/testPathInjection.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -317,9 +317,9 @@ func test() {
317317

318318
var config = Realm.Configuration() // GOOD
319319
config.fileURL = safeUrl // GOOD
320-
config.fileURL = remoteUrl // $ MISSING: hasPathInjection=208
320+
config.fileURL = remoteUrl // $ hasPathInjection=208
321321
config.seedFilePath = safeUrl // GOOD
322-
config.seedFilePath = remoteUrl // $ MISSING: hasPathInjection=208
322+
config.seedFilePath = remoteUrl // $ hasPathInjection=208
323323
}
324324

325325
func testBarriers() {

0 commit comments

Comments
 (0)