partialFlow exists, but the corresponding partialFlowRev doesn't for a Codeql Query

[yuval-piiano]

I am working on this project:
https://github.com/trydent-io/jooby-samples , and interested in a flow in this file:
https://github.com/trydent-io/jooby-samples/blob/master/jooby-sample-rest-api-jackson/src/main/java/org/jooby/sample/RestApiJacksonMain.java

public final class RestApiJacksonMain extends Jooby {
  {
    use(new Jackson());

    use(RestController.class);

    use("/script/rest")
      .get(request -> Person.LUKE)
      .post(request -> new Person(
        request.param("firstName").value(),
        request.param("lastName").value(),
        request.param("birthDate").toOptional(LocalDate.class).orElse(LocalDate.now())
      ))
      .consumes("application/x-www-form-urlencoded")
      .produces("application/json");
  }

  public static void main(String... args) {
    run(RestApiJacksonMain::new, args);
  }
}

where a firstName / lastName rest parameter is returned from method (inside the Person Class) - so Created this Configuration:

/**
 * @kind path-problem
 * @id pii-to-rest-return
 * @problem.severity warning
 */

import java
import DataFlow::PathGraph
import semmle.code.java.security.LogInjection
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.StringFormat

// a function that returns the string representation of an input parameter (param, function, file)
// a function that returns the string representation of a source Node
// configuration for data flow query
module ReturnedName implements DataFlow::ConfigSig {
  predicate isARelevantCall(MethodAccess call) {
    call.getMethod().hasName("param") and
    (
      call.getArgument(0).(StringLiteral).getValue() = "firstName" or
      call.getArgument(0).(StringLiteral).getValue() = "lastName"
    )
    or
    exists(MethodAccess subAccess |
      call.getQualifier() = subAccess and
      isARelevantCall(subAccess)
    )
  }

  predicate isSource(DataFlow::Node source) {
    exists(MethodAccess call |
      //call.
      call = source.asExpr() and
      isARelevantCall(call)
    )
  }

  //a sink is a return statement of a rest api function
  //override
  predicate isSink(DataFlow::Node sink) {
    exists(ReturnStmt stmt | stmt.getResult() = sink.asExpr())
  }
}

the isSource and isSink return the expected values, but I can't get the flow , so tried partial data flow:

module RestReturnDataFlow = TaintTracking::Global<ReturnedName>;

int myExplorationLimit() { result = 10 }

module MyPartialDataFlow = RestReturnDataFlow::FlowExploration<myExplorationLimit/0>;

from MyPartialDataFlow::PartialPathNode source, 
MyPartialDataFlow::PartialPathNode sink, int dist
where
  ReturnedName::isSink(sink.getNode()) and 
  ReturnedName::isSource(source.getNode()) and
  MyPartialDataFlow::partialFlow( source, sink, dist)
select source.getNode(), sink.getNode(), "dist is " + dist + " interprocedural flow steps away from source"

And got the expected results...
When trying
RestReturnDataFlow::flowPath(source,sink) returns Nothing or
MyPartialDataFlow::partialFlowRev( source, sink, dist)
instead of the partialFlow statement
Nothing is returned ,

How can I get the flow (+ the path) ?
I tried importing the module PartialPathGraph like it was suggested in this documenation https://codeql.github.com/docs/writing-codeql-queries/debugging-data-flow-queries-using-partial-flow/ to understand better what happens, but the import doesn't work ,

[smowton]

The issue is access paths. These can be seen if you print source and sink, not the .getNode()s of each, and are shown in square brackets when non-empty.

In brief, the partial flow goes .value() -> parameter of Person's constructor -> value assigned to Person's field -> result of new Person(...) call with access path of [.firstName]

That means we arrive at the sink (the return value return new Person(...)) with a non-empty access path which represents the fact that the Person itself is not tainted, but it has a field .firstName that holds tainted data. For a complete path, we'd have to read it back out with getFirstName() or a plain somePerson.firstName field read.

By contrast the reverse partial flow insists that the access path is empty at the sink (the return new Person... site), takes a backwards step into Person's constructor, but can't traverse the store step this.someField = someValue because store steps extend the access path, so they can't result in an empty access path.

Sometimes though, we don't want isSink (or maybe other points in a path) to distinguish the idea of a directly tainted piece of data from tainted data stored inside something (in a field, in an array, in a map...) -- we want to consider stored taint the same as direct taint. For this purpose we have allowImplicitRead.

If we add the following to the ReturnedName module:

   predicate allowImplicitRead(DataFlow::Node n, DataFlow::ContentSet c) {
    isSink(n) and c instanceof DataFlow::FieldContent
   }

then data-flow will allow us, upon reaching a sink, to implicitly read out any remaining access path. In fact if you look at the library module AddTaintDefaults, you can see by default such implicit read steps are allowed quite broadly by taint tracking -- but by default only for arrays and containers, not for fields, hence our needing to add our own extra implicit reads.

With this added, we're able to make the final step new Person(...) [ .firstName ] -> new Person(...) [] and the path is complete -- the query now returns results as expected (using flowPath not partialFlow).