Rule for defering methods which return errors (#441)

Author: caccavale

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

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

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)}
+}

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},

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)
 		})

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,13 +1512,21 @@ import (
 	"log"
 	"os"
 )
+
 func main() {
 	f, err := os.Open("file.txt")
 	if err != nil {
 		log.Fatal(err)
 	}
 	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)