diff --git a/README.md b/README.md index 76d906a..fcd2fae 100644 --- a/README.md +++ b/README.md @@ -83,6 +83,7 @@ directory you can supply `./...` as the input argument. - G304: File path provided as taint input - G305: File traversal when extracting zip archive - G306: Poor file permissions used when writing to a new file +- G307: Deferring a method which returns an error - G401: Detect the usage of DES, RC4, MD5 or SHA1 - G402: Look for bad TLS connection settings - G403: Ensure minimum RSA key length of 2048 bits diff --git a/cmd/gosec/main.go b/cmd/gosec/main.go index 236c499..e6f98cc 100644 --- a/cmd/gosec/main.go +++ b/cmd/gosec/main.go @@ -153,7 +153,7 @@ func loadConfig(configFile string) (gosec.Config, error) { if err != nil { return nil, err } - defer file.Close() + defer file.Close() // #nosec G307 if _, err := config.ReadFrom(file); err != nil { return nil, err } @@ -201,7 +201,7 @@ func saveOutput(filename, format string, paths []string, issues []*gosec.Issue, if err != nil { return err } - defer outfile.Close() + defer outfile.Close() // #nosec G307 err = output.CreateReport(outfile, format, rootPaths, issues, metrics, errors) if err != nil { return err diff --git a/rules/bad_defer.go b/rules/bad_defer.go new file mode 100644 index 0000000..9d8b211 --- /dev/null +++ b/rules/bad_defer.go @@ -0,0 +1,69 @@ +package rules + +import ( + "fmt" + "go/ast" + "strings" + + "github.com/securego/gosec" +) + +type deferType struct { + typ string + methods []string +} + +type badDefer struct { + gosec.MetaData + types []deferType +} + +func (r *badDefer) ID() string { + return r.MetaData.ID +} + +func normalize(typ string) string { + return strings.TrimPrefix(typ, "*") +} + +func contains(methods []string, method string) bool { + for _, m := range methods { + if m == method { + return true + } + } + return false +} + +func (r *badDefer) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) { + if deferStmt, ok := n.(*ast.DeferStmt); ok { + for _, deferTyp := range r.types { + if typ, method, err := gosec.GetCallInfo(deferStmt.Call, c); err == nil { + if normalize(typ) == deferTyp.typ && contains(deferTyp.methods, method) { + return gosec.NewIssue(c, n, r.ID(), fmt.Sprintf(r.What, typ, method), r.Severity, r.Confidence), nil + } + } + } + + } + + return nil, nil +} + +// NewDeferredClosing detects unsafe defer of error returning methods +func NewDeferredClosing(id string, conf gosec.Config) (gosec.Rule, []ast.Node) { + return &badDefer{ + types: []deferType{ + { + typ: "os.File", + methods: []string{"Close"}, + }, + }, + MetaData: gosec.MetaData{ + ID: id, + Severity: gosec.Medium, + Confidence: gosec.High, + What: "Deferring unsafe method %q on type %q", + }, + }, []ast.Node{(*ast.DeferStmt)(nil)} +} diff --git a/rules/rulelist.go b/rules/rulelist.go index 2cc6ab5..c45f517 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -82,6 +82,7 @@ func Generate(filters ...RuleFilter) RuleList { {"G304", "File path provided as taint input", NewReadFile}, {"G305", "File path traversal when extracting zip archive", NewArchive}, {"G306", "Poor file permissions used when writing to a file", NewWritePerms}, + {"G307", "Unsafe defer call of a method returning an error", NewDeferredClosing}, // crypto {"G401", "Detect the usage of DES, RC4, MD5 or SHA1", NewUsesWeakCryptography}, diff --git a/rules/rules_test.go b/rules/rules_test.go index c786402..ca06c7c 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -131,6 +131,10 @@ var _ = Describe("gosec rules", func() { runner("G306", testutils.SampleCodeG306) }) + It("should detect unsafe defer of os.Close", func() { + runner("G307", testutils.SampleCodeG307) + }) + It("should detect weak crypto algorithms", func() { runner("G401", testutils.SampleCodeG401) }) diff --git a/testutils/source.go b/testutils/source.go index d55cdfa..af69b27 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -1435,7 +1435,57 @@ func main() { d2 := []byte{115, 111, 109, 101, 10} n2, err := f.Write(d2) + + defer check(err) + fmt.Printf("wrote %d bytes\n", n2) + + n3, err := f.WriteString("writes\n") + fmt.Printf("wrote %d bytes\n", n3) + + f.Sync() + + w := bufio.NewWriter(f) + n4, err := w.WriteString("buffered\n") + fmt.Printf("wrote %d bytes\n", n4) + + w.Flush() + +}`}, 1, gosec.NewConfig()}} + // SampleCodeG307 - Unsafe defer of os.Close + SampleCodeG307 = []CodeSample{ + {[]string{`package main + +import ( + "bufio" + "fmt" + "io/ioutil" + "os" +) + +func check(e error) { + if e != nil { + panic(e) + } +} + +func main() { + + d1 := []byte("hello\ngo\n") + err := ioutil.WriteFile("/tmp/dat1", d1, 0744) check(err) + + allowed := ioutil.WriteFile("/tmp/dat1", d1, 0600) + check(allowed) + + f, err := os.Create("/tmp/dat2") + check(err) + + defer f.Close() + + d2 := []byte{115, 111, 109, 101, 10} + n2, err := f.Write(d2) + + defer check(err) fmt.Printf("wrote %d bytes\n", n2) n3, err := f.WriteString("writes\n") @@ -1462,6 +1512,7 @@ import ( "log" "os" ) + func main() { f, err := os.Open("file.txt") if err != nil { @@ -1469,6 +1520,13 @@ func main() { } defer f.Close() + defer func() { + err := f.Close() + if err != nil { + log.Printf("error closing the file: %s", err) + } + }() + h := md5.New() if _, err := io.Copy(h, f); err != nil { log.Fatal(err)