The CodeQL query raises false positive issues when finding a path from a user-invoked public function A to a function B called inside A

[spidermana]

Hi, everyone. I want to find a path from a public API A with user input to a function B using CodeQL Taint Tracking. That is, I want to find some pattern like this:

//A->call B or A->call others->call B
public A(source){
	B(); // or indirectly call B
}

But CodeQL raises some false positive issues in the following example:

private function hidden(int fakeSource)
{
   test = A(fakeSource);
   B(test);
}
public function A(int source)
{
 	return;
}

As shown in the example, there does not exist a path starting from a user-invoked A to B

However, CodeQL considers a possible path starting from the hidden function in which B is called after A function returns

So the tainted path shown by CodeQL is (un-displayed hidden)->A->return->B, which is not the pattern indicating B is directly or indirectly called inside A. Is there any idea can help?

For example, I am trying to sanitize the taint tracking of a specific return statement at a function where the taint tracking process starts, but failed to do so.

The related ql code is presented as below:

/** 
 * @kind path-problem 
*/ 
import java 
import semmle.code.java.dataflow.TaintTracking 
import semmle.code.java.dataflow.TaintTracking2 
import DataFlow2::PathGraph 
import semmle.code.java.dataflow.DataFlow 
 
class TestTaint extends TaintTracking2::Configuration { 
    TestTaint() { this = "TestTaint" } 
    override predicate isSource(DataFlow2::Node source) { 
        exists(Method method | method.getAParameter() = source.asParameter() and method.isPublic()) 
    } 
   
    override predicate isSink(DataFlow2::Node sink) { 
        exists(MethodAccess call | sink.asExpr() = call.getAnArgument()) 
    } 
     
} 
from TestTaint config1, DataFlow2::PathNode source1, DataFlow2::PathNode sink1 
where config1.hasFlowPath(source1, sink1) 
select source1, source1, sink1, "123"

[smowton]

I can't reproduce this--

Your example appears to be a mixture of Java and JavaScript syntax, but given you import semmle.code.java... in the QL example I have guessed you're intending to work in Java and adapted your example to

public class Test {

  private void hidden(int nonSource)
  {
    int test = a(nonSource);
    b(test);
  }

  public int a(int source) { return 0; }

  private void b(int source) { } 

}

With this input and the CodeQL you posted, I get no results, as expected.

[spidermana]

Thanks for your reply! Actually, the Java code example we were trying to analyze is like this:

public class Test {
    private void hidden(int nonSource)
    {
      int test = a(nonSource);
      b(test);
    }
    public int a(int source) { return source; }
    private void b(int source) { } 
}

At this time, there will be a data flow like this:

which considers a situation where a is returned and then passes the data to b. This is a false positive to us, since we want to find a data flow from only user-invokable public API a to b.
Could you please help us look at this issue? Thank you very much!

[smowton]

How about something like

/**
 * @kind path-problem
*/
import java
import semmle.code.java.dataflow.TaintTracking
import semmle.code.java.dataflow.TaintTracking2
import DataFlow2::PathGraph
import semmle.code.java.dataflow.DataFlow

class TestTaint extends TaintTracking2::Configuration {
    TestTaint() { this = "TestTaint" }
    override predicate isSource(DataFlow2::Node source) {
        exists(Method method | method.getAParameter() = source.asParameter() and method.isPublic())
    }

    override predicate isSanitizerIn(DataFlow2::Node node) {
        this.isSource(node)
    }

    override predicate isSanitizer(DataFlow2::Node node) {
        node.asExpr() = any(MethodAccess call | call.getCallee().getName() = "a")
    }

    override predicate isSink(DataFlow2::Node sink) {
        exists(MethodAccess call | sink.asExpr() = call.getAnArgument())
    }

}
from TestTaint config1, DataFlow2::PathNode source1, DataFlow2::PathNode sink1
where config1.hasFlowPath(source1, sink1)
select source1, source1, sink1, "123"

If I adapt your test case to

public class Test {
    private void hidden(int nonSource)
    {
      int test = a(nonSource);
      b(test);
    }
    public int a(int source) { b(source); return source; }
    private void b(int source) { } 
}

The isSanitizerIn says that taint should not flow into a from a caller (i.e., the call is coming from internal code), and the isSanitizer says that taint should also not flow back out to the callsite (again, this would necessarily occur on a path where the call came from internal code).

[spidermana]

Thanks so much for your help!

Actually, I want to find possible vulnerable APIs in Java libraries, which pass user arguments to a sensitive sink.

Considering this task, there may be two minor problems in the provided solution.

1. In fact, I want to find data flow from all possible user-invokable public API to a sink, so we do not know the specific function name of the source. Thus the isSanitizer predicate using specific function name “a” as filtering condition might not be workable in our circumstances.

       override predicate isSanitizer(DataFlow2::Node node) {
           node.asExpr() = any(MethodAccess call | call.getCallee().getName() = "a")
       }

Specifically, our source is all the public user-invokable function arguments that conform the following conditions:

override predicate isSource(DataFlow2::Node source) {
    exists(Method method | method.getAParameter() = source.asParameter() and method.isPublic())
}

2. The provided isSanitizerIn predicate might raise some false negative issue since it rules out all dataflow into public functions. For example, consider the folllowing code whose hidden function is public instead of private:

   public class Test {
       public void hidden(int nonSource)
       {
         int test = a(nonSource);
         b(test);
       }
       public int a(int source) { return source; }
       private void b(int source) { } 
   }

   Please note that the following path is legitimate now: public hidden call -> a-> b

However, the isSanitizerIn predicate will block Node 2 to Node 3 (i.e., line 11 to line 14).

Thanks again for your help.

[atorralba]

It sounds like what you want is not having flow out of a method containing a source to an arbitrary call site. In that case, you can use a FlowFeature to tell the dataflow configuration that you don't want it to assume arbitrary call contexts when reaching a return statement from a source.

Try adding this to your configuration:

  override DataFlow::FlowFeature getAFeature() {
    result instanceof DataFlow::FeatureHasSourceCallContext
  }

[smowton]

This is much better than my FlowState idea, now deleted. Didn't know this feature existed :)

I will persist with my health warning though: I had assumed that your source and sink definitions were just a toy example. If you really are seeking to enumerate every path from the parameter of any public function to any argument to any function, this will likely be a very large result and prohibitively expensive to compute. If you want this query to be useful on anything but a toy program, I suggest you should restrict your set of interesting sources and sinks further.

[spidermana]

@atorralba Awesome! This feature is just all I want! Thanks so much for the idea!
I would pay more attention to the predicates of DataFlow::Configuration.

@smowton Thanks for the attention. The code I presented is just an example for simplicity. In fact, I have imposed several restrictions on the source and sink for my goal. However, I am still working on reducing the amount of results. This issue is exactly the kind of possible optimization I found when improving outcomes. Thanks again for your tips!