Merge pull request #4842 from hvitved/csharp/format-method-no-insertion-param

hvitved · web-flow · commit 16aee6e71e56 · 2020-12-21T09:25:18.000+01:00
C#: Recognize format methods without insertion parameters
diff --git a/csharp/change-notes/2020-12-17-format-method-empty-overload.md b/csharp/change-notes/2020-12-17-format-method-empty-overload.md
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* For string formatting methods, such as `System.Console.WriteLine(string format, params object[] arg)`, we now also recognize overloads without insertion parameters as string formatting methods. For example, `System.Console.WriteLine(string value)` is now also a member of the class `FormatMethod` in `frameworks/Format.qll`.
diff --git a/csharp/ql/src/API Abuse/FormatInvalid.ql b/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"
diff --git a/csharp/ql/src/API Abuse/FormatInvalidBad.cs b/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");
     }
 }
diff --git a/csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql b/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()
   }
 }
 
diff --git a/csharp/ql/src/semmle/code/csharp/frameworks/Format.qll b/csharp/ql/src/semmle/code/csharp/frameworks/Format.qll
@@ -13,15 +13,13 @@ class FormatMethod extends Method {
     exists(Class declType | declType = this.getDeclaringType() |
       this.getParameter(0).getType() instanceof SystemIFormatProviderInterface and
       this.getParameter(1).getType() instanceof StringType and
-      this.getNumberOfParameters() >= 3 and
       (
         this = any(SystemStringClass c).getFormatMethod()
         or
         this = any(SystemTextStringBuilderClass c).getAppendFormatMethod()
       )
       or
       this.getParameter(0).getType() instanceof StringType and
-      this.getNumberOfParameters() >= 2 and
       (
         this = any(SystemStringClass c).getFormatMethod()
         or
@@ -220,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
diff --git a/csharp/ql/test/query-tests/API Abuse/FormatInvalid/FormatInvalid.cs b/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;
diff --git a/csharp/ql/test/query-tests/API Abuse/FormatInvalid/FormatInvalid.expected b/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 |
diff --git a/csharp/ql/test/query-tests/API Abuse/FormatInvalid/FormatInvalidBad.cs b/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");
     }
 }
diff --git a/csharp/ql/test/query-tests/API Abuse/FormatMissingArgument/FormatMissingArgument.cs b/csharp/ql/test/query-tests/API Abuse/FormatMissingArgument/FormatMissingArgument.cs
@@ -20,6 +20,9 @@ void TestFormatMissingArgument()
         String.Format("{0} {1} {2} {3}", 0, 1, 2, 3);
 
         helper("{1}");
+
+        // BAD: Missing {0}
+        Console.WriteLine("{0}");
     }
 
     void helper(string format)
diff --git a/csharp/ql/test/query-tests/API Abuse/FormatMissingArgument/FormatMissingArgument.expected b/csharp/ql/test/query-tests/API Abuse/FormatMissingArgument/FormatMissingArgument.expected
@@ -5,18 +5,20 @@ nodes
 | FormatMissingArgument.cs:17:23:17:35 | "{0} {1} {2}" | semmle.label | "{0} {1} {2}" |
 | FormatMissingArgument.cs:20:23:20:39 | "{0} {1} {2} {3}" | semmle.label | "{0} {1} {2} {3}" |
 | FormatMissingArgument.cs:22:16:22:20 | "{1}" : String | semmle.label | "{1}" : String |
-| FormatMissingArgument.cs:25:24:25:29 | format : String | semmle.label | format : String |
-| FormatMissingArgument.cs:28:23:28:28 | access to parameter format | semmle.label | access to parameter format |
+| FormatMissingArgument.cs:25:27:25:31 | "{0}" | semmle.label | "{0}" |
+| FormatMissingArgument.cs:28:24:28:29 | format : String | semmle.label | format : String |
+| FormatMissingArgument.cs:31:23:31:28 | access to parameter format | semmle.label | access to parameter format |
 | FormatMissingArgumentBad.cs:7:27:7:41 | "Hello {0} {1}" | semmle.label | "Hello {0} {1}" |
 | FormatMissingArgumentBad.cs:8:27:8:41 | "Hello {1} {2}" | semmle.label | "Hello {1} {2}" |
 | FormatMissingArgumentGood.cs:7:27:7:41 | "Hello {0} {1}" | semmle.label | "Hello {0} {1}" |
 edges
