Merge pull request #9673 from geoffw0/stringlengthconflation2

Swift: String length conflation query

Author: MathiasVP

swift/ql/lib/codeql/swift/elements/type/Type.qll

@@ -2,4 +2,6 @@ private import codeql.swift.generated.type.Type
 
 class Type extends TypeBase {
   override string toString() { result = this.getDiagnosticsName() }
+
+  string getName() { result = this.getDiagnosticsName() }
 }

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

@@ -0,0 +1,34 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<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>
+
+</overview>
+<recommendation>
+
+<p>Use <code>String.count</code> when working with a <code>String</code>. Use <code>NSString.length</code> when working with an <code>NSString</code>. Do not mix values for lengths and offsets between the two types as they are not compatible measures.</p>
+
+<p>If you need to convert between <code>Range</code> and <code>NSRange</code>, do so directly using the appropriate constructor. Do not attempt to use incompatible length and offset values to accomplish conversion.</p>
+
+</recommendation>
+<example>
+
+<p>In the following example, a <code>String</code> is converted to <code>NSString</code>, but a range is created from the <code>String</code> to do some processing on it.</p>
+
+<sample src="StringLengthConflationBad.swift" />
+
+<p>This is dangerous because, if the input contains certain characters, the range computed on the <code>String</code> will be wrong for the <code>NSString</code>.  This will lead to incorrect behaviour in the string processing that follows. To fix the problem, we can use <code>NSString.length</code> to create the <code>NSRange</code> instead, as follows:</p>
+
+<sample src="StringLengthConflationGood.swift" />
+
+</example>
+<references>
+
+<li>
+  <a href="https://talk.objc.io/episodes/S01E80-swift-string-vs-nsstring">Swift String vs. NSString</a>
+</li>
+
+</references>
+</qhelp>

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

@@ -1,15 +1,97 @@
 /**
  * @name String length conflation
- * @description TODO
+ * @description Using a length value from an `NSString` in a `String`, or a count from a `String` in an `NSString`, may cause unexpected behavior.
  * @kind problem
- * @problem.severity TODO
- * @security-severity TODO
+ * @problem.severity error
+ * @security-severity 7.8
  * @precision TODO
  * @id swift/string-length-conflation
  * @tags security
  *       external/cwe/cwe-135
  */
 
 import swift
+import codeql.swift.dataflow.DataFlow
+import DataFlow::PathGraph
 
-select "TODO"
+class StringLengthConflationConfiguration extends DataFlow::Configuration {
+  StringLengthConflationConfiguration() { this = "StringLengthConflationConfiguration" }
+
+  override predicate isSource(DataFlow::Node node, string flowstate) {
+    // result of a call to to `String.count`
+    exists(MemberRefExpr member |
+      member.getBaseExpr().getType().getName() = "String" and
+      member.getMember().(VarDecl).getName() = "count" and
+      node.asExpr() = member and
+      flowstate = "String"
+    )
+  }
+
+  override predicate isSink(DataFlow::Node node, string flowstate) {
+    // arguments to method calls...
+    exists(
+      string className, string methodName, string paramName, ClassDecl c, AbstractFunctionDecl f,
+      CallExpr call, int arg
+    |
+      (
+        // `NSRange.init`
+        className = "NSRange" and
+        methodName = "init(location:length:)" and
+        paramName = ["location", "length"]
+        or
+        // `NSString.character`
+        className = ["NSString", "NSMutableString"] and
+        methodName = "character(at:)" and
+        paramName = "at"
+        or
+        // `NSString.character`
+        className = ["NSString", "NSMutableString"] and
+        methodName = "substring(from:)" and
+        paramName = "from"
+        or
+        // `NSString.character`
+        className = ["NSString", "NSMutableString"] and
+        methodName = "substring(to:)" and
+        paramName = "to"
+        or
+        // `NSMutableString.insert`
+        className = "NSMutableString" and
+        methodName = "insert(_:at:)" and
+        paramName = "at"
+      ) and
+      c.getName() = className and
+      c.getAMember() = f and // TODO: will this even work if its defined in a parent class?
+      call.getFunction().(ApplyExpr).getStaticTarget() = f and
+      f.getName() = methodName and
+      f.getParam(arg).getName() = paramName and
+      call.getArgument(arg).getExpr() = node.asExpr() and
+      flowstate = "String" // `String` length flowing into `NSString`
+    )
+    or
+    // arguments to function calls...
+    exists(string funcName, string paramName, CallExpr call, int arg |
+      // `NSMakeRange`
+      funcName = "NSMakeRange(_:_:)" and
+      paramName = ["loc", "len"] and
+      call.getStaticTarget().getName() = funcName and
+      call.getStaticTarget().getParam(arg).getName() = paramName and
+      call.getArgument(arg).getExpr() = node.asExpr() and
+      flowstate = "String" // `String` length flowing into `NSString`
+    )
+  }
+}
+
+from
+  StringLengthConflationConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink,
+  string flowstate, 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."
+  )
+select sink.getNode(), source, sink, message

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

@@ -0,0 +1,7 @@
+
+func myFunction(s: String) {
+	let ns = NSString(string: s)
+	let nsrange = NSMakeRange(0, s.count) // BAD: String length used in NSMakeRange
+
+	// ... use nsrange to process ns
+}

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

@@ -0,0 +1,7 @@
+
+func myFunction(s: String) {
+	let ns = NSString(string: s)
+	let nsrange = NSMakeRange(0, ns.length) // Fixed: NSString length used in NSMakeRange
+
+	// ... use nsrange to process ns
+}

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

@@ -1 +1,8 @@
-| TODO |
+edges
+nodes
+| StringLengthConflation.swift:72:33:72:35 | .count | semmle.label | .count |
+| StringLengthConflation.swift:78:47:78:49 | .count | semmle.label | .count |
+subpaths
+#select
+| StringLengthConflation.swift:72:33:72:35 | .count | StringLengthConflation.swift:72:33:72:35 | .count | StringLengthConflation.swift:72:33:72:35 | .count | This String length is used in an NSString, but it may not be equivalent. |
+| StringLengthConflation.swift:78:47:78:49 | .count | StringLengthConflation.swift:78:47:78:49 | .count | StringLengthConflation.swift:78:47:78:49 | .count | This String length is used in an NSString, but it may not be equivalent. |