C#: Address review comments.

Author: michaelnebel

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

@@ -7,7 +7,7 @@ private import semmle.code.csharp.commons.Collections
 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()`. */
+/** A method that formats a string (or parses a format string), for example `string.Format()` */
 abstract private class FormatMethodImpl extends Method {
   /**
    * Gets the argument containing the format string. For example, the argument of
@@ -124,6 +124,17 @@ private class SystemDiagnosticsFormatMethods extends FormatMethodImpl {
   override int getFormatArgument() { result = 0 }
 }
 
+/**
+ * The `System.Text.CompositeFormat.Parse` method.
+ *
+ * Note that this method is not an ordinary format method, but it parses the format argument.
+ */
+class CompositeFormatParseMethod extends FormatMethodImpl {
+  CompositeFormatParseMethod() { this = any(SystemTextCompositeFormatClass x).getParseMethod() }
+
+  override int getFormatArgument() { result = 0 }
+}
+
 pragma[nomagic]
 private predicate parameterReadPostDominatesEntry(ParameterRead pr) {
   pr.getAControlFlowNode().postDominates(pr.getEnclosingCallable().getEntryPoint()) and
@@ -289,31 +300,3 @@ class FormatCall extends MethodCall {
     result = this.getArgument(this.getFirstArgument() + index)
   }
 }
-
-/**
- * A method call to a method that parses a format string, for example a call
- * to `string.Format()`.
- */
-abstract private class FormatStringParseCallImpl extends MethodCall {
-  /**
-   * Gets the expression used as the format string.
-   */
-  abstract Expr getFormatExpr();
-}
-
-final class FormatStringParseCall = FormatStringParseCallImpl;
-
-private class OrdinaryFormatCall extends FormatStringParseCallImpl instanceof FormatCall {
-  override Expr getFormatExpr() { result = FormatCall.super.getFormatExpr() }
-}
-
-/**
- * A method call to `System.Text.CompositeFormat.Parse`.
- */
-class ParseFormatStringCall extends FormatStringParseCallImpl {
-  ParseFormatStringCall() {
-    this.getTarget() = any(SystemTextCompositeFormatClass x).getParseMethod()
-  }
-
-  override Expr getFormatExpr() { result = this.getArgument(0) }
-}

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

@@ -15,22 +15,13 @@ import semmle.code.csharp.frameworks.system.Text
 import semmle.code.csharp.frameworks.Format
 import FormatFlow::PathGraph
 
-module FormatInvalidConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node n) { n.asExpr() instanceof StringLiteral }
-
-  predicate isSink(DataFlow::Node n) {
-    exists(FormatStringParseCall c | n.asExpr() = c.getFormatExpr())
-  }
-}
-
-module FormatInvalid = DataFlow::Global<FormatInvalidConfig>;
-
-module FormatLiteralConfig implements DataFlow::ConfigSig {
+module FormatFlowConfig 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 |
+    exists(FormatCall call |
+      call.getTarget() instanceof CompositeFormatParseMethod and
       pred.asExpr() = call.getFormatExpr() and
       succ.asExpr() = call
     )
@@ -39,31 +30,28 @@ module FormatLiteralConfig implements DataFlow::ConfigSig {
   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>;
+module FormatFlow = DataFlow::Global<FormatFlowConfig>;
 
 private predicate invalidFormatString(
-  InvalidFormatString src, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
-  FormatStringParseCall call, string callString
+  InvalidFormatString src, FormatFlow::PathNode source, FormatFlow::PathNode sink, string msg,
+  FormatCall call, string callString
 ) {
   source.getNode().asExpr() = src and
   sink.getNode().asExpr() = call.getFormatExpr() and
-  FormatInvalid::flowPath(source, sink) and
+  FormatFlow::flowPath(source, sink) and
   msg = "Invalid format string used in $@ formatting call." and
   callString = "this"
 }
 
 private predicate unusedArgument(
-  FormatCall call, FormatLiteral::PathNode source, FormatLiteral::PathNode sink, string msg,
+  FormatCall call, FormatFlow::PathNode source, FormatFlow::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
-    FormatLiteral::flowPath(source, sink) and
+    not call.getTarget() instanceof CompositeFormatParseMethod and
+    FormatFlow::flowPath(source, sink) and
     unused = call.getASuppliedArgument() and
     not unused = src.getAnInsert() and
     not src.getValue() = "" and
@@ -75,13 +63,14 @@ private predicate unusedArgument(
 }
 
 private predicate missingArgument(
-  FormatCall call, FormatLiteral::PathNode source, FormatLiteral::PathNode sink, string msg,
+  FormatCall call, FormatFlow::PathNode source, FormatFlow::PathNode sink, string msg,
   ValidFormatString src, string srcString
 ) {
   exists(int used, int supplied |
     source.getNode().asExpr() = src and
     sink.getNode().asExpr() = call.getFormatExpr() and
-    FormatLiteral::flowPath(source, sink) and
+    not call.getTarget() instanceof CompositeFormatParseMethod and
+    FormatFlow::flowPath(source, sink) and
     used = src.getAnInsert() and
     supplied = call.getSuppliedArguments() and
     used >= supplied and
@@ -94,14 +83,13 @@ from
   Element alert, FormatFlow::PathNode source, FormatFlow::PathNode sink, string msg, Element extra1,
   string extra1String, Element extra2, string extra2String
 where
-  invalidFormatString(alert, source.asPathNode1(), sink.asPathNode1(), msg, extra1, extra1String) and
+  invalidFormatString(alert, source, sink, msg, extra1, extra1String) and
   extra2 = extra1 and
   extra2String = extra1String
   or
-  unusedArgument(alert, source.asPathNode2(), sink.asPathNode2(), msg, extra1, extra1String, extra2,
-    extra2String)
+  unusedArgument(alert, source, sink, msg, extra1, extra1String, extra2, extra2String)
   or
-  missingArgument(alert, source.asPathNode2(), sink.asPathNode2(), msg, extra1, extra1String) and
+  missingArgument(alert, source, sink, msg, extra1, extra1String) and
   extra2 = extra1 and
   extra2String = extra1String
 select alert, source, sink, msg, extra1, extra1String, extra2, extra2String

csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql

@@ -19,9 +19,7 @@ import FormatString::PathGraph
 module FormatStringConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource }
 
-  predicate isSink(DataFlow::Node sink) {
-    sink.asExpr() = any(FormatStringParseCall call).getFormatExpr()
-  }
+  predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(FormatCall call).getFormatExpr() }
 }
 
 module FormatString = TaintTracking::Global<FormatStringConfig>;