Skip to content

Commit 79688ef

Browse files
michaelnebelmichaelnebel
authored
Merge pull request #19194 from michaelnebel/csharp/enumsimpletype
C#: Extend simple type sanitizers with enums and `System.DateTimeOffset`.
2 parents befc2fd + 22c9436 commit 79688ef

File tree

7 files changed

+79
-12
lines changed

7 files changed

+79
-12
lines changed

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

+5
Original file line numberDiff line numberDiff line change
@@ -756,6 +756,11 @@ class SystemDateTimeStruct extends SystemStruct {
756756
SystemDateTimeStruct() { this.hasName("DateTime") }
757757
}
758758

759+
/** The `System.DateTimeOffset` struct. */
760+
class SystemDateTimeOffsetStruct extends SystemStruct {
761+
SystemDateTimeOffsetStruct() { this.hasName("DateTimeOffset") }
762+
}
763+
759764
/** The `System.Span<T>` struct. */
760765
class SystemSpanStruct extends SystemUnboundGenericStruct {
761766
SystemSpanStruct() {

‎csharp/ql/lib/semmle/code/csharp/security/Sanitizers.qll

+3-1
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,9 @@ class SimpleTypeSanitizedExpr extends DataFlow::ExprNode {
5757
SimpleTypeSanitizedExpr() {
5858
exists(Type t | t = this.getType() or t = this.getType().(NullableType).getUnderlyingType() |
5959
t instanceof SimpleType or
60-
t instanceof SystemDateTimeStruct
60+
t instanceof SystemDateTimeStruct or
61+
t instanceof SystemDateTimeOffsetStruct or
62+
t instanceof Enum
6163
)
6264
}
6365
}

‎csharp/ql/src/change-notes/2025-04-02-simple-type-enum.md

+4
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Enums and `System.DateTimeOffset` are now treated as *simple* types, which means that they are considered to have a sanitizing effect. This impacts many queries, among others the `cs/log-forging` query.

‎csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.cs

+4-4
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ public class LogForgingHandler : IHttpHandler
1515

1616
public void ProcessRequest(HttpContext ctx)
1717
{
18-
String username = ctx.Request.QueryString["username"];
18+
String username = ctx.Request.QueryString["username"]; // $ Source
1919
ILogger logger = new ILogger();
2020
// BAD: Logged as-is
21-
logger.Warn(username + " logged in");
21+
logger.Warn(username + " logged in"); // $ Alert
2222
// GOOD: New-lines removed
2323
logger.Warn(username.Replace(Environment.NewLine, "") + " logged in");
2424
// GOOD: New-lines removed
@@ -28,11 +28,11 @@ public void ProcessRequest(HttpContext ctx)
2828
// GOOD: Html encoded
2929
logger.Warn(WebUtility.HtmlEncode(username) + " logged in");
3030
// BAD: Logged as-is to TraceSource
31-
new TraceSource("Test").TraceInformation(username + " logged in");
31+
new TraceSource("Test").TraceInformation(username + " logged in"); // $ Alert
3232

3333
Microsoft.Extensions.Logging.ILogger logger2 = null;
3434
// BAD: Logged as-is
35-
logger2.LogError(username);
35+
logger2.LogError(username); // $ Alert
3636
}
3737

3838
public bool IsReusable

‎csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.expected

+4-4
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,15 @@
22
| LogForging.cs:21:21:21:43 | ... + ... | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:21:21:21:43 | ... + ... | This log entry depends on a $@. | LogForging.cs:18:27:18:49 | access to property QueryString | user-provided value |
33
| LogForging.cs:31:50:31:72 | ... + ... | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:31:50:31:72 | ... + ... | This log entry depends on a $@. | LogForging.cs:18:27:18:49 | access to property QueryString | user-provided value |
44
| LogForging.cs:35:26:35:33 | access to local variable username | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:35:26:35:33 | access to local variable username | This log entry depends on a $@. | LogForging.cs:18:27:18:49 | access to property QueryString | user-provided value |
5-
| LogForgingAsp.cs:12:21:12:43 | ... + ... | LogForgingAsp.cs:8:32:8:39 | username : String | LogForgingAsp.cs:12:21:12:43 | ... + ... | This log entry depends on a $@. | LogForgingAsp.cs:8:32:8:39 | username | user-provided value |
5+
| LogForgingAsp.cs:17:21:17:43 | ... + ... | LogForgingAsp.cs:13:32:13:39 | username : String | LogForgingAsp.cs:17:21:17:43 | ... + ... | This log entry depends on a $@. | LogForgingAsp.cs:13:32:13:39 | username | user-provided value |
66
edges
77
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:21:21:21:43 | ... + ... | provenance | |
88
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:31:50:31:72 | ... + ... | provenance | |
99
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:35:26:35:33 | access to local variable username | provenance | |
1010
| LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:18:16:18:23 | access to local variable username : String | provenance | |
1111
| LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:18:27:18:61 | access to indexer : String | provenance | MaD:1 |
1212
| LogForging.cs:18:27:18:61 | access to indexer : String | LogForging.cs:18:16:18:23 | access to local variable username : String | provenance | |
13-
| LogForgingAsp.cs:8:32:8:39 | username : String | LogForgingAsp.cs:12:21:12:43 | ... + ... | provenance | |
13+
| LogForgingAsp.cs:13:32:13:39 | username : String | LogForgingAsp.cs:17:21:17:43 | ... + ... | provenance | |
1414
models
1515
| 1 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated |
1616
nodes
@@ -20,6 +20,6 @@ nodes
2020
| LogForging.cs:21:21:21:43 | ... + ... | semmle.label | ... + ... |
2121
| LogForging.cs:31:50:31:72 | ... + ... | semmle.label | ... + ... |
2222
| LogForging.cs:35:26:35:33 | access to local variable username | semmle.label | access to local variable username |
23-
| LogForgingAsp.cs:8:32:8:39 | username : String | semmle.label | username : String |
24-
| LogForgingAsp.cs:12:21:12:43 | ... + ... | semmle.label | ... + ... |
23+
| LogForgingAsp.cs:13:32:13:39 | username : String | semmle.label | username : String |
24+
| LogForgingAsp.cs:17:21:17:43 | ... + ... | semmle.label | ... + ... |
2525
subpaths

‎csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.qlref

+3-1
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,4 @@
11
query: Security Features/CWE-117/LogForging.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-117/LogForgingAsp.cs

+56-2
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,18 @@
33
using Microsoft.AspNetCore.Http.Headers;
44
using Microsoft.AspNetCore.Mvc;
55

6+
public enum TestEnum
7+
{
8+
TestEnumValue
9+
}
10+
611
public class AspController : ControllerBase
712
{
8-
public void Action1(string username)
13+
public void Action1(string username) // $ Source
914
{
1015
var logger = new ILogger();
1116
// BAD: Logged as-is
12-
logger.Warn(username + " logged in");
17+
logger.Warn(username + " logged in"); // $ Alert
1318
}
1419

1520
public void Action1(DateTime date)
@@ -38,4 +43,53 @@ public void Action2(bool? b)
3843
logger.Warn($"Warning about the bool: {b}");
3944
}
4045
}
46+
47+
public void ActionInt(int i)
48+
{
49+
var logger = new ILogger();
50+
// GOOD: int is a sanitizer.
51+
logger.Warn($"Warning about the int: {i}");
52+
}
53+
54+
public void ActionLong(long l)
55+
{
56+
var logger = new ILogger();
57+
// GOOD: long is a sanitizer.
58+
logger.Warn($"Warning about the long: {l}");
59+
}
60+
61+
public void ActionFloat(float f)
62+
{
63+
var logger = new ILogger();
64+
// GOOD: float is a sanitizer.
65+
logger.Warn($"Warning about the float: {f}");
66+
}
67+
68+
public void ActionDouble(double d)
69+
{
70+
var logger = new ILogger();
71+
// GOOD: double is a sanitizer.
72+
logger.Warn($"Warning about the double: {d}");
73+
}
74+
75+
public void ActionDecimal(decimal d)
76+
{
77+
var logger = new ILogger();
78+
// GOOD: decimal is a sanitizer.
79+
logger.Warn($"Warning about the decimal: {d}");
80+
}
81+
82+
public void ActionEnum(TestEnum e)
83+
{
84+
var logger = new ILogger();
85+
// GOOD: Enum is a sanitizer.
86+
logger.Warn($"Warning about the enum: {e}");
87+
}
88+
89+
public void ActionDateTime(DateTimeOffset dt)
90+
{
91+
var logger = new ILogger();
92+
// GOOD: DateTimeOffset is a sanitizer.
93+
logger.Warn($"Warning about the DateTimeOffset: {dt}");
94+
}
4195
}

0 commit comments

Comments
 (0)