Repository navigation
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| isRecognizedLoggerFactoryCreate(call) or isRecognizedConfigureLogging(call) | ||
| ) | ||
| or | ||
| exists(MethodCall call | |
| or | ||
| exists(MethodCall call | | ||
| isRecognizedSerilogServiceRegistration(call) and | ||
| not exists(Expr hostCreation | isHostCreation(hostCreation)) |
| /** Holds if the database is eligible for JSON logging suppression under this model. */ | ||
| predicate isJsonLoggingSuppressionEligible() { | ||
| hasRecognizedJsonLoggingEvidence() and | ||
| not exists(Element element, string reason | jsonLoggingConfigurationVeto(element, reason)) |
| /** Holds if the database is eligible for JSON logging suppression under this model. */ | ||
| predicate isJsonLoggingSuppressionEligible() { | ||
| hasRecognizedJsonLoggingEvidence() and | ||
| not exists(Element element, string reason | jsonLoggingConfigurationVeto(element, reason)) |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Gaps in configuration recognition and veto checks can suppress genuine log-forging alerts database-wide.
Review effort: Balanced
Findings: 3
Open (6)
What changed in this PR
This PR reduces false positives in cs/log-forging by recognizing supported JSON-only .NET and Serilog configurations.
Changes:
- Adds database-wide configuration checks and conservative vetoes.
- Excludes eligible framework logging arguments from log-forging sinks.
- Adds regression fixtures, Serilog stubs, and a change note.
| File | Description |
|---|---|
| csharp/ql/test/resources/stubs/Serilog/Serilog.csproj | Defines the Serilog stub project. |
| csharp/ql/test/resources/stubs/Serilog/Serilog.cs | Supplies logging and formatter APIs. |
| csharp/ql/test/resources/stubs/Serilog/CompanyExtensions.cs | Supplies external-helper stubs. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingVetoClassification/Veto.ql | Queries veto classifications. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingVetoClassification/Veto.expected | Records expected veto classifications. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingVetoClassification/Test.cs | Exercises configuration vetoes. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingVetoClassification/options | Configures fixture extraction. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingUnsafeBootstrap/Test.cs | Covers unsafe bootstrap logging. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingUnsafeBootstrap/options | Configures fixture extraction. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingUnsafeBootstrap/LogForging.qlref | Runs log-forging checks. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingUnsafeBootstrap/LogForging.expected | Records retained alerts. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingUnconfiguredHost/Test.cs | Covers an unconfigured host. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingUnconfiguredHost/options | Configures fixture extraction. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingUnconfiguredHost/LogForging.qlref | Runs log-forging checks. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingUnconfiguredHost/LogForging.expected | Records retained alerts. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingStandaloneLogger/Test.cs | Separates static and injected logger configuration. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingStandaloneLogger/options | Configures fixture extraction. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingStandaloneLogger/LogForging.qlref | Runs log-forging checks. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingStandaloneLogger/LogForging.expected | Records retained alerts. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogUnsafe/Test.cs | Covers external Serilog configuration. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogUnsafe/options | Configures fixture extraction. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogUnsafe/LogForging.qlref | Runs log-forging checks. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogUnsafe/LogForging.expected | Records retained alerts. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogSafe/Test.cs | Covers supported JSON Serilog setups. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogSafe/options | Configures fixture extraction. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogSafe/LogForging.qlref | Runs log-forging checks. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogSafe/LogForging.expected | Records alerts outside supported loggers. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogForwarding/Test.cs | Covers provider forwarding. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogForwarding/options | Configures fixture extraction. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogForwarding/LogForging.qlref | Runs log-forging checks. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingSerilogForwarding/LogForging.expected | Records retained alerts. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingHostSafe/Test.cs | Covers JSON-only host logging. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingHostSafe/options | Configures fixture extraction. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingHostSafe/LogForging.qlref | Runs log-forging checks. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingHostSafe/LogForging.expected | Records suppression expectations. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingBuiltInUnsafe/Test.cs | Covers mixed built-in providers. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingBuiltInUnsafe/options | Configures fixture extraction. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingBuiltInUnsafe/LogForging.qlref | Runs log-forging checks. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingBuiltInUnsafe/LogForging.expected | Records retained alerts. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingBuiltInSafe/Test.cs | Covers JSON-only logger factories. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingBuiltInSafe/options | Configures fixture extraction. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingBuiltInSafe/LogForging.qlref | Runs log-forging checks. |
| csharp/ql/test/query-tests/Security Features/CWE-117-JsonLoggingBuiltInSafe/LogForging.expected | Records suppression expectations. |
| csharp/ql/src/change-notes/2026-10-01-json-logging-log-forging.md | Documents the analysis change. |
| csharp/ql/lib/semmle/code/csharp/security/dataflow/LogForgingQuery.qll | Applies JSON-based sink suppression. |
| csharp/ql/lib/semmle/code/csharp/security/dataflow/JsonLoggingConfiguration.qll | Implements configuration recognition and vetoes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| isSerilogTerminalCall(call) and | ||
| exists(MethodCall output | | ||
| isRecognizedJsonEmittingCall(output) and | ||
| DataFlow::localExprFlow(output, getCallReceiver(call)) |
| callMentionsBuiltInLoggingSetup(call) or | ||
| isAddLoggingCall(call) or | ||
| isConfigureLoggingCall(call) or | ||
| isLoggerFactoryCreateCall(call) |
| build.getTarget().getName() = "Build" and | ||
| // Safe setup needs an unambiguous receiver, but a possibly invalidating build must use | ||
| // may-flow so that builds through merged aliases cannot be overlooked. | ||
| DataFlow::localExprFlow(hostCreation, getCallReceiver(build)) and |
| isOrImplementsLoggingService(call.getAnArgument() | ||
| .stripImplicit() | ||
| .(TypeofExpr) | ||
| .getTypeAccess() | ||
| .getTarget()) |
|
|
||
| private predicate isLoggingOptionsConfiguration(MethodCall call) { | ||
| call.fromSource() and | ||
| call.getTarget().getUnboundDeclaration().getName().matches(["Configure%", "PostConfigure%"]) and |
| exists(ObjectCreation creation | | ||
| element = creation and | ||
| isUnresolvedLoggerFactoryConstruction(creation) and | ||
| reason = "unresolved logger factory construction" | ||
| ) |
michaelnebel
left a comment
michaelnebel
left a comment
There was a problem hiding this comment.
Thank you very much for providing suggestions to improve our queries.
I have a design question: Is it possible to avoid having a global database wide check for "disabling" results? Would it be enough to just remove results, where we can explicitly establish that the logger being used is "safe" instead of checking whether we belive "all" loggers are safe?
| { | ||
| public void Action(HttpContext context) | ||
| { | ||
| using var factory = LoggerFactory.Create(logging => logging.AddJsonConsole()); |
There was a problem hiding this comment.
In some of the tests there is a description of what is being tested; I would prefer that there is a pretty elborate description in all of them - as the test-cases are now somewhat more complicated.
| @@ -0,0 +1,10 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
No need to add a project file for hand-written stub implementations.
I'm sure it's possible and that would be ideal. I was concerned it would introduce a lot of complexity because we'd need codeql to understand a few more things like the DI container setup, and I wasn't sure if there was any other query that did something similar that I could reuse. If you know of anything else that could be used as a starting point that would be helpful, but otherwise I can do a bit more research and see what it would look like to make it work like that - give me a few days. |
| @@ -0,0 +1,167 @@ | |||
| using System; | |||
There was a problem hiding this comment.
Hand-written stubs are located in the root of the stubs folder.
Did you by any chance try to generate the stubs for the Serilog? 😄
There is a script scripts/stubs/make_stubs_nuget.py that might help.
| @@ -0,0 +1,6 @@ | |||
| import csharp | |||
There was a problem hiding this comment.
Generally, we would like to have a subfolder in CWE-117 for each test, eg. CWE-117/JsonLoggingVetoClassification. This requires that the original testcase is put into a subfolder as well.
Yes, that would complicate things. Perhaps, there is some middle ground (and I apologize, if this is already handled correctly in the code - but I am trying to get a bit more understanding before starting any kind of detailed review)? Perhaps,
|
There are many ways a configured logging provider can be consumed in .NET and especially ASP.NET apps. Like you mentioned an ILogger can come from the DI container into nearly any class through a constructor, or through a controller method via If an application is using 1 instance of a safe log sink it doesn't mean every logger in the application is using that specific sink, or that additional unsafe sinks aren't also being used. So if we want reasonable confidence a call to a specific ILogger is "safe", we would need to trace/correlate it all the way back to where where that ILogger was created from and therefore what configured log sinks are attached to it. There's probably edge cases I haven't even thought of. Shortcuts are possible, but my thinking is that most simple heuristics would introduce a fair number of false negatives. I view my current approach/heuristic of "only known safe logging sinks are set up in the code being scanned" being the simplest and most conservative heuristic to start with, since we don't have to trace where an ILogger came from. Perhaps in the future we could incrementally enhance the approach to trace the easier cases such as static logger or logging being handled outside of any DI? Or are there other risks of false negatives with my current approach you have in mind that I've missed? We could consider having the "safe" log sink detection opt-in via an extensible predicate - that would allow enterprises to also mark their own in-house logging libraries as safe if they desired. |
Summary
Log injection / log forging is one of the biggest sources of false positive flaws from CodeQL according to this analysis and my own personal experience.
This change prevents false positives from
cs/log-forgingwhen all present logging configuration uses a safe JSON format, either from .NET or Serilog.The query suppresses alerts for standard
Microsoft.Extensions.Loggingand Serilog logging calls only when a conservative, database-wide configuration check succeeds. It recognizes supported code-configured JSON logging setups and deliberately retains alerts whenever it finds a potentially incompatible, unresolved, or unsupported logging configuration.Over time this could be extended to support additional popular logging frameworks or safe output formats. In particular .NET 11 may soon have a safe console logger.
Risks
This is intentionally a database-wide heuristic: if it incorrectly classifies an application as JSON-only, it can suppress genuine
cs/log-forgingresults throughout that database.The false-negative risk is primarily configuration that is not visible or not recognized by the extractor/model—for example, logging changes in external dependencies, reflection, runtime-loaded configuration, or an external extension method that mutates logging without exposing a logging-related signature. The implementation partially mitigates this by requiring narrow recognized patterns and by vetoing on visible ambiguity or unsupported configuration, at the cost of retaining some false positives.
Suppression criteria
Suppression requires both positive evidence of a supported JSON-only configuration and no vetoing configuration anywhere visible in the database.
Recognized configurations are:
LoggerFactory.Createcallbacks that configureAddJsonConsole().ClearProviders();AddJsonConsole();Build().AddSerilogconfiguration callbacks that unconditionally configure supported JSON-formatted console, audit-console, or file sinks, including supportedWriteTo.Asyncwrappers.JsonFormatterwith an omitted ornullclosingDelimiter, andCompactJsonFormatter/RenderedCompactJsonFormatterwith their default formatter or a built-inJsonValueFormatter.writeToProvidersandpreserveStaticLoggermust be omitted orfalse.Suppression applies only to calls to supported framework logger APIs whose receiver is a library-provided
Microsoft.Extensions.Loggingor Serilog logger type. It does not suppress non-logger sinks such as tracing APIs or source-defined logger implementations.Conservative vetoes
The suppression is disabled if the database contains, among other things:
Build();new LoggerFactory()construction;ILoggerimplementations; orTests
Added coverage for:
HostApplicationBuilder, and service-registration setup;