Golang TaintTracking Sanitizer found in Quick Eval but not considered for hasFlowPath()

[rh-tguittet]

Hi,

I am trying to come up with a taint tracking configuration for removing SQL injection false positives. All of it is for a Go project. For some reason, I cannot come up with a sanitizer that cuts the taint flow. Quick eval for my sanitizer finds what I expect of it but doesn't seem to be taken into account for the full query. Trying to leverage the FlowState to mark (and exclude the mark from my sink) did not work.

Contrary to examples I could see, the method call modelled for the sanitizer is not attempting to filter the input (as in x = removeBadStuff(x)). Instead it ensures a set of conditions are met.

(Also I could not find/use hasPartialFlow(), looks like it's not exported.)

Here is the minimal code reproducer and codeql queries I have attempted:

package main

import (
	"errors"
	"gorm.io/driver/sqlite"
	"gorm.io/gorm"
	"os"
	"regexp"
	"strings"
)

type ListArguments struct {
	OrderBy []string
}

func Contains(values []string, s string) bool {
	for _, elem := range values {
		if elem == s {
			return true
		}
	}
	return false
}

func (la *ListArguments) Validate(acceptedOrderByParams []string) error {
	if len(la.OrderBy) > 0 {
		space := regexp.MustCompile(`\s+`)
		for _, orderByClause := range la.OrderBy {
			field := strings.ToLower(orderByClause)
			field = space.ReplaceAllString(field, " ")
			keywords := strings.Split(field, " ")
			// other checks
			if !Contains(acceptedOrderByParams, keywords[0]) {
				return errors.New("unknown order by field " + keywords[0])
			}
		}
	}
	return nil
}

type TestService struct {
	db *gorm.DB
}

func (ts *TestService) List(listArgs *ListArguments) {
	if err := listArgs.Validate([]string{"col1", "col2"}); err != nil {
		panic(err)
	}
	// snip... Some other stuff to perform the query ...snip
	ts.db.Order(listArgs.OrderBy) // my sink
}

func main() {
	inputString := os.Getenv("a") // my source
	listArgs := &ListArguments{
		OrderBy: []string{inputString},
	}

	db, _ := gorm.Open(sqlite.Open(":memory:"), nil)
	ts := &TestService{
		db: db,
	}

	ts.List(listArgs)
}

1. Vanilla flow:

/**
 * @kind path-problem
 * @id testing
 */
import go
import semmle.go.security.SqlInjection
import DataFlow
import DataFlow::PathGraph

class Source extends DataFlow::CallExpr {
    Source() {
        this.getTarget().hasQualifiedName("os", "Getenv")
    }
}

/*
 * Vanilla TaintTracking configuration
 */
class VerySimpleFlowCfg extends TaintTracking::Configuration {
    VerySimpleFlowCfg() { this = "VerySimpleFlowCfg" }

    override predicate isSource(DataFlow::Node source) {
        source.asExpr() instanceof Source
    }

    override predicate isSink(DataFlow::Node sink) {
        sink instanceof SQL::QueryString::Range
    }
}

// returns one result, as expected
from VerySimpleFlowCfg cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "Possible SQLi"

2. With Sanitizer

/**
 * @kind path-problem
 * @id testing
 */
import go
import semmle.go.security.SqlInjection
import DataFlow
import DataFlow::PathGraph

class Source extends DataFlow::CallExpr {
    Source() {
        this.getTarget().hasQualifiedName("os", "Getenv")
    }
}

class ValidateMethodCall extends DataFlow::MethodCallNode {
    ValidateMethodCall() {
        this.getTarget().hasQualifiedName("sampleProject.ListArguments", "Validate")
    }

    predicate nodeIsReceiver(DataFlow::Node node) {
        node = this.getReceiver()
    }
}

/*
 * Vanilla TaintTracking configuration
 */
class VerySimpleFlowWithSanitizerCfg extends TaintTracking::Configuration {
    VerySimpleFlowWithSanitizerCfg() { this = "VerySimpleFlowWithSanitizerCfg" }

    override predicate isSource(DataFlow::Node source) {
        source.asExpr() instanceof Source
    }

    override predicate isSink(DataFlow::Node sink) {
        sink instanceof SQL::QueryString::Range
    }

    // Quick eval finds 2 results
    override predicate isSanitizer(DataFlow::Node node) {
        exists(ValidateMethodCall call | call.nodeIsReceiver(node) ) or
        node instanceof ValidateMethodCall
    }
}

// returns one result, none expected
from VerySimpleFlowWithSanitizerCfg cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "Possible SQLi"

3. Sanitizer with FlowState markings

/**
 * @kind path-problem
 * @id testing
 */
import go
import semmle.go.security.SqlInjection
import DataFlow
import DataFlow::PathGraph

class Source extends DataFlow::CallExpr {
    Source() {
        this.getTarget().hasQualifiedName("os", "Getenv")
    }
}

class ValidateMethodCall extends DataFlow::MethodCallNode {
    ValidateMethodCall() {
        this.getTarget().hasQualifiedName("sampleProject.ListArguments", "Validate")
    }

    predicate nodeIsReceiver(DataFlow::Node node) {
        node = this.getReceiver()
    }
}

/*
 * Vanilla TaintTracking configuration
 */
class VerySimpleFlowWithSanitizerStateCfg extends TaintTracking::Configuration {
    VerySimpleFlowWithSanitizerStateCfg() { this = "VerySimpleFlowWithSanitizerStateCfg" }

    override predicate isSource(DataFlow::Node source) {
        source.asExpr() instanceof Source
    }

    override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
        sink instanceof SQL::QueryString::Range and state = ""
    }

    // Quick eval finds 2 results
    override predicate isSanitizer(DataFlow::Node node) {
        exists(ValidateMethodCall call | call.nodeIsReceiver(node) ) or
        node instanceof ValidateMethodCall
    }

    // Doing a second one to prevent state from crying about being not bound to a value
    override predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) {
        state = "validated"
    }

    // Tries to find out if the taint saw the call by marking state2 to "validated"
    // Quick eval looks ok.
    override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, DataFlow::FlowState state2) {
        node1 instanceof ValidateMethodCall and
        node1.(ValidateMethodCall).nodeIsReceiver(node2) and
        state1 instanceof FlowStateEmpty and
        state2 = "validated"
    }
}

