Remove rule G307 which checks when an error is not handled when a file or socket connection is closed (#935)

* Remove read only types from unsafe defer rules

* Remove rule G307 which checks when an error is not handled when a file or socket connection is closed

This doesn't seem to bring much value from security perspective, and it caused a lot of controversy since
is a very common pattern in Go.

* Mentioned in documentation that rule G307 is retired

* Clean up the test for rule G307

README.md

@@ -157,7 +157,6 @@ directory you can supply `./...` as the input argument.
 - G304: File path provided as taint input
 - G305: File traversal when extracting zip/tar 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
@@ -172,6 +171,7 @@ directory you can supply `./...` as the input argument.
 ### Retired rules
 
 - G105: Audit the use of math/big.Int.Exp - [CVE is fixed](https://github.com/golang/go/issues/15184)
+- G307: Deferring a method which returns an error - causing more inconvenience than fixing a security issue, despite the details from this [blog post](https://www.joeshaw.org/dont-defer-close-on-writable-files/)
 
 ### Selecting rules
 

issue/issue.go

@@ -77,7 +77,6 @@ var ruleToCWE = map[string]string{
 	"G304": "22",
 	"G305": "22",
 	"G306": "276",
-	"G307": "703",
 	"G401": "326",
 	"G402": "295",
 	"G403": "310",

rules/bad_defer.go

@@ -1,97 +0,0 @@
-package rules
-
-import (
-	"fmt"
-	"go/ast"
-	"strings"
-
-	"github.com/securego/gosec/v2"
-	"github.com/securego/gosec/v2/issue"
-)
-
-type deferType struct {
-	typ     string
-	methods []string
-}
-
-type badDefer struct {
-	issue.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) (*issue.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 c.NewIssue(n, r.ID(), fmt.Sprintf(r.What, method, typ), 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"},
-			},
-			{
-				typ:     "io.ReadCloser",
-				methods: []string{"Close"},
-			},
-			{
-				typ:     "io.WriteCloser",
-				methods: []string{"Close"},
-			},
-			{
-				typ:     "io.ReadWriteCloser",
-				methods: []string{"Close"},
-			},
-			{
-				typ:     "io.ReadSeekCloser",
-				methods: []string{"Close"},
-			},
-			{
-				typ:     "io.Closer",
-				methods: []string{"Close"},
-			},
-			{
-				typ:     "net.Conn",
-				methods: []string{"Close"},
-			},
-			{
-				typ:     "net.Listener",
-				methods: []string{"Close"},
-			},
-		},
-		MetaData: issue.MetaData{
-			ID:         id,
-			Severity:   issue.Medium,
-			Confidence: issue.High,
-			What:       "Deferring unsafe method %q on type %q",
-		},
-	}, []ast.Node{(*ast.DeferStmt)(nil)}
-}

rules/rulelist.go

@@ -91,7 +91,6 @@ func Generate(trackSuppressions bool, 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

@@ -146,10 +146,6 @@ 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

@@ -2777,58 +2777,6 @@ func main() {
 
 }`}, 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")
-	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()}, {[]string{`
-package main
-
-import (
-	"net"
-	"net/http"
-)
-
-func main() {
-	response, _ := http.Get("https://127.0.0.1")
-
-	defer response.Body.Close() // io.ReadCloser
-
-	conn, _ := net.Dial("tcp", "127.0.0.1:8080")
-	defer conn.Close() // net.Conn
-
-}`}, 2, gosec.NewConfig()},
-	}
 
 	// SampleCodeG401 - Use of weak crypto MD5
 	SampleCodeG401 = []CodeSample{