Swift: Replace sinks with (extendable) CSV.

2026-04-25 00:35:20 +02:00 · 2023-03-20 18:45:36 +00:00
parent 4c0d02a87d
commit e266132f0a
4 changed files with 66 additions and 97 deletions
--- a/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll
+++ b/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll
@@ -16,7 +16,8 @@
 * 1. The `namespace` column selects a package.
 * 2. The `type` column selects a type within that package.
 * 3. The `subtypes` is a boolean that indicates whether to jump to an
- *    arbitrary subtype of that type.
+ *    arbitrary subtype of that type. Set this to `false` if leaving the `type`
+ *    blank (for example, a free function).
 * 4. The `name` column optionally selects a specific named member of the type.
 * 5. The `signature` column optionally restricts the named member. If
 *    `signature` is blank then no such filtering is done. The format of the
--- a/swift/ql/lib/codeql/swift/security/StringLengthConflationExtensions.qll
+++ b/swift/ql/lib/codeql/swift/security/StringLengthConflationExtensions.qll
@@ -5,6 +5,7 @@

 import swift
 import codeql.swift.dataflow.DataFlow
+import codeql.swift.dataflow.ExternalFlow

 /**
 * A type of Swift string encoding. This class is used as a flow state for
@@ -13,31 +14,50 @@ import codeql.swift.dataflow.DataFlow
 class StringType extends string {
  string singular;
  string equivClass;
+  string csvLabel;

  StringType() {
-    this = "String" and singular = "a String" and equivClass = "String"
+    this = "String" and
+    singular = "a String" and
+    equivClass = "String" and
+    csvLabel = "string-length"
    or
-    this = "NSString" and singular = "an NSString" and equivClass = "NSString"
+    this = "NSString" and
+    singular = "an NSString" and
+    equivClass = "NSString" and
+    csvLabel = "nsstring-length"
    or
-    this = "String.utf8" and singular = "a String.utf8" and equivClass = "String.utf8"
+    this = "String.utf8" and
+    singular = "a String.utf8" and
+    equivClass = "String.utf8" and
+    csvLabel = "string-utf8-length"
    or
-    this = "String.utf16" and singular = "a String.utf16" and equivClass = "NSString"
+    this = "String.utf16" and
+    singular = "a String.utf16" and
+    equivClass = "NSString" and
+    csvLabel = "string-utf16-length"
    or
    this = "String.unicodeScalars" and
    singular = "a String.unicodeScalars" and
-    equivClass = "String.unicodeScalars"
+    equivClass = "String.unicodeScalars" and
+    csvLabel = "string-unicodescalars-length"
  }

  /**
-   * Gets the equivalence class for this flow state. If these are equal,
+   * Gets the equivalence class for this string type. If these are equal,
   * they should be treated as equivalent.
   */
  string getEquivClass() { result = equivClass }

  /**
-   * Gets text for the singular form of this flow state.
+   * Gets text for the singular form of this string type.
   */
  string getSingular() { result = singular }
