diff --git a/csharp/ql/lib/change-notes/2026-03-19-fix-log-forging-extension-methods.md b/csharp/ql/lib/change-notes/2026-03-19-fix-log-forging-extension-methods.md new file mode 100644 index 000000000000..7bf22b65221e --- /dev/null +++ b/csharp/ql/lib/change-notes/2026-03-19-fix-log-forging-extension-methods.md @@ -0,0 +1,8 @@ +--- +category: minorAnalysis +--- +* The `cs/log-forging` query no longer treats arguments to user-defined extension methods + on `ILogger` types as sinks. Instead, taint is tracked interprocedurally through extension + method bodies, reducing false positives when extension methods sanitize input internally. + Known framework extension methods (`Microsoft.Extensions.Logging.LoggerExtensions`) are + now modeled as explicit sinks via Models as Data. diff --git a/csharp/ql/lib/ext/Microsoft.Extensions.Logging.Sinks.model.yml b/csharp/ql/lib/ext/Microsoft.Extensions.Logging.Sinks.model.yml new file mode 100644 index 000000000000..c42173c3f7a5 --- /dev/null +++ b/csharp/ql/lib/ext/Microsoft.Extensions.Logging.Sinks.model.yml @@ -0,0 +1,40 @@ +extensions: + - addsTo: + pack: codeql/csharp-all + extensible: sinkModel + data: + # Log overloads + - ["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"] + - ["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"] + - ["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"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "Log", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.LogLevel,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + # LogCritical overloads + - ["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"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogCritical", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogCritical", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogCritical", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"] + # LogDebug overloads + - ["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"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogDebug", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogDebug", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogDebug", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"] + # LogError overloads + - ["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"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogError", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogError", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogError", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"] + # LogInformation overloads + - ["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"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogInformation", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogInformation", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogInformation", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"] + # LogTrace overloads + - ["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"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogTrace", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogTrace", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogTrace", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"] + # LogWarning overloads + - ["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"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogWarning", "(Microsoft.Extensions.Logging.ILogger,Microsoft.Extensions.Logging.EventId,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogWarning", "(Microsoft.Extensions.Logging.ILogger,System.Exception,System.String,System.Object[])", "", "Argument[2..3]", "log-injection", "manual"] + - ["Microsoft.Extensions.Logging", "LoggerExtensions", false, "LogWarning", "(Microsoft.Extensions.Logging.ILogger,System.String,System.Object[])", "", "Argument[1..2]", "log-injection", "manual"] diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/LogForgingQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/LogForgingQuery.qll index 22023ebc4090..b3e0149c2f0c 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/LogForgingQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/LogForgingQuery.qll @@ -10,6 +10,7 @@ private import semmle.code.csharp.frameworks.system.text.RegularExpressions private import semmle.code.csharp.security.Sanitizers private import semmle.code.csharp.security.dataflow.flowsinks.ExternalLocationSink private import semmle.code.csharp.dataflow.internal.ExternalFlow +private import semmle.code.csharp.commons.Loggers /** * A data flow source for untrusted user input used in log entries. @@ -52,9 +53,16 @@ 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 + * which are analyzed interprocedurally. */ -private class LogForgingLogMessageSink extends Sink, LogMessageSink { } +private class LogForgingLogMessageSink extends Sink { + LogForgingLogMessageSink() { + this.getExpr() = any(LoggerType i).getAMethod().getACall().getAnArgument() or + this.getExpr() = + any(MethodCall call | call.getQualifier().getType() instanceof LoggerType).getAnArgument() + } +} /** * An argument to a call to a method on a trace class. diff --git a/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.cs b/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.cs index 18169e4a4b0b..70cce8493ff0 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.cs +++ b/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 LoggerExtensions +{ + public static void WarnSafe(this ILogger logger, string message) + { + logger.Warn(message.Replace(Environment.NewLine, "")); + } + + public static void WarnUnsafe(this ILogger logger, string message) + { + logger.Warn(message); // $ Alert + } +} diff --git a/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.expected b/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.expected index 1820eaa07d96..04397886a0b3 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.expected @@ -2,17 +2,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 | | 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:35:26:35:33 | access to local variable username | provenance | Sink:MaD:1 | +| 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:49 | access to property QueryString : NameValueCollection | LogForging.cs:18:27:18:61 | access to indexer : String | provenance | MaD:2 | | 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 | +| 1 | Sink: Microsoft.Extensions.Logging; LoggerExtensions; false; LogError; (Microsoft.Extensions.Logging.ILogger,System.String,System.Object[]); ; Argument[1..2]; log-injection; manual | +| 2 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated | nodes | LogForging.cs:18:16:18:23 | access to local variable username : String | semmle.label | access to local variable username : String | | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection | @@ -20,6 +25,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