Skip to content

Commit e3b9b0a

Browse files
geoffw0geoffw0
authored
Merge pull request #17210 from geoffw0/mailto
Swift: Fix false positives in the swift/cleartext-transmission query
2 parents 3a96107 + 8646643 commit e3b9b0a

File tree

5 files changed

+95
-7
lines changed

5 files changed

+95
-7
lines changed

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

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import swift
77
import codeql.swift.security.SensitiveExprs
88
import codeql.swift.dataflow.DataFlow
99
import codeql.swift.dataflow.ExternalFlow
10+
import codeql.swift.dataflow.TaintTracking
1011

1112
/**
1213
* A dataflow sink for cleartext transmission vulnerabilities. That is,
@@ -48,6 +49,48 @@ private class AlamofireTransmittedSink extends CleartextTransmissionSink {
4849
}
4950
}
5051

52+
/**
53+
* A call to `URL.init`.
54+
*/
55+
private predicate urlInit(CallExpr urlInit, Expr withString) {
56+
urlInit
57+
.getStaticTarget()
58+
.(Method)
59+
.hasQualifiedName("URL", ["init(string:)", "init(string:relativeTo:)"]) and
60+
urlInit.getArgument(0).getExpr() = withString
61+
}
62+
63+
/**
64+
* A data flow configuration for tracking string literals representing `tel:` and similar
65+
* URLs to creation of URL objects.
66+
*/
67+
private module ExcludeUrlConfig implements DataFlow::ConfigSig {
68+
predicate isSource(DataFlow::Node node) {
69+
node.asExpr()
70+
.(StringLiteralExpr)
71+
.getValue()
72+
.regexpMatch("^(mailto|file|tel|telprompt|callto|sms):.*")
73+
}
74+
75+
predicate isSink(DataFlow::Node node) { urlInit(_, node.asExpr()) }
76+
}
77+
78+
private module ExcludeUrlFlow = TaintTracking::Global<ExcludeUrlConfig>;
79+
80+
/**
81+
* A `URL` that is a sink for this query. Not all URLs are considered sinks, depending
82+
* on their content.
83+
*/
84+
private class UrlTransmittedSink extends CleartextTransmissionSink {
85+
UrlTransmittedSink() {
86+
urlInit(_, this.asExpr()) and
87+
// exclude `tel:` and similar URLs. These URLs necessarily contain
88+
// sensitive data which you expect to transmit only by making the
89+
// phone call (or similar operation).
90+
not ExcludeUrlFlow::flow(_, this)
91+
}
92+
}
93+
5194
/**
5295
* A barrier for cleartext transmission vulnerabilities.
5396
* - encryption; encrypted values are not cleartext.
@@ -81,12 +124,6 @@ private class DefaultCleartextTransmissionSink extends CleartextTransmissionSink
81124
private class TransmissionSinks extends SinkModelCsv {
82125
override predicate row(string row) {
83126
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-
]
127+
";NWConnection;true;send(content:contentContext:isComplete:completion:);;;Argument[0];transmission"
91128
}
92129
}

‎swift/ql/src/change-notes/2024-08-12-cleartext-transmission.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* False positive results from the `swift/cleartext-transmission` ("Cleartext transmission of sensitive information") query involving `tel:`, `mailto:` and similar URLs have been fixed.

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ edges
3232
| testURL.swift:105:19:105:53 | call to String.init(data:encoding:) | testURL.swift:106:20:106:20 | "..." | provenance | |
3333
| testURL.swift:105:19:105:53 | call to String.init(data:encoding:) [some:0] | testURL.swift:105:6:105:10 | let ...? [some:0] | provenance | |
3434
| testURL.swift:105:32:105:32 | data | testURL.swift:105:19:105:53 | call to String.init(data:encoding:) [some:0] | provenance | |
35+
| testURL.swift:116:52:116:52 | email | testURL.swift:116:18:116:18 | "..." | provenance | |
36+
| testURL.swift:123:52:123:52 | phone_number | testURL.swift:123:18:123:18 | "..." | provenance | |
37+
| testURL.swift:132:39:132:39 | account_no | testURL.swift:132:18:132:18 | "..." | provenance | |
3538
nodes
3639
| file://:0:0:0:0 | .value | semmle.label | .value |
3740
| file://:0:0:0:0 | self | semmle.label | self |
@@ -90,6 +93,12 @@ nodes
9093
| testURL.swift:105:19:105:53 | call to String.init(data:encoding:) [some:0] | semmle.label | call to String.init(data:encoding:) [some:0] |
9194
| testURL.swift:105:32:105:32 | data | semmle.label | data |
9295
| testURL.swift:106:20:106:20 | "..." | semmle.label | "..." |
96+
| testURL.swift:116:18:116:18 | "..." | semmle.label | "..." |
97+
| testURL.swift:116:52:116:52 | email | semmle.label | email |
98+
| testURL.swift:123:18:123:18 | "..." | semmle.label | "..." |
99+
| testURL.swift:123:52:123:52 | phone_number | semmle.label | phone_number |
100+
| testURL.swift:132:18:132:18 | "..." | semmle.label | "..." |
101+
| testURL.swift:132:39:132:39 | account_no | semmle.label | account_no |
93102
subpaths
94103
| 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(_:) |
95104
| 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 |
@@ -121,3 +130,6 @@ subpaths
121130
| testURL.swift:75:18:75:69 | ... .+(_:_:) ... | testURL.swift:75:53:75:69 | call to get_cert_string() | testURL.swift:75:18:75:69 | ... .+(_:_:) ... | This operation transmits '... .+(_:_:) ...', which may contain unencrypted sensitive data from $@. | testURL.swift:75:53:75:69 | call to get_cert_string() | call to get_cert_string() |
122131
| testURL.swift:96:18:96:18 | "..." | testURL.swift:96:51:96:51 | certificate | testURL.swift:96:18:96:18 | "..." | This operation transmits '"..."', which may contain unencrypted sensitive data from $@. | testURL.swift:96:51:96:51 | certificate | certificate |
123132
| testURL.swift:106:20:106:20 | "..." | testURL.swift:104:16:104:57 | call to SecKeyCopyExternalRepresentation(_:_:) | testURL.swift:106:20:106:20 | "..." | This operation transmits '"..."', which may contain unencrypted sensitive data from $@. | testURL.swift:104:16:104:57 | call to SecKeyCopyExternalRepresentation(_:_:) | call to SecKeyCopyExternalRepresentation(_:_:) |
133+
| testURL.swift:116:18:116:18 | "..." | testURL.swift:116:52:116:52 | email | testURL.swift:116:18:116:18 | "..." | This operation transmits '"..."', which may contain unencrypted sensitive data from $@. | testURL.swift:116:52:116:52 | email | email |
134+
| testURL.swift:123:18:123:18 | "..." | testURL.swift:123:52:123:52 | phone_number | testURL.swift:123:18:123:18 | "..." | This operation transmits '"..."', which may contain unencrypted sensitive data from $@. | testURL.swift:123:52:123:52 | phone_number | phone_number |
135+
| testURL.swift:132:18:132:18 | "..." | testURL.swift:132:39:132:39 | account_no | testURL.swift:132:18:132:18 | "..." | This operation transmits '"..."', which may contain unencrypted sensitive data from $@. | testURL.swift:132:39:132:39 | account_no | account_no |

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,3 +182,14 @@
182182
| testURL.swift:75:53:75:69 | call to get_cert_string() | label:get_cert_string, type:credential |
183183
| testURL.swift:96:51:96:51 | certificate | label:certificate, type:credential |
184184
| testURL.swift:104:16:104:57 | call to SecKeyCopyExternalRepresentation(_:_:) | label:password, type:password |
185+
| testURL.swift:116:52:116:52 | email | label:email, type:private information |
186+
| testURL.swift:117:28:117:28 | email | label:email, type:private information |
187+
| testURL.swift:118:53:118:53 | secret_key | label:secret_key, type:credential |
188+
| testURL.swift:119:60:119:60 | email | label:email, type:private information |
189+
| testURL.swift:123:52:123:52 | phone_number | label:phone_number, type:private information |
190+
| testURL.swift:124:25:124:25 | phone_number | label:phone_number, type:private information |
191+
| testURL.swift:125:31:125:31 | phone_number | label:phone_number, type:private information |
192+
| testURL.swift:126:28:126:28 | phone_number | label:phone_number, type:private information |
193+
| testURL.swift:127:25:127:25 | phone_number | label:phone_number, type:private information |
194+
| testURL.swift:131:37:131:37 | account_no | label:account_no, type:private information |
195+
| testURL.swift:132:39:132:39 | account_no | label:account_no, type:private information |

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,3 +107,27 @@ func test4(key: SecKey) {
107107
}
108108
}
109109
}
110+
111+
func test5() {
112+
// variant URL types...
113+
let email = get_string()
114+
let secret_key = get_string()
115+
116+
_ = URL(string: "http://example.com/login?email=\(email)"); // BAD
117+
_ = URL(string: "mailto:\(email)"); // GOOD (revealing your e-amil address in an e-mail is expected)
118+
_ = URL(string: "mailto:[email protected]?subject=\(secret_key)"); // BAD [NOT DETECTED]
119+
_ = URL(string: "mailto:[email protected]?subject=foo&cc=\(email)"); // GOOD
120+
121+
let phone_number = get_string()
122+
123+
_ = URL(string: "http://example.com/profile?tel=\(phone_number)"); // BAD
124+
_ = URL(string: "tel:\(phone_number)") // GOOD
125+
_ = URL(string: "telprompt:\(phone_number)") // GOOD
126+
_ = URL(string: "callto:\(phone_number)") // GOOD
127+
_ = URL(string: "sms:\(phone_number)") // GOOD
128+
129+
let account_no = get_string()
130+
131+
_ = URL(string: "file:///foo/bar/\(account_no).csv") // GOOD (local, so not transmitted)
132+
_ = URL(string: "ftp://example.com/\(account_no).csv") // BAD
133+
}

0 commit comments

Comments
 (0)