C#: Restrict FormatInvalid.ql and UncontrolledFormatString.ql to calls with insertions

Author: hvitved

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

@@ -14,5 +14,7 @@ import semmle.code.csharp.frameworks.Format
 import FormatFlow
 
 from FormatCall s, InvalidFormatString src, PathNode source, PathNode sink
-where hasFlowPath(src, source, s, sink)
+where
+  hasFlowPath(src, source, s, sink) and
+  s.hasInsertions()
 select src, source, sink, "Invalid format string used in $@ formatting call.", s, "this"

csharp/ql/src/API Abuse/FormatInvalidBad.cs

@@ -4,6 +4,6 @@ class Bad
 {
     string GenerateEmptyClass(string c)
     {
-        return string.Format("class {0} { }");
+        return string.Format("class {0} { }", "C");
     }
 }

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

@@ -27,7 +27,7 @@ class FormatStringConfiguration extends TaintTracking::Configuration {
   }
 
   override predicate isSink(DataFlow::Node sink) {
-    sink.asExpr() = any(FormatCall call).getFormatExpr()
+    sink.asExpr() = any(FormatCall call | call.hasInsertions()).getFormatExpr()
   }
 }
 

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

@@ -218,6 +218,9 @@ class FormatCall extends MethodCall {
   /** Gets the argument number of the first supplied insert. */
   int getFirstArgument() { result = this.getFormatArgument() + 1 }
 
+  /** Holds if this call has one or more insertions. */
+  predicate hasInsertions() { exists(this.getArgument(this.getFirstArgument())) }
+
   /** Holds if the arguments are supplied in an array, not individually. */
   predicate hasArrayExpr() {
     this.getNumberOfArguments() = this.getFirstArgument() + 1 and

csharp/ql/test/query-tests/API Abuse/FormatInvalid/FormatInvalid.cs

@@ -115,6 +115,8 @@ void FormatMethodTests()
         System.Diagnostics.Debug.Assert(true, "Error", "}", ps);
         sw.Write("}", 0);
         System.Diagnostics.Debug.Print("}", ps);
+
+        Console.WriteLine("}"); // GOOD
     }
 
     System.IO.StringWriter sw;

csharp/ql/test/query-tests/API Abuse/FormatInvalid/FormatInvalid.expected

@@ -51,6 +51,7 @@ nodes
 | FormatInvalid.cs:115:56:115:58 | [assertion success] "}" | semmle.label | [assertion success] "}" |
 | FormatInvalid.cs:116:18:116:20 | "}" | semmle.label | "}" |
 | FormatInvalid.cs:117:40:117:42 | "}" | semmle.label | "}" |
+| FormatInvalid.cs:119:27:119:29 | "}" | semmle.label | "}" |
 | FormatInvalidBad.cs:7:30:7:44 | "class {0} { }" | semmle.label | "class {0} { }" |
 | FormatInvalidGood.cs:7:30:7:46 | "class {0} {{ }}" | semmle.label | "class {0} {{ }}" |
 edges
@@ -96,4 +97,4 @@ edges
 | FormatInvalid.cs:115:57:115:58 | "}" | FormatInvalid.cs:115:56:115:58 | [assertion success] "}" | FormatInvalid.cs:115:56:115:58 | [assertion success] "}" | Invalid format string used in $@ formatting call. | FormatInvalid.cs:115:9:115:63 | call to method Assert | this |
 | FormatInvalid.cs:116:19:116:20 | "}" | FormatInvalid.cs:116:18:116:20 | "}" | FormatInvalid.cs:116:18:116:20 | "}" | Invalid format string used in $@ formatting call. | FormatInvalid.cs:116:9:116:24 | call to method Write | this |
 | FormatInvalid.cs:117:41:117:42 | "}" | FormatInvalid.cs:117:40:117:42 | "}" | FormatInvalid.cs:117:40:117:42 | "}" | Invalid format string used in $@ formatting call. | FormatInvalid.cs:117:9:117:47 | call to method Print | this |
-| FormatInvalidBad.cs:7:41:7:44 | "class {0} { }" | FormatInvalidBad.cs:7:30:7:44 | "class {0} { }" | FormatInvalidBad.cs:7:30:7:44 | "class {0} { }" | Invalid format string used in $@ formatting call. | FormatInvalidBad.cs:7:16:7:45 | call to method Format | this |
+| FormatInvalidBad.cs:7:41:7:44 | "class {0} { }" | FormatInvalidBad.cs:7:30:7:44 | "class {0} { }" | FormatInvalidBad.cs:7:30:7:44 | "class {0} { }" | Invalid format string used in $@ formatting call. | FormatInvalidBad.cs:7:16:7:50 | call to method Format | this |

csharp/ql/test/query-tests/API Abuse/FormatInvalid/FormatInvalidBad.cs

@@ -4,6 +4,6 @@ class Bad
 {
     string GenerateEmptyClass(string c)
     {
-        return string.Format("class {0} { }");
+        return string.Format("class {0} { }", "C");
     }
 }

csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs

@@ -21,6 +21,9 @@ public void ProcessRequest(HttpContext ctx)
 
         // GOOD: Not the format string.
         String.Format((IFormatProvider)null, "Do not do this", path);
+
+        // GOOD: Not a formatting call
+        Console.WriteLine(path);
     }
 
     System.Windows.Forms.TextBox box1;

csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected

@@ -6,11 +6,11 @@ nodes
 | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
 | UncontrolledFormatString.cs:14:23:14:26 | access to local variable path | semmle.label | access to local variable path |
 | UncontrolledFormatString.cs:17:46:17:49 | access to local variable path | semmle.label | access to local variable path |
-| UncontrolledFormatString.cs:31:23:31:31 | access to property Text | semmle.label | access to property Text |
+| UncontrolledFormatString.cs:34:23:34:31 | access to property Text | semmle.label | access to property Text |
 | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
 | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format | semmle.label | access to local variable format |
 #select
 | UncontrolledFormatString.cs:14:23:14:26 | access to local variable path | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:14:23:14:26 | access to local variable path | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString | access to property QueryString |
 | UncontrolledFormatString.cs:17:46:17:49 | access to local variable path | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:17:46:17:49 | access to local variable path | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString | access to property QueryString |
-| UncontrolledFormatString.cs:31:23:31:31 | access to property Text | UncontrolledFormatString.cs:31:23:31:31 | access to property Text | UncontrolledFormatString.cs:31:23:31:31 | access to property Text | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:31:23:31:31 | access to property Text | access to property Text |
+| UncontrolledFormatString.cs:34:23:34:31 | access to property Text | UncontrolledFormatString.cs:34:23:34:31 | access to property Text | UncontrolledFormatString.cs:34:23:34:31 | access to property Text | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:34:23:34:31 | access to property Text | access to property Text |
 | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString : NameValueCollection | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format | $@ flows to here and is used as a format string. | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString | access to property QueryString |