False positive path traversal with unrelated untrusted input indicated

[stevenjohnstone]

trafficstars

Version 0.1.0.

package foo

import (
	"io/ioutil"
	"log"
	"os"
	"path/filepath"
)

func output(name string) error {
	f, err := os.Open(name)
	if err != nil {
		return err
	}
	f.Write([]byte("foo"))
	defer f.Close()

	g, err := os.OpenFile("foo", os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600)
	if err != nil {
		return err
	}
	g.Write([]byte("foo"))
	return g.Close()
}

func Foo() (err error) {
	tmpdir, err := ioutil.TempDir("", "foo")
	if err != nil {
		return err
	}
	defer func() {
		if rerr := os.RemoveAll(tmpdir); rerr != nil {
			log.Println(err)
			err = rerr
			return
		}
	}()

	filename := filepath.Join(tmpdir, "foo.tmp")

	return output(filename)
}

gives output

$ gokart scan
Using default analyzers config found at "~/.gokart/analyzers.yml".

Revving engines VRMMM VRMMM
3...2...1...Go!

(Path Traversal) Danger: possible path traversal injection detected

/home/stevie/gokart/foo.go:11
Vulnerable Function: [ output(...) error ]
      10:	func output(name string) error {
    > 11:		f, err := os.Open(name)
      12:		if err != nil {

/home/stevie/gokart/foo.go:18
Source of Untrusted Input: [ output(...) error ]
      17:	
    > 18:		g, err := os.OpenFile("foo", os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600)
      19:		if err != nil {
------------------------------------------------------------------------------

Race Complete! Analysis took 146.430242ms and 63 Go files were scanned (including imported packages)
GoKart found 1 potentially vulnerable functions

Clearly, the line flagged as untrusted input is unrelated to name. If I remove the defer statement from Foo, no path traversal issue is detected.

[praetorian-harry]

@stevenjohnstone thanks again for the involvement and another issue submission! I am able to reproduce this issue using the test code that you've provided. We'll take a closer look!

[isp1r0]

@stevenjohnstone thanks for so much for this great input right out of the starting gate! We're just now getting some additional time and resources to continue the progression here and have recently put in place some of the fast-follow usability features over this past week.

Next up in adding some additional security checks while improving the underlying analysis engine.

This issue points out a general weakness in the intra-procedural analysis which clearly isn't context sensitive but it's unclear why this False Positive would occur only when the deferred function is present. In any case, this particular finding is no longer an issue (due to the removal of the taint source) but we're going to incorporate this test into our data flow tests as we move forward.