Skip to content

Swift: Fix some FPs from the sensitive data library #13167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MathiasVP merged 5 commits into from
May 16, 2023
Merged

Swift: Fix some FPs from the sensitive data library #13167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dba9511
Swift: Add more sensitive data test cases.
geoffw0 May 12, 2023
e59d7e0
Swift: Remove assumption that 'username' is not sensitive (in the tes…
geoffw0 May 12, 2023
27c8eb3
Swift: Fix URL-related FPs.
geoffw0 May 15, 2023
a0cba8c
Swift: Address boolean value FPs.
geoffw0 May 12, 2023
4781881
Swift: Improve mobile/phone number regexp.
geoffw0 May 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions swift/ql/lib/codeql/swift/security/CleartextLoggingExtensions.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,18 @@ private class DefaultCleartextLoggingSink extends CleartextLoggingSink {
DefaultCleartextLoggingSink() { sinkNode(this, "log-injection") }
}

/**
* An barrier for cleartext logging vulnerabilities.
* - encryption; encrypted values are not cleartext.
* - booleans; these are more likely to be settings, rather than actual sensitive data.
*/
private class CleartextLoggingDefaultBarrier extends CleartextLoggingBarrier {
CleartextLoggingDefaultBarrier() {
this.asExpr() instanceof EncryptedExpr or
this.asExpr().getType().getUnderlyingType() instanceof BoolType
}
}
Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MathiasVP do you happen to know whether a potentially large set of barriers (here: all expressions of Bool type) is likely to be a performance problem or will only be evaluated in the restricted context of nodes on a potential flow path? (if you're not sure, I will spend a bit of time investigating this)

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A large barrier predicate isn't generally an issue, no 🙂. The barrier will be evaluated prior to any real dataflow computation takes place so it won't be restricted to be nodes along path candidates, but I doubt that this will be an issue.

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've done similar things in C++ in the past (i.e., block flow through all integral-typed expressions), and performance for this has always been fine. So I'm not concerned about this at all.

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!


/**
* A barrier for `OSLogMessage`s configured with the appropriate privacy option.
* Numeric and boolean arguments aren't redacted unless the `private` or `sensitive` options are used.
Expand Down
11 changes: 8 additions & 3 deletions swift/ql/lib/codeql/swift/security/CleartextStorageDatabaseExtensions.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,15 @@ private class CleartextStorageDatabaseSinks extends SinkModelCsv {
}

/**
* An encryption barrier for cleartext database storage vulnerabilities.
* An barrier for cleartext database storage vulnerabilities.
* - encryption; encrypted values are not cleartext.
* - booleans; these are more likely to be settings, rather than actual sensitive data.
*/
private class CleartextStorageDatabaseEncryptionBarrier extends CleartextStorageDatabaseBarrier {
CleartextStorageDatabaseEncryptionBarrier() { this.asExpr() instanceof EncryptedExpr }
private class CleartextStorageDatabaseDefaultBarrier extends CleartextStorageDatabaseBarrier {
CleartextStorageDatabaseDefaultBarrier() {
this.asExpr() instanceof EncryptedExpr or
this.asExpr().getType().getUnderlyingType() instanceof BoolType
}
}

/**
Expand Down
12 changes: 8 additions & 4 deletions swift/ql/lib/codeql/swift/security/CleartextStoragePreferencesExtensions.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,15 @@ private class NSUserDefaultsControllerStore extends CleartextStoragePreferencesS
}

/**
* An encryption barrier for cleartext preferences storage vulnerabilities.
* An barrier for cleartext preferences storage vulnerabilities.
* - encryption; encrypted values are not cleartext.
* - booleans; these are more likely to be settings, rather than actual sensitive data.
*/
private class CleartextStoragePreferencesEncryptionBarrier extends CleartextStoragePreferencesBarrier
{
CleartextStoragePreferencesEncryptionBarrier() { this.asExpr() instanceof EncryptedExpr }
private class CleartextStoragePreferencesDefaultBarrier extends CleartextStoragePreferencesBarrier {
CleartextStoragePreferencesDefaultBarrier() {
this.asExpr() instanceof EncryptedExpr or
this.asExpr().getType().getUnderlyingType() instanceof BoolType
}
}

/**
Expand Down
11 changes: 8 additions & 3 deletions swift/ql/lib/codeql/swift/security/CleartextTransmissionExtensions.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,15 @@ private class AlamofireTransmittedSink extends CleartextTransmissionSink {
}

/**
* An encryption barrier for cleartext transmission vulnerabilities.
* An barrier for cleartext transmission vulnerabilities.
* - encryption; encrypted values are not cleartext.
* - booleans; these are more likely to be settings, rather than actual sensitive data.
*/
private class CleartextTransmissionEncryptionBarrier extends CleartextTransmissionBarrier {
CleartextTransmissionEncryptionBarrier() { this.asExpr() instanceof EncryptedExpr }
private class CleartextTransmissionDefaultBarrier extends CleartextTransmissionBarrier {
CleartextTransmissionDefaultBarrier() {
this.asExpr() instanceof EncryptedExpr or
this.asExpr().getType().getUnderlyingType() instanceof BoolType
}
}

/**
Expand Down
4 changes: 2 additions & 2 deletions swift/ql/lib/codeql/swift/security/SensitiveExprs.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class SensitivePrivateInfo extends SensitiveDataType, TPrivateInfo {
// Contact information, such as home addresses
"post.?code|zip.?code|home.?address|" +
// and telephone numbers
"telephone|home.?phone|mobile|fax.?no|fax.?number|" +
"(mob(ile)?|home).?(num|no|tel|phone)|(tel|fax).?(num|no)|telephone|" +
// Geographic location - where the user is (or was)
"latitude|longitude|" +
// Financial data - such as credit card numbers, salary, bank accounts, and debts
Expand All @@ -69,7 +69,7 @@ class SensitivePrivateInfo extends SensitiveDataType, TPrivateInfo {
* contain hashed or encrypted data, or are only a reference to data that is
* actually stored elsewhere.
*/
private string regexpProbablySafe() { result = ".*(hash|crypt|file|path|invalid).*" }
private string regexpProbablySafe() { result = ".*(hash|crypt|file|path|url|invalid).*" }

/**
* A `VarDecl` that might be used to contain sensitive data.
Expand Down
36 changes: 20 additions & 16 deletions swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ edges
| testSend.swift:33:19:33:19 | passwordPlain | testSend.swift:5:5:5:29 | [summary param] 0 in Data.init(_:) |
| testSend.swift:33:19:33:19 | passwordPlain | testSend.swift:33:14:33:32 | call to Data.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 |
| testSend.swift:47:13:47:25 | call to pad(_:) | testSend.swift:54:27:54:27 | str3 |
| testSend.swift:47:17:47:17 | password | testSend.swift:41:10:41:18 | data |
| testSend.swift:47:17:47:17 | password | testSend.swift:47:13:47:25 | call to pad(_:) |
| testSend.swift:52:13:52:13 | password | testSend.swift:59:27:59:27 | str1 |
| testSend.swift:53:13:53:13 | password | testSend.swift:60:27:60:27 | str2 |
| testSend.swift:54:13:54:25 | call to pad(_:) | testSend.swift:61:27:61:27 | str3 |
| testSend.swift:54:17:54:17 | password | testSend.swift:41:10:41:18 | data |
| testSend.swift:54:17:54:17 | password | testSend.swift:54:13:54:25 | call to pad(_:) |
| 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
Expand All @@ -29,30 +29,34 @@ nodes
| 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 |
| testSend.swift:46:13:46:13 | password | semmle.label | password |
| testSend.swift:47:13:47:25 | call to pad(_:) | semmle.label | call to pad(_:) |
| testSend.swift:47:17:47:17 | password | semmle.label | password |
| testSend.swift:52:27:52:27 | str1 | semmle.label | str1 |
| testSend.swift:53:27:53:27 | str2 | semmle.label | str2 |
| testSend.swift:54:27:54:27 | str3 | semmle.label | str3 |
| testSend.swift:52:13:52:13 | password | semmle.label | password |
| testSend.swift:53:13:53:13 | password | semmle.label | password |
| testSend.swift:54:13:54:25 | call to pad(_:) | semmle.label | call to pad(_:) |
| testSend.swift:54:17:54:17 | password | semmle.label | password |
| testSend.swift:59:27:59:27 | str1 | semmle.label | str1 |
| testSend.swift:60:27:60:27 | str2 | semmle.label | str2 |
| testSend.swift:61:27:61:27 | str3 | semmle.label | str3 |
| testSend.swift:65:27:65:27 | license_key | semmle.label | license_key |
| testSend.swift:66:27:66:30 | .mobileNumber | semmle.label | .mobileNumber |
| testURL.swift:13:22:13:54 | ... .+(_:_:) ... | semmle.label | ... .+(_:_:) ... |
| testURL.swift:13:54:13:54 | passwd | semmle.label | passwd |
| testURL.swift:16:22:16:55 | ... .+(_:_:) ... | semmle.label | ... .+(_:_:) ... |
| 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 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(_:) |
| 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(_:) |
| 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(_:) |
#select
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
15 changes: 9 additions & 6 deletions swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,15 @@
| testRealm.swift:73:15:73:15 | myPassword | label:myPassword, type:credential |
| testSend.swift:29:19:29:19 | passwordPlain | label:passwordPlain, type:credential |
| testSend.swift:33:19:33:19 | passwordPlain | label:passwordPlain, type:credential |
| testSend.swift:45:13:45:13 | password | label:password, type:credential |
| testSend.swift:46:13:46:13 | password | label:password, type:credential |
| testSend.swift:47:17:47:17 | password | label:password, type:credential |
| testSend.swift:48:23:48:23 | password | label:password, type:credential |
| testSend.swift:49:27:49:27 | password | label:password, type:credential |
| testSend.swift:50:27:50:27 | password | label:password, type:credential |
| testSend.swift:52:13:52:13 | password | label:password, type:credential |
| testSend.swift:53:13:53:13 | password | label:password, type:credential |
| testSend.swift:54:17:54:17 | password | label:password, type:credential |
| testSend.swift:55:23:55:23 | password | label:password, type:credential |
| testSend.swift:56:27:56:27 | password | label:password, type:credential |
| testSend.swift:57:27:57:27 | password | label:password, type:credential |
| testSend.swift:65:27:65:27 | license_key | label:license_key, type:credential |
| testSend.swift:66:27:66:30 | .mobileNumber | label:mobileNumber, type:private information |
| testSend.swift:69:27:69:30 | .passwordFeatureEnabled | label:passwordFeatureEnabled, type:credential |
| testURL.swift:13:54:13:54 | passwd | label:passwd, type:credential |
| testURL.swift:16:55:16:55 | credit_card_no | label:credit_card_no, type:private information |
| testURL.swift:20:22:20:22 | passwd | label:passwd, type:credential |
6 changes: 3 additions & 3 deletions swift/ql/test/query-tests/Security/CWE-311/testRealm.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ class MyRealmSwiftObject : RealmSwiftObject {
class MyRealmSwiftObject2 : Object {
override init() { password = "" }

var username: String?
var harmless: String?
var password: String?
}

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

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

let h = MyRealmSwiftObject2()
h.username = myUsername // GOOD (not sensitive)
h.harmless = myHarmless // GOOD (not sensitive)
h.password = myPassword // BAD
realm.add(h)
}
Expand Down
14 changes: 13 additions & 1 deletion swift/ql/test/query-tests/Security/CWE-311/testSend.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,14 @@ func test1(passwordPlain : String, passwordHash : String) {
func pad(_ data: String) -> String { return data }
func aes_crypt(_ data: String) -> String { return data }

func test2(password : String, connection : NWConnection) {
struct MyStruct {
var mobileNumber: String
var mobileUrl: String
var mobilePlayer: String
var passwordFeatureEnabled: Bool
}

func test2(password : String, license_key: String, ms: MyStruct, connection : NWConnection) {
let str1 = password
let str2 = password + " "
let str3 = pad(password)
Expand All @@ -55,4 +62,9 @@ func test2(password : String, connection : NWConnection) {
connection.send(content: str4, completion: .idempotent) // GOOD (encrypted)
connection.send(content: str5, completion: .idempotent) // GOOD (encrypted)
connection.send(content: str6, completion: .idempotent) // GOOD (encrypted)
connection.send(content: license_key, completion: .idempotent) // BAD
connection.send(content: ms.mobileNumber, completion: .idempotent) // BAD
connection.send(content: ms.mobileUrl, completion: .idempotent) // GOOD (not sensitive)
connection.send(content: ms.mobilePlayer, completion: .idempotent) // GOOD (not sensitive)
connection.send(content: ms.passwordFeatureEnabled, completion: .idempotent) // GOOD (not sensitive)
}