Skip to content

Commit a9eb801

Browse files
GregroGregro
committed
C#: Fix false positives in cs/log-forging for extension methods
1 parent 9a4bc69 commit a9eb801

File tree

5 files changed

+87
-5
lines changed

5 files changed

+87
-5
lines changed

‎csharp/ql/lib/change-notes/2026-03-19-fix-log-forging-extension-methods.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `cs/log-forging` query no longer treats arguments to user-defined extension methods
5+
on `ILogger` types as sinks. Instead, taint is tracked interprocedurally through extension
6+
method bodies, reducing false positives when extension methods sanitize input internally.
7+
Known framework extension methods (`Microsoft.Extensions.Logging.LoggerExtensions`) are
8+
now modeled as explicit sinks via Models as Data.

‎csharp/ql/lib/ext/Microsoft.Extensions.Logging.Sinks.model.yml‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/csharp-all
4+
extensible: sinkModel
5+
data:
6+
# Log overloads
7+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "Log", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.LogLevel,Microsoft.Extensions.Logging.EventId,System.Exception,System.String,System.Object[])", "", "Argument[4..5]", "log-injection", "manual"]
8+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "Log", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.LogLevel,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[3..4]", "log-injection", "manual"]
9+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "Log", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.LogLevel,System.Exception,System.String,System.Object[])", "", "Argument[3..4]", "log-injection", "manual"]
10+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "Log", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.LogLevel,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
11+
# LogCritical overloads
12+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogCritical", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.Exception,System.String,System.Object[])", "", "Argument[3..4]", "log-injection", "manual"]
13+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogCritical", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
14+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogCritical", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
15+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogCritical", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"]
16+
# LogDebug overloads
17+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogDebug", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.Exception,System.String,System.Object[])", "", "Argument[3..4]", "log-injection", "manual"]
18+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogDebug", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
19+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogDebug", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
20+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogDebug", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"]
21+
# LogError overloads
22+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogError", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.Exception,System.String,System.Object[])", "", "Argument[3..4]", "log-injection", "manual"]
23+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogError", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
24+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogError", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
25+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogError", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"]
26+
# LogInformation overloads
27+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogInformation", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.Exception,System.String,System.Object[])", "", "Argument[3..4]", "log-injection", "manual"]
28+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogInformation", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
29+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogInformation", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
30+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogInformation", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"]
31+
# LogTrace overloads
32+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogTrace", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.Exception,System.String,System.Object[])", "", "Argument[3..4]", "log-injection", "manual"]
33+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogTrace", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
34+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogTrace", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
35+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogTrace", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"]
36+
# LogWarning overloads
37+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogWarning", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.Exception,System.String,System.Object[])", "", "Argument[3..4]", "log-injection", "manual"]
38+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogWarning", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
39+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogWarning", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"]
40+
- ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogWarning", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"]

‎csharp/ql/lib/semmle/code/csharp/security/dataflow/LogForgingQuery.qll‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ private import semmle.code.csharp.frameworks.system.text.RegularExpressions
1010
private import semmle.code.csharp.security.Sanitizers
1111
private import semmle.code.csharp.security.dataflow.flowsinks.ExternalLocationSink
1212
private import semmle.code.csharp.dataflow.internal.ExternalFlow
13+
private import semmle.code.csharp.commons.Loggers
1314

1415
/**
1516
* A data flow source for untrusted user input used in log entries.
@@ -52,9 +53,16 @@ private class HtmlSanitizer extends Sanitizer {
5253
}
5354

5455
/**
55-
* An argument to a call to a method on a logger class.
56+
* An argument to a call to a method on a logger class, excluding extension methods
57+
* which are analyzed interprocedurally.
5658
*/
57-
private class LogForgingLogMessageSink extends Sink, LogMessageSink { }
59+
private class LogForgingLogMessageSink extends Sink {
60+
LogForgingLogMessageSink() {
61+
this.getExpr() = any(LoggerType i).getAMethod().getACall().getAnArgument() or
62+
this.getExpr() =
63+
any(MethodCall call | call.getQualifier().getType() instanceof LoggerType).getAnArgument()
64+
}
65+
}
5866