+
+  /**
+   * Gets the label for this string type in CSV models.
+   */
+  string getCsvLabel() { result = csvLabel }
 }

 /**
@@ -115,83 +135,41 @@ private class DefaultStringLengthConflationSource extends StringLengthConflation
  override StringType getStringType() { result = stringType }
 }

+/**
+ * A sink defined in a CSV model.
+ */
 private class DefaultStringLengthConflationSink extends StringLengthConflationSink {
-  StringType correctStringType;
+  StringType stringType;

-  DefaultStringLengthConflationSink() {
-    exists(AbstractFunctionDecl funcDecl, CallExpr call, string funcName, int arg |
-      (
-        // arguments to method calls...
-        exists(string className, ClassOrStructDecl c |
-          (
-            // `NSRange.init`
-            className = "NSRange" and
-            funcName = "init(location:length:)" and
-            arg = [0, 1]
-            or
-            // `NSString.character`
-            className = ["NSString", "NSMutableString"] and
-            funcName = "character(at:)" and
-            arg = 0
-            or
-            // `NSString.character`
-            className = ["NSString", "NSMutableString"] and
-            funcName = "substring(from:)" and
-            arg = 0
-            or
-            // `NSString.character`
-            className = ["NSString", "NSMutableString"] and
-            funcName = "substring(to:)" and
-            arg = 0
-            or
-            // `NSMutableString.insert`
-            className = "NSMutableString" and
-            funcName = "insert(_:at:)" and
-            arg = 1
-          ) and
-          c.getName() = className and
-          c.getABaseTypeDecl*().(ClassOrStructDecl).getAMember() = funcDecl and
-          call.getStaticTarget() = funcDecl and
-          correctStringType = "NSString"
-        )
-        or
-        // arguments to function calls...
-        // `NSMakeRange`
-        funcName = "NSMakeRange(_:_:)" and
-        arg = [0, 1] and
-        call.getStaticTarget() = funcDecl and
-        correctStringType = "NSString"
-        or
-        // arguments to method calls...
-        (
-          // `String.dropFirst`, `String.dropLast`, `String.removeFirst`, `String.removeLast`
-          funcName = ["dropFirst(_:)", "dropLast(_:)", "removeFirst(_:)", "removeLast(_:)"] and
-          arg = 0
-          or
-          // `String.prefix`, `String.suffix`
-          funcName = ["prefix(_:)", "suffix(_:)"] and
-          arg = 0
-          or
-          // `String.Index.init`
-          funcName = "init(encodedOffset:)" and
-          arg = 0
-          or
-          // `String.index`
-          funcName = ["index(_:offsetBy:)", "index(_:offsetBy:limitBy:)"] and
-          arg = [0, 1]
-          or
-          // `String.formIndex`
-          funcName = ["formIndex(_:offsetBy:)", "formIndex(_:offsetBy:limitBy:)"] and
-          arg = [0, 1]
-        ) and
-        call.getStaticTarget() = funcDecl and
-        correctStringType = "String"
-      ) and
-      // match up `funcName`, `arg`, `node`.
-      funcDecl.getName() = funcName and
-      call.getArgument(arg).getExpr() = this.asExpr()
-    )
-  }
+  DefaultStringLengthConflationSink() { sinkNode(this, stringType.getCsvLabel()) }

-  override StringType getCorrectStringType() { result = correctStringType }
+  override StringType getCorrectStringType() { result = stringType }
+}
+
+private class StringLengthConflationSinks extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        ";Sequence;true;dropFirst(_:);;;Argument[0];string-length",
+        ";Sequence;true;dropLast(_:);;;Argument[0];string-length",
+        ";Sequence;true;prefix(_:);;;Argument[0];string-length",
+        ";Sequence;true;suffix(_:);;;Argument[0];string-length",
+        ";Collection;true;formIndex(_:offsetBy:);;;Argument[0..1];string-length",
+        ";Collection;true;formIndex(_:offsetBy:limitBy:);;;Argument[0..1];string-length",
+        ";Collection;true;removeFirst(_:);;;Argument[0];string-length",
+        ";RangeReplaceableCollection;true;removeLast(_:);;;Argument[0];string-length",
+        ";String;true;index(_:offsetBy:);;;Argument[0..1];string-length",
+        ";String;true;index(_:offsetBy:limitBy:);;;Argument[0..1];string-length",
+        ";String.Index;true;init(encodedOffset:);;;Argument[0];string-length",
+        ";NSRange;true;init(location:length:);;;Argument[0..1];nsstring-length",
+        ";NSString;true;character(at:);;;Argument[0];nsstring-length",
+        ";NSString;true;substring(from:);;;Argument[0];nsstring-length",
+        ";NSString;true;substring(to:);;;Argument[0];nsstring-length",
+        ";NSMutableString;true;character(at:);;;Argument[0];nsstring-length",
+        ";NSMutableString;true;substring(from:);;;Argument[0];nsstring-length",
+        ";NSMutableString;true;substring(to:);;;Argument[0];nsstring-length",
+        ";NSMutableString;true;insert(_:at:);;;Argument[1];nsstring-length",
+        ";;false;NSMakeRange(_:_:);;;Argument[0..1];nsstring-length",
+      ]
+  }
 }
