Skip to content

Commit e109892

Browse files
geoffw0geoffw0
authored
Merge pull request github#14189 from geoffw0/protocol2
Swift: Consistent additional taint steps between the cleartext-* queries
2 parents 00c83f1 + d65f2b4 commit e109892

10 files changed

+121
-34
lines changed

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,16 @@ private class OsLogPrivacyRef extends MemberRefExpr {
8383
predicate isPublic() { optionName = "public" }
8484
}
8585

86+
/**
87+
* An additional taint step for cleartext logging vulnerabilities.
88+
*/
89+
private class CleartextLoggingFieldAdditionalFlowStep extends CleartextLoggingAdditionalFlowStep {
90+
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
91+
// if an object is sensitive, its fields are always sensitive.
92+
nodeTo.asExpr().(MemberRefExpr).getBase() = nodeFrom.asExpr()
93+
}
94+
}
95+
8696
private class LoggingSinks extends SinkModelCsv {
8797
override predicate row(string row) {
8898
row =

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

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,9 @@ class CleartextStoragePreferencesAdditionalFlowStep extends Unit {
3333
abstract predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo);
3434
}
3535

36-
/** The `DataFlow::Node` of an expression that gets written to the user defaults database */
36+
/**
37+
* The `DataFlow::Node` of an expression that gets written to the user defaults database.
38+
*/
3739
private class UserDefaultsStore extends CleartextStoragePreferencesSink {
3840
UserDefaultsStore() {
3941
exists(CallExpr call |
@@ -45,7 +47,9 @@ private class UserDefaultsStore extends CleartextStoragePreferencesSink {
4547
override string getStoreName() { result = "the user defaults database" }
4648
}
4749

48-
/** The `DataFlow::Node` of an expression that gets written to the iCloud-backed NSUbiquitousKeyValueStore */
50+
/**
51+
* The `DataFlow::Node` of an expression that gets written to the iCloud-backed `NSUbiquitousKeyValueStore`.
52+
*/
4953
private class NSUbiquitousKeyValueStore extends CleartextStoragePreferencesSink {
5054
NSUbiquitousKeyValueStore() {
5155
exists(CallExpr call |
@@ -85,6 +89,17 @@ private class CleartextStoragePreferencesDefaultBarrier extends CleartextStorage
8589
}
8690
}
8791

92+
/**
93+
* An additional taint step for cleartext preferences storage vulnerabilities.
94+
*/
95+
private class CleartextStoragePreferencesFieldAdditionalFlowStep extends CleartextStoragePreferencesAdditionalFlowStep
96+
{
97+
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
98+
// if an object is sensitive, its fields are always sensitive.
99+
nodeTo.asExpr().(MemberRefExpr).getBase() = nodeFrom.asExpr()
100+
}
101+
}
102+
88103
/**
89104
* A sink defined in a CSV model.
90105
*/

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

Lines changed: 24 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -30,38 +30,6 @@ class CleartextTransmissionAdditionalFlowStep extends Unit {
3030
abstract predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo);
3131
}
3232

33-
/**
34-
* An `Expr` that is transmitted with `NWConnection.send`.
35-
*/
36-
private class NWConnectionSendSink extends CleartextTransmissionSink {
37-
NWConnectionSendSink() {
38-
// `content` arg to `NWConnection.send` is a sink
39-
exists(CallExpr call |
40-
call.getStaticTarget()
41-
.(Method)
42-
.hasQualifiedName("NWConnection", "send(content:contentContext:isComplete:completion:)") and
43-
call.getArgument(0).getExpr() = this.asExpr()
44-
)
45-
}
46-
}
47-
48-
/**
49-
* An `Expr` that is used to form a `URL`. Such expressions are very likely to
50-
* be transmitted over a network, because that's what URLs are for.
51-
*/
52-
private class UrlSink extends CleartextTransmissionSink {
53-
UrlSink() {
54-
// `string` arg in `URL.init` is a sink
55-
// (we assume here that the URL goes on to be used in a network operation)
56-
exists(CallExpr call |
57-
call.getStaticTarget()
58-
.(Method)
59-
.hasQualifiedName("URL", ["init(string:)", "init(string:relativeTo:)"]) and
60-
call.getArgument(0).getExpr() = this.asExpr()
61-
)
62-
}
63-
}
64-
6533
/**
6634
* An `Expr` that transmitted through the Alamofire library.
6735
*/
@@ -92,9 +60,33 @@ private class CleartextTransmissionDefaultBarrier extends CleartextTransmissionB
9260
}
9361
}
9462

63+
/**
64+
* An additional taint step for cleartext transmission vulnerabilities.
65+
*/
66+
private class CleartextTransmissionFieldAdditionalFlowStep extends CleartextTransmissionAdditionalFlowStep
67+
{
68+
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
69+
// if an object is sensitive, its fields are always sensitive.
70+
nodeTo.asExpr().(MemberRefExpr).getBase() = nodeFrom.asExpr()
71+
}
72+
}
73+
9574
/**
9675
* A sink defined in a CSV model.
9776
*/
9877
private class DefaultCleartextTransmissionSink extends CleartextTransmissionSink {
9978
DefaultCleartextTransmissionSink() { sinkNode(this, "transmission") }
10079
}
80+
81+
private class TransmissionSinks extends SinkModelCsv {
82+
override predicate row(string row) {
83+
row =
84+
[
85+
";NWConnection;true;send(content:contentContext:isComplete:completion:);;;Argument[0];transmission",
86+
// an `Expr` that is used to form a `URL` is very likely to be transmitted over a network, because
87+
// that's what URLs are for.
88+
";URL;true;init(string:);;;Argument[0];transmission",
89+
";URL;true;init(string:relativeTo:);;;Argument[0];transmission",
90+
]
91+
}
92+
}

