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)) |
Contributor
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 |
Comment on lines
+559
to
+563
| isOrImplementsLoggingService(call.getAnArgument() | ||
| .stripImplicit() | ||
| .(TypeofExpr) | ||
| .getTypeAccess() | ||
| .getTarget()) |
|
|
||
| private predicate isLoggingOptionsConfiguration(MethodCall call) { | ||
| call.fromSource() and | ||
| call.getTarget().getUnboundDeclaration().getName().matches(["Configure%", "PostConfigure%"]) and |
Comment on lines
+700
to
+704
| exists(ObjectCreation creation | | ||
| element = creation and | ||
| isUnresolvedLoggerFactoryConstruction(creation) and | ||
| reason = "unresolved logger factory construction" | ||
| ) |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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;