Java: Potentially duplicated results and paths from a custom data flow query

[KiruaLawliet]

project：

log4j 2.14.0 -> https://github.com/apache/logging-log4j2/tree/rel/2.14.0

environment：

vscode version：1.63.2
vscode plugin：CodeQL v1.5.9
CodeQL command-line toolchain release 2.7.0
os platform：windows10 20H2

ql script：

/** @kind path-problem */
import java
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.dataflow.TaintTracking

import DataFlow::PathGraph

module TaintPropagation{

    // strategy.log(this, getName(), fqcn, marker, level, tainted, t);
    class ReliabilityStrategySubClass extends Parameter
    {
        ReliabilityStrategySubClass()
        {
            (
            this.getCallable().getDeclaringType().getASourceSupertype*().hasQualifiedName("org.apache.logging.log4j.core.config", "LocationAwareReliabilityStrategy") 
            ) 
            and this.getCallable().hasName("log")
            and this.hasName("data")
        }
    }

    predicate strategylog(DataFlow::Node node1, DataFlow::Node node2) {
        exists(MethodAccess ma |
            ma.getMethod().hasName("log")
            and (
                ma.getCallee().getDeclaringType().hasQualifiedName("org.apache.logging.log4j.core.config", "LocationAwareReliabilityStrategy")
            )
            and node2.asParameter() instanceof ReliabilityStrategySubClass
            and ma.getMethod().getSignature() = node2.asParameter().getCallable().getSignature()
            and ma.getArgument(node2.asParameter().getPosition()) = node1.asExpr()
            )
    }

    // loggerConfig.log(loggerName, fqcn, marker, level, data, t);
    class LoggerConfigSubClass extends Parameter
    {
        LoggerConfigSubClass()
        {
            this.getCallable().getDeclaringType().getASourceSupertype*().hasQualifiedName("org.apache.logging.log4j.core.config", "LoggerConfig")
            and this.getCallable().hasName("log")
            and this.hasName("data")
        }
    }

    predicate loggerConfiglog(DataFlow::Node node1, DataFlow::Node node2) {
        exists(MethodAccess ma |
            ma.getMethod().hasName("log")
            and ma.getCallee().getDeclaringType().hasQualifiedName("org.apache.logging.log4j.core.config", "LoggerConfig")
            and node2.asParameter() instanceof LoggerConfigSubClass
            and ma.getMethod().getSignature() = node2.asParameter().getCallable().getSignature()
            and ma.getArgument(node2.asParameter().getPosition()) = node1.asExpr()
            )
    }

}

class Test extends TaintTracking::Configuration
{
    Test(){ this = "Test"}
    override predicate isSource(DataFlow::Node source) {
        source.asParameter().getCallable().getDeclaringType().hasQualifiedName("org.apache.logging.log4j.core", "Logger")
        and source.asParameter().getCallable().hasName("log")
        and source.asParameter().hasName("message")
        and source.asParameter().getPosition() = 4
    }
    
    override predicate isSink(DataFlow::Node sink) {
        sink.asParameter().hasName("data")
        and sink.asParameter().getCallable().hasQualifiedName("org.apache.logging.log4j.core.config", "LoggerConfig", "log")
    }

    override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
        TaintPropagation::strategylog(node1, node2)
        or TaintPropagation::loggerConfiglog(node1, node2)
    }

}

from Test config, DataFlow::PathNode source, DataFlow::PathNode sink
where config.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "jndi injection vulnerability due to $@.",
source.getNode(), "user-provided value"

query result：

Question：

There're four classes（AwaitCompletionReliabilityStrategy、AwaitUnconditionallyReliabilityStrategy、DefaultReliabilityStrategy、LockingReliabilityStrategy）implements LocationAwareReliabilityStrategy interface and override this method：
void log(Supplier<LoggerConfig> reconfigured, String loggerName, String fqcn, StackTraceElement location, Marker marker, Level level, Message data, Throwable t);
All of this method have a call statement like： LoggerConfig.log(...)， and signature of log method is here below：
public void log(final String loggerName, final String fqcn, final Marker marker, final Level level, final Message data, final Throwable t)
The Message data formal parameter is the sink as you can see from ql script I paste above。

my question is：
why just only two classes（AwaitCompletionReliabilityStrategy、AwaitUnconditionallyReliabilityStrateg）are shown in taint flow result and their path node taint tracking result even have been presented twice ？Is it a BUG ?

[adityasharad]

Thanks for your question and the detailed summary.
It's possible those paths are not exactly identical, but they are flagging different source code elements that happen to be given the same source code location in the CodeQL libraries (for example due to different parameterisations of the same generic type).
I've passed this to the CodeQL Java analysis team to provide their suggestions in the new year.

[atorralba]

I built a database for Log4j2 2.14.0, ran the provided query and could replicate the results.

After further inspection, I noticed that the two taint steps being added (TaintPropagation::strategylog and TaintPropagation::loggerConfiglog) aren't necessary, since they model flow that is already handled by the virtual dispatch module of the data flow library.

After removing the taint steps, I ran the query again and the duplicated results disappeared:

@KiruaLawliet, try removing those taint steps, since they appear to be duplicating already existent logic of the default libraries.

Also, regarding only 4 paths being shown, check the configuration of your CodeQL extension in VSCode. The Max Paths option is 4 by default:

[KiruaLawliet]

@atorralba Thasks for your reply. Yeah，After some experiments I also noticed this phenomenon is calused by custom taint steps. Is that mean native flow model will be affected by custom taint steps even when they share some kind of similarities in taint working logic？emmm...，I am worry that I can't make sure all custom taint steps is not handled by the virtual dispatch module of the data flow library...

[KiruaLawliet]

oops，I forgot to try your advice. Query result is correct now after getting vscode extension configuration changed except duplicated part

[atorralba]

I am worry that I can't make sure all custom taint steps is not handled by the virtual dispatch module of the data flow library...

The usual approach is running the data flow/taint tracking configuration with only sources, sinks, and optionally sanitizers, defined. If you spot the flow being interrupted somewhere, you can debug it (normally using partial flow) and, once you determine where it happens, you can add the appropriate additional steps to complete the flow.

Is that mean native flow model will be affected by custom taint steps even when they share some kind of similarities in taint working logic？

The additional taint steps you were creating should actually be value-preserving steps — that is, the object in node1 is exactly the same (same value) as the one in node2. The library is adding value-preserving steps through virtual dispatch for those nodes, but you were additionally adding taint steps, which are meant to indicate that one object taints a different one. So, my guess would be that the duplication happens because there were two edges between node1 and node2: one for taint flow and one for value flow.

By following the approach I described above, the issue can be avoided, because only the strictly necessary additional steps are added.