[Java] How to taint data (during deserialization) when nothing is passed as parameter but accessed via a field ?

[hugo-syn]

The title might not be clear but here is an example of what I want to do:

package org.example;

import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.Serializable;

public class SerializableObject implements Serializable {
    // Dangerous sink
    private  String commandSink;

    // call during deserialization
    private void readObject(ObjectInputStream stream) throws IOException, ClassNotFoundException {
        stream.defaultReadObject();

        // starting point
        func();
    }

    public void func(){
        func2();
    }

    public void func2(){
        func3();
    }

    public void func3(){
        try {
            // dangerous call
            Runtime.getRuntime().exec(commandSink);
        } catch (IOException e) {
            throw new RuntimeException(e);
        }
    }
}

I have a vulnerable object with a commandSink field. The readObject method will be called during deserialization and will call func() -> func2() -> func3() -> ...exec(commandSink).

How to detect this kind of vulnerability ? I would like to follow the call graph and detect when a field is accessed in a dangerous function. I've tried this, but it seems that the first call to func() is not considered as a DataFLow::node :

/**
 * @kind path-problem
 */

import java
import semmle.code.java.dataflow.TaintTracking
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.dataflow.ExternalFlow
import MyFlow::PathGraph
 
private module MyConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
      source.getEnclosingCallable().hasName("readObject") and
      (
        source.asExpr() instanceof Call
      )
   }
 
   predicate isSink(DataFlow::Node sink) {
     exists(MethodCall mc |  mc.getMethod().hasQualifiedName("java.lang", "Runtime", "exec") and
        mc.getEnclosingCallable() = sink.getEnclosingCallable()
     )
   }
}
 
module MyFlow = TaintTracking::Global<MyConfig>;

from MyFlow::PathNode source, MyFlow::PathNode sink
where MyFlow::flowPath(source, sink)
select sink.getNode(), source, sink, "Sample TaintTracking query"

I think I should use the is isAdditionalFlowStep predicate, but I'm not getting anything with it. So is there a way to do this ?

Thanks :)

[atorralba]

First, there's a couple of misconceptions in your source and sink definitions.

Your isSource predicate is currently saying that the return value of any call inside a callable named readObject is a source. I don't think that's what you want — see below a couple of suggestions to improve this.

Also, your isSink predicate is saying that your sink is any node inside a callable that calls to Runtime.exec. But you probably want to say that your sink is the argument of exec:

predicate isSink(DataFlow::Node sink) {
  exists(MethodCall mc |
    mc.getMethod().hasQualifiedName("java.lang", "Runtime", "exec") and
    sink.asExpr() = mc.getArgument(0)
  )
}

Now, I want to mention that the easiest way of having a result using your query and the modified sink would be tainting the commandSink field itself:

predicate isSource(DataFlow::Node source) {
  source.(DataFlow::FieldValueNode).getField().hasName("commandSink")
}

But in that case you wouldn't see the full flow from func to func3. So, to address your original question:

You want to taint the SerializableObject instance when readObject is called and track it across methods until it reaches the sink. To do that, you can use the instance parameter (this) that gets passed implicitly between method calls (from Java's point of view, you're doing something like: func(this) -> func2(this) -> func3(this) -> exec(this.commandSink)).

You implement that in CodeQL using the InstanceParameterNode of readObject as your source:

predicate isSource(DataFlow::Node source) {
  source.(DataFlow::InstanceParameterNode).getCallable().hasName("readObject")
}

And then you'll need another trick to tell CodeQL that, if an instance of SerializableObject is tainted, all its fields are tainted too:

private class FieldInheritTaint extends DataFlow::FieldContent, TaintInheritingContent {
  FieldInheritTaint() { this.getField().getDeclaringType().hasName("SerializableObject") }
}

(You could restrict this to affect the commandSink field only if you wanted)

With all that in place, you should see the path that you want.

Full query for reference:

/**
 * @kind path-problem
 */

import java
import semmle.code.java.dataflow.TaintTracking
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.dataflow.ExternalFlow
import MyFlow::PathGraph

private module MyConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) {
    source.(DataFlow::InstanceParameterNode).getCallable().hasName("readObject")
    //source.(DataFlow::FieldValueNode).getField().hasName("commandSink")
  }

  predicate isSink(DataFlow::Node sink) {
    exists(MethodCall mc |
      mc.getMethod().hasQualifiedName("java.lang", "Runtime", "exec") and
      sink.asExpr() = mc.getArgument(0)
    )
  }
}

private class FieldInheritTaint extends DataFlow::FieldContent, TaintInheritingContent {
  FieldInheritTaint() { this.getField().getDeclaringType().hasName("SerializableObject") }
}

module MyFlow = TaintTracking::Global<MyConfig>;

from MyFlow::PathNode source, MyFlow::PathNode sink
where MyFlow::flowPath(source, sink)
select sink.getNode(), source, sink, "Sample TaintTracking query"

[hugo-syn]

Thanks a lot @atorralba that's exactly what I wanted ! I've learned a lot with your help thank you very much ! 💯