diff --git a/rules/readfile.go b/rules/readfile.go index 579f2fa44..18e977ca8 100644 --- a/rules/readfile.go +++ b/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") diff --git a/testutils/source.go b/testutils/source.go index 0683baead..b637febaa 100644 --- a/testutils/source.go +++ b/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)