G304: false positive with filepath.Join

[ldez]

Summary

filepath.Join calls filepath.Clean so it's safe.

Steps to reproduce the behavior

func foo(parts []string) error {
	file, err := os.Open(filepath.Join(parts...))
	if err != nil {
		return err
	}

	defer func() { _ = file.Close() }()

	fmt.Println(file.Name())

	return nil
}

gosec version

v0.0.0-20200103095621-79fbf3af8d83

Go version (output of 'go version')

go version go1.13.7 linux/amd64

Operating system / Environment

linux/amd64

Expected behavior

The filepath.Join be considered as safe.

Actual behavior

G304: Potential file inclusion via variable (gosec)
        file, err := os.Open(filepath.Join(parts...))

[gcmurphy]

One of the reasons that alert is getting triggered is that gosec currently doesn't have the taint analysis / backtracking required to resolve all usage of the foo function or the content of the parts variable inside the function.

However if as a user I am able to control part of the parts variable it could result in an issue. Here's an example: https://play.golang.org/p/DT30UgrE9sr

package main

import (
	"fmt"
	"path/filepath"
	"os"
)


func foo(parts []string) error {
	file, err := os.Open(filepath.Join(parts...))
	if err != nil {
		return err
	}

	defer func() { _ = file.Close() }()
	

	fmt.Println(file.Name())
	
	buf := make([]byte, 32)
	file.Read(buf)
	fmt.Println(string(buf))

	return nil
}

func main() {
	foo([]string{"usr", "share", "../../../../../../../../etc/hosts"})

}

Will output

../../../../../../etc/hosts
127.0.0.1 localhost

This is the type of issue we're trying to warn of here I believe.

[ldez]

so in this case, there is another issue because filepath.Clean don't really prevent of this:

https://play.golang.org/p/iFrj0shla2e

package main

import (
	"fmt"
	"path/filepath"
	"os"
)


func foo1(parts []string) error {
	// G304
	file, err := os.Open(filepath.Join(parts...))
	if err != nil {
		return err
	}

	defer func() { _ = file.Close() }()

	fmt.Println(file.Name())

	buf := make([]byte, 32)
	file.Read(buf)
	fmt.Println(string(buf))

	return nil
}

func foo2(parts []string) error {
	// no G304
	file, err := os.Open(filepath.Clean(filepath.Join(parts...)))
	if err != nil {
		return err
	}

	defer func() { _ = file.Close() }()

	fmt.Println(file.Name())

	buf := make([]byte, 32)
	file.Read(buf)
	fmt.Println(string(buf))

	return nil
}

func main() {
	_ = foo1([]string{"usr", "share", "../../../../../../../../etc/hosts"})
	_ = foo2([]string{"usr", "share", "../../../../../../../../etc/hosts"})
}

../../../../../../etc/hosts
127.0.0.1 localhost

../../../../../../etc/hosts
127.0.0.1 localhost

filepath.Join call filepath.Clean:

• https://github.com/golang/go/blob/6917529cc604bad3b7d67579ca8d569442e3d880/src/path/filepath/path.go#L211-L213
• https://github.com/golang/go/blob/6917529cc604bad3b7d67579ca8d569442e3d880/src/path/filepath/path_unix.go#L45

package main

import (
	"fmt"
	"path/filepath"
)

func main() {
	fmt.Println("Clean", filepath.Clean("/usr/share/../../../../../../../../etc/hosts"))
	fmt.Println("Join", filepath.Join("/", "usr", "share", "../../../../../../../../etc/hosts"))

	fmt.Println("Clean2", filepath.Clean("usr/share/../../../../../../../../etc/hosts"))
	fmt.Println("Join2", filepath.Join("usr", "share", "../../../../../../../../etc/hosts"))
}

Clean /etc/hosts
Join /etc/hosts
Clean2 ../../../../../../etc/hosts
Join2 ../../../../../../etc/hosts

https://play.golang.org/p/02EFDbiW4Py

[gcmurphy]

In all the cases above I think we only alert if an argument to filepath.Join is being called with a identifier that we can't resolve. If it is a string literal or resolvable in the current scope to a string literal we shouldn't alert. What we're aiming for is 'best effort' notification of this being a risk.

I actually don't think filepath.Clean actually is designed to prevent path traversal bugs, but I'm actually surprised that the second case (without leading the leading '/') occurs. I guess by providing a relative path, we receive a relative path?

Using filepath.Rel here is what you probably need to use if you want to be sure that path being opened is contained within a certain base directory etc.

I'll keep this bug open to see in which ways we can enhance the existing rule. Possibly inclusion of filepath.Clean("/tmp" + userControlled) to the things we inspect.

[ldez]

@ccojocar I think you closed the wrong issue 🤔