Merge pull request #21498 from Gregro/csharp/fix-log-forging-extension-methods

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

Author: hvitved

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

@@ -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.

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

@@ -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.

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

@@ -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
@@ -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
+    }
+}

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

@@ -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 |
@@ -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