‎swift/ql/src/change-notes/2023-09-12-cleartext.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
5+
* Added additional taint steps to the `swift/cleartext-transmission`, `swift/cleartext-logging` and `swift/cleartext-storage-preferences` queries to identify data within sensitive containers. This is similar to an existing additional taint step in the `swift/cleartext-storage-database` query.

‎swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
edges
2+
| file://:0:0:0:0 | self | file://:0:0:0:0 | .value |
23
| testAlamofire.swift:150:45:150:45 | password | testAlamofire.swift:150:13:150:45 | ... .+(_:_:) ... |
34
| testAlamofire.swift:152:51:152:51 | password | testAlamofire.swift:152:19:152:51 | ... .+(_:_:) ... |
45
| testAlamofire.swift:154:38:154:38 | email | testAlamofire.swift:154:14:154:46 | ... .+(_:_:) ... |
@@ -10,13 +11,18 @@ edges
1011
| testSend.swift:60:13:60:25 | call to pad(_:) | testSend.swift:67:27:67:27 | str3 |
1112
| testSend.swift:60:17:60:17 | password | testSend.swift:41:10:41:18 | data |
1213
| testSend.swift:60:17:60:17 | password | testSend.swift:60:13:60:25 | call to pad(_:) |
14+
| testSend.swift:86:7:86:7 | self | file://:0:0:0:0 | self |
15+
| testSend.swift:94:27:94:30 | .password | testSend.swift:86:7:86:7 | self |
16+
| testSend.swift:94:27:94:30 | .password | testSend.swift:94:27:94:39 | .value |
1317
| testURL.swift:17:54:17:54 | passwd | testURL.swift:17:22:17:54 | ... .+(_:_:) ... |
1418
| testURL.swift:19:55:19:55 | account_no | testURL.swift:19:22:19:55 | ... .+(_:_:) ... |
1519
| testURL.swift:20:55:20:55 | credit_card_no | testURL.swift:20:22:20:55 | ... .+(_:_:) ... |
1620
| testURL.swift:28:55:28:55 | e_mail | testURL.swift:28:22:28:55 | ... .+(_:_:) ... |
1721
| testURL.swift:30:57:30:57 | a_homeaddr_z | testURL.swift:30:22:30:57 | ... .+(_:_:) ... |
1822
| testURL.swift:32:55:32:55 | resident_ID | testURL.swift:32:22:32:55 | ... .+(_:_:) ... |
1923
nodes
24+
| file://:0:0:0:0 | .value | semmle.label | .value |
25+
| file://:0:0:0:0 | self | semmle.label | self |
2026
| testAlamofire.swift:150:13:150:45 | ... .+(_:_:) ... | semmle.label | ... .+(_:_:) ... |
2127
| testAlamofire.swift:150:45:150:45 | password | semmle.label | password |
2228
| testAlamofire.swift:152:19:152:51 | ... .+(_:_:) ... | semmle.label | ... .+(_:_:) ... |
@@ -43,6 +49,9 @@ nodes
4349
| testSend.swift:78:27:78:30 | .CarePlanID | semmle.label | .CarePlanID |
4450
| testSend.swift:79:27:79:30 | .BankCardNo | semmle.label | .BankCardNo |
4551
| testSend.swift:80:27:80:30 | .MyCreditRating | semmle.label | .MyCreditRating |
52+
| testSend.swift:86:7:86:7 | self | semmle.label | self |
53+
| testSend.swift:94:27:94:30 | .password | semmle.label | .password |
54+
| testSend.swift:94:27:94:39 | .value | semmle.label | .value |
4655
| testURL.swift:17:22:17:54 | ... .+(_:_:) ... | semmle.label | ... .+(_:_:) ... |
4756
| testURL.swift:17:54:17:54 | passwd | semmle.label | passwd |
4857
| testURL.swift:19:22:19:55 | ... .+(_:_:) ... | semmle.label | ... .+(_:_:) ... |
@@ -58,6 +67,7 @@ nodes
5867
| testURL.swift:32:55:32:55 | resident_ID | semmle.label | resident_ID |
5968
subpaths
6069
| testSend.swift:60:17:60:17 | password | testSend.swift:41:10:41:18 | data | testSend.swift:41:45:41:45 | data | testSend.swift:60:13:60:25 | call to pad(_:) |
70+
| testSend.swift:94:27:94:30 | .password | testSend.swift:86:7:86:7 | self | file://:0:0:0:0 | .value | testSend.swift:94:27:94:39 | .value |
6171
#select
6272
| testAlamofire.swift:150:13:150:45 | ... .+(_:_:) ... | testAlamofire.swift:150:45:150:45 | password | testAlamofire.swift:150:13:150:45 | ... .+(_:_:) ... | This operation transmits '... .+(_:_:) ...', which may contain unencrypted sensitive data from $@. | testAlamofire.swift:150:45:150:45 | password | password |
6373
| testAlamofire.swift:152:19:152:51 | ... .+(_:_:) ... | testAlamofire.swift:152:51:152:51 | password | testAlamofire.swift:152:19:152:51 | ... .+(_:_:) ... | This operation transmits '... .+(_:_:) ...', which may contain unencrypted sensitive data from $@. | testAlamofire.swift:152:51:152:51 | password | password |
@@ -74,6 +84,7 @@ subpaths
7484
| testSend.swift:78:27:78:30 | .CarePlanID | testSend.swift:78:27:78:30 | .CarePlanID | testSend.swift:78:27:78:30 | .CarePlanID | This operation transmits '.CarePlanID', which may contain unencrypted sensitive data from $@. | testSend.swift:78:27:78:30 | .CarePlanID | .CarePlanID |
7585
| testSend.swift:79:27:79:30 | .BankCardNo | testSend.swift:79:27:79:30 | .BankCardNo | testSend.swift:79:27:79:30 | .BankCardNo | This operation transmits '.BankCardNo', which may contain unencrypted sensitive data from $@. | testSend.swift:79:27:79:30 | .BankCardNo | .BankCardNo |
7686
| testSend.swift:80:27:80:30 | .MyCreditRating | testSend.swift:80:27:80:30 | .MyCreditRating | testSend.swift:80:27:80:30 | .MyCreditRating | This operation transmits '.MyCreditRating', which may contain unencrypted sensitive data from $@. | testSend.swift:80:27:80:30 | .MyCreditRating | .MyCreditRating |
87+
| testSend.swift:94:27:94:39 | .value | testSend.swift:94:27:94:30 | .password | testSend.swift:94:27:94:39 | .value | This operation transmits '.value', which may contain unencrypted sensitive data from $@. | testSend.swift:94:27:94:30 | .password | .password |
7788
| testURL.swift:17:22:17:54 | ... .+(_:_:) ... | testURL.swift:17:54:17:54 | passwd | testURL.swift:17:22:17:54 | ... .+(_:_:) ... | This operation transmits '... .+(_:_:) ...', which may contain unencrypted sensitive data from $@. | testURL.swift:17:54:17:54 | passwd | passwd |
7889
| testURL.swift:19:22:19:55 | ... .+(_:_:) ... | testURL.swift:19:55:19:55 | account_no | testURL.swift:19:22:19:55 | ... .+(_:_:) ... | This operation transmits '... .+(_:_:) ...', which may contain unencrypted sensitive data from $@. | testURL.swift:19:55:19:55 | account_no | account_no |
7990
| testURL.swift:20:22:20:55 | ... .+(_:_:) ... | testURL.swift:20:55:20:55 | credit_card_no | testURL.swift:20:22:20:55 | ... .+(_:_:) ... | This operation transmits '... .+(_:_:) ...', which may contain unencrypted sensitive data from $@. | testURL.swift:20:55:20:55 | credit_card_no | credit_card_no |

