[Go] Trouble getting the resolved type of an argument from nested function

[rh-tguittet]

Hi, I am facing an issue to obtain the resolved type of an argument in the following situation (simplified from what some flavour of the Kubernetes client-go do) :

package main

type Object interface{ Bar() bool }

type SpecificObject struct {
	Foo      string
	otherFoo string
}

func (so SpecificObject) Bar() bool { return true }

func cacheReader(key string) interface{} {
	if len(key) > 0 {
		// The details don't matter so much here, what counts is that an interface{} is returned
		return new(interface{})
	}

	return nil
}

func ReadObject(key string, obj Object) {
	obj = cacheReader(key).(Object)
}

func ReadObjectWrapper(obj Object) {
	ReadObject("foobar", obj)
}

func ReadObjectAfterCheck(obj Object) {
	if obj.Bar() { // The nature of the check does not matter here
		ReadObjectWrapper(obj)
	}
}

func sink(s string) { /**/ }

func main() {
	so := &SpecificObject{Foo: "Bar"}
	ReadObjectAfterCheck(so)
	sink(so.otherFoo)
}

With the following query:

/**
 * @kind path-problem
 */

import go
import DataFlow

class MySource extends Function {
  MySource() { this.getName() = "ReadObject" }

  FunctionInput getInput() { result.isParameter(1) }

  FunctionOutput getOutput() { result.isParameter(1) }
}

private module TestFlow implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node node) {
    exists(MySource ms | node = ms.getOutput().getNode(ms.getACall()))
  }

  predicate isSink(DataFlow::Node node) {
    exists(Function sink | sink.getName() = "sink" and sink.getACall().getAnArgument() = node)
  }
}

module Flow = TaintTracking::Global<TestFlow>;

import Flow::PathGraph

from Flow::PathNode source, Flow::PathNode sink, Type t
where Flow::flowPath(source, sink) and t = source.getNode().getType()
select source.getNode(), source, sink, "$@ (with type $@) -> $@", source.getNode(), "__source__", t,
  t.pp(), sink.getNode(), "__sink__"

This returns: __source__ (with type Object) -> __sink__
Whereas I want Object to be a PointerType to SpecificObject. The CodeQL query works fine as long as there isn't nested calls to "parent types" (e.g. ReadObjectAfterCheck>ReadObjectWrapper>ReadObject)

It's not clear to me if the expectation I have are misguided or if some additional bits (like a flow to model StructLit ~> Argument) are required in the query. Please advise :)

[owen-mc]

Thank you for including a minimized example and a working query, which made it very easy to understand your question. The problem is that your source, which is obj in ReadObjectWrapper, has type Object. If you want it to give you the type from so in main, then you will have to make so the source. We want to find paths A -> B -> C, where B -> C is what you are currently finding and A is the definition of the variable in the first place. This can be done using flow states. I suggest the following approach:

• Use data flow with state (implement DataFlow::StateConfigSig and use TaintTracking::GlobalWithState) where the state is a newtype with two values (example here), called something like InitialState and HasBeenPassedToReadObject.
• Make your source something like the initial definition of a variable (do you know it will always be a struct?) and always give it the state InitialState.
• Have an additional flow step where state1 is InitialState, state2 is HasBeenPassedToReadObject, with node1 being the argument to ReadObject and node2 being the post-update node of node1 (which represents the value of node after the function call has happened). (This replaces the source definition in your current query.)

I hope that this approach works for you. I have been very vague on exactly how to define the source because I am not sure and it depends a bit on the exact details of your situation, so do try some different things and ask more questions if you need more help with that bit.

[owen-mc]

MergePathGraph doesn't do quite what you are trying to use it to do. (It doesn't concatenate paths.) In this case I think you don't need to show paths from both configs, so we don't need to deal with two different path graphs at all. This code worked for me.

/**
 * @kind path-problem
 */

import go

DataFlow::Node getArgumentToReadObject() {
  exists(Function f | f.getName() = "ReadObject" and result = f.getACall().getArgument(1))
}

module AtoB implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node node) { node.asExpr() instanceof StructLit }

  predicate neverSkip(DataFlow::Node n) { any() }

  predicate isSink(DataFlow::Node node) { node = getArgumentToReadObject() }
}

module BtoC implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node node) {
    node.(DataFlow::PostUpdateNode).getPreUpdateNode() = getArgumentToReadObject()
  }

  predicate isSink(DataFlow::Node node) {
    exists(Function sink | sink.getName() = "sink" and sink.getACall().getAnArgument() = node)
  }
}

