C#: Improve format logic to take CompositeFormat and generics into account.

Author: michaelnebel

Diff for: csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll

@@ -16,11 +16,19 @@ abstract class FormatMethod extends Method {
   abstract int getFormatArgument();
 }
 
+/** A class of types used for formatting. */
+private class FormatType extends Type {
+  FormatType() {
+    this instanceof StringType or
+    this instanceof SystemTextCompositeFormatClass
+  }
+}
+
 private class StringAndStringBuilderFormatMethods extends FormatMethod {
   StringAndStringBuilderFormatMethods() {
     (
       this.getParameter(0).getType() instanceof SystemIFormatProviderInterface and
-      this.getParameter(1).getType() instanceof StringType
+      this.getParameter(1).getType() instanceof FormatType
       or
       this.getParameter(0).getType() instanceof StringType
     ) and
@@ -38,6 +46,16 @@ private class StringAndStringBuilderFormatMethods extends FormatMethod {
   }
 }
 
+private class SystemMemoryExtensionsFormatMethods extends FormatMethod {
+  SystemMemoryExtensionsFormatMethods() {
+    this = any(SystemMemoryExtensionsClass c).getTryWriteMethod() and
+    this.getParameter(1).getType() instanceof SystemIFormatProviderInterface and
+    this.getParameter(2).getType() instanceof SystemTextCompositeFormatClass
+  }
+
+  override int getFormatArgument() { result = 2 }
+}
+
 private class SystemConsoleAndSystemIoTextWriterFormatMethods extends FormatMethod {
   SystemConsoleAndSystemIoTextWriterFormatMethods() {
     this.getParameter(0).getType() instanceof StringType and

Diff for: csharp/ql/lib/semmle/code/csharp/frameworks/System.qll

@@ -365,7 +365,7 @@ class SystemStringClass extends StringType {
   /** Gets a `Format(...)` method. */
   Method getFormatMethod() {
     result.getDeclaringType() = this and
-    result.hasName("Format") and
+    result.getName().regexpMatch("Format(<.*>)?") and
     result.getNumberOfParameters() in [2 .. 5] and
     result.getReturnType() instanceof StringType
   }
@@ -751,6 +751,18 @@ class SystemNotImplementedExceptionClass extends SystemClass {
   SystemNotImplementedExceptionClass() { this.hasName("NotImplementedException") }
 }
 
+/** The `System.MemoryExtensions` class. */
+class SystemMemoryExtensionsClass extends SystemClass {
+  SystemMemoryExtensionsClass() { this.hasName("MemoryExtensions") }
+
+  /** Gets a `TryWrite` method. */
+  Method getTryWriteMethod() {
+    result.getDeclaringType() = this and
+    result.getName().regexpMatch("TryWrite(<.*>)?") and
+    result.getParameter(0).getType().getUnboundDeclaration() instanceof SystemSpanStruct
+  }
+}
+
 /** The `System.DateTime` struct. */
 class SystemDateTimeStruct extends SystemStruct {
   SystemDateTimeStruct() { this.hasName("DateTime") }

Diff for: csharp/ql/lib/semmle/code/csharp/frameworks/system/Text.qll

@@ -22,7 +22,12 @@ class SystemTextStringBuilderClass extends SystemTextClass {
   SystemTextStringBuilderClass() { this.hasName("StringBuilder") }
 
   /** Gets the `AppendFormat` method. */
-  Method getAppendFormatMethod() { result = this.getAMethod("AppendFormat") }
+  Method getAppendFormatMethod() {
+    exists(string name |
+      name.regexpMatch("AppendFormat(<.*>)?") and
+      result = this.getAMethod(name)
+    )
+  }
 }
 
 /** The `System.Text.Encoding` class. */
@@ -38,3 +43,11 @@ class SystemTextEncodingClass extends SystemTextClass {
   /** Gets the `GetChars` method. */
   Method getGetCharsMethod() { result = this.getAMethod("GetChars") }
 }
+
+/** The `System.Text.CompositeFormat` class */
+class SystemTextCompositeFormatClass extends SystemTextClass {
+  SystemTextCompositeFormatClass() { this.hasName("CompositeFormat") }
+
+  /** Gets the `Parse` method. */
+  Method getParseMethod() { result = this.getAMethod("Parse") }
+}

Diff for: csharp/ql/src/API Abuse/FormatInvalid.ql

@@ -11,20 +11,59 @@
  */
 
 import csharp
+import semmle.code.csharp.frameworks.system.Text
 import semmle.code.csharp.frameworks.Format
-import FormatInvalid::PathGraph
+import FormatFlow::PathGraph
+
+abstract private class FormatStringParseCall extends MethodCall {
+  abstract Expr getFormatExpr();
+}
+
+private class OrdinaryFormatCall extends FormatStringParseCall instanceof FormatCall {
+  override Expr getFormatExpr() { result = FormatCall.super.getFormatExpr() }
+}
+
+private class ParseFormatStringCall extends FormatStringParseCall {
+  ParseFormatStringCall() {
+    this.getTarget() = any(SystemTextCompositeFormatClass x).getParseMethod()
+  }
+
+  override Expr getFormatExpr() { result = this.getArgument(0) }
+}
 
 module FormatInvalidConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node n) { n.asExpr() instanceof StringLiteral }
 
-  predicate isSink(DataFlow::Node n) { exists(FormatCall c | n.asExpr() = c.getFormatExpr()) }
+  predicate isSink(DataFlow::Node n) {
+    exists(FormatStringParseCall c | n.asExpr() = c.getFormatExpr())
+  }
 }
 
 module FormatInvalid = DataFlow::Global<FormatInvalidConfig>;
 
+module FormatLiteralConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node n) { n.asExpr() instanceof StringLiteral }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    // Add flow via `System.Text.CompositeFormat.Parse`.
+    exists(ParseFormatStringCall call |
+      pred.asExpr() = call.getFormatExpr() and
+      succ.asExpr() = call
+    )
+  }
+
+  predicate isSink(DataFlow::Node n) { exists(FormatCall c | n.asExpr() = c.getFormatExpr()) }
+}
+
+module FormatLiteral = DataFlow::Global<FormatLiteralConfig>;
+
+module FormatFlow =
+  DataFlow::MergePathGraph<FormatInvalid::PathNode, FormatLiteral::PathNode,
+    FormatInvalid::PathGraph, FormatLiteral::PathGraph>;
+
 private predicate invalidFormatString(
   InvalidFormatString src, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
-  FormatCall call, string callString
+  FormatStringParseCall call, string callString
 ) {
   source.getNode().asExpr() = src and
   sink.getNode().asExpr() = call.getFormatExpr() and
@@ -34,13 +73,13 @@ private predicate invalidFormatString(
 }
 
 private predicate unusedArgument(
-  FormatCall call, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
+  FormatCall call, FormatLiteral::PathNode source, FormatLiteral::PathNode sink, string msg,
   ValidFormatString src, string srcString, Expr unusedExpr, string unusedString
 ) {
   exists(int unused |
     source.getNode().asExpr() = src and
     sink.getNode().asExpr() = call.getFormatExpr() and
-    FormatInvalid::flowPath(source, sink) and
+    FormatLiteral::flowPath(source, sink) and
     unused = call.getASuppliedArgument() and
     not unused = src.getAnInsert() and
     not src.getValue() = "" and
@@ -52,13 +91,13 @@ private predicate unusedArgument(
 }
 
 private predicate missingArgument(
-  FormatCall call, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
+  FormatCall call, FormatLiteral::PathNode source, FormatLiteral::PathNode sink, string msg,
   ValidFormatString src, string srcString
 ) {
   exists(int used, int supplied |
     source.getNode().asExpr() = src and
     sink.getNode().asExpr() = call.getFormatExpr() and
-    FormatInvalid::flowPath(source, sink) and
+    FormatLiteral::flowPath(source, sink) and
     used = src.getAnInsert() and
     supplied = call.getSuppliedArguments() and
     used >= supplied and
@@ -68,16 +107,17 @@ private predicate missingArgument(
 }
 
 from
-  Element alert, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
-  Element extra1, string extra1String, Element extra2, string extra2String
+  Element alert, FormatFlow::PathNode source, FormatFlow::PathNode sink, string msg, Element extra1,
+  string extra1String, Element extra2, string extra2String
 where
-  invalidFormatString(alert, source, sink, msg, extra1, extra1String) and
+  invalidFormatString(alert, source.asPathNode1(), sink.asPathNode1(), msg, extra1, extra1String) and
   extra2 = extra1 and
   extra2String = extra1String
   or
-  unusedArgument(alert, source, sink, msg, extra1, extra1String, extra2, extra2String)
+  unusedArgument(alert, source.asPathNode2(), sink.asPathNode2(), msg, extra1, extra1String, extra2,
+    extra2String)
   or
-  missingArgument(alert, source, sink, msg, extra1, extra1String) and
+  missingArgument(alert, source.asPathNode2(), sink.asPathNode2(), msg, extra1, extra1String) and
   extra2 = extra1 and
   extra2String = extra1String
 select alert, source, sink, msg, extra1, extra1String, extra2, extra2String