Merge pull request #9909 from geoffw0/stringlengthconflation6

Swift: Understand String.utf8.count etc in the string length conflation CVE query

Author: MathiasVP

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

@@ -5,6 +5,8 @@
 <overview>
 <p>Using a length value from an <code>NSString</code> in a <code>String</code>, or a count from a <code>String</code> in an <code>NSString</code>, may cause unexpected behavior including (in some cases) buffer overwrites. This is because certain unicode sequences are represented as one character in a <code>String</code> but as a sequence of multiple characters in an <code>NSString</code>. For example, a 'thumbs up' emoji with a skin tone modifier (&#x1F44D;&#x1F3FF;) is represented as U+1F44D (&#x1F44D;) then the modifier U+1F3FF.</p>
 
+<p>This issue can also arise from using the values of <code>String.utf8.count</code>, <code>String.utf16.count</code> or <code>String.unicodeScalars.count</code> in an unsuitable place.</p>
+
 </overview>
 <recommendation>
 

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

@@ -14,6 +14,39 @@ import swift
 import codeql.swift.dataflow.DataFlow
 import DataFlow::PathGraph
 
+/**
+ * A flow state for this query, which is a type of Swift string encoding.
+ */
+class StringLengthConflationFlowState extends string {
+  string equivClass;
+  string singular;
+
+  StringLengthConflationFlowState() {
+    this = "String" and singular = "a String" and equivClass = "String"
+    or
+    this = "NSString" and singular = "an NSString" and equivClass = "NSString"
+    or
+    this = "String.utf8" and singular = "a String.utf8" and equivClass = "String.utf8"
+    or
+    this = "String.utf16" and singular = "a String.utf16" and equivClass = "NSString"
+    or
+    this = "String.unicodeScalars" and
+    singular = "a String.unicodeScalars" and
+    equivClass = "String.unicodeScalars"
+  }
+
+  /**
+   * Gets the equivalence class for this flow state. If these are equal,
+   * they should be treated as equivalent.
+   */
+  string getEquivClass() { result = equivClass }
+
+  /**
+   * Gets text for the singular form of this flow state.
+   */
+  string getSingular() { result = singular }
+}
+
 /**
  * A configuration for tracking string lengths originating from source that is
  * a `String` or an `NSString` object, to a sink of a different kind that
@@ -38,9 +71,37 @@ class StringLengthConflationConfiguration extends DataFlow::Configuration {
       node.asExpr() = member and
       flowstate = "NSString"
     )
+    or
+    // result of a call to `String.utf8.count`
+    exists(MemberRefExpr member |
+      member.getBaseExpr().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.getBaseExpr().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.getBaseExpr().getType().getName() = "String.UnicodeScalarView" and
+      member.getMember().(VarDecl).getName() = "count" and
+      node.asExpr() = member and
+      flowstate = "String.unicodeScalars"
+    )
   }
 
-  override predicate isSink(DataFlow::Node node, string flowstate) {
+  /**
+   * Holds if `node` is a sink and `flowstate` is the *correct* flow state for
+   * that sink. We actually want to report incorrect flow states.
+   */
+  predicate isSinkImpl(DataFlow::Node node, string flowstate) {
     exists(
       AbstractFunctionDecl funcDecl, CallExpr call, string funcName, string paramName, int arg
     |
@@ -76,15 +137,15 @@ class StringLengthConflationConfiguration extends DataFlow::Configuration {
           c.getName() = className and
           c.getAMember() = funcDecl and
           call.getFunction().(ApplyExpr).getStaticTarget() = funcDecl and
-          flowstate = "String" // `String` length flowing into `NSString`
+          flowstate = "NSString"
         )
         or
         // arguments to function calls...
         // `NSMakeRange`
         funcName = "NSMakeRange(_:_:)" and
         paramName = ["loc", "len"] and
         call.getStaticTarget() = funcDecl and
-        flowstate = "String" // `String` length flowing into `NSString`
+        flowstate = "NSString"
         or
         // arguments to method calls...
         (
@@ -109,7 +170,7 @@ class StringLengthConflationConfiguration extends DataFlow::Configuration {
           paramName = "distance"
         ) and
         call.getFunction().(ApplyExpr).getStaticTarget() = funcDecl and
-        flowstate = "NSString" // `NSString` length flowing into `String`
+        flowstate = "String"
       ) and
       // match up `funcName`, `paramName`, `arg`, `node`.
       funcDecl.getName() = funcName and
@@ -118,6 +179,16 @@ class StringLengthConflationConfiguration extends DataFlow::Configuration {
     )
   }
 
+  override predicate isSink(DataFlow::Node node, string flowstate) {
+    // Permit any *incorrect* flowstate, as those are the results the query
+    // should report.
+    exists(string correctFlowState |
+      isSinkImpl(node, correctFlowState) and
+      flowstate.(StringLengthConflationFlowState).getEquivClass() !=
+        correctFlowState.(StringLengthConflationFlowState).getEquivClass()
+    )
+  }
+
   override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     // allow flow through `+`, `-`, `*` etc.
     node2.asExpr().(ArithmeticOperation).getAnOperand() = node1.asExpr()
@@ -126,15 +197,13 @@ class StringLengthConflationConfiguration extends DataFlow::Configuration {
 
 from
   StringLengthConflationConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink,
-  string flowstate, string message
+  StringLengthConflationFlowState sourceFlowState, StringLengthConflationFlowState sinkFlowstate,
+  string message
 where
   config.hasFlowPath(source, sink) and
-  config.isSink(sink.getNode(), flowstate) and
-  (
-    flowstate = "String" and
-    message = "This String length is used in an NSString, but it may not be equivalent."
-    or
-    flowstate = "NSString" and
-    message = "This NSString length is used in a String, but it may not be equivalent."
-  )
+  config.isSource(source.getNode(), sourceFlowState) and
+  config.isSinkImpl(sink.getNode(), sinkFlowstate) and
+  message =
+    "This " + sourceFlowState + " length is used in " + sinkFlowstate.getSingular() +
+      ", but it may not be equivalent."
 select sink.getNode(), source, sink, message