// returns one result, none expected. The sink's state being an empty string, "validated" expected
from VerySimpleFlowWithSanitizerStateCfg cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "Possible SQLi with flowstate = $@", sink.getState(), sink.getState()

Thanks in advance!

[mbg]

Hi @rh-tguittet! I hope you've had a good weekend and thank you for this interesting question 🙂

If you run the "Vanilla flow" version of your query and inspect the path that is returned, you can see that the listArgs.Validate call is not part of the data flow. This is why your isSanitizer predicate in the "With Sanitizer" version of the query does not pick up on it. This is expected, because the data flow model does not treat the call to listArgs.Validate as a predecessor of the call to ts.db.Order(listArgs.OrderBy) since the data does not flow through it.

We have a mechanism referred to as BarrierGuard to deal with such cases. A BarrierGuard allows you to implement a sanitizer in terms of a check that is performed on all control flow paths leading to the sink. To explain how this works, let's consider a simpler variant of your Validate function which returns a boolean rather than an error:

func (la *ListArguments) Validate(acceptedOrderByParams []string) bool {
	if len(la.OrderBy) > 0 {
		space := regexp.MustCompile(`\s+`)
		for _, orderByClause := range la.OrderBy {
			field := strings.ToLower(orderByClause)
			field = space.ReplaceAllString(field, " ")
			keywords := strings.Split(field, " ")
			// other checks
			if !Contains(acceptedOrderByParams, keywords[0]) {
				return false
			}
		}
	}
	return true
}

Then the List method would be:

func (ts *TestService) List(listArgs *ListArguments) {
	if !listArgs.Validate([]string{"col1", "col2"}) {
		panic("unknown order by field")
	}
	// snip... Some other stuff to perform the query ...snip
	ts.db.Order(listArgs.OrderBy) // my sink
}

To implement a barrier guard, we need a predicate with the following signature which determines whether some data flow node g is a barrier guard:

predicate validateMethodCallCheck(DataFlow::Node g, Expr e, boolean outcome) {
    e = g.(ValidateMethodCall).getReceiver().asExpr() and
    outcome = true
}

This says: g is a barrier guard for e if the call evaluates to outcome, which we expect to be true since Validate returns true if validation is successful. We set e to the object that Validate is called on. We can now use this predicate in the definition of isSanitizer:

override predicate isSanitizer(DataFlow::Node node) {
    node = DataFlow::BarrierGuard<validateMethodCallCheck/3>::getABarrierNode()
}

With the help of our validateMethodCallCheck predicate, getABarrierNode implements all the necessary machinery to check that the call to Validate dominates all control flow paths to a sink and, if so, acts as a sanitizer for a given data flow path. If you test this, your query should no longer return a result.

Your implementation of Validate makes things a little bit more complicated because it returns an error. Our barrier guard predicate therefore needs to check that the error value that is returned is nil to validate the path. We can implement this e.g. by checking for local data flow from the result of the call to Validate to an operand of EqualityTestNode where it is compared to nil:

predicate validateMethodCallCheck(DataFlow::Node g, Expr e, boolean outcome) {
    exists(ValidateMethodCall call, DataFlow::EqualityTestNode etn |
        e = call.getReceiver().asExpr() and
        etn = g and
        DataFlow::localFlow(call.getResult(), etn.getAnOperand()) and
        etn.getAnOperand().asExpr() = Builtin::nil().getAReference() and
        etn.getPolarity() = outcome
    )
}

getPolarity() is true if the comparison uses == and false if it uses !=. Since we want the barrier guard to validate the path if Validate returns nil, the expected outcome corresponds to the type of comparison used.

I hope this helps and has answered your question, but let us know if you have any follow-up questions!

[rh-tguittet]

Thank you Michael that solved it :) I really appreciate the detailed answer. (If you codeql folks could add some of the more complex taint/dataflow examples in the docs that'd be brilliant.)