--- a/swift/ql/test/query-tests/Security/CWE-135/StringLengthConflation.expected
+++ b/swift/ql/test/query-tests/Security/CWE-135/StringLengthConflation.expected
@@ -1,5 +1,4 @@
 edges
-| StringLengthConflation2.swift:35:36:35:38 | .count :  | StringLengthConflation2.swift:35:36:35:46 | ... .-(_:_:) ... |
 | StringLengthConflation2.swift:37:34:37:36 | .count :  | StringLengthConflation2.swift:37:34:37:44 | ... .-(_:_:) ... |
 | StringLengthConflation.swift:36:30:36:37 | len :  | StringLengthConflation.swift:36:93:36:93 | len |
 | StringLengthConflation.swift:60:47:60:50 | .length :  | StringLengthConflation.swift:60:47:60:59 | ... ./(_:_:) ... |
@@ -28,8 +27,6 @@ edges
 | file://:0:0:0:0 | .length :  | StringLengthConflation.swift:114:23:114:26 | .length :  |
 | file://:0:0:0:0 | .length :  | StringLengthConflation.swift:120:22:120:25 | .length :  |
 nodes
-| StringLengthConflation2.swift:35:36:35:38 | .count :  | semmle.label | .count :  |
-| StringLengthConflation2.swift:35:36:35:46 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
 | StringLengthConflation2.swift:37:34:37:36 | .count :  | semmle.label | .count :  |
 | StringLengthConflation2.swift:37:34:37:44 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
 | StringLengthConflation.swift:36:30:36:37 | len :  | semmle.label | len :  |
@@ -74,15 +71,11 @@ nodes
 | StringLengthConflation.swift:144:28:144:30 | .count :  | semmle.label | .count :  |
 | StringLengthConflation.swift:144:28:144:38 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
 | StringLengthConflation.swift:151:45:151:53 | .count | semmle.label | .count |
-| StringLengthConflation.swift:152:57:152:65 | .count | semmle.label | .count |
 | StringLengthConflation.swift:156:45:156:52 | .count | semmle.label | .count |
-| StringLengthConflation.swift:157:55:157:62 | .count | semmle.label | .count |
 | StringLengthConflation.swift:161:45:161:53 | .count | semmle.label | .count |
-| StringLengthConflation.swift:162:57:162:65 | .count | semmle.label | .count |
 | file://:0:0:0:0 | .length :  | semmle.label | .length :  |
 subpaths
 #select
-| StringLengthConflation2.swift:35:36:35:46 | ... .-(_:_:) ... | StringLengthConflation2.swift:35:36:35:38 | .count :  | StringLengthConflation2.swift:35:36:35:46 | ... .-(_:_:) ... | This String length is used in an NSString, but it may not be equivalent. |
 | StringLengthConflation2.swift:37:34:37:44 | ... .-(_:_:) ... | StringLengthConflation2.swift:37:34:37:36 | .count :  | StringLengthConflation2.swift:37:34:37:44 | ... .-(_:_:) ... | This String length is used in an NSString, but it may not be equivalent. |
 | StringLengthConflation.swift:36:93:36:93 | len | StringLengthConflation.swift:72:33:72:35 | .count :  | StringLengthConflation.swift:36:93:36:93 | len | This String length is used in an NSString, but it may not be equivalent. |
 | StringLengthConflation.swift:53:43:53:46 | .length | StringLengthConflation.swift:53:43:53:46 | .length | StringLengthConflation.swift:53:43:53:46 | .length | This NSString length is used in a String, but it may not be equivalent. |
