Skip to content

Commit 17f58c9

Browse files
michaelnebelmichaelnebel
authored
Merge pull request #19148 from michaelnebel/csharp/invalid-string-format
C#: Improve `cs/invalid-string-formatting` and add to the Code Quality suite.
2 parents 91f1183 + 65ac951 commit 17f58c9

File tree

16 files changed

+799
-280
lines changed

16 files changed

+799
-280
lines changed

‎csharp/ql/lib/semmle/code/csharp/commons/Collections.qll‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,8 @@ private class ParamsConstructedCollectionTypes extends ParamsCollectionTypeImpl
9797
unboundbase instanceof SystemCollectionsGenericIReadOnlyListTInterface or
9898
unboundbase instanceof SystemSpanStruct or
9999
unboundbase instanceof SystemReadOnlySpanStruct
100-
)
100+
) and
101+
not this instanceof SystemStringClass
101102
}
102103

103104
override Type getElementType() { result = base.getTypeArgument(0) }

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

Lines changed: 118 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -8,67 +8,120 @@ private import semmle.code.csharp.frameworks.System
88
private import semmle.code.csharp.frameworks.system.Text
99

1010
/** A method that formats a string, for example `string.Format()`. */
11-
class FormatMethod extends Method {
12-
FormatMethod() {
13-
exists(Class declType | declType = this.getDeclaringType() |
11+
abstract private class FormatMethodImpl extends Method {
12+
/**
13+
* Gets the argument containing the format string. For example, the argument of
14+
* `string.Format(IFormatProvider, String, Object)` is `1`.
15+
*/
16+
abstract int getFormatArgument();
17+
18+
/**
19+
* Gets the argument number of the first supplied insert.
20+
*/
21+
int getFirstArgument() { result = this.getFormatArgument() + 1 }
22+
}
23+
24+
final class FormatMethod = FormatMethodImpl;
25+
26+
/** A class of types used for formatting. */
27+
private class FormatType extends Type {
28+
FormatType() {
29+
this instanceof StringType or
30+
this instanceof SystemTextCompositeFormatClass
31+
}
32+
}
33+
34+
private class StringAndStringBuilderFormatMethods extends FormatMethodImpl {
35+
StringAndStringBuilderFormatMethods() {
36+
(
1437
this.getParameter(0).getType() instanceof SystemIFormatProviderInterface and
15-
this.getParameter(1).getType() instanceof StringType and
38+
this.getParameter(1).getType() instanceof FormatType
39+
or
40+
this.getParameter(0).getType() instanceof StringType
41+
) and
42+
(
43+
this = any(SystemStringClass c).getFormatMethod()
44+
or
45+
this = any(SystemTextStringBuilderClass c).getAppendFormatMethod()
46+
)
47+
}
48+
49+
override int getFormatArgument() {
50+
if this.getParameter(0).getType() instanceof SystemIFormatProviderInterface
51+
then result = 1
52+
else result = 0
53+
}
54+
}
55+
56+
private class SystemMemoryExtensionsFormatMethods extends FormatMethodImpl {
57+
SystemMemoryExtensionsFormatMethods() {
58+
this = any(SystemMemoryExtensionsClass c).getTryWriteMethod() and
59+
this.getParameter(1).getType() instanceof SystemIFormatProviderInterface and
60+
this.getParameter(2).getType() instanceof SystemTextCompositeFormatClass
61+
}
62+
63+
override int getFormatArgument() { result = 2 }
64+
65+
override int getFirstArgument() { result = this.getFormatArgument() + 2 }
66+
}
67+
68+
private class SystemConsoleAndSystemIoTextWriterFormatMethods extends FormatMethodImpl {
69+
SystemConsoleAndSystemIoTextWriterFormatMethods() {
70+
this.getParameter(0).getType() instanceof StringType and
71+
this.getNumberOfParameters() > 1 and
72+
exists(Class declType | declType = this.getDeclaringType() |
73+
this.hasName(["Write", "WriteLine"]) and
1674
(
17-
this = any(SystemStringClass c).getFormatMethod()
75+
declType.hasFullyQualifiedName("System", "Console")
1876
or
19-
this = any(SystemTextStringBuilderClass c).getAppendFormatMethod()
77+
declType.hasFullyQualifiedName("System.IO", "TextWriter")
2078
)
21-
or
22-
this.getParameter(0).getType() instanceof StringType and
79+
)
80+
}
81+
82+
override int getFormatArgument() { result = 0 }
83+
}
84+
85+
private class SystemDiagnosticsDebugAssert extends FormatMethodImpl {
86+
SystemDiagnosticsDebugAssert() {
87+
this.hasName("Assert") and
88+
this.getDeclaringType().hasFullyQualifiedName("System.Diagnostics", "Debug") and
89+
this.getNumberOfParameters() = 4
90+
}
91+
92+
override int getFormatArgument() { result = 2 }
93+
}
94+
95+
private class SystemDiagnosticsFormatMethods extends FormatMethodImpl {
96+
SystemDiagnosticsFormatMethods() {
97+
this.getParameter(0).getType() instanceof StringType and
98+
this.getNumberOfParameters() > 1 and
99+
exists(Class declType |
100+
declType = this.getDeclaringType() and
101+
declType.getNamespace().getFullName() = "System.Diagnostics"
102+
|
103+
declType.hasName("Trace") and
23104
(
24-
this = any(SystemStringClass c).getFormatMethod()
25-
or
26-
this = any(SystemTextStringBuilderClass c).getAppendFormatMethod()
27-
or
28-
(this.hasName("Write") or this.hasName("WriteLine")) and
29-
(
30-
declType.hasFullyQualifiedName("System", "Console")
31-
or
32-
declType.hasFullyQualifiedName("System.IO", "TextWriter")
33-
or
34-
declType.hasFullyQualifiedName("System.Diagnostics", "Debug") and
35-
this.getParameter(1).getType() instanceof ArrayType
36-
)
105+
this.hasName("TraceError")
37106
or
38-
declType.hasFullyQualifiedName("System.Diagnostics", "Trace") and
39-
(
40-
this.hasName("TraceError") or
41-
this.hasName("TraceInformation") or
42-
this.hasName("TraceWarning")
43-
)
107+
this.hasName("TraceInformation")
44108
or
45-
this.hasName("TraceInformation") and
46-
declType.hasFullyQualifiedName("System.Diagnostics", "TraceSource")
47-
or
48-
this.hasName("Print") and
49-
declType.hasFullyQualifiedName("System.Diagnostics", "Debug")
109+
this.hasName("TraceWarning")
50110
)
51111
or
52-
this.hasName("Assert") and
53-
declType.hasFullyQualifiedName("System.Diagnostics", "Debug") and
54-
this.getNumberOfParameters() = 4
112+
declType.hasName("TraceSource") and this.hasName("TraceInformation")
113+
or
114+
declType.hasName("Debug") and
115+
(
116+
this.hasName("Print")
117+
or
118+
this.hasName(["Write", "WriteLine"]) and
119+
this.getParameter(1).getType() instanceof ArrayType
120+
)
55121
)
56122
}
57123

58-
/**
59-
* Gets the argument containing the format string. For example, the argument of
60-
* `string.Format(IFormatProvider, String, Object)` is `1`.
61-
*/
62-
int getFormatArgument() {
63-
if this.getParameter(0).getType() instanceof SystemIFormatProviderInterface
64-
then result = 1
65-
else
66-
if
67-
this.hasName("Assert") and
68-
this.getDeclaringType().hasFullyQualifiedName("System.Diagnostics", "Debug")
69-
then result = 2
70-
else result = 0
71-
}
124+
override int getFormatArgument() { result = 0 }
72125
}
73126

74127
pragma[nomagic]
@@ -194,24 +247,36 @@ class FormatCall extends MethodCall {
194247
int getFormatArgument() { result = this.getTarget().(FormatMethod).getFormatArgument() }
195248

196249
/** Gets the argument number of the first supplied insert. */
197-
int getFirstArgument() { result = this.getFormatArgument() + 1 }
250+
int getFirstArgument() { result = this.getTarget().(FormatMethod).getFirstArgument() }
198251

199252
/** Holds if this call has one or more insertions. */
200253
predicate hasInsertions() { exists(this.getArgument(this.getFirstArgument())) }
201254

202-
/** Holds if the arguments are supplied in an array, not individually. */
203-
predicate hasArrayExpr() {
255+
/**
256+
* DEPRECATED: use `hasCollectionExpr` instead.
257+
*
258+
* Holds if the arguments are supplied in an array, not individually.
259+
*/
260+
deprecated predicate hasArrayExpr() {
204261
this.getNumberOfArguments() = this.getFirstArgument() + 1 and
205262
this.getArgument(this.getFirstArgument()).getType() instanceof ArrayType
206263
}
207264

265+
/**
266+
* Holds if the arguments are supplied in a collection, not individually.
267+
*/
268+
predicate hasCollectionExpr() {
269+
this.getNumberOfArguments() = this.getFirstArgument() + 1 and
270+
this.getArgument(this.getFirstArgument()).getType() instanceof ParamsCollectionType
271+
}
272+
208273
/**
209274
* Gets the number of supplied arguments (excluding the format string and format
210275
* provider). Does not return a value if the arguments are supplied in an array,
211276
* in which case we generally can't assess the size of the array.
212277
*/
213278
int getSuppliedArguments() {
214-
not this.hasArrayExpr() and
279+
not this.hasCollectionExpr() and
215280
result = this.getNumberOfArguments() - this.getFirstArgument()
216281
}
217282

‎csharp/ql/lib/semmle/code/csharp/frameworks/System.qll‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,7 @@ class SystemStringClass extends StringType {
365365
/** Gets a `Format(...)` method. */
366366
Method getFormatMethod() {
367367
result.getDeclaringType() = this and
368-
result.hasName("Format") and
368+
result.getName().regexpMatch("Format(<.*>)?") and
369369
result.getNumberOfParameters() in [2 .. 5] and
370370
result.getReturnType() instanceof StringType
371371
}
@@ -751,6 +751,18 @@ class SystemNotImplementedExceptionClass extends SystemClass {
751751
SystemNotImplementedExceptionClass() { this.hasName("NotImplementedException") }
752752
}
753753

754+
/** The `System.MemoryExtensions` class. */
755+
class SystemMemoryExtensionsClass extends SystemClass {
756+
SystemMemoryExtensionsClass() { this.hasName("MemoryExtensions") }
757+
758+
/** Gets a `TryWrite` method. */
759+
Method getTryWriteMethod() {
760+
result.getDeclaringType() = this and
761+
result.getName().regexpMatch("TryWrite(<.*>)?") and
762+
result.getParameter(0).getType().getUnboundDeclaration() instanceof SystemSpanStruct
763+
}
764+
}
765+
754766
/** The `System.DateTime` struct. */
755767
class SystemDateTimeStruct extends SystemStruct {
756768
SystemDateTimeStruct() { this.hasName("DateTime") }

‎csharp/ql/lib/semmle/code/csharp/frameworks/system/Text.qll‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,12 @@ class SystemTextStringBuilderClass extends SystemTextClass {
2222
SystemTextStringBuilderClass() { this.hasName("StringBuilder") }
2323

2424
/** Gets the `AppendFormat` method. */
25-
Method getAppendFormatMethod() { result = this.getAMethod("AppendFormat") }
25+
Method getAppendFormatMethod() {
26+
exists(string name |
27+
name.regexpMatch("AppendFormat(<.*>)?") and
28+
result = this.getAMethod(name)
29+
)
30+
}
2631
}
2732

2833
/** The `System.Text.Encoding` class. */
@@ -38,3 +43,11 @@ class SystemTextEncodingClass extends SystemTextClass {
3843
/** Gets the `GetChars` method. */
3944
Method getGetCharsMethod() { result = this.getAMethod("GetChars") }
4045
}
46+
47+
/** The `System.Text.CompositeFormat` class */
48+
class SystemTextCompositeFormatClass extends SystemTextClass {
49+
SystemTextCompositeFormatClass() { this.hasName("CompositeFormat") }
50+
51+
/** Gets the `Parse` method. */
52+
Method getParseMethod() { result = this.getAMethod("Parse") }
53+
}

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

Lines changed: 52 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,37 +11,75 @@
1111
*/
1212

1313
import csharp
14+
import semmle.code.csharp.frameworks.system.Text
1415
import semmle.code.csharp.frameworks.Format
15-
import FormatInvalid::PathGraph
16+
import FormatFlow::PathGraph
17+
18+
abstract class FormatStringParseCall extends MethodCall {
19+
abstract Expr getFormatExpr();
20+
}
21+
22+
class OrdinaryFormatCall extends FormatStringParseCall instanceof FormatCall {
23+
override Expr getFormatExpr() { result = FormatCall.super.getFormatExpr() }
24+
}
25+
26+
class ParseFormatStringCall extends FormatStringParseCall {
27+
ParseFormatStringCall() {
28+
this.getTarget() = any(SystemTextCompositeFormatClass x).getParseMethod()
29+
}
30+
31+
override Expr getFormatExpr() { result = this.getArgument(0) }
32+
}
1633

1734
module FormatInvalidConfig implements DataFlow::ConfigSig {
1835
predicate isSource(DataFlow::Node n) { n.asExpr() instanceof StringLiteral }
1936

20-
predicate isSink(DataFlow::Node n) { exists(FormatCall c | n.asExpr() = c.getFormatExpr()) }
37+
predicate isSink(DataFlow::Node n) {
38+
exists(FormatStringParseCall c | n.asExpr() = c.getFormatExpr())
39+
}
2140
}
2241

2342
module FormatInvalid = DataFlow::Global<FormatInvalidConfig>;
2443

44+
module FormatLiteralConfig implements DataFlow::ConfigSig {
45+
predicate isSource(DataFlow::Node n) { n.asExpr() instanceof StringLiteral }
46+
47+
predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
48+
// Add flow via `System.Text.CompositeFormat.Parse`.
49+
exists(ParseFormatStringCall call |
50+
pred.asExpr() = call.getFormatExpr() and
51+
succ.asExpr() = call
52+
)
53+
}
54+
55+
predicate isSink(DataFlow::Node n) { exists(FormatCall c | n.asExpr() = c.getFormatExpr()) }
56+
}
57+
58+
module FormatLiteral = DataFlow::Global<FormatLiteralConfig>;
59+
60+
module FormatFlow =
61+
DataFlow::MergePathGraph<FormatInvalid::PathNode, FormatLiteral::PathNode,
62+
FormatInvalid::PathGraph, FormatLiteral::PathGraph>;
63+
2564
private predicate invalidFormatString(
2665
InvalidFormatString src, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
27-
FormatCall call, string callString
66+
FormatStringParseCall call, string callString
2867
) {
2968
source.getNode().asExpr() = src and
3069
sink.getNode().asExpr() = call.getFormatExpr() and
3170
FormatInvalid::flowPath(source, sink) and
32-
call.hasInsertions() and
3371
msg = "Invalid format string used in $@ formatting call." and
3472
callString = "this"
3573
}
3674

3775
private predicate unusedArgument(
38-
FormatCall call, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
76+
FormatCall call, FormatLiteral::PathNode source, FormatLiteral::PathNode sink, string msg,
3977
ValidFormatString src, string srcString, Expr unusedExpr, string unusedString
4078
) {
4179
exists(int unused |
4280
source.getNode().asExpr() = src and
4381
sink.getNode().asExpr() = call.getFormatExpr() and
44-
FormatInvalid::flowPath(source, sink) and
82+
FormatLiteral::flowPath(source, sink) and
4583
unused = call.getASuppliedArgument() and
4684
not unused = src.getAnInsert() and
4785
not src.getValue() = "" and
@@ -53,13 +91,13 @@ private predicate unusedArgument(
5391
}
5492

5593
private predicate missingArgument(
56-
FormatCall call, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
94+
FormatCall call, FormatLiteral::PathNode source, FormatLiteral::PathNode sink, string msg,
5795
ValidFormatString src, string srcString
5896
) {
5997
exists(int used, int supplied |
6098
source.getNode().asExpr() = src and
6199
sink.getNode().asExpr() = call.getFormatExpr() and
62-
FormatInvalid::flowPath(source, sink) and
100+
FormatLiteral::flowPath(source, sink) and
63101
used = src.getAnInsert() and
64102
supplied = call.getSuppliedArguments() and
65103
used >= supplied and
@@ -69,16 +107,17 @@ private predicate missingArgument(
69107
}
70108

71109
from
72-
Element alert, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
73-
Element extra1, string extra1String, Element extra2, string extra2String
110+
Element alert, FormatFlow::PathNode source, FormatFlow::PathNode sink, string msg, Element extra1,
111+
string extra1String, Element extra2, string extra2String
74112
where
75-
invalidFormatString(alert, source, sink, msg, extra1, extra1String) and
113+
invalidFormatString(alert, source.asPathNode1(), sink.asPathNode1(), msg, extra1, extra1String) and
76114
extra2 = extra1 and
77115
extra2String = extra1String
78116
or
79-
unusedArgument(alert, source, sink, msg, extra1, extra1String, extra2, extra2String)
117+
unusedArgument(alert, source.asPathNode2(), sink.asPathNode2(), msg, extra1, extra1String, extra2,
118+
extra2String)
80119
or
81-
missingArgument(alert, source, sink, msg, extra1, extra1String) and
120+
missingArgument(alert, source.asPathNode2(), sink.asPathNode2(), msg, extra1, extra1String) and
82121
extra2 = extra1 and
83122
extra2String = extra1String
84123
select alert, source, sink, msg, extra1, extra1String, extra2, extra2String

0 commit comments

Comments
 (0)