Skip to content

C#: Fix false positives in cs/log-forging for extension methods #21498

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

Open
Gregro wants to merge 4 commits into
base: main
Choose a base branch
from
+42 −2
Open

C#: Fix false positives in cs/log-forging for extension methods #21498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a9eb801
C#: Fix false positives in cs/log-forging for extension methods
Gregro Mar 19, 2026
d99247c
Clarify static extension method class name
Gregro Mar 21, 2026
d0c4889
update test helper to use more robust .ReplaceLineEndings() sanitizer
Gregro Mar 21, 2026
a59c865
let interprocedural analysis handle source-available extension method…
Gregro Mar 21, 2026
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
7 changes: 7 additions & 0 deletions csharp/ql/lib/change-notes/2026-03-19-fix-log-forging-extension-methods.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
category: minorAnalysis
---
* The `cs/log-forging` query no longer treats arguments to extension methods with
source code on `ILogger` types as sinks. Instead, taint is tracked interprocedurally
through extension method bodies, reducing false positives when extension methods
sanitize input internally.
12 changes: 10 additions & 2 deletions csharp/ql/lib/semmle/code/csharp/security/dataflow/LogForgingQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,17 @@ private class HtmlSanitizer extends Sanitizer {
}

/**
* An argument to a call to a method on a logger class.
* An argument to a call to a method on a logger class, excluding extension methods
* with source code which are analyzed interprocedurally.
*/
private class LogForgingLogMessageSink extends Sink, LogMessageSink { }
private class LogForgingLogMessageSink extends Sink, LogMessageSink {
LogForgingLogMessageSink() {
not exists(ExtensionMethodCall mc |
this.getExpr() = mc.getAnArgument() and
mc.getTarget().fromSource()
)
}
}

/**
* An argument to a call to a method on a trace class.
Expand Down
18 changes: 18 additions & 0 deletions csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ public void ProcessRequest(HttpContext ctx)
Microsoft.Extensions.Logging.ILogger logger2 = null;
// BAD: Logged as-is
logger2.LogError(username); // $ Alert

// GOOD: uses safe extension method that sanitizes internally
logger.WarnSafe(username + " logged in");
// BAD: uses unsafe extension method that does not sanitize
logger.WarnUnsafe(username + " logged in");
}

public bool IsReusable
Expand All @@ -43,3 +48,16 @@ public bool IsReusable
}
}
}

static class UserLoggerExtensions
{
public static void WarnSafe(this ILogger logger, string message)
{
logger.Warn(message.ReplaceLineEndings(""));
}

public static void WarnUnsafe(this ILogger logger, string message)
{
logger.Warn(message); // $ Alert
}
}
7 changes: 7 additions & 0 deletions csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,18 @@
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
edges
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:21:21:21:43 | ... + ... | provenance | |
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:31:50:31:72 | ... + ... | provenance | |
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:35:26:35:33 | access to local variable username | provenance | |
| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:40:27:40:49 | ... + ... : String | provenance | |
| LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:18:16:18:23 | access to local variable username : String | provenance | |
| LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:18:27:18:61 | access to indexer : String | provenance | MaD:1 |
| LogForging.cs:18:27:18:61 | access to indexer : String | LogForging.cs:18:16:18:23 | access to local variable username : String | provenance | |
| LogForging.cs:40:27:40:49 | ... + ... : String | LogForging.cs:59:63:59:69 | message : String | provenance | |
| LogForging.cs:59:63:59:69 | message : String | LogForging.cs:61:21:61:27 | access to parameter message | provenance | |
| LogForgingAsp.cs:13:32:13:39 | username : String | LogForgingAsp.cs:17:21:17:43 | ... + ... | provenance | |
models
| 1 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated |
Expand All @@ -20,6 +24,9 @@ nodes
| LogForging.cs:21:21:21:43 | ... + ... | semmle.label | ... + ... |
| LogForging.cs:31:50:31:72 | ... + ... | semmle.label | ... + ... |
| LogForging.cs:35:26:35:33 | access to local variable username | semmle.label | access to local variable username |
| LogForging.cs:40:27:40:49 | ... + ... : String | semmle.label | ... + ... : String |
| LogForging.cs:59:63:59:69 | message : String | semmle.label | message : String |
| LogForging.cs:61:21:61:27 | access to parameter message | semmle.label | access to parameter message |
| LogForgingAsp.cs:13:32:13:39 | username : String | semmle.label | username : String |
| LogForgingAsp.cs:17:21:17:43 | ... + ... | semmle.label | ... + ... |
subpaths
Loading