Do you think the following methods (and their overloads) would be interesting as well?

• readTree
• readValues
• treeToValue
• Maybe: treeAsTokens

I think the only one that I'd add additionally would probably be readValues because it lets you read the JSON as an iterator. I think that someone else should add readTree and treeToValue when they understand the use case and how the data flows out of those methods.

I think the only one that I'd add additionally would probably be readValues because it lets you read the JSON as an iterator.

Right, the other methods probably don't make much sense at the moment since there is no modeling for TreeNode respectively JsonNode yet.

Do you think an AdditionalTaintStep for methods creating a JsonParser from tainted data should be added? (e.g. ObjectReader.createParser or JsonFactory.createParser)

That way, the readValue methods that receive a JsonParser would propagate taint correctly too (e.g. ObjectReader.readValue(JsonParser p))

@JLLeitschuh A clarifying followup question: In the jacksonTwoStepDeserialization test case the om.valueToTree(taintedParams) call takes a HashMap with tainted values as input. Is that always the case for valueToTree? The reason I'm asking is that we're looking into more precise collection-flow modelling, which means that we'll begin distinguishing between a tainted object and a non-tainted map containing tainted values or tainted keys. This means that this test fails as-is, since the map no longer is considered tainted, and I need to understand the proper way to fix it.

https://fasterxml.github.io/jackson-databind/javadoc/2.8/com/fasterxml/jackson/databind/ObjectMapper.html#valueToTree(java.lang.Object)

According to the documentation, valueToTree takes a java bean, but also supports taking a map.

	public static void jacksonTwoStepDeserialization() throws java.io.IOException {
		String s = taint();
		Map<String, Object> taintedParams = new HashMap<>();
		taintedParams.put("name", s); // Value is tainted
		ObjectMapper om = new ObjectMapper();
		JsonNode jn = om.valueToTree(taintedParams); // The jn.get("name").asText() is tainted
		sink(jn); //$hasTaintFlow
		Potato p = om.convertValue(jn, Potato.class); // the p.name field is tainted
		sink(p); //$hasTaintFlow // I just assume the whole object is tainted, because it's easier
		sink(p.getName()); //$hasTaintFlow // This is the actual tainted data
	}

I'm not totally certain I understand your question, but this code comes from an actual example that we have in our production code at Gradle.

I don't see any mention of a map in the doc you linked?
In any case, let me try to elaborate: Currently the taint model of valueToTree is described as

"com.fasterxml.jackson.databind;ObjectMapper;true;valueToTree;;;Argument[0];ReturnValue;taint"

I'd be looking to supplement that with

"com.fasterxml.jackson.databind;ObjectMapper;true;valueToTree;;;MapValue of Argument[0];ReturnValue;taint"

to explicitly say that taint input can also be extracted from values of a map. What I'm trying to understand is whether this would complete the model or whether valueToTree also meaningfully can be applied to, say, a list of tainted values, or an array of tainted values, or some nested structure where the taint is hidden more deeply.

@aschackmull supplementing it with MapValue of sounds reasonable, but the same applies to map keys. As Jonathan mentioned, the method returns the JSON representation (as in-memory JsonNode) of whatever you provide to it, e.g.:

import java.util.Arrays;
import java.util.Collections;

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;

public class JacksonTest {
    static class MyClass {
        public String f;
    }
    
    public static void main(String args[]) {
        ObjectMapper mapper = new ObjectMapper();
        JsonNode node = mapper.valueToTree(Arrays.asList("tainted"));
        System.out.println(node);
        
        node = mapper.valueToTree(Collections.singletonMap("tainted", 2));
        System.out.println(node);
        
        MyClass obj = new MyClass();
        obj.f = "tainted";
        node = mapper.valueToTree(obj);
        System.out.println(node);
    }
}

So any Object is supported and tainted data can indeed be hidden more deeply inside of it.

So any Object is supported and tainted data can indeed be hidden more deeply inside of it.

Thanks for the confirmation. That was also what I expected. This is going to need some more dedicated support to model correctly. I've made an issue for tracking this: #5985