module FlowAtoB = TaintTracking::Global<AtoB>;

module FlowBtoC = TaintTracking::Global<BtoC>;

import FlowBtoC::PathGraph

from DataFlow::Node a, DataFlow::Node b1, FlowBtoC::PathNode b2, FlowBtoC::PathNode c, Type t
where
  FlowAtoB::flow(a, b1) and
  t = a.getType() and
  b2.getNode().(DataFlow::PostUpdateNode).getPreUpdateNode() = b1 and
  FlowBtoC::flowPath(b2, c)
select c.getNode(), b2, c, "$@ (initialised $@ with type $@) -> $@", b1, b1.toString(), a, "here",
  t, t.pp(), c.getNode(), c.getNode().toString()

[rh-tguittet]

OK, let's do it this way then. I assume there is no easy way of adding a step to the path?

[rh-tguittet]

Sorry, had to unmark this as the answer: it doesn't work as expected when there are 2 types flowing through ReadObject but only one instance goes to the sink.

New code, adds an OtherObject type compatible with Object:

package main

import (
	"fmt"
)

type Object interface{ Bar() bool }

type SpecificObject struct {
	Foo      string
	otherFoo string
}

type OtherObject struct {
	otherFoo string
}

func (so SpecificObject) Bar() bool { return true }
func (oo OtherObject) Bar() bool    { return true }

func ReadObject(key string, obj Object) {
	switch obj.(type) {
	case *SpecificObject:
		obj.(*SpecificObject).otherFoo = "bar"
	case *OtherObject:
		obj.(*OtherObject).otherFoo = "baz"
	}
}

func ReadObjectWrapper(obj Object) {
	ReadObject("foobar", obj)
}

func ReadObjectAfterCheck(obj Object) {
	if obj.Bar() { // The nature of the check does not matter here
		ReadObjectWrapper(obj)
	}
}

func sink(s string) { /**/ }

func main() {
	so := &SpecificObject{Foo: "Bar"}
	ReadObjectAfterCheck(so)
	fmt.Printf("so: %+v\n", so)
	oo := &OtherObject{}
	ReadObjectAfterCheck(oo)
	fmt.Printf("oo: %+v\n", oo)
	sink(so.otherFoo)
}

[owen-mc]

Good point. Try this:

/**
 * @kind path-problem
 */

import go

DataFlow::Node getA() { result.asExpr() instanceof StructLit }

DataFlow::Node getB() {
  exists(Function f | f.getName() = "ReadObject" and result = f.getACall().getArgument(1))
}

DataFlow::Node getC() {
  exists(Function sink | sink.getName() = "sink" and result = sink.getACall().getAnArgument())
}

module AtoB implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node node) { node = getA() }

  predicate isSink(DataFlow::Node node) { node = getB() }
}

module BtoC implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node node) {
    node.(DataFlow::PostUpdateNode).getPreUpdateNode() = getB()
  }

  predicate isSink(DataFlow::Node node) { node = getC() }
}

module AtoC implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node node) { node = getA() }

  predicate isSink(DataFlow::Node node) { node = getC() }
}

module FlowAtoB = TaintTracking::Global<AtoB>;

module FlowBtoC = TaintTracking::Global<BtoC>;

module FlowAtoC = TaintTracking::Global<AtoC>;

import FlowBtoC::PathGraph

from DataFlow::Node a, DataFlow::Node b1, FlowBtoC::PathNode b2, FlowBtoC::PathNode c, Type t
where
  FlowAtoB::flow(a, b1) and
  t = a.getType() and
  b2.getNode().(DataFlow::PostUpdateNode).getPreUpdateNode() = b1 and
  FlowBtoC::flowPath(b2, c) and
  FlowAtoC::flow(a, c.getNode())
select c.getNode(), b2, c, "$@ (initialised $@ with type $@) -> $@", b1, b1.toString(), a, "here",
  t, t.pp(), c.getNode(), c.getNode().toString()

It is a shame that using flow states did not work in your case. Perhaps I will see if I can get something added to allow over-riding that that predicate in the config. If I can I will report back here.

[rh-tguittet]

That worked. One note from adapting this to the non-example code base: isAdditionalFlowStep for A->C and B->C must be shared.

Thank you Owen for seeing it through!