diff --git a/swift/ql/lib/codeql/swift/security/CleartextTransmissionExtensions.qll b/swift/ql/lib/codeql/swift/security/CleartextTransmissionExtensions.qll index 752dc278cce8..4b31a7ab23f6 100644 --- a/swift/ql/lib/codeql/swift/security/CleartextTransmissionExtensions.qll +++ b/swift/ql/lib/codeql/swift/security/CleartextTransmissionExtensions.qll @@ -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; + +/** + * 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" } } diff --git a/swift/ql/src/change-notes/2024-08-12-cleartext-transmission.md b/swift/ql/src/change-notes/2024-08-12-cleartext-transmission.md new file mode 100644 index 000000000000..d8f3f3d16d5d --- /dev/null +++ b/swift/ql/src/change-notes/2024-08-12-cleartext-transmission.md @@ -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. diff --git a/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected b/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected index e936d57298a5..c772466344ae 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected +++ b/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected @@ -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 | diff --git a/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected b/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected index 353d6d0a6315..c4ff7f42b2e1 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected +++ b/swift/ql/test/query-tests/Security/CWE-311/SensitiveExprs.expected @@ -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 | diff --git a/swift/ql/test/query-tests/Security/CWE-311/testURL.swift b/swift/ql/test/query-tests/Security/CWE-311/testURL.swift index 1a43fcd66791..8e6a10ce077d 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/testURL.swift +++ b/swift/ql/test/query-tests/Security/CWE-311/testURL.swift @@ -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 +}