Merge pull request #10284 from geoffw0/stringlengthcleanup

Swift: Improve swift/string-length-conflation

Author: geoffw0

swift/ql/src/queries/Security/CWE-135/StringLengthConflation.ql

@@ -56,44 +56,36 @@ class StringLengthConflationConfiguration extends DataFlow::Configuration {
   StringLengthConflationConfiguration() { this = "StringLengthConflationConfiguration" }
 
   override predicate isSource(DataFlow::Node node, string flowstate) {
-    // result of a call to `String.count`
-    exists(MemberRefExpr member |
-      member.getBase().getType().getName() = "String" and
-      member.getMember().(VarDecl).getName() = "count" and
-      node.asExpr() = member and
-      flowstate = "String"
-    )
-    or
-    // result of a call to `NSString.length`
-    exists(MemberRefExpr member |
-      member.getBase().getType().getName() = ["NSString", "NSMutableString"] and
-      member.getMember().(VarDecl).getName() = "length" and
-      node.asExpr() = member and
-      flowstate = "NSString"
-    )
-    or
-    // result of a call to `String.utf8.count`
-    exists(MemberRefExpr member |
-      member.getBase().getType().getName() = "String.UTF8View" and
-      member.getMember().(VarDecl).getName() = "count" and
-      node.asExpr() = member and
-      flowstate = "String.utf8"
-    )
-    or
-    // result of a call to `String.utf16.count`
-    exists(MemberRefExpr member |
-      member.getBase().getType().getName() = "String.UTF16View" and
-      member.getMember().(VarDecl).getName() = "count" and
-      node.asExpr() = member and
-      flowstate = "String.utf16"
-    )
-    or
-    // result of a call to `String.unicodeScalars.count`
-    exists(MemberRefExpr member |
-      member.getBase().getType().getName() = "String.UnicodeScalarView" and
-      member.getMember().(VarDecl).getName() = "count" and
-      node.asExpr() = member and
-      flowstate = "String.unicodeScalars"
+    exists(MemberRefExpr memberRef, string className, string varName |
+      memberRef.getBase().getType().(NominalType).getABaseType*().getName() = className and
+      memberRef.getMember().(VarDecl).getName() = varName and
+      node.asExpr() = memberRef and
+      (
+        // result of a call to `String.count`
+        className = "String" and
+        varName = "count" and
+        flowstate = "String"
+        or
+        // result of a call to `NSString.length`
+        className = ["NSString", "NSMutableString"] and
+        varName = "length" and
+        flowstate = "NSString"
+        or
+        // result of a call to `String.utf8.count`
+        className = "String.UTF8View" and
+        varName = "count" and
+        flowstate = "String.utf8"
+        or
+        // result of a call to `String.utf16.count`
+        className = "String.UTF16View" and
+        varName = "count" and
+        flowstate = "String.utf16"
+        or
+        // result of a call to `String.unicodeScalars.count`
+        className = "String.UnicodeScalarView" and
+        varName = "count" and
+        flowstate = "String.unicodeScalars"
+      )
     )
   }
 
@@ -135,7 +127,7 @@ class StringLengthConflationConfiguration extends DataFlow::Configuration {
             paramName = "at"
           ) and
           c.getName() = className and
-          c.getAMember() = funcDecl and
+          c.getABaseTypeDecl*().(ClassDecl).getAMember() = funcDecl and
           call.getStaticTarget() = funcDecl and
           flowstate = "NSString"
         )

swift/ql/test/query-tests/Security/CWE-135/StringLengthConflation.expected

@@ -1,4 +1,5 @@
 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 | ... ./(_:_:) ... |
@@ -27,6 +28,8 @@ 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 :  |
@@ -73,6 +76,7 @@ nodes
 | 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. |

swift/ql/test/query-tests/Security/CWE-135/StringLengthConflation2.swift

@@ -32,7 +32,7 @@ func test(s: String) {
     let ns = NSString(string: s)
 
     let nstr1 = ns.substring(from: ns.length - 1) // GOOD
-    let nstr2 = ns.substring(from: s.count - 1) // BAD: String length used in NSString [NOT DETECTED]
+    let nstr2 = ns.substring(from: s.count - 1) // BAD: String length used in NSString
     let nstr3 = ns.substring(to: ns.length - 1) // GOOD
     let nstr4 = ns.substring(to: s.count - 1) // BAD: String length used in NSString
     print("substrings '\(nstr1)' '\(nstr2)' / '\(nstr3)' '\(nstr4)'")