‎swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@
133133
| testSend.swift:78:27:78:30 | .CarePlanID | label:CarePlanID, type:private information |
134134
| testSend.swift:79:27:79:30 | .BankCardNo | label:BankCardNo, type:private information |
135135
| testSend.swift:80:27:80:30 | .MyCreditRating | label:MyCreditRating, type:private information |
136+
| testSend.swift:94:27:94:30 | .password | label:password, type:credential |
136137
| testURL.swift:17:54:17:54 | passwd | label:passwd, type:credential |
137138
| testURL.swift:19:55:19:55 | account_no | label:account_no, type:private information |
138139
| testURL.swift:20:55:20:55 | credit_card_no | label:credit_card_no, type:private information |

‎swift/ql/test/query-tests/Security/CWE-311/testSend.swift

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,3 +80,17 @@ func test2(password : String, license_key: String, ms: MyStruct, connection : NW
8080
connection.send(content: ms.MyCreditRating, completion: .idempotent) // BAD
8181
connection.send(content: ms.OneTimeCode, completion: .idempotent) // BAD [NOT DETECTED]
8282
}
83+
84+
struct MyOuter {
85+
struct MyInner {
86+
var value: String
87+
}
88+
89+
var password: MyInner
90+
var harmless: MyInner
91+
}
92+
93+
func test3(mo : MyOuter, connection : NWConnection) {
94+
connection.send(content: mo.password.value, completion: .idempotent) // BAD
95+
connection.send(content: mo.harmless.value, completion: .idempotent) // GOOD
96+
}