-| FormatMissingArgument.cs:22:16:22:20 | "{1}" : String | FormatMissingArgument.cs:25:24:25:29 | format : String |
-| FormatMissingArgument.cs:25:24:25:29 | format : String | FormatMissingArgument.cs:28:23:28:28 | access to parameter format |
+| FormatMissingArgument.cs:22:16:22:20 | "{1}" : String | FormatMissingArgument.cs:28:24:28:29 | format : String |
+| FormatMissingArgument.cs:28:24:28:29 | format : String | FormatMissingArgument.cs:31:23:31:28 | access to parameter format |
 #select
 | FormatMissingArgument.cs:11:9:11:31 | call to method Format | FormatMissingArgument.cs:11:23:11:27 | "{1}" | FormatMissingArgument.cs:11:23:11:27 | "{1}" | Argument '{1}' has not been supplied to $@ format string. | FormatMissingArgument.cs:11:23:11:27 | "{1}" | this |
 | FormatMissingArgument.cs:14:9:14:38 | call to method Format | FormatMissingArgument.cs:14:23:14:31 | "{2} {3}" | FormatMissingArgument.cs:14:23:14:31 | "{2} {3}" | Argument '{2}' has not been supplied to $@ format string. | FormatMissingArgument.cs:14:23:14:31 | "{2} {3}" | this |
 | FormatMissingArgument.cs:14:9:14:38 | call to method Format | FormatMissingArgument.cs:14:23:14:31 | "{2} {3}" | FormatMissingArgument.cs:14:23:14:31 | "{2} {3}" | Argument '{3}' has not been supplied to $@ format string. | FormatMissingArgument.cs:14:23:14:31 | "{2} {3}" | this |
-| FormatMissingArgument.cs:28:9:28:32 | call to method Format | FormatMissingArgument.cs:22:16:22:20 | "{1}" : String | FormatMissingArgument.cs:28:23:28:28 | access to parameter format | Argument '{1}' has not been supplied to $@ format string. | FormatMissingArgument.cs:22:16:22:20 | "{1}" | this |
+| FormatMissingArgument.cs:25:9:25:32 | call to method WriteLine | FormatMissingArgument.cs:25:27:25:31 | "{0}" | FormatMissingArgument.cs:25:27:25:31 | "{0}" | Argument '{0}' has not been supplied to $@ format string. | FormatMissingArgument.cs:25:27:25:31 | "{0}" | this |
+| FormatMissingArgument.cs:31:9:31:32 | call to method Format | FormatMissingArgument.cs:22:16:22:20 | "{1}" : String | FormatMissingArgument.cs:31:23:31:28 | access to parameter format | Argument '{1}' has not been supplied to $@ format string. | FormatMissingArgument.cs:22:16:22:20 | "{1}" | this |
 | FormatMissingArgumentBad.cs:7:9:7:49 | call to method WriteLine | FormatMissingArgumentBad.cs:7:27:7:41 | "Hello {0} {1}" | FormatMissingArgumentBad.cs:7:27:7:41 | "Hello {0} {1}" | Argument '{1}' has not been supplied to $@ format string. | FormatMissingArgumentBad.cs:7:27:7:41 | "Hello {0} {1}" | this |
 | FormatMissingArgumentBad.cs:8:9:8:55 | call to method WriteLine | FormatMissingArgumentBad.cs:8:27:8:41 | "Hello {1} {2}" | FormatMissingArgumentBad.cs:8:27:8:41 | "Hello {1} {2}" | Argument '{2}' has not been supplied to $@ format string. | FormatMissingArgumentBad.cs:8:27:8:41 | "Hello {1} {2}" | this |
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs b/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;
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected b/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 |

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+lgtm,codescanning`
	`2`	+* For string formatting methods, such as `System.Console.WriteLine(string format, params object[] arg)`, we now also recognize overloads without insertion parameters as string formatting methods. For example, `System.Console.WriteLine(string value)` is now also a member of the class `FormatMethod` in `frameworks/Format.qll`.
Original file line number	Diff line number	Diff line change
`@@ -4,6 +4,6 @@ class Bad`
`4`	`4`	`{`
`5`	`5`	`string GenerateEmptyClass(string c)`
`6`	`6`	`{`
`7`		`- return string.Format("class {0} { }");`
	`7`	`+ return string.Format("class {0} { }", "C");`
`8`	`8`	`}`
`9`	`9`	`}`
Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@ class FormatStringConfiguration extends TaintTracking::Configuration {`
`27`	`27`	`}`
`28`	`28`
`29`	`29`	`override predicate isSink(DataFlow::Node sink) {`
`30`		`- sink.asExpr() = any(FormatCall call).getFormatExpr()`
	`30`	`+ sink.asExpr() = any(FormatCall call \| call.hasInsertions()).getFormatExpr()`
`31`	`31`	`}`
`32`	`32`	`}`
`33`	`33`
Original file line number	Diff line number	Diff line change
`@@ -115,6 +115,8 @@ void FormatMethodTests()`
`115`	`115`	`System.Diagnostics.Debug.Assert(true, "Error", "}", ps);`
`116`	`116`	`sw.Write("}", 0);`
`117`	`117`	`System.Diagnostics.Debug.Print("}", ps);`
	`118`	`+`
	`119`	`+ Console.WriteLine("}"); // GOOD`
`118`	`120`	`}`
`119`	`121`
`120`	`122`	`System.IO.StringWriter sw;`
Original file line number	Diff line number	Diff line change
`@@ -20,6 +20,9 @@ void TestFormatMissingArgument()`
`20`	`20`	`String.Format("{0} {1} {2} {3}", 0, 1, 2, 3);`
`21`	`21`
`22`	`22`	`helper("{1}");`
	`23`	`+`
	`24`	`+ // BAD: Missing {0}`
	`25`	`+ Console.WriteLine("{0}");`
`23`	`26`	`}`
`24`	`27`
`25`	`28`	`void helper(string format)`