[Java] taint query find weird step when calling super

[hugo-syn]

Hi, I'm trying to follow the value parameter in this put function:

It goes well until the super call

Until here where it goes in a random function calling put on a map:

I have the following isAdditionalFlowStep (for some needs):

  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
    any(GetterTaintStep s).step(node1, node2) or
  }

Which contains the following piece of code:

class GetterTaintStep extends GadgetAdditionalTaintStep {
    override predicate step(DataFlow::Node fromNode, DataFlow::Node toNode) {
        exists(MethodCall ma, Method m | 
        
        ma.getMethod() = m and
        m.getName().matches("get%") and
        m.getNumberOfParameters() = 0 and
        
        fromNode.asExpr() = ma.getQualifier() and
        toNode.asExpr() = ma
        
        )
    }
}

If I remove my isAdditionalFlowStep it don't goes in this function but I don't understand why is it influencing something what am I missing here ? is my isAdditionalFlowStep wrong ? Thanks :)

[aibaars]

The flow seems to somehow jump from one instance of HashMap (the super in the definition of put) to all instances of HashMap. I have no idea how the taintstep for the get-method could cause that behaviour. At a glance your definition of isAdditionalFlowStep looks fine, and I don't understand either why you get the strange flow.

I see there is a trailing or in your definition of isAdditionalFlowStep. Is there anything else in your isAdditionalFlowStep predicate? Also, could you share the definition of the GadgetAdditionalTaintStep class?

In cases like this I'd use the "quick eval" feature of the VScode plugin on the isAdditionalFlowStep and the subexpressions in its definition to inspect their results to see if there are any unexpected steps.

[hugo-syn]

Hi @aibaars thanks for your answer, I'll try the quick eval feature as you said.

The code can be found here:

• https://github.com/synacktiv/QLinspector/blob/d145a5914118c84fc0a605f591f7a860aa8cbf3d/ql/libs/GadgetTaintHelpers.qll#L5
• https://github.com/synacktiv/QLinspector/blob/d145a5914118c84fc0a605f591f7a860aa8cbf3d/ql/queries/QLinspector.ql#L54

I actually removed other additional steps in my example because it didn't change anything. Every one of them leads to the same results but if I remove them all it works as expected.

[aschackmull]

If database.newStringMap can return a CaseInsensitiveMap then this flow path looks completely correct to me (the value parameter is returned as a side-effect stored in the map-value content on the qualifier of the put call). If map cannot be of type CaseInsensitiveMap then sure, you've got some false positive flow, but whether or not we should expect that depends on how hard it is to determine that fact statically.
One thing to potentially check: Get the DataFlow::Node corresponding to database.newStringMap and quick-eval its getTypeBound() - that should tell you the type that the library has inferred and should help explain why you see a potential virtual dispatch edge from the put call to the implementation in CaseInsensitiveMap.

[hugo-syn]

Thanks @aschackmull for your answer, but from my understanding it shouldn't go backwards ? Because here we go from put to fillMap it's not the put implementation in the parent class. In my codebase I can call the source method put but this code path is not possible I would never call the fillMap method from the put. I'm a bit confused

[aschackmull]

What you see is a return from a method call to put. You can say that the returned value is the modified map which now contains the tracked value. There's nothing "backwards" about that.

[hugo-syn]

Alright I understand, do you know a way to prevent this with an isBarrier predicate or something else ?

[aschackmull]

That depends on what you're actually trying to achieve. What you currently have is a source definition that looks somewhat weird to me - starting from the value parameter in a map.put method seems odd. But if what you want is to e.g. start from some set of parameters in general and see where flow goes without returning, such that the path is applicable in any call context, then you have the option of using DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } in your configuration, since that will instruct the library to never follow return-edges from the callable containing the source.

[hugo-syn]

Thank you for your help it's working as I want now 🙏

Just for my curiosity, what are the scenarios where you need the initial behavior where you taint return values of the source ?

[aschackmull]

I think a few examples should make that very obvious: The simplest is just something of the form return somethingTainted; and for side-effects the pattern could e.g. be this.someField = somethingTainted;. On return you definitely want the return value, respectively, the .someField of the qualifier fo the call to be tainted.