Skip to content

Commit f5be8cf

Browse files
MathiasVPMathiasVP
authored
Merge pull request #13167 from geoffw0/sensitivefps
Swift: Fix some FPs from the sensitive data library
2 parents afd1a12 + 4781881 commit f5be8cf

File tree

9 files changed

+83
-38
lines changed

9 files changed

+83
-38
lines changed

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,18 @@ private class DefaultCleartextLoggingSink extends CleartextLoggingSink {
2929
DefaultCleartextLoggingSink() { sinkNode(this, "log-injection") }
3030
}
3131

32+
/**
33+
* An barrier for cleartext logging vulnerabilities.
34+
* - encryption; encrypted values are not cleartext.
35+
* - booleans; these are more likely to be settings, rather than actual sensitive data.
36+
*/
37+
private class CleartextLoggingDefaultBarrier extends CleartextLoggingBarrier {
38+
CleartextLoggingDefaultBarrier() {
39+
this.asExpr() instanceof EncryptedExpr or
40+
this.asExpr().getType().getUnderlyingType() instanceof BoolType
41+
}
42+
}
43+
3244
/**
3345
* A barrier for `OSLogMessage`s configured with the appropriate privacy option.
3446
* Numeric and boolean arguments aren't redacted unless the `private` or `sensitive` options are used.

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,10 +114,15 @@ private class CleartextStorageDatabaseSinks extends SinkModelCsv {
114114
}
115115

116116
/**
117-
* An encryption barrier for cleartext database storage vulnerabilities.
117+
* An barrier for cleartext database storage vulnerabilities.
118+
* - encryption; encrypted values are not cleartext.
119+
* - booleans; these are more likely to be settings, rather than actual sensitive data.
118120
*/
119-
private class CleartextStorageDatabaseEncryptionBarrier extends CleartextStorageDatabaseBarrier {
120-
CleartextStorageDatabaseEncryptionBarrier() { this.asExpr() instanceof EncryptedExpr }
121+
private class CleartextStorageDatabaseDefaultBarrier extends CleartextStorageDatabaseBarrier {
122+
CleartextStorageDatabaseDefaultBarrier() {
123+
this.asExpr() instanceof EncryptedExpr or
124+
this.asExpr().getType().getUnderlyingType() instanceof BoolType
125+
}
121126
}
122127

123128
/**

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,15 @@ private class NSUserDefaultsControllerStore extends CleartextStoragePreferencesS
7474
}
7575

7676
/**
77-
* An encryption barrier for cleartext preferences storage vulnerabilities.
77+
* An barrier for cleartext preferences storage vulnerabilities.
78+
* - encryption; encrypted values are not cleartext.
79+
* - booleans; these are more likely to be settings, rather than actual sensitive data.
7880
*/
79-
private class CleartextStoragePreferencesEncryptionBarrier extends CleartextStoragePreferencesBarrier
80-
{
81-
CleartextStoragePreferencesEncryptionBarrier() { this.asExpr() instanceof EncryptedExpr }
81+
private class CleartextStoragePreferencesDefaultBarrier extends CleartextStoragePreferencesBarrier {
82+
CleartextStoragePreferencesDefaultBarrier() {
83+
this.asExpr() instanceof EncryptedExpr or
84+
this.asExpr().getType().getUnderlyingType() instanceof BoolType
85+
}
8286
}
8387

8488
/**

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,10 +81,15 @@ private class AlamofireTransmittedSink extends CleartextTransmissionSink {
8181
}
8282

8383
/**
84-
* An encryption barrier for cleartext transmission vulnerabilities.
84+
* An barrier for cleartext transmission vulnerabilities.
85+
* - encryption; encrypted values are not cleartext.
86+
* - booleans; these are more likely to be settings, rather than actual sensitive data.
8587
*/
86-
private class CleartextTransmissionEncryptionBarrier extends CleartextTransmissionBarrier {
87-
CleartextTransmissionEncryptionBarrier() { this.asExpr() instanceof EncryptedExpr }
88+
private class CleartextTransmissionDefaultBarrier extends CleartextTransmissionBarrier {
89+
CleartextTransmissionDefaultBarrier() {
90+
this.asExpr() instanceof EncryptedExpr or
91+
this.asExpr().getType().getUnderlyingType() instanceof BoolType
92+
}
8893
}
8994

9095
/**

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ class SensitivePrivateInfo extends SensitiveDataType, TPrivateInfo {
4848
// Contact information, such as home addresses
4949
"post.?code|zip.?code|home.?address|" +
5050
// and telephone numbers
51-
"telephone|home.?phone|mobile|fax.?no|fax.?number|" +
51+
"(mob(ile)?|home).?(num|no|tel|phone)|(tel|fax).?(num|no)|telephone|" +
5252
// Geographic location - where the user is (or was)
5353
"latitude|longitude|" +
5454
// Financial data - such as credit card numbers, salary, bank accounts, and debts
@@ -69,7 +69,7 @@ class SensitivePrivateInfo extends SensitiveDataType, TPrivateInfo {
6969
* contain hashed or encrypted data, or are only a reference to data that is
7070
* actually stored elsewhere.
7171
*/
72-
private string regexpProbablySafe() { result = ".*(hash|crypt|file|path|invalid).*" }
72+
private string regexpProbablySafe() { result = ".*(hash|crypt|file|path|url|invalid).*" }
7373

7474
/**
7575
* A `VarDecl` that might be used to contain sensitive data.

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

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@ edges
77
| testSend.swift:33:19:33:19 | passwordPlain | testSend.swift:5:5:5:29 | [summary param] 0 in Data.init(_:) |
88
| testSend.swift:33:19:33:19 | passwordPlain | testSend.swift:33:14:33:32 | call to Data.init(_:) |
99
| testSend.swift:41:10:41:18 | data | testSend.swift:41:45:41:45 | data |
10-
| testSend.swift:45:13:45:13 | password | testSend.swift:52:27:52:27 | str1 |
11-
| testSend.swift:46:13:46:13 | password | testSend.swift:53:27:53:27 | str2 |
12-
| testSend.swift:47:13:47:25 | call to pad(_:) | testSend.swift:54:27:54:27 | str3 |
13-
| testSend.swift:47:17:47:17 | password | testSend.swift:41:10:41:18 | data |
14-
| testSend.swift:47:17:47:17 | password | testSend.swift:47:13:47:25 | call to pad(_:) |
10+
| testSend.swift:52:13:52:13 | password | testSend.swift:59:27:59:27 | str1 |
11+
| testSend.swift:53:13:53:13 | password | testSend.swift:60:27:60:27 | str2 |
12+
| testSend.swift:54:13:54:25 | call to pad(_:) | testSend.swift:61:27:61:27 | str3 |
13+
| testSend.swift:54:17:54:17 | password | testSend.swift:41:10:41:18 | data |
14+
| testSend.swift:54:17:54:17 | password | testSend.swift:54:13:54:25 | call to pad(_:) |
1515
| testURL.swift:13:54:13:54 | passwd | testURL.swift:13:22:13:54 | ... .+(_:_:) ... |
1616
| testURL.swift:16:55:16:55 | credit_card_no | testURL.swift:16:22:16:55 | ... .+(_:_:) ... |
1717
nodes
@@ -29,30 +29,34 @@ nodes
2929
| testSend.swift:37:19:37:19 | data2 | semmle.label | data2 |
3030
| testSend.swift:41:10:41:18 | data | semmle.label | data |
3131
| testSend.swift:41:45:41:45 | data | semmle.label | data |
32-
| testSend.swift:45:13:45:13 | password | semmle.label | password |
33-
| testSend.swift:46:13:46:13 | password | semmle.label | password |
34-
| testSend.swift:47:13:47:25 | call to pad(_:) | semmle.label | call to pad(_:) |
35-
| testSend.swift:47:17:47:17 | password | semmle.label | password |
36-
| testSend.swift:52:27:52:27 | str1 | semmle.label | str1 |
37-
| testSend.swift:53:27:53:27 | str2 | semmle.label | str2 |
38-
| testSend.swift:54:27:54:27 | str3 | semmle.label | str3 |
32+
| testSend.swift:52:13:52:13 | password | semmle.label | password |
33+
| testSend.swift:53:13:53:13 | password | semmle.label | password |
34+
| testSend.swift:54:13:54:25 | call to pad(_:) | semmle.label | call to pad(_:) |
35+
| testSend.swift:54:17:54:17 | password | semmle.label | password |
36+
| testSend.swift:59:27:59:27 | str1 | semmle.label | str1 |
37+
| testSend.swift:60:27:60:27 | str2 | semmle.label | str2 |
38+
| testSend.swift:61:27:61:27 | str3 | semmle.label | str3 |
39+
| testSend.swift:65:27:65:27 | license_key | semmle.label | license_key |
40+
| testSend.swift:66:27:66:30 | .mobileNumber | semmle.label | .mobileNumber |
3941
| testURL.swift:13:22:13:54 | ... .+(_:_:) ... | semmle.label | ... .+(_:_:) ... |
4042
| testURL.swift:13:54:13:54 | passwd | semmle.label | passwd |
4143
| testURL.swift:16:22:16:55 | ... .+(_:_:) ... | semmle.label | ... .+(_:_:) ... |
4244
| testURL.swift:16:55:16:55 | credit_card_no | semmle.label | credit_card_no |
4345
| testURL.swift:20:22:20:22 | passwd | semmle.label | passwd |
4446
subpaths
4547
| testSend.swift:33:19:33:19 | passwordPlain | testSend.swift:5:5:5:29 | [summary param] 0 in Data.init(_:) | file://:0:0:0:0 | [summary] to write: return (return) in Data.init(_:) | testSend.swift:33:14:33:32 | call to Data.init(_:) |
46-
| 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(_:) |
48+
| testSend.swift:54:17:54:17 | password | testSend.swift:41:10:41:18 | data | testSend.swift:41:45:41:45 | data | testSend.swift:54:13:54:25 | call to pad(_:) |
4749
#select
4850
| 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 |
4951
| 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 |
5052
| testAlamofire.swift:154:14:154:46 | ... .+(_:_:) ... | testAlamofire.swift:154:38:154:38 | email | testAlamofire.swift:154:14:154:46 | ... .+(_:_:) ... | This operation transmits '... .+(_:_:) ...', which may contain unencrypted sensitive data from $@. | testAlamofire.swift:154:38:154:38 | email | email |
5153
| 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 |
5254
| 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 |
53-
| 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 |
54-
| 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 |
55-
| 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 |
55+
| testSend.swift:59:27:59:27 | str1 | testSend.swift:52:13:52:13 | password | testSend.swift:59:27:59:27 | str1 | This operation transmits 'str1', which may contain unencrypted sensitive data from $@. | testSend.swift:52:13:52:13 | password | password |
56+
| testSend.swift:60:27:60:27 | str2 | testSend.swift:53:13:53:13 | password | testSend.swift:60:27:60:27 | str2 | This operation transmits 'str2', which may contain unencrypted sensitive data from $@. | testSend.swift:53:13:53:13 | password | password |
57+
| testSend.swift:61:27:61:27 | str3 | testSend.swift:54:17:54:17 | password | testSend.swift:61:27:61:27 | str3 | This operation transmits 'str3', which may contain unencrypted sensitive data from $@. | testSend.swift:54:17:54:17 | password | password |
58+
| testSend.swift:65:27:65:27 | license_key | testSend.swift:65:27:65:27 | license_key | testSend.swift:65:27:65:27 | license_key | This operation transmits 'license_key', which may contain unencrypted sensitive data from $@. | testSend.swift:65:27:65:27 | license_key | license_key |
59+
| testSend.swift:66:27:66:30 | .mobileNumber | testSend.swift:66:27:66:30 | .mobileNumber | testSend.swift:66:27:66:30 | .mobileNumber | This operation transmits '.mobileNumber', which may contain unencrypted sensitive data from $@. | testSend.swift:66:27:66:30 | .mobileNumber | .mobileNumber |
5660
| testURL.swift:13:22:13:54 | ... .+(_:_:) ... | testURL.swift:13:54:13:54 | passwd | testURL.swift:13:22:13:54 | ... .+(_:_:) ... | This operation transmits '... .+(_:_:) ...', which may contain unencrypted sensitive data from $@. | testURL.swift:13:54:13:54 | passwd | passwd |
5761
| testURL.swift:16:22:16:55 | ... .+(_:_:) ... | testURL.swift:16:55:16:55 | credit_card_no | testURL.swift:16:22:16:55 | ... .+(_:_:) ... | This operation transmits '... .+(_:_:) ...', which may contain unencrypted sensitive data from $@. | testURL.swift:16:55:16:55 | credit_card_no | credit_card_no |
5862
| testURL.swift:20:22:20:22 | passwd | testURL.swift:20:22:20:22 | passwd | testURL.swift:20:22:20:22 | passwd | This operation transmits 'passwd', which may contain unencrypted sensitive data from $@. | testURL.swift:20:22:20:22 | passwd | passwd |

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -120,12 +120,15 @@
120120
| testRealm.swift:73:15:73:15 | myPassword | label:myPassword, type:credential |
121121
| testSend.swift:29:19:29:19 | passwordPlain | label:passwordPlain, type:credential |
122122
| testSend.swift:33:19:33:19 | passwordPlain | label:passwordPlain, type:credential |
123-
| testSend.swift:45:13:45:13 | password | label:password, type:credential |
124-
| testSend.swift:46:13:46:13 | password | label:password, type:credential |
125-
| testSend.swift:47:17:47:17 | password | label:password, type:credential |
126-
| testSend.swift:48:23:48:23 | password | label:password, type:credential |
127-
| testSend.swift:49:27:49:27 | password | label:password, type:credential |
128-
| testSend.swift:50:27:50:27 | password | label:password, type:credential |
123+
| testSend.swift:52:13:52:13 | password | label:password, type:credential |
124+
| testSend.swift:53:13:53:13 | password | label:password, type:credential |
125+
| testSend.swift:54:17:54:17 | password | label:password, type:credential |
126+
| testSend.swift:55:23:55:23 | password | label:password, type:credential |
127+
| testSend.swift:56:27:56:27 | password | label:password, type:credential |
128+
| testSend.swift:57:27:57:27 | password | label:password, type:credential |
129+
| testSend.swift:65:27:65:27 | license_key | label:license_key, type:credential |
130+
| testSend.swift:66:27:66:30 | .mobileNumber | label:mobileNumber, type:private information |
131+
| testSend.swift:69:27:69:30 | .passwordFeatureEnabled | label:passwordFeatureEnabled, type:credential |
129132
| testURL.swift:13:54:13:54 | passwd | label:passwd, type:credential |
130133
| testURL.swift:16:55:16:55 | credit_card_no | label:credit_card_no, type:private information |
131134
| testURL.swift:20:22:20:22 | passwd | label:passwd, type:credential |

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,11 @@ class MyRealmSwiftObject : RealmSwiftObject {
3030
class MyRealmSwiftObject2 : Object {
3131
override init() { password = "" }
3232

33-
var username: String?
33+
var harmless: String?
3434
var password: String?
3535
}
3636

37-
func test1(realm : Realm, myUsername: String, myPassword : String, myHashedPassword : String) {
37+
func test1(realm : Realm, myHarmless: String, myPassword : String, myHashedPassword : String) {
3838
// add objects (within a transaction) ...
3939

4040
let a = MyRealmSwiftObject()
@@ -69,7 +69,7 @@ func test1(realm : Realm, myUsername: String, myPassword : String, myHashedPassw
6969
// MyRealmSwiftObject2...
7070

7171
let h = MyRealmSwiftObject2()
72-
h.username = myUsername // GOOD (not sensitive)
72+
h.harmless = myHarmless // GOOD (not sensitive)
7373
h.password = myPassword // BAD
7474
realm.add(h)
7575
}

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,14 @@ func test1(passwordPlain : String, passwordHash : String) {
4141
func pad(_ data: String) -> String { return data }
4242
func aes_crypt(_ data: String) -> String { return data }
4343

44-
func test2(password : String, connection : NWConnection) {
44+
struct MyStruct {
45+
var mobileNumber: String
46+
var mobileUrl: String
47+
var mobilePlayer: String
48+
var passwordFeatureEnabled: Bool
49+
}
50+
51+
func test2(password : String, license_key: String, ms: MyStruct, connection : NWConnection) {
4552
let str1 = password
4653
let str2 = password + " "
4754
let str3 = pad(password)
@@ -55,4 +62,9 @@ func test2(password : String, connection : NWConnection) {
5562
connection.send(content: str4, completion: .idempotent) // GOOD (encrypted)
5663
connection.send(content: str5, completion: .idempotent) // GOOD (encrypted)
5764
connection.send(content: str6, completion: .idempotent) // GOOD (encrypted)
65+
connection.send(content: license_key, completion: .idempotent) // BAD
66+
connection.send(content: ms.mobileNumber, completion: .idempotent) // BAD
67+
connection.send(content: ms.mobileUrl, completion: .idempotent) // GOOD (not sensitive)
68+
connection.send(content: ms.mobilePlayer, completion: .idempotent) // GOOD (not sensitive)
69+
connection.send(content: ms.passwordFeatureEnabled, completion: .idempotent) // GOOD (not sensitive)
5870
}

0 commit comments

Comments
 (0)