Taint Tracking in Java - Restrict tracking to the code reachable from the source

[dvec01]

Here is the simplified version of the code I'm working on:

import java.lang.Runtime;

public class Main {

    private static String unused = "";

    public static void source(String userInput) {
        this.unused = userInput;
    }

    private void execUnused() {
        Runtime.getRuntime().exec(this.unused);
    }
}

and the query I'm using to track userInput in the code reachable from the source method:

/**
 * @kind path-problem
 */

import java
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.security.CommandLineQuery

class SourceMethod extends Method {
  SourceMethod() { this.getName() = "source" }
}

module MyConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) {
    source.(DataFlow::InstanceParameterNode).getCallable() instanceof SourceMethod or
    source.asParameter().getCallable() instanceof SourceMethod
  }

  predicate isSink(DataFlow::Node sink) {
    exists(Call c |
      c.getCallee().hasQualifiedName("java.lang", "Runtime", "exec") and
      (
        sink.asExpr() = c.getAnArgument() or
        sink.asExpr() = c.getQualifier()
      )
    )
  }
}

module MyFlow = TaintTracking::Global<MyConfig>;

import MyFlow::PathGraph

from MyFlow::PathNode source, MyFlow::PathNode sink
where MyFlow::flowPath(source, sink)
select sink.getNode(), source, sink, "sink: $@", source.getNode(), sink.toStringWithContext()

I expect the query to return an empty result, since the tainted input does not reach the sink from source.
However, this query returns this.unused as flowing into the sink.

Is there a way to prevent this and restrict the results to code reachable from the source method?

[ginsbach]

Thank you for the question!
Do I understand correctly that you want to exclude sinks that are in dead methods? There is a standard query for finding those in the codeql repo: DeadMethod.ql. Perhaps you can could use that code for inspiration to extend your query?

[dvec01]

Excuse the poor choice of method names. Also, I noticed that the code snippet I posted was wrong. To clarify, I am not looking for a way to exclude sinks that are (solely) in dead methods, but in methods that cannot be reached from the source method. To extend the previous code snippet:

import java.lang.Runtime;
import java.lang.Exception;

public class Main {

    private static String s1 = "";

    public static void source(String userInput) throws Exception  {
        s1 = userInput;
        method2();
    }

    private static void method1() throws Exception {
        Runtime.getRuntime().exec(s1);
    }

    private static void method2() throws Exception {
        Runtime.getRuntime().exec(s1);
    }
}

I would expect that the query I posted above lists one path, being source->method2->Runtime.getRuntime().exec, as s1 is set in source to a user-controlled value and method2 is called afterwards. However, it returns also method1, as can be seen from the attached image.

I am not quite sure why this happens. It seems that the field s1 is considered as a possible source to each method, even if that same method cannot be reached from the original source method.

[ginsbach]

Excuse the poor choice of method names. Also, I noticed that the code snippet I posted was wrong. To clarify, I am not looking for a way to exclude sinks that are (solely) in dead methods, but in methods that cannot be reached from the source method. To extend the previous code snippet:

You could use the Java call graph classes to enforce that restriction.

However, note that there might be good reasons for the existing behaviour. Consider the following example:

import java.lang.Runtime;
import java.lang.Exception;

public class Main {

    private static String s1 = "";

    public static void runBoth(String userInput) {
        source(userInput);
        method2();
    }

    public static void source(String userInput)  {
        s1 = userInput;
    }

    private static void method2() {
        Runtime.getRuntime().exec(s1);
    }
}

In this example, method2 cannot be reached from source. Nonetheless, userInput will end up in Runtime.getRuntime().exec(...) when runBoth is called.

[dvec01]

Thank you for the suggestion!

I now updated the isSink predicate to:

predicate isSink(DataFlow::Node sink) {
  exists(Call c |
    c.getCallee().hasQualifiedName("java.lang", "Runtime", "exec") and
    (
      sink.asExpr() = c.getAnArgument() or
      sink.asExpr() = c.getQualifier()
    )
  ) and
  exists(DataFlow::Node source |
    isSource(source) and
    source.getEnclosingCallable().polyCalls(sink.getEnclosingCallable())
  )
}

My assumption that isSource also specifies from where the possible sinks can be reached was wrong. Sad that there doesnt seem to be a way to opt out of this behaviour through a config option (similar to DataFlow::FeatureHasSourceCallContext for example), as there exist use cases which require this restriction, such as looking for gadget chains.