‎swift/ql/test/query-tests/Security/CWE-312/CleartextStoragePreferences.expected

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
edges
2+
| file://:0:0:0:0 | self | file://:0:0:0:0 | .value |
23
| testNSUbiquitousKeyValueStore.swift:41:24:41:24 | x | testNSUbiquitousKeyValueStore.swift:42:40:42:40 | x |
34
| testNSUbiquitousKeyValueStore.swift:44:10:44:22 | call to getPassword() | testNSUbiquitousKeyValueStore.swift:45:40:45:40 | y |
45
| testNSUbiquitousKeyValueStore.swift:55:10:55:10 | passwd | testNSUbiquitousKeyValueStore.swift:59:40:59:40 | x |
@@ -9,7 +10,12 @@ edges
910
| testUserDefaults.swift:55:10:55:10 | passwd | testUserDefaults.swift:59:28:59:28 | x |
1011
| testUserDefaults.swift:56:10:56:10 | passwd | testUserDefaults.swift:60:28:60:28 | y |
1112
| testUserDefaults.swift:57:10:57:10 | passwd | testUserDefaults.swift:61:28:61:28 | z |
13+
| testUserDefaults.swift:74:7:74:7 | self | file://:0:0:0:0 | self |
14+
| testUserDefaults.swift:82:28:82:31 | .password | testUserDefaults.swift:74:7:74:7 | self |
15+
| testUserDefaults.swift:82:28:82:31 | .password | testUserDefaults.swift:82:28:82:40 | .value |
1216
nodes
17+
| file://:0:0:0:0 | .value | semmle.label | .value |
18+
| file://:0:0:0:0 | self | semmle.label | self |
1319
| testNSUbiquitousKeyValueStore.swift:28:12:28:12 | password | semmle.label | password |
1420
| testNSUbiquitousKeyValueStore.swift:41:24:41:24 | x | semmle.label | x |
1521
| testNSUbiquitousKeyValueStore.swift:42:40:42:40 | x | semmle.label | x |
@@ -34,7 +40,11 @@ nodes
3440
| testUserDefaults.swift:59:28:59:28 | x | semmle.label | x |
3541
| testUserDefaults.swift:60:28:60:28 | y | semmle.label | y |
3642
| testUserDefaults.swift:61:28:61:28 | z | semmle.label | z |
43+
| testUserDefaults.swift:74:7:74:7 | self | semmle.label | self |
44+
| testUserDefaults.swift:82:28:82:31 | .password | semmle.label | .password |
45+
| testUserDefaults.swift:82:28:82:40 | .value | semmle.label | .value |
3746
subpaths
47+
| testUserDefaults.swift:82:28:82:31 | .password | testUserDefaults.swift:74:7:74:7 | self | file://:0:0:0:0 | .value | testUserDefaults.swift:82:28:82:40 | .value |
3848
#select
3949
| testNSUbiquitousKeyValueStore.swift:28:12:28:12 | password | testNSUbiquitousKeyValueStore.swift:28:12:28:12 | password | testNSUbiquitousKeyValueStore.swift:28:12:28:12 | password | This operation stores 'password' in iCloud. It may contain unencrypted sensitive data from $@. | testNSUbiquitousKeyValueStore.swift:28:12:28:12 | password | password |
4050
| testNSUbiquitousKeyValueStore.swift:42:40:42:40 | x | testNSUbiquitousKeyValueStore.swift:41:24:41:24 | x | testNSUbiquitousKeyValueStore.swift:42:40:42:40 | x | This operation stores 'x' in iCloud. It may contain unencrypted sensitive data from $@. | testNSUbiquitousKeyValueStore.swift:41:24:41:24 | x | x |
@@ -50,3 +60,4 @@ subpaths
5060
| testUserDefaults.swift:59:28:59:28 | x | testUserDefaults.swift:55:10:55:10 | passwd | testUserDefaults.swift:59:28:59:28 | x | This operation stores 'x' in the user defaults database. It may contain unencrypted sensitive data from $@. | testUserDefaults.swift:55:10:55:10 | passwd | passwd |
5161
| testUserDefaults.swift:60:28:60:28 | y | testUserDefaults.swift:56:10:56:10 | passwd | testUserDefaults.swift:60:28:60:28 | y | This operation stores 'y' in the user defaults database. It may contain unencrypted sensitive data from $@. | testUserDefaults.swift:56:10:56:10 | passwd | passwd |
5262
| testUserDefaults.swift:61:28:61:28 | z | testUserDefaults.swift:57:10:57:10 | passwd | testUserDefaults.swift:61:28:61:28 | z | This operation stores 'z' in the user defaults database. It may contain unencrypted sensitive data from $@. | testUserDefaults.swift:57:10:57:10 | passwd | passwd |
63+
| testUserDefaults.swift:82:28:82:40 | .value | testUserDefaults.swift:82:28:82:31 | .password | testUserDefaults.swift:82:28:82:40 | .value | This operation stores '.value' in the user defaults database. It may contain unencrypted sensitive data from $@. | testUserDefaults.swift:82:28:82:31 | .password | .password |

