Skip to content

Commit 7531a95

Browse files
michaelnebelmichaelnebel
authored
Merge pull request #19271 from michaelnebel/csharp/uncontrolled-format-string
C#: Improve precision of `cs/uncontrolled-format-string`.
2 parents e903d76 + 3449a34 commit 7531a95

File tree

9 files changed

+63
-42
lines changed

9 files changed

+63
-42
lines changed

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

+26-2
Original file line numberDiff line numberDiff line change
@@ -233,15 +233,28 @@ class InvalidFormatString extends StringLiteral {
233233
}
234234
}
235235

236+
/**
237+
* A method call to a method that parses a format string, for example a call
238+
* to `string.Format()`.
239+
*/
240+
abstract private class FormatStringParseCallImpl extends MethodCall {
241+
/**
242+
* Gets the expression used as the format string.
243+
*/
244+
abstract Expr getFormatExpr();
245+
}
246+
247+
final class FormatStringParseCall = FormatStringParseCallImpl;
248+
236249
/**
237250
* A method call to a method that formats a string, for example a call
238251
* to `string.Format()`.
239252
*/
240-
class FormatCall extends MethodCall {
253+
class FormatCall extends FormatStringParseCallImpl {
241254
FormatCall() { this.getTarget() instanceof FormatMethod }
242255

243256
/** Gets the expression used as the format string. */
244-
Expr getFormatExpr() { result = this.getArgument(this.getFormatArgument()) }
257+
override Expr getFormatExpr() { result = this.getArgument(this.getFormatArgument()) }
245258

246259
/** Gets the argument number containing the format string. */
247260
int getFormatArgument() { result = this.getTarget().(FormatMethod).getFormatArgument() }
@@ -289,3 +302,14 @@ class FormatCall extends MethodCall {
289302
result = this.getArgument(this.getFirstArgument() + index)
290303
}
291304
}
305+
306+
/**
307+
* A method call to `System.Text.CompositeFormat.Parse`.
308+
*/
309+
class ParseFormatStringCall extends FormatStringParseCallImpl {
310+
ParseFormatStringCall() {
311+
this.getTarget() = any(SystemTextCompositeFormatClass x).getParseMethod()
312+
}
313+
314+
override Expr getFormatExpr() { result = this.getArgument(0) }
315+
}

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

-16
Original file line numberDiff line numberDiff line change
@@ -16,22 +16,6 @@ import semmle.code.csharp.frameworks.system.Text
1616
import semmle.code.csharp.frameworks.Format
1717
import FormatFlow::PathGraph
1818

19-
abstract class FormatStringParseCall extends MethodCall {
20-
abstract Expr getFormatExpr();
21-
}
22-
23-
class OrdinaryFormatCall extends FormatStringParseCall instanceof FormatCall {
24-
override Expr getFormatExpr() { result = FormatCall.super.getFormatExpr() }
25-
}
26-
27-
class ParseFormatStringCall extends FormatStringParseCall {
28-
ParseFormatStringCall() {
29-
this.getTarget() = any(SystemTextCompositeFormatClass x).getParseMethod()
30-
}
31-
32-
override Expr getFormatExpr() { result = this.getArgument(0) }
33-
}
34-
3519
module FormatInvalidConfig implements DataFlow::ConfigSig {
3620
predicate isSource(DataFlow::Node n) { n.asExpr() instanceof StringLiteral }
3721

‎csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql

+1-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ module FormatStringConfig implements DataFlow::ConfigSig {
2020
predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource }
2121

2222
predicate isSink(DataFlow::Node sink) {
23-
sink.asExpr() = any(FormatCall call | call.hasInsertions()).getFormatExpr()
23+
sink.asExpr() = any(FormatStringParseCall call).getFormatExpr()
2424
}
2525
}
2626

‎csharp/ql/src/change-notes/2025-04-10-uncontrolled-format-string.md

+4
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The precision of the query `cs/uncontrolled-format-string` has been improved (false negative reduction). Calls to `System.Text.CompositeFormat.Parse` are now considered a format like method call.

‎csharp/ql/test/query-tests/Security Features/CWE-134/ConsoleUncontrolledFormatString.cs

+2-2
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ public class Program
55
{
66
public static void Main()
77
{
8-
var format = Console.ReadLine();
8+
var format = Console.ReadLine(); // $ Source
99

1010
// BAD: Uncontrolled format string.
11-
var x = string.Format(format, 1, 2);
11+
var x = string.Format(format, 1, 2); // $ Alert
1212
}
1313
}

‎csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs

+8-4
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,19 @@
11
using System;
2+
using System.Text;
23
using System.IO;
34
using System.Web;
45

56
public class TaintedPathHandler : IHttpHandler
67
{
78
public void ProcessRequest(HttpContext ctx)
89
{
9-
String path = ctx.Request.QueryString["page"];
10+
String path = ctx.Request.QueryString["page"]; // $ Source
1011

1112
// BAD: Uncontrolled format string.
12-
String.Format(path, "Do not do this");
13+
String.Format(path, "Do not do this"); // $ Alert
1314

1415
// BAD: Using an IFormatProvider.
15-
String.Format((IFormatProvider)null, path, "Do not do this");
16+
String.Format((IFormatProvider)null, path, "Do not do this"); // $ Alert
1617

1718
// GOOD: Not the format string.
1819
String.Format("Do not do this", path);
@@ -22,13 +23,16 @@ public void ProcessRequest(HttpContext ctx)
2223

2324
// GOOD: Not a formatting call
2425
Console.WriteLine(path);
26+
27+
// BAD: Uncontrolled format string.
28+
CompositeFormat.Parse(path); // $ Alert
2529
}
2630

2731
System.Windows.Forms.TextBox box1;
2832

2933
void OnButtonClicked()
3034
{
3135
// BAD: Uncontrolled format string.
32-
String.Format(box1.Text, "Do not do this");
36+
String.Format(box1.Text, "Do not do this"); // $ Alert
3337
}
3438
}

‎csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected

+17-14
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,19 @@
11
#select
22
| 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 |
3-
| 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 |
4-
| 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 |
5-
| 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 |
3+
| 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 |
4+
| 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 |
5+
| 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 |
6+
| 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 |
67
| 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 |
78
edges
89
| ConsoleUncontrolledFormatString.cs:8:13:8:18 | access to local variable format : String | ConsoleUncontrolledFormatString.cs:11:31:11:36 | access to local variable format | provenance | |
910
| 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 |
10-
| UncontrolledFormatString.cs:9:16:9:19 | access to local variable path : String | UncontrolledFormatString.cs:12:23:12:26 | access to local variable path | provenance | |
11-
| UncontrolledFormatString.cs:9:16:9:19 | access to local variable path : String | UncontrolledFormatString.cs:15:46:15:49 | access to local variable path | provenance | |
12-
| UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:9:16:9:19 | access to local variable path : String | provenance | |
13-
| UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:9:23:9:53 | access to indexer : String | provenance | MaD:2 |
14-
| UncontrolledFormatString.cs:9:23:9:53 | access to indexer : String | UncontrolledFormatString.cs:9:16:9:19 | access to local variable path : String | provenance | |
11+
| UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | UncontrolledFormatString.cs:13:23:13:26 | access to local variable path | provenance | |
12+
| UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | UncontrolledFormatString.cs:16:46:16:49 | access to local variable path | provenance | |
13+
| UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | UncontrolledFormatString.cs:28:31:28:34 | access to local variable path | provenance | |
14+
| UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | provenance | |
15+
| UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:10:23:10:53 | access to indexer : String | provenance | MaD:2 |
16+
| UncontrolledFormatString.cs:10:23:10:53 | access to indexer : String | UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | provenance | |
1517
| UncontrolledFormatStringBad.cs:9:16:9:21 | access to local variable format : String | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format | provenance | |
1618
| UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString : NameValueCollection | UncontrolledFormatStringBad.cs:9:16:9:21 | access to local variable format : String | provenance | |
1719
| 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
2325
| ConsoleUncontrolledFormatString.cs:8:13:8:18 | access to local variable format : String | semmle.label | access to local variable format : String |
2426
| ConsoleUncontrolledFormatString.cs:8:22:8:39 | call to method ReadLine : String | semmle.label | call to method ReadLine : String |
2527
| ConsoleUncontrolledFormatString.cs:11:31:11:36 | access to local variable format | semmle.label | access to local variable format |
26-
| UncontrolledFormatString.cs:9:16:9:19 | access to local variable path : String | semmle.label | access to local variable path : String |
27-
| UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
28-
| UncontrolledFormatString.cs:9:23:9:53 | access to indexer : String | semmle.label | access to indexer : String |
29-
| UncontrolledFormatString.cs:12:23:12:26 | access to local variable path | semmle.label | access to local variable path |
30-
| UncontrolledFormatString.cs:15:46:15:49 | access to local variable path | semmle.label | access to local variable path |
31-
| UncontrolledFormatString.cs:32:23:32:31 | access to property Text | semmle.label | access to property Text |
28+
| UncontrolledFormatString.cs:10:16:10:19 | access to local variable path : String | semmle.label | access to local variable path : String |
29+
| UncontrolledFormatString.cs:10:23:10:45 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
30+
| UncontrolledFormatString.cs:10:23:10:53 | access to indexer : String | semmle.label | access to indexer : String |
31+
| UncontrolledFormatString.cs:13:23:13:26 | access to local variable path | semmle.label | access to local variable path |
32+
| UncontrolledFormatString.cs:16:46:16:49 | access to local variable path | semmle.label | access to local variable path |
33+
| UncontrolledFormatString.cs:28:31:28:34 | access to local variable path | semmle.label | access to local variable path |
34+
| UncontrolledFormatString.cs:36:23:36:31 | access to property Text | semmle.label | access to property Text |
3235
| UncontrolledFormatStringBad.cs:9:16:9:21 | access to local variable format : String | semmle.label | access to local variable format : String |
3336
| UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
3437
| UncontrolledFormatStringBad.cs:9:25:9:61 | access to indexer : String | semmle.label | access to indexer : String |

‎csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.qlref

+3-1
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,4 @@
11
query: Security Features/CWE-134/UncontrolledFormatString.ql
2-
postprocess: utils/test/PrettyPrintModels.ql
2+
postprocess:
3+
- utils/test/PrettyPrintModels.ql
4+
- utils/test/InlineExpectationsTestQuery.ql

‎csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs

+2-2
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,9 @@ public class HttpHandler : IHttpHandler
66

77
public void ProcessRequest(HttpContext ctx)
88
{
9-
string format = ctx.Request.QueryString["nameformat"];
9+
string format = ctx.Request.QueryString["nameformat"]; // $ Source
1010

1111
// BAD: Uncontrolled format string.
12-
FormattedName = string.Format(format, Surname, Forenames);
12+
FormattedName = string.Format(format, Surname, Forenames); // $ Alert
1313
}
1414
}

0 commit comments

Comments
 (0)