Track back when a file path was sanitized with filepath.Clean (#912)

* Track back when a file path was sanitized with filepath.Clean

* Remove unused argument to fix lint warnings

rules/readfile.go

@@ -24,8 +24,9 @@ import (
 type readfile struct {
 	gosec.MetaData
 	gosec.CallList
-	pathJoin gosec.CallList
-	clean    gosec.CallList
+	pathJoin   gosec.CallList
+	clean      gosec.CallList
+	cleanedVar map[any]ast.Node
 }
 
 // ID returns the identifier for this rule
@@ -57,24 +58,29 @@ func (r *readfile) isJoinFunc(n ast.Node, c *gosec.Context) bool {
 	return false
 }
 
-// isFilepathClean checks if there is a filepath.Clean before assigning to a variable
-func (r *readfile) isFilepathClean(n *ast.Ident, c *gosec.Context) bool {
-	if n.Obj.Kind != ast.Var {
-		return false
+// isFilepathClean checks if there is a filepath.Clean for given variable
+func (r *readfile) isFilepathClean(n *ast.Ident) bool {
+	if _, ok := r.cleanedVar[n.Obj.Decl]; ok {
+		return true
 	}
-	if node, ok := n.Obj.Decl.(*ast.AssignStmt); ok {
-		if call, ok := node.Rhs[0].(*ast.CallExpr); ok {
-			if clean := r.clean.ContainsPkgCallExpr(call, c, false); clean != nil {
-				return true
-			}
+	return false
+}
+
+// trackFilepathClean tracks back the declaration of variable from filepath.Clean argument
+func (r *readfile) trackFilepathClean(n ast.Node) {
+	if clean, ok := n.(*ast.CallExpr); ok && len(clean.Args) > 0 {
+		if ident, ok := clean.Args[0].(*ast.Ident); ok {
+			r.cleanedVar[ident.Obj.Decl] = n
 		}
 	}
-	return false
 }
 
 // Match inspects AST nodes to determine if the match the methods `os.Open` or `ioutil.ReadFile`
 func (r *readfile) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {
-	if node := r.ContainsPkgCallExpr(n, c, false); node != nil {
+	if node := r.clean.ContainsPkgCallExpr(n, c, false); node != nil {
+		r.trackFilepathClean(n)
+		return nil, nil
+	} else if node := r.ContainsPkgCallExpr(n, c, false); node != nil {
 		for _, arg := range node.Args {
 			// handles path joining functions in Arg
 			// eg. os.Open(filepath.Join("/tmp/", file))
@@ -95,7 +101,7 @@ func (r *readfile) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {
 				obj := c.Info.ObjectOf(ident)
 				if _, ok := obj.(*types.Var); ok &&
 					!gosec.TryResolve(ident, c) &&
-					!r.isFilepathClean(ident, c) {
+					!r.isFilepathClean(ident) {
 					return gosec.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
 				}
 			}
@@ -116,6 +122,7 @@ func NewReadFile(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {
 			Severity:   gosec.Medium,
 			Confidence: gosec.High,
 		},
+		cleanedVar: map[any]ast.Node{},
 	}
 	rule.pathJoin.Add("path/filepath", "Join")
 	rule.pathJoin.Add("path", "Join")

testutils/source.go

@@ -2463,6 +2463,28 @@ import (
     "path/filepath"
 )
 
+func openFile(dir string, filePath string) {
+	fp := filepath.Join(dir, filePath)
+	fp = filepath.Clean(fp)
+	_, err := os.OpenFile(fp, os.O_RDONLY, 0600)
+	if err != nil {
+		panic(err)
+	}
+}
+
+func main() {
+    repoFile := "path_of_file"
+	dir := "path_of_dir"
+	openFile(dir, repoFile)
+}
+`}, 0, gosec.NewConfig()}, {[]string{`
+package main
+
+import (
+    "os"
+    "path/filepath"
+)
+
 func main() {
     repoFile := "path_of_file"
 	relFile, err := filepath.Rel("./", repoFile)