Java Taint Tracking: How to track string concatenations?

[dvec01]

Given the following code:

import java.io.IOException;

public class ToStringThroughConcat {

    public static class MyList {
        private String s;
        public void setS(String s) {
            this.s = s;
        }
        public String toString() {
            try{
                Runtime.getRuntime().exec(this.s);
            } catch (IOException e) {
                throw new RuntimeException(e);
            }
            return "";
        }
    }

    private void source(String input) throws IOException {
        MyList list = new MyList();
        list.setS(input);
        String x = list + "";

        list.toString();

        java.lang.String.valueOf(list);
    }

}

The following query finds the exec sink through list.toString();, but not via String x = list + ""; or java.lang.String.valueOf(list);, which both trigger the toString method indirectly:

/**
 * @kind path-problem
 */

import java
import semmle.code.java.dataflow.FlowSources

module Config implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) {
    source.asParameter().getCallable().hasName("source") and
    source.getEnclosingCallable().getDeclaringType().hasName("ToStringThroughConcat")
  }

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

  predicate isSink(DataFlow::Node sink) { any() }
}

module Flow = TaintTracking::Global<Config>;

import Flow::PathGraph

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

Is this intended? Are there methods to modify this behaviour, without having to change the internal implementation?

Thanks in advance.

[aschackmull]

It is perhaps a bit unusual to have an exec sink inside a toString method, but your point is valid. As you've noticed, we lack flow into implicit toString calls. This is a known shortcoming, and we have an internal issue tracking this, but a fix has yet to be prioritized because it hasn't been deemed important enough. So far I'm only aware of relevant examples of this in contrived snippets like the above that were made to deliberately check whether we follow flow like this. Do you have an example of a real-life vulnerability (CVE) involving a flow-path through implicit toString like this? If so, then I'd be much more inclined to prioritise this.

[dvec01]

Hi. I am not aware of any real life vulnerabilities that exploit this exact behaviour. However, I do think that there exist real life scenarios where tracking these implicit string concatenations is helpful.

For example, assume a java application which has a deserialization vulnerability and a dependency on jackson-databind. It is known, that calling toString on com.fasterxml.jackson.databind.node.POJONode allows to call any getter of a java bean, a primitive which usually can be exploited further.

Thats where the snippet I posted above comes into play. I was looking for a way to reach the toString method of an arbitrary object from a serializable method (readObject, ..) from a jdk class. There exists a way to do this and the chain relies on the implicit call of the toString method via concatenation. I was not able to find this via codeql, but through manual analysis. I can share the chain if this helps in prioritizing the issue.

But I also want to add that this is not restricted to implicit ".toString". For example, passing an object of a class that implements "java.lang.Comparable" to "java.util.TreeSet.add()" will trigger the "compareTo" function of the object. This is not tracked (and I think similar points can be made for .equals, .hashCode and other methods):

import java.io.IOException;
import java.util.*;

public class TestCompare {

    public static class MyList implements Comparable<MyList> {
      private String s;

        public void setS(String s) {
            this.s = s;
        }

        @Override
        public int compareTo(MyList myList) {
            try{
                Runtime.getRuntime().exec(myList.s);
            } catch (Exception e) {
                // throw new RuntimeException(e);
            }
            return 0;
        }

    }

    public static void main(String[] args) {
        MyList list1 = new MyList();
        list1.setS(args[0]);
        MyList list2 = new MyList();
        list1.compareTo(list2);
        Set<MyList> l = new TreeSet<>();
        l.add(list1);
        l.add(list2);
    }

}

/**
 * @kind path-problem
 */

import java
import semmle.code.java.dataflow.FlowSources

module Config implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) {
    source.asParameter().getCallable().hasName("main") and
    source.getEnclosingCallable().getDeclaringType().hasName("TestCompare")
  }

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

  predicate isSink(DataFlow::Node sink) { any() }
}

module Flow = TaintTracking::Global<Config>;

import Flow::PathGraph

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

[aschackmull]

For example, assume a java application which has a deserialization vulnerability and a dependency on jackson-databind. It is known, that calling toString on com.fasterxml.jackson.databind.node.POJONode allows to call any getter of a java bean, a primitive which usually can be exploited further.

Thats where the snippet I posted above comes into play. I was looking for a way to reach the toString method of an arbitrary object from a serializable method (readObject, ..) from a jdk class. There exists a way to do this and the chain relies on the implicit call of the toString method via concatenation. I was not able to find this via codeql, but through manual analysis. I can share the chain if this helps in prioritizing the issue.

Thank you, this is helpful context. No need to share the chain. I'll add your context to our internal issue and bump its priority a bit, but I still can't give any guarantees about when we'll get to it.

But I also want to add that this is not restricted to implicit ".toString". For example, passing an object of a class that implements "java.lang.Comparable" to "java.util.TreeSet.add()" will trigger the "compareTo" function of the object. This is not tracked (and I think similar points can be made for .equals, .hashCode and other methods):

That's true. Noted.