Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b4ceda7
C#: Add cs/invalid-string-formatting to the codeql quality suite.
michaelnebel Mar 28, 2025
b449497
C#: Convert testing of cs/invalid-string-formatting to inline expecta…
michaelnebel Apr 3, 2025
1034bda
C#: Re-factor FormatMethod.
michaelnebel Apr 4, 2025
c4b3b9b
C#: Add more format testcases.
michaelnebel Apr 4, 2025
67148d9
C#: Remove false positive example.
michaelnebel Apr 4, 2025
422a9bd
C#: Remove some false positives and add more true positives for cs/in…
michaelnebel Apr 4, 2025
cfe6cbd
C#: Update test expected output.
michaelnebel Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 75 additions & 49 deletions csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,67 +8,93 @@ 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()`. */
class FormatMethod extends Method {
FormatMethod() {
exists(Class declType | declType = this.getDeclaringType() |
abstract class FormatMethod extends Method {
/**
* Gets the argument containing the format string. For example, the argument of
* `string.Format(IFormatProvider, String, Object)` is `1`.
*/
abstract int getFormatArgument();
}

private class StringAndStringBuilderFormatMethods extends FormatMethod {
StringAndStringBuilderFormatMethods() {
(
this.getParameter(0).getType() instanceof SystemIFormatProviderInterface and
this.getParameter(1).getType() instanceof StringType and
this.getParameter(1).getType() instanceof StringType
or
this.getParameter(0).getType() instanceof StringType
) and
(
this = any(SystemStringClass c).getFormatMethod()
or
this = any(SystemTextStringBuilderClass c).getAppendFormatMethod()
)
}

override int getFormatArgument() {
if this.getParameter(0).getType() instanceof SystemIFormatProviderInterface
then result = 1
else result = 0
}
}

private class SystemConsoleAndSystemIoTextWriterFormatMethods extends FormatMethod {
SystemConsoleAndSystemIoTextWriterFormatMethods() {
this.getParameter(0).getType() instanceof StringType and
this.getNumberOfParameters() > 1 and
exists(Class declType | declType = this.getDeclaringType() |
this.hasName(["Write", "WriteLine"]) and
(
this = any(SystemStringClass c).getFormatMethod()
declType.hasFullyQualifiedName("System", "Console")
or
this = any(SystemTextStringBuilderClass c).getAppendFormatMethod()
declType.hasFullyQualifiedName("System.IO", "TextWriter")
)
or
this.getParameter(0).getType() instanceof StringType and
)
}

override int getFormatArgument() { result = 0 }
}

private class SystemDiagnosticsDebugAssert extends FormatMethod {
SystemDiagnosticsDebugAssert() {
this.hasName("Assert") and
this.getDeclaringType().hasFullyQualifiedName("System.Diagnostics", "Debug") and
this.getNumberOfParameters() = 4
}

override int getFormatArgument() { result = 2 }
}

private class SystemDiagnosticsFormatMethods extends FormatMethod {
SystemDiagnosticsFormatMethods() {
this.getParameter(0).getType() instanceof StringType and
this.getNumberOfParameters() > 1 and
exists(Class declType |
declType = this.getDeclaringType() and
declType.getNamespace().getFullName() = "System.Diagnostics"
|
declType.hasName("Trace") and
(
this = any(SystemStringClass c).getFormatMethod()
or
this = any(SystemTextStringBuilderClass c).getAppendFormatMethod()
or
(this.hasName("Write") or this.hasName("WriteLine")) and
(
declType.hasFullyQualifiedName("System", "Console")
or
declType.hasFullyQualifiedName("System.IO", "TextWriter")
or
declType.hasFullyQualifiedName("System.Diagnostics", "Debug") and
this.getParameter(1).getType() instanceof ArrayType
)
this.hasName("TraceError")
or
declType.hasFullyQualifiedName("System.Diagnostics", "Trace") and
(
this.hasName("TraceError") or
this.hasName("TraceInformation") or
this.hasName("TraceWarning")
)
this.hasName("TraceInformation")
or
this.hasName("TraceInformation") and
declType.hasFullyQualifiedName("System.Diagnostics", "TraceSource")
or
this.hasName("Print") and
declType.hasFullyQualifiedName("System.Diagnostics", "Debug")
this.hasName("TraceWarning")
)
or
this.hasName("Assert") and
declType.hasFullyQualifiedName("System.Diagnostics", "Debug") and
this.getNumberOfParameters() = 4
declType.hasName("TraceSource") and this.hasName("TraceInformation")
or
declType.hasName("Debug") and
(
this.hasName("Print")
or
this.hasName(["Write", "WriteLine"]) and
this.getParameter(1).getType() instanceof ArrayType
)
)
}

/**
* Gets the argument containing the format string. For example, the argument of
* `string.Format(IFormatProvider, String, Object)` is `1`.
*/
int getFormatArgument() {
if this.getParameter(0).getType() instanceof SystemIFormatProviderInterface
then result = 1
else
if
this.hasName("Assert") and
this.getDeclaringType().hasFullyQualifiedName("System.Diagnostics", "Debug")
then result = 2
else result = 0
}
override int getFormatArgument() { result = 0 }
}

pragma[nomagic]
Expand Down
1 change: 0 additions & 1 deletion csharp/ql/src/API Abuse/FormatInvalid.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ private predicate invalidFormatString(
source.getNode().asExpr() = src and
sink.getNode().asExpr() = call.getFormatExpr() and
FormatInvalid::flowPath(source, sink) and
call.hasInsertions() and
msg = "Invalid format string used in $@ formatting call." and
callString = "this"
}
Expand Down
1 change: 1 addition & 0 deletions csharp/ql/src/codeql-suites/csharp-code-quality.qls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@
- cs/useless-gethashcode-call
- cs/non-short-circuit
- cs/useless-assignment-to-local
- cs/invalid-string-formatting
117 changes: 66 additions & 51 deletions csharp/ql/test/query-tests/API Abuse/FormatInvalid/FormatInvalid.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,38 +24,38 @@ void FormatStringTests()
String.Format("{0, -10 :0.000}", 1);

// BAD: Invalid format string
String.Format("{ 0}", 1);
String.Format("{ 0}", 1); // $ Alert

// BAD: Invalid format string
String.Format("{0,--1}", 1);
String.Format("{0,--1}", 1); // $ Alert

// BAD: Invalid format string
String.Format("{0:{}}", 1);
String.Format("{0:{}}", 1); // $ Alert

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

// BAD: } { in the middle.
String.Format("{{0}-{1}}", 0, 1);
String.Format("{{0}-{1}}", 0, 1); // $ Alert

// BAD: This is invalid
String.Format("{0}}", 0, 1);
String.Format("{0}}", 0, 1); // $ Alert

// BAD: Invalid
string.Format("{foo{0}}", 0);
string.Format("{foo{0}}", 0); // $ Alert

// GOOD: {{ is output as {
String.Format("{{sdc}}", 0);
String.Format("{{sdc}}{0}", 0);

// BAD: Invalid: Stray }
String.Format("}", 0);
String.Format("}{0}", 0); // $ Alert

// GOOD: {{ output as {
String.Format("new {0} ({1} => {{", 0);
String.Format("new {0} ({1} => {{", 0, 1);

// GOOD: Literal {{ and }}
String.Format("{{", "");
String.Format("{{{{}}", "");
String.Format("{0}{{", 0);
String.Format("{0}{{{{}}", 0);
}

IFormatProvider fp;
Expand All @@ -72,51 +72,66 @@ void FormatMethodTests()
Format("}", 0);

// BAD: All of these are format methods with an invalid string.
String.Format("}", 0);
String.Format("}", ps);
String.Format(fp, "}", ps);
String.Format("}", 0, 1);
String.Format("}", 0, 1, 2);
String.Format("}", 0, 1, 2, 3);

sb.AppendFormat("}", 0);
sb.AppendFormat("}", ps);
sb.AppendFormat(fp, "}", ps);
sb.AppendFormat("}", 0, 1);
sb.AppendFormat("}", 0, 1, 2);
sb.AppendFormat("}", 0, 1, 2, 3);

Console.WriteLine("}", 0);
Console.WriteLine("}", ps);
Console.WriteLine("}", 0, 1);
Console.WriteLine("}", 0, 1, 2);
Console.WriteLine("}", 0, 1, 2, 3);

tw.WriteLine("}", 0);
tw.WriteLine("}", ps);
tw.WriteLine("}", 0, 1);
tw.WriteLine("}", 0, 1, 2);
tw.WriteLine("}", 0, 1, 2, 3);

System.Diagnostics.Debug.WriteLine("}", ps);
System.Diagnostics.Trace.TraceError("}", 0);
System.Diagnostics.Trace.TraceInformation("}", 0);
System.Diagnostics.Trace.TraceWarning("}", 0);
ts.TraceInformation("}", 0);

Console.Write("}", 0);
Console.Write("}", 0, 1);
Console.Write("}", 0, 1, 2);
Console.Write("}", 0, 1, 2, 3);
String.Format("}"); // $ Alert
String.Format("}", 0); // $ Alert
String.Format("}", ps); // $ Alert
String.Format(fp, "}", ps); // $ Alert
String.Format("}", 0, 1); // $ Alert
String.Format("}", 0, 1, 2); // $ Alert
String.Format("}", 0, 1, 2, 3); // $ Alert

sb.AppendFormat("}"); // $ Alert
sb.AppendFormat("}", 0); // $ Alert
sb.AppendFormat("}", ps); // $ Alert
sb.AppendFormat(fp, "}", ps); // $ Alert
sb.AppendFormat("}", 0, 1); // $ Alert
sb.AppendFormat("}", 0, 1, 2); // $ Alert
sb.AppendFormat("}", 0, 1, 2, 3); // $ Alert

Console.WriteLine("}", 0); // $ Alert
Console.WriteLine("}", ps); // $ Alert
Console.WriteLine("}", 0, 1); // $ Alert
Console.WriteLine("}", 0, 1, 2); // $ Alert
Console.WriteLine("}", 0, 1, 2, 3); // $ Alert

tw.WriteLine("}", 0); // $ Alert
tw.WriteLine("}", ps); // $ Alert
tw.WriteLine("}", 0, 1); // $ Alert
tw.WriteLine("}", 0, 1, 2); // $ Alert
tw.WriteLine("}", 0, 1, 2, 3); // $ Alert

System.Diagnostics.Debug.WriteLine("}", ps); // $ Alert
System.Diagnostics.Trace.TraceError("}", 0); // $ Alert
System.Diagnostics.Trace.TraceInformation("}", 0); // $ Alert
System.Diagnostics.Trace.TraceWarning("}", 0); // $ Alert
ts.TraceInformation("}", 0); // $ Alert

Console.Write("}", 0); // $ Alert
Console.Write("}", 0, 1); // $ Alert
Console.Write("}", 0, 1, 2); // $ Alert
Console.Write("}", 0, 1, 2, 3); // $ Alert

System.Diagnostics.Debug.WriteLine("}", ""); // GOOD
System.Diagnostics.Debug.Write("}", ""); // GOOD

System.Diagnostics.Debug.Assert(true, "Error", "}", ps);
sw.Write("}", 0);
System.Diagnostics.Debug.Print("}", ps);
System.Diagnostics.Debug.Assert(true, "Error", "}", ps); // $ Alert
sw.Write("}", 0); // $ Alert
System.Diagnostics.Debug.Print("}", ps); // $ Alert

Console.WriteLine("}"); // GOOD

// The Following methods are not recognised as format methods.
Console.WriteLine("{0}"); // GOOD
Console.Write("{0}"); // GOOD
tw.WriteLine("{0}"); // GOOD
tw.Write("{0}"); // GOOD
System.Diagnostics.Debug.Print("{0}"); // GOOD
System.Diagnostics.Debug.WriteLine("{0}"); // GOOD
System.Diagnostics.Debug.Write("{0}"); // GOOD
System.Diagnostics.Trace.TraceError("{0}"); // GOOD
System.Diagnostics.Trace.TraceInformation("{0}"); // GOOD
System.Diagnostics.Trace.TraceWarning("{0}"); // GOOD
ts.TraceInformation("{0}"); // GOOD
}

System.IO.StringWriter sw;
Expand Down
Loading