@@ -118,8 +111,5 @@ subpaths
 | StringLengthConflation.swift:138:36:138:46 | ... .-(_:_:) ... | StringLengthConflation.swift:138:36:138:38 | .count :  | StringLengthConflation.swift:138:36:138:46 | ... .-(_:_:) ... | This String length is used in an NSString, but it may not be equivalent. |
 | StringLengthConflation.swift:144:28:144:38 | ... .-(_:_:) ... | StringLengthConflation.swift:144:28:144:30 | .count :  | StringLengthConflation.swift:144:28:144:38 | ... .-(_:_:) ... | This String length is used in an NSString, but it may not be equivalent. |
 | StringLengthConflation.swift:151:45:151:53 | .count | StringLengthConflation.swift:151:45:151:53 | .count | StringLengthConflation.swift:151:45:151:53 | .count | This String.unicodeScalars length is used in a String, but it may not be equivalent. |
-| StringLengthConflation.swift:152:57:152:65 | .count | StringLengthConflation.swift:152:57:152:65 | .count | StringLengthConflation.swift:152:57:152:65 | .count | This String.unicodeScalars length is used in a String, but it may not be equivalent. |
 | StringLengthConflation.swift:156:45:156:52 | .count | StringLengthConflation.swift:156:45:156:52 | .count | StringLengthConflation.swift:156:45:156:52 | .count | This String.utf8 length is used in a String, but it may not be equivalent. |
-| StringLengthConflation.swift:157:55:157:62 | .count | StringLengthConflation.swift:157:55:157:62 | .count | StringLengthConflation.swift:157:55:157:62 | .count | This String.utf8 length is used in a String, but it may not be equivalent. |
 | StringLengthConflation.swift:161:45:161:53 | .count | StringLengthConflation.swift:161:45:161:53 | .count | StringLengthConflation.swift:161:45:161:53 | .count | This String.utf16 length is used in a String, but it may not be equivalent. |
-| StringLengthConflation.swift:162:57:162:65 | .count | StringLengthConflation.swift:162:57:162:65 | .count | StringLengthConflation.swift:162:57:162:65 | .count | This String.unicodeScalars length is used in a String, but it may not be equivalent. |
--- a/swift/ql/test/query-tests/Security/CWE-135/StringLengthConflation.swift
+++ b/swift/ql/test/query-tests/Security/CWE-135/StringLengthConflation.swift
@@ -149,17 +149,17 @@ func test(s: String) {
    let scalars = s.unicodeScalars
    let _ = s.index(s.startIndex, offsetBy: s.count) // GOOD
    let _ = s.index(s.startIndex, offsetBy: scalars.count) // BAD
-    let _ = scalars.index(scalars.startIndex, offsetBy: scalars.count) // GOOD [FALSE POSITIVE]
+    let _ = scalars.index(scalars.startIndex, offsetBy: scalars.count) // GOOD
    let _ = scalars.index(scalars.startIndex, offsetBy: s.count) // BAD [NOT DETECTED]

    let s_utf8 = s.utf8
    let _ = s.index(s.startIndex, offsetBy: s_utf8.count) // BAD
-    let _ = s_utf8.index(s_utf8.startIndex, offsetBy: s_utf8.count) // GOOD [FALSE POSITIVE]
+    let _ = s_utf8.index(s_utf8.startIndex, offsetBy: s_utf8.count) // GOOD
    let _ = s_utf8.index(s_utf8.startIndex, offsetBy: s.count) // BAD [NOT DETECTED]

    let s_utf16 = s.utf16
    let _ = s.index(s.startIndex, offsetBy: s_utf16.count) // BAD
-    let _ = s_utf16.index(s_utf16.startIndex, offsetBy: scalars.count) // GOOD [FALSE POSITIVE]
+    let _ = s_utf16.index(s_utf16.startIndex, offsetBy: scalars.count) // GOOD
    let _ = s_utf16.index(s_utf16.startIndex, offsetBy: s.count) // BAD [NOT DETECTED]
 }