5967
/**
6068
* An argument to a call to a method on a trace class.

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ public void ProcessRequest(HttpContext ctx)
3333
Microsoft.Extensions.Logging.ILogger logger2 = null;
3434
// BAD: Logged as-is
3535
logger2.LogError(username); // $ Alert
36+
37+
// GOOD: uses safe extension method that sanitizes internally
38+
logger.WarnSafe(username + " logged in");
39+
// BAD: uses unsafe extension method that does not sanitize
40+
logger.WarnUnsafe(username + " logged in");
3641
}
3742

3843
public bool IsReusable
@@ -43,3 +48,16 @@ public bool IsReusable
4348
}
4449
}
4550
}
51+
52+
static class LoggerExtensions
53+
{
54+
public static void WarnSafe(this ILogger logger, string message)
55+
{
56+
logger.Warn(message.Replace(Environment.NewLine, ""));
57+
}
58+
59+
public static void WarnUnsafe(this ILogger logger, string message)
60+
{
61+
logger.Warn(message); // $ Alert
62+
}
63+
}

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,24 +2,32 @@
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+
| LogForging.cs:61:21:61:27 | access to parameter message | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:61:21:61:27 | access to parameter message | This log entry depends on a $@. | LogForging.cs:18:27:18:49 | access to property QueryString | user-provided value |
56
| 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 |
67
edges
78
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:21:21:21:43 | ... + ... | provenance | |
89
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:31:50:31:72 | ... + ... | provenance | |
9-
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:35:26:35:33 | access to local variable username | provenance | |
10+
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:35:26:35:33 | access to local variable username | provenance | Sink:MaD:1 |
11+
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:40:27:40:49 | ... + ... : String | provenance | |
1012
| LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:18:16:18:23 | access to local variable username : String | provenance | |
11-
| LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:18:27:18:61 | access to indexer : String | provenance | MaD:1 |
13+
| LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:18:27:18:61 | access to indexer : String | provenance | MaD:2 |
1214
| LogForging.cs:18:27:18:61 | access to indexer : String | LogForging.cs:18:16:18:23 | access to local variable username : String | provenance | |
15+
| LogForging.cs:40:27:40:49 | ... + ... : String | LogForging.cs:59:63:59:69 | message : String | provenance | |
16+
| LogForging.cs:59:63:59:69 | message : String | LogForging.cs:61:21:61:27 | access to parameter message | provenance | |
1317
| LogForgingAsp.cs:13:32:13:39 | username : String | LogForgingAsp.cs:17:21:17:43 | ... + ... | provenance | |
1418
models
15-
| 1 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated |
19+
| 1 | Sink: Microsoft.Extensions.Logging; LoggerExtensions; false; LogError; (Microsoft.Extensions.Logging.ILogger,System.String,System.Object[]); ; Argument[1..2]; log-injection; manual |
20+
| 2 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated |
1621
nodes
1722
| LogForging.cs:18:16:18:23 | access to local variable username : String | semmle.label | access to local variable username : String |
1823
| LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
1924
| LogForging.cs:18:27:18:61 | access to indexer : String | semmle.label | access to indexer : String |
2025
| LogForging.cs:21:21:21:43 | ... + ... | semmle.label | ... + ... |
2126
| LogForging.cs:31:50:31:72 | ... + ... | semmle.label | ... + ... |
2227
| LogForging.cs:35:26:35:33 | access to local variable username | semmle.label | access to local variable username |
28+
| LogForging.cs:40:27:40:49 | ... + ... : String | semmle.label | ... + ... : String |
29+
| LogForging.cs:59:63:59:69 | message : String | semmle.label | message : String |
30+
| LogForging.cs:61:21:61:27 | access to parameter message | semmle.label | access to parameter message |
2331
| LogForgingAsp.cs:13:32:13:39 | username : String | semmle.label | username : String |
2432
| LogForgingAsp.cs:17:21:17:43 | ... + ... | semmle.label | ... + ... |
2533
subpaths

0 commit comments

Comments
 (0)