C#: Improve `cs/invalid-string-formatting` and add to the Code Quality suite. #19148

michaelnebel · 2025-03-28T14:11:33Z

In this PR we

Add the cs/invalid-string-formatting to the code quality suite.
Re-write tests to use inline expectations tests; In this process a couple of faulty testcases were discovered and these were corrected (for instance - we only check that format strings are valid in case there are insertions).

Improve the cs/invalid-string-formatting query by

Adding invalid string format results for string.Format where no additional arguments are provided (for instance string.Format("{")). Also start taking CompositeFormat versions of Format methods into account and consider System.Text.CompositeFormat.Parse a method that parses format strings and might throw exceptions and cause a runtime crash.
Add support for more format methods like generic versions of string.Format and StringBuilder.AppendFormat and also add support for MemoryExtensions.TryWrite.
Remove false positives related to Console.WriteLine(string) and friends (methods that itsn't format methods).
Remove false positives related to `Console.WriteLine(string, ReadOnlySpan) (methods that use params collection type overloads).
Review commit by commit is encouraged.

@hvitved : The data flow configurations are quite similar. Is it a better approach to use flow state to track calls to CompositeFormat.Parse? (instead of merging two similar graphs).

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

michaelnebel · 2025-04-10T06:43:53Z

DCA

There doesn't appear to be any changes to performance or results for our DCA suite.
The level produced on our DCA suite for this query looks acceptable (query is not unreasonable noisy).

hvitved

Great work, thanks for breaking up into individual commits, that made reviewing a lot easier.

hvitved · 2025-04-23T07:22:47Z

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

        String.Format("{0} {1} {2} {3}", 0, 1, 2, 3);

-        helper("{1}");
+        helper("{1}"); // $ Source


I think this should be e.g. Source=source1 and then $ Alert=source1 further down.

hvitved · 2025-04-23T07:25:04Z

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


        // BAD: Invalid format string
-        String.Format("%d", 1);
+        String.Format("%d", 1); // $ Alert Sink


I don't think the Sink part is needed when it is on the same line as the alert.

The Sink and Alert doesn't have the exact same location (only the same line numbers), so the test fails, if we don't add both tags.

Oh, right. Looks like we need to do the same as #18653 for sinks then (cc @asgerf ).

https://github.com/github/codeql/pull/19354/files

CoPilot managed to do it 😸

hvitved · 2025-04-23T07:27:02Z

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

-class FormatMethod extends Method {
-  FormatMethod() {
-    exists(Class declType | declType = this.getDeclaringType() |
+abstract class FormatMethod extends Method {


Now we are exposing an abstract method, which is not backwards-compatible (and also not best practice). So Instead replace the current abstract FormatMethod class with a private FormatMethodImpl class, and add final class FormatMethod = FormatMethodImpl;.

Never mind, I see that you did exactly this in a subsequent commit.

hvitved · 2025-04-23T07:30:52Z

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

+    void TestCompositeFormatMissingArgument()
+    {
+        var format0 = CompositeFormat.Parse("{0}");
+        var format1 = CompositeFormat.Parse("{1}"); // $ Source


Again, it is better to uniquely identify the sources so we can match them up against the alerts.

hvitved · 2025-04-23T07:57:21Z

@hvitved : The data flow configurations are quite similar. Is it a better approach to use flow state to track calls to CompositeFormat.Parse? (instead of merging two similar graphs).

I think it is fine to keep as-is.

…tions and adjust some of the testcases.

…valid-string-format.

…count.

github-actions bot added the C# label Mar 28, 2025

michaelnebel force-pushed the csharp/invalid-string-format branch 5 times, most recently from cfe6cbd to 286ae79 Compare April 8, 2025 09:29

github-advanced-security bot found potential problems Apr 8, 2025

View reviewed changes

csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll Fixed Show fixed Hide fixed

michaelnebel force-pushed the csharp/invalid-string-format branch 2 times, most recently from 852eab9 to e8f8145 Compare April 9, 2025 12:43

michaelnebel force-pushed the csharp/invalid-string-format branch from 51a03d2 to be8d379 Compare April 10, 2025 06:48

github-actions bot added the documentation label Apr 10, 2025

michaelnebel marked this pull request as ready for review April 10, 2025 08:12

michaelnebel requested a review from a team as a code owner April 10, 2025 08:12

michaelnebel changed the title ~~C#: Add cs/invalid-string-formatting to the codeql quality suite.~~ C#: Improve cs/invalid-string-formatting and add to the codeql quality suite. Apr 10, 2025

michaelnebel force-pushed the csharp/invalid-string-format branch from be8d379 to 5c6b1dd Compare April 10, 2025 11:46

michaelnebel mentioned this pull request Apr 10, 2025

C#: Improve precision of cs/uncontrolled-format-string. #19271

Merged

michaelnebel changed the title ~~C#: Improve cs/invalid-string-formatting and add to the codeql quality suite.~~ C#: Improve cs/invalid-string-formatting and add to the Code Quality suite. Apr 11, 2025

hvitved requested changes Apr 23, 2025

View reviewed changes

michaelnebel force-pushed the csharp/invalid-string-format branch from 5c6b1dd to b9183ff Compare April 23, 2025 10:59

michaelnebel requested a review from a team as a code owner April 23, 2025 13:36

github-actions bot added the JS label Apr 23, 2025

michaelnebel force-pushed the csharp/invalid-string-format branch from c10b5ce to b9183ff Compare April 23, 2025 13:38

github-actions bot removed the JS label Apr 23, 2025

michaelnebel added 5 commits April 24, 2025 08:54

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

68f9349

C#: Convert testing of cs/invalid-string-formatting to inline expecta…

9cfd6e3

…tions and adjust some of the testcases.

C#: Re-factor FormatMethod.

327ddb0

C#: Add more format testcases.

175e4ec

C#: Remove false positive example.

f73b742

michaelnebel added 13 commits April 24, 2025 08:54

C#: Remove some false positives and add more true positives for cs/in…

1d9d878

…valid-string-format.

C#: Update test expected output.

8fb5fe9

C#: Add more invalid-string-formatting testcases.

11dffc6

C#: Improve format logic to take CompositeFormat and generics into ac…

f31235d

…count.

C#: Update test expected output.

39abd5c

C#: Add FP for string.Format using params collection.

930bb6b

C#: Generalize array logic to params collection like types.

042c7e5

C#: Update test expected output.

6de5920

C#: Hide the abstract FormatMethod class.

f2dddd6

C#: Update string format item parameter expected test case.

22ae3e7

C#: Add change note.

b6d2f14

C#: Match up sources, alerts and sinks in the tests.

dcf11c2

C#: Remove all Sink tags after rebase.

65ac951

michaelnebel force-pushed the csharp/invalid-string-format branch from b9183ff to 65ac951 Compare April 24, 2025 06:56

hvitved approved these changes Apr 24, 2025

View reviewed changes

michaelnebel merged commit 17f58c9 into github:main Apr 24, 2025
23 checks passed

michaelnebel deleted the csharp/invalid-string-format branch April 24, 2025 08:03

C#: Improve cs/invalid-string-formatting and add to the Code Quality suite. #19148

C#: Improve cs/invalid-string-formatting and add to the Code Quality suite. #19148

Uh oh!

Conversation

michaelnebel commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

michaelnebel commented Apr 10, 2025

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvitved commented Apr 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

C#: Improve `cs/invalid-string-formatting` and add to the Code Quality suite. #19148

C#: Improve `cs/invalid-string-formatting` and add to the Code Quality suite. #19148

michaelnebel commented Mar 28, 2025 •

edited

Loading