C#: Add cs/invalid-string-formatting to the codeql quality suite.

csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll

@@ -8,67 +8,111 @@ private import semmle.code.csharp.frameworks.System
 private import semmle.code.csharp.frameworks.system.Text
 
 /** A method that formats a string, for example `string.Format()`. */
-class FormatMethod extends Method {
-  FormatMethod() {
-    exists(Class declType | declType = this.getDeclaringType() |
+abstract class FormatMethod extends Method {
+  /**
+   * Gets the argument containing the format string. For example, the argument of
+   * `string.Format(IFormatProvider, String, Object)` is `1`.
+   */
+  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 and
+      this.getParameter(1).getType() instanceof FormatType
+      or
+      this.getParameter(0).getType() instanceof StringType
+    ) and
+    (
+      this = any(SystemStringClass c).getFormatMethod()
+      or
+      this = any(SystemTextStringBuilderClass c).getAppendFormatMethod()
+    )
+  }
+
+  override int getFormatArgument() {
+    if this.getParameter(0).getType() instanceof SystemIFormatProviderInterface
+    then result = 1
+    else result = 0
+  }
+}
+
+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
+    this.getNumberOfParameters() > 1 and
+    exists(Class declType | declType = this.getDeclaringType() |
+      this.hasName(["Write", "WriteLine"]) and
       (
-        this = any(SystemStringClass c).getFormatMethod()
+        declType.hasFullyQualifiedName("System", "Console")
         or
-        this = any(SystemTextStringBuilderClass c).getAppendFormatMethod()
+        declType.hasFullyQualifiedName("System.IO", "TextWriter")
       )
-      or
-      this.getParameter(0).getType() instanceof StringType and
+    )
+  }
+
+  override int getFormatArgument() { result = 0 }
+}
+
+private class SystemDiagnosticsDebugAssert extends FormatMethod {
+  SystemDiagnosticsDebugAssert() {
+    this.hasName("Assert") and
+    this.getDeclaringType().hasFullyQualifiedName("System.Diagnostics", "Debug") and
+    this.getNumberOfParameters() = 4
+  }
+
+  override int getFormatArgument() { result = 2 }
+}
+
+private class SystemDiagnosticsFormatMethods extends FormatMethod {
+  SystemDiagnosticsFormatMethods() {
+    this.getParameter(0).getType() instanceof StringType and
+    this.getNumberOfParameters() > 1 and
+    exists(Class declType |
+      declType = this.getDeclaringType() and
+      declType.getNamespace().getFullName() = "System.Diagnostics"
+    |
+      declType.hasName("Trace") and
       (
-        this = any(SystemStringClass c).getFormatMethod()
-        or
-        this = any(SystemTextStringBuilderClass c).getAppendFormatMethod()
-        or
-        (this.hasName("Write") or this.hasName("WriteLine")) and
-        (
-          declType.hasFullyQualifiedName("System", "Console")
-          or
-          declType.hasFullyQualifiedName("System.IO", "TextWriter")
-          or
-          declType.hasFullyQualifiedName("System.Diagnostics", "Debug") and
-          this.getParameter(1).getType() instanceof ArrayType
-        )
+        this.hasName("TraceError")
         or
-        declType.hasFullyQualifiedName("System.Diagnostics", "Trace") and
-        (
-          this.hasName("TraceError") or
-          this.hasName("TraceInformation") or
-          this.hasName("TraceWarning")
-        )
+        this.hasName("TraceInformation")
         or
-        this.hasName("TraceInformation") and
-        declType.hasFullyQualifiedName("System.Diagnostics", "TraceSource")
-        or
-        this.hasName("Print") and
-        declType.hasFullyQualifiedName("System.Diagnostics", "Debug")
+        this.hasName("TraceWarning")
       )
       or
-      this.hasName("Assert") and
-      declType.hasFullyQualifiedName("System.Diagnostics", "Debug") and
-      this.getNumberOfParameters() = 4
+      declType.hasName("TraceSource") and this.hasName("TraceInformation")
+      or
+      declType.hasName("Debug") and
+      (
+        this.hasName("Print")
+        or
+        this.hasName(["Write", "WriteLine"]) and
+        this.getParameter(1).getType() instanceof ArrayType
+      )
     )
   }
 
-  /**
-   * Gets the argument containing the format string. For example, the argument of
-   * `string.Format(IFormatProvider, String, Object)` is `1`.
-   */
-  int getFormatArgument() {
-    if this.getParameter(0).getType() instanceof SystemIFormatProviderInterface
-    then result = 1
-    else
-      if
-        this.hasName("Assert") and
-        this.getDeclaringType().hasFullyQualifiedName("System.Diagnostics", "Debug")
-      then result = 2
-      else result = 0
-  }
+  override int getFormatArgument() { result = 0 }
 }
 
 pragma[nomagic]

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") }

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") }
+}

csharp/ql/src/API Abuse/FormatInvalid.ql

@@ -11,37 +11,75 @@
  */
 
 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
   FormatInvalid::flowPath(source, sink) and
-  call.hasInsertions() and
   msg = "Invalid format string used in $@ formatting call." and
   callString = "this"
 }
 
 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
@@ -53,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
@@ -69,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

csharp/ql/src/codeql-suites/csharp-code-quality.qls

@@ -13,3 +13,4 @@
       - cs/useless-gethashcode-call
       - cs/non-short-circuit
       - cs/useless-assignment-to-local
+      - cs/invalid-string-formatting