diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll index cf61a5d75aab..f2fd292693d8 100644 --- a/csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll +++ b/csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll @@ -289,3 +289,31 @@ 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) } +} diff --git a/csharp/ql/src/API Abuse/FormatInvalid.ql b/csharp/ql/src/API Abuse/FormatInvalid.ql index a2b8ef5e2220..575613902921 100644 --- a/csharp/ql/src/API Abuse/FormatInvalid.ql +++ b/csharp/ql/src/API Abuse/FormatInvalid.ql @@ -15,22 +15,6 @@ import semmle.code.csharp.frameworks.system.Text import semmle.code.csharp.frameworks.Format import FormatFlow::PathGraph -abstract class FormatStringParseCall extends MethodCall { - abstract Expr getFormatExpr(); -} - -class OrdinaryFormatCall extends FormatStringParseCall instanceof FormatCall { - override Expr getFormatExpr() { result = FormatCall.super.getFormatExpr() } -} - -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 } diff --git a/csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql b/csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql index a027170dc372..b99839226c59 100644 --- a/csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql +++ b/csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql @@ -20,7 +20,7 @@ module FormatStringConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource } predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(FormatCall call | call.hasInsertions()).getFormatExpr() + sink.asExpr() = any(FormatStringParseCall call).getFormatExpr() } } diff --git a/csharp/ql/src/change-notes/2025-04-10-uncontrolled-format-string.md b/csharp/ql/src/change-notes/2025-04-10-uncontrolled-format-string.md new file mode 100644 index 000000000000..184f84b51761 --- /dev/null +++ b/csharp/ql/src/change-notes/2025-04-10-uncontrolled-format-string.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The precision of the query `cs/uncontrolled-format-string` has been improved. Calls to `System.Text.CompositeFormat.Parse` are now considered a format like method call. diff --git a/csharp/ql/test/query-tests/Security Features/CWE-134/ConsoleUncontrolledFormatString.cs b/csharp/ql/test/query-tests/Security Features/CWE-134/ConsoleUncontrolledFormatString.cs index c9d4440cf787..c7d8c38a71b4 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-134/ConsoleUncontrolledFormatString.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-134/ConsoleUncontrolledFormatString.cs @@ -5,9 +5,9 @@ public class Program { public static void Main() { - var format = Console.ReadLine(); + var format = Console.ReadLine(); // $ Source // BAD: Uncontrolled format string. - var x = string.Format(format, 1, 2); + var x = string.Format(format, 1, 2); // $ Alert } } 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 index 37da55bec766..814167b15d91 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs @@ -1,4 +1,5 @@ using System; +using System.Text; using System.IO; using System.Web; @@ -6,13 +7,13 @@ public class TaintedPathHandler : IHttpHandler { public void ProcessRequest(HttpContext ctx) { - String path = ctx.Request.QueryString["page"]; + String path = ctx.Request.QueryString["page"]; // $ Source // BAD: Uncontrolled format string. - String.Format(path, "Do not do this"); + String.Format(path, "Do not do this"); // $ Alert // BAD: Using an IFormatProvider. - String.Format((IFormatProvider)null, path, "Do not do this"); + String.Format((IFormatProvider)null, path, "Do not do this"); // $ Alert // GOOD: Not the format string. String.Format("Do not do this", path); @@ -22,6 +23,9 @@ public void ProcessRequest(HttpContext ctx) // GOOD: Not a formatting call Console.WriteLine(path); + + // BAD: Uncontrolled format string. + CompositeFormat.Parse(path); // $ Alert } System.Windows.Forms.TextBox box1; @@ -29,6 +33,6 @@ public void ProcessRequest(HttpContext ctx) void OnButtonClicked() { // BAD: Uncontrolled format string. - String.Format(box1.Text, "Do not do this"); + String.Format(box1.Text, "Do not do this"); // $ Alert } } 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 index 6c70f8450b2e..fa6aa70abf7c 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected @@ -1,17 +1,19 @@ #select | ConsoleUncontrolledFormatString.cs:11:31:11:36 | access to local variable format | ConsoleUncontrolledFormatString.cs:8:22:8:39 | call to method ReadLine : String | ConsoleUncontrolledFormatString.cs:11:31:11:36 | access to local variable format | This format string depends on $@. | ConsoleUncontrolledFormatString.cs:8:22:8:39 | call to method ReadLine | thisread from stdin | -| UncontrolledFormatString.cs:12:23:12:26 | access to local variable path | UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:12:23:12:26 | access to local variable path | This format string depends on $@. | UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString | thisASP.NET query string | -| UncontrolledFormatString.cs:15:46:15:49 | access to local variable path | UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:15:46:15:49 | access to local variable path | This format string depends on $@. | UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString | thisASP.NET query string | -| UncontrolledFormatString.cs:32:23:32:31 | access to property Text | UncontrolledFormatString.cs:32:23:32:31 | access to property Text | UncontrolledFormatString.cs:32:23:32:31 | access to property Text | This format string depends on $@. | UncontrolledFormatString.cs:32:23:32:31 | access to property Text | thisTextBox text | +| UncontrolledFormatString.cs:13:23:13:26 | access to local variable path | UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:13:23:13:26 | access to local variable path | This format string depends on $@. | UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString | thisASP.NET query string | +| UncontrolledFormatString.cs:16:46:16:49 | access to local variable path | UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:16:46:16:49 | access to local variable path | This format string depends on $@. | UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString | thisASP.NET query string | +| UncontrolledFormatString.cs:28:31:28:34 | access to local variable path | UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:28:31:28:34 | access to local variable path | This format string depends on $@. | UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString | thisASP.NET query string | +| UncontrolledFormatString.cs:36:23:36:31 | access to property Text | UncontrolledFormatString.cs:36:23:36:31 | access to property Text | UncontrolledFormatString.cs:36:23:36:31 | access to property Text | This format string depends on $@. | UncontrolledFormatString.cs:36:23:36:31 | access to property Text | thisTextBox 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 | This format string depends on $@. | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString | thisASP.NET query string | edges | ConsoleUncontrolledFormatString.cs:8:13:8:18 | access to local variable format : String | ConsoleUncontrolledFormatString.cs:11:31:11:36 | access to local variable format | provenance | | | ConsoleUncontrolledFormatString.cs:8:22:8:39 | call to method ReadLine : String | ConsoleUncontrolledFormatString.cs:8:13:8:18 | access to local variable format : String | provenance | Src:MaD:1 | -| UncontrolledFormatString.cs:9:16:9:19 | access to local variable path : String | UncontrolledFormatString.cs:12:23:12:26 | access to local variable path | provenance | | -| UncontrolledFormatString.cs:9:16:9:19 | access to local variable path : String | UncontrolledFormatString.cs:15:46:15:49 | access to local variable path | provenance | | -| UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:9:16:9:19 | access to local variable path : String | provenance | | -| UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:9:23:9:53 | access to indexer : String | provenance | MaD:2 | -| UncontrolledFormatString.cs:9:23:9:53 | access to indexer : String | UncontrolledFormatString.cs:9:16:9:19 | access to local variable path : String | provenance | | +| UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | UncontrolledFormatString.cs:13:23:13:26 | access to local variable path | provenance | | +| UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | UncontrolledFormatString.cs:16:46:16:49 | access to local variable path | provenance | | +| UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | UncontrolledFormatString.cs:28:31:28:34 | access to local variable path | provenance | | +| UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | provenance | | +| UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:10:23:10:53 | access to indexer : String | provenance | MaD:2 | +| UncontrolledFormatString.cs:10:23:10:53 | access to indexer : String | UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | provenance | | | UncontrolledFormatStringBad.cs:9:16:9:21 | access to local variable format : String | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format | provenance | | | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString : NameValueCollection | UncontrolledFormatStringBad.cs:9:16:9:21 | access to local variable format : String | provenance | | | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString : NameValueCollection | UncontrolledFormatStringBad.cs:9:25:9:61 | access to indexer : String | provenance | MaD:2 | @@ -23,12 +25,13 @@ nodes | ConsoleUncontrolledFormatString.cs:8:13:8:18 | access to local variable format : String | semmle.label | access to local variable format : String | | ConsoleUncontrolledFormatString.cs:8:22:8:39 | call to method ReadLine : String | semmle.label | call to method ReadLine : String | | ConsoleUncontrolledFormatString.cs:11:31:11:36 | access to local variable format | semmle.label | access to local variable format | -| UncontrolledFormatString.cs:9:16:9:19 | access to local variable path : String | semmle.label | access to local variable path : String | -| UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection | -| UncontrolledFormatString.cs:9:23:9:53 | access to indexer : String | semmle.label | access to indexer : String | -| UncontrolledFormatString.cs:12:23:12:26 | access to local variable path | semmle.label | access to local variable path | -| UncontrolledFormatString.cs:15:46:15:49 | access to local variable path | semmle.label | access to local variable path | -| UncontrolledFormatString.cs:32:23:32:31 | access to property Text | semmle.label | access to property Text | +| UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | semmle.label | access to local variable path : String | +| UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection | +| UncontrolledFormatString.cs:10:23:10:53 | access to indexer : String | semmle.label | access to indexer : String | +| UncontrolledFormatString.cs:13:23:13:26 | access to local variable path | semmle.label | access to local variable path | +| UncontrolledFormatString.cs:16:46:16:49 | access to local variable path | semmle.label | access to local variable path | +| UncontrolledFormatString.cs:28:31:28:34 | access to local variable path | semmle.label | access to local variable path | +| UncontrolledFormatString.cs:36:23:36:31 | access to property Text | semmle.label | access to property Text | | UncontrolledFormatStringBad.cs:9:16:9:21 | access to local variable format : String | semmle.label | access to local variable format : String | | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection | | UncontrolledFormatStringBad.cs:9:25:9:61 | access to indexer : String | semmle.label | access to indexer : String | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.qlref b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.qlref index 88de17860f9c..10aa9e825cdd 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.qlref +++ b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.qlref @@ -1,2 +1,4 @@ query: Security Features/CWE-134/UncontrolledFormatString.ql -postprocess: utils/test/PrettyPrintModels.ql +postprocess: + - utils/test/PrettyPrintModels.ql + - utils/test/InlineExpectationsTestQuery.ql diff --git a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs index dc0c689eefa5..aeb252b18a71 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs @@ -6,9 +6,9 @@ public class HttpHandler : IHttpHandler public void ProcessRequest(HttpContext ctx) { - string format = ctx.Request.QueryString["nameformat"]; + string format = ctx.Request.QueryString["nameformat"]; // $ Source // BAD: Uncontrolled format string. - FormattedName = string.Format(format, Surname, Forenames); + FormattedName = string.Format(format, Surname, Forenames); // $ Alert } }