mirror of
https://github.com/github/codeql.git
synced 2025-12-16 16:53:25 +01:00
Merge pull request #17210 from geoffw0/mailto
Swift: Fix false positives in the swift/cleartext-transmission query
This commit is contained in:
Geoffrey White
@@ -7,6 +7,7 @@ import swift
|
||||
import codeql.swift.security.SensitiveExprs
|
||||
import codeql.swift.dataflow.DataFlow
|
||||
import codeql.swift.dataflow.ExternalFlow
|
||||
import codeql.swift.dataflow.TaintTracking
|
||||
|
||||
/**
|
||||
* A dataflow sink for cleartext transmission vulnerabilities. That is,
|
||||
@@ -48,6 +49,48 @@ private class AlamofireTransmittedSink extends CleartextTransmissionSink {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A call to `URL.init`.
|
||||
*/
|
||||
private predicate urlInit(CallExpr urlInit, Expr withString) {
|
||||
urlInit
|
||||
.getStaticTarget()
|
||||
.(Method)
|
||||
.hasQualifiedName("URL", ["init(string:)", "init(string:relativeTo:)"]) and
|
||||
urlInit.getArgument(0).getExpr() = withString
|
||||
}
|
||||
|
||||
/**
|
||||
* A data flow configuration for tracking string literals representing `tel:` and similar
|
||||
* URLs to creation of URL objects.
|
||||
*/
|
||||
private module ExcludeUrlConfig implements DataFlow::ConfigSig {
|
||||
predicate isSource(DataFlow::Node node) {
|
||||
node.asExpr()
|
||||
.(StringLiteralExpr)
|
||||
.getValue()
|
||||
.regexpMatch("^(mailto|file|tel|telprompt|callto|sms):.*")
|
||||
}
|
||||
|
||||
predicate isSink(DataFlow::Node node) { urlInit(_, node.asExpr()) }
|
||||
}
|
||||
|
||||
private module ExcludeUrlFlow = TaintTracking::Global<ExcludeUrlConfig>;
|
||||
|
||||
/**
|
||||
* A `URL` that is a sink for this query. Not all URLs are considered sinks, depending
|
||||
* on their content.
|
||||
*/
|
||||
private class UrlTransmittedSink extends CleartextTransmissionSink {
|
||||
UrlTransmittedSink() {
|
||||
urlInit(_, this.asExpr()) and
|
||||
// exclude `tel:` and similar URLs. These URLs necessarily contain
|
||||
// sensitive data which you expect to transmit only by making the
|
||||
// phone call (or similar operation).
|
||||
not ExcludeUrlFlow::flow(_, this)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A barrier for cleartext transmission vulnerabilities.
|
||||
* - encryption; encrypted values are not cleartext.
|
||||
@@ -81,12 +124,6 @@ private class DefaultCleartextTransmissionSink extends CleartextTransmissionSink
|
||||
private class TransmissionSinks extends SinkModelCsv {
|
||||
override predicate row(string row) {
|
||||
row =
|
||||
[
|
||||
";NWConnection;true;send(content:contentContext:isComplete:completion:);;;Argument[0];transmission",
|
||||
// an `Expr` that is used to form a `URL` is very likely to be transmitted over a network, because
|
||||
// that's what URLs are for.
|
||||
";URL;true;init(string:);;;Argument[0];transmission",
|
||||
";URL;true;init(string:relativeTo:);;;Argument[0];transmission",
|
||||
]
|
||||
";NWConnection;true;send(content:contentContext:isComplete:completion:);;;Argument[0];transmission"
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,4 @@
|
||||
---
|
||||
category: minorAnalysis
|
||||
---
|
||||
* False positive results from the `swift/cleartext-transmission` ("Cleartext transmission of sensitive information") query involving `tel:`, `mailto:` and similar URLs have been fixed.
|
||||
@@ -32,6 +32,9 @@ edges
|
||||
| testURL.swift:105:19:105:53 | call to String.init(data:encoding:) | testURL.swift:106:20:106:20 | "..." | provenance | |
|
||||
| testURL.swift:105:19:105:53 | call to String.init(data:encoding:) [some:0] | testURL.swift:105:6:105:10 | let ...? [some:0] | provenance | |
|
||||
| testURL.swift:105:32:105:32 | data | testURL.swift:105:19:105:53 | call to String.init(data:encoding:) [some:0] | provenance | |
|
||||
| testURL.swift:116:52:116:52 | email | testURL.swift:116:18:116:18 | "..." | provenance | |
|
||||
| testURL.swift:123:52:123:52 | phone_number | testURL.swift:123:18:123:18 | "..." | provenance | |
|
||||
| testURL.swift:132:39:132:39 | account_no | testURL.swift:132:18:132:18 | "..." | provenance | |
|
||||
nodes
|
||||
| file://:0:0:0:0 | .value | semmle.label | .value |
|
||||
| file://:0:0:0:0 | self | semmle.label | self |
|
||||
@@ -90,6 +93,12 @@ nodes
|
||||
| testURL.swift:105:19:105:53 | call to String.init(data:encoding:) [some:0] | semmle.label | call to String.init(data:encoding:) [some:0] |
|
||||
| testURL.swift:105:32:105:32 | data | semmle.label | data |
|
||||
| testURL.swift:106:20:106:20 | "..." | semmle.label | "..." |
|
||||
| testURL.swift:116:18:116:18 | "..." | semmle.label | "..." |
|
||||
| testURL.swift:116:52:116:52 | email | semmle.label | email |
|
||||
| testURL.swift:123:18:123:18 | "..." | semmle.label | "..." |
|
||||
| testURL.swift:123:52:123:52 | phone_number | semmle.label | phone_number |
|
||||
| testURL.swift:132:18:132:18 | "..." | semmle.label | "..." |
|
||||
| testURL.swift:132:39:132:39 | account_no | semmle.label | account_no |
|
||||
subpaths
|
||||
| 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(_:) |
|
||||
| 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
|
||||
| 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() |
|
||||
| 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 |
|
||||
| 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(_:_:) |
|
||||
| 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 |
|
||||
| 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 |
|
||||
| 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 |
|
||||
|
||||
@@ -182,3 +182,14 @@
|
||||
| testURL.swift:75:53:75:69 | call to get_cert_string() | label:get_cert_string, type:credential |
|
||||
| testURL.swift:96:51:96:51 | certificate | label:certificate, type:credential |
|
||||
| testURL.swift:104:16:104:57 | call to SecKeyCopyExternalRepresentation(_:_:) | label:password, type:password |
|
||||
| testURL.swift:116:52:116:52 | email | label:email, type:private information |
|
||||
| testURL.swift:117:28:117:28 | email | label:email, type:private information |
|
||||
| testURL.swift:118:53:118:53 | secret_key | label:secret_key, type:credential |
|
||||
| testURL.swift:119:60:119:60 | email | label:email, type:private information |
|
||||
| testURL.swift:123:52:123:52 | phone_number | label:phone_number, type:private information |
|
||||
| testURL.swift:124:25:124:25 | phone_number | label:phone_number, type:private information |
|
||||
| testURL.swift:125:31:125:31 | phone_number | label:phone_number, type:private information |
|
||||
| testURL.swift:126:28:126:28 | phone_number | label:phone_number, type:private information |
|
||||
| testURL.swift:127:25:127:25 | phone_number | label:phone_number, type:private information |
|
||||
| testURL.swift:131:37:131:37 | account_no | label:account_no, type:private information |
|
||||
| testURL.swift:132:39:132:39 | account_no | label:account_no, type:private information |
|
||||
|
||||
@@ -107,3 +107,27 @@ func test4(key: SecKey) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func test5() {
|
||||
// variant URL types...
|
||||
let email = get_string()
|
||||
let secret_key = get_string()
|
||||
|
||||
_ = URL(string: "http://example.com/login?email=\(email)"); // BAD
|
||||
_ = URL(string: "mailto:\(email)"); // GOOD (revealing your e-amil address in an e-mail is expected)
|
||||
_ = URL(string: "mailto:info@example.com?subject=\(secret_key)"); // BAD [NOT DETECTED]
|
||||
_ = URL(string: "mailto:info@example.com?subject=foo&cc=\(email)"); // GOOD
|
||||
|
||||
let phone_number = get_string()
|
||||
|
||||
_ = URL(string: "http://example.com/profile?tel=\(phone_number)"); // BAD
|
||||
_ = URL(string: "tel:\(phone_number)") // GOOD
|
||||
_ = URL(string: "telprompt:\(phone_number)") // GOOD
|
||||
_ = URL(string: "callto:\(phone_number)") // GOOD
|
||||
_ = URL(string: "sms:\(phone_number)") // GOOD
|
||||
|
||||
let account_no = get_string()
|
||||
|
||||
_ = URL(string: "file:///foo/bar/\(account_no).csv") // GOOD (local, so not transmitted)
|
||||
_ = URL(string: "ftp://example.com/\(account_no).csv") // BAD
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user