‎swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,3 +159,17 @@ func test3(x: String) {
159159
NSLog(z.harmless) // Safe
160160
NSLog(z.password) // $ hasCleartextLogging=160
161161
}
162+
163+
struct MyOuter {
164+
struct MyInner {
165+
var value: String
166+
}
167+
168+
var password: MyInner
169+
var harmless: MyInner
170+
}
171+
172+
func test3(mo : MyOuter) {
173+
NSLog(mo.password.value) // $ hasCleartextLogging=173
174+
NSLog(mo.harmless.value) // Safe
175+
}

‎swift/ql/test/query-tests/Security/CWE-312/testUserDefaults.swift

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,3 +68,17 @@ func test4(passwd: String) {
6868
UserDefaults.standard.set(y, forKey: "myKey") // GOOD (not sensitive)
6969
UserDefaults.standard.set(z, forKey: "myKey") // GOOD (not sensitive)
7070
}
71+
72+
struct MyOuter {
73+
struct MyInner {
74+
var value: String
75+
}
76+
77+
var password: MyInner
78+
var harmless: MyInner
79+
}
80+
81+
func test5(mo : MyOuter) {
82+
UserDefaults.standard.set(mo.password.value, forKey: "myKey") // BAD
83+
UserDefaults.standard.set(mo.harmless.value, forKey: "myKey") // GOOD
84+
}

0 commit comments

Comments
 (0)