G307: gosec starts detecting G307 (CWE-703) even with proposed way to safely handle errors

[achiku]

Summary

gosec v2.9.1 starts detecting G307 (CWE-703): Deferring unsafe method "Close" on type "*os.File" (Confidence: HIGH, Severity: MEDIUM) which could have been avoided by following #512 with previous version of gosec.

Steps to reproduce the behavior

package main

import (
        "log"
        "os"
)

func main() {
        f, err := os.Open("./testfile.txt")
        if err != nil {
                log.Fatal(err)
                return
        }
        defer func() {
                if err := f.Close(); err != nil {
                        log.Fatal("failed to close file")
                }
        }()
        log.Println("success")
        return
}

(~/work/achiku/gosec-issue)
❯❯❯ ll
total 8
-rw-r--r--  1 chiku  staff  268 10 18 11:32 main.go
-rw-r--r--  1 chiku  staff    0 10 18 11:31 testfile.txt
(~/work/achiku/gosec-issue)
❯❯❯ go run main.go
2021/10/18 11:32:22 success
(~/work/achiku/gosec-issue)
❯❯❯ gosec .
[gosec] 2021/10/18 11:32:27 Including rules: default
[gosec] 2021/10/18 11:32:27 Excluding rules: default
[gosec] 2021/10/18 11:32:27 Import directory: /Users/chiku/work/achiku/gosec-issue
[gosec] 2021/10/18 11:32:28 Checking package: main
[gosec] 2021/10/18 11:32:28 Checking file: /Users/chiku/work/achiku/gosec-issue/main.go
Results:


[/Users/chiku/work/achiku/gosec-issue/main.go:14-18] - G307 (CWE-703): Deferring unsafe method "Close" on type "*os.File" (Confidence: HIGH, Severity: MEDIUM)
    13:         }
  > 14:         defer func() {
  > 15:                 if err := f.Close(); err != nil {
  > 16:                         log.Fatal("failed to close file")
  > 17:                 }
  > 18:         }()
    19:         log.Println("success")



Summary:
  Gosec  : 2.9.1
  Files  : 1
  Lines  : 21
  Nosec  : 0
  Issues : 1

with v2.8.1

(~/work/achiku/gosec-issue)
❯❯❯ curl -sfL https://raw.githubusercontent.com/securego/gosec/master/install.sh | sh -s -- -b $GOPATH/bin v2.8.1
securego/gosec info checking GitHub for tag 'v2.8.1'
securego/gosec info found version: 2.8.1 for v2.8.1/darwin/amd64
securego/gosec info installed /Users/chiku/sdk/go1.16.7/bin/gosec
(~/work/achiku/gosec-issue)
❯❯❯ gosec .
[gosec] 2021/10/18 11:32:43 Including rules: default
[gosec] 2021/10/18 11:32:43 Excluding rules: default
[gosec] 2021/10/18 11:32:43 Import directory: /Users/chiku/work/achiku/gosec-issue
[gosec] 2021/10/18 11:32:44 Checking package: main
[gosec] 2021/10/18 11:32:44 Checking file: /Users/chiku/work/achiku/gosec-issue/main.go
Results:


Summary:
  Gosec  : 2.8.1
  Files  : 1
  Lines  : 21
  Nosec  : 0
  Issues : 0

gosec version

• v2.9.1
• https://github.com/securego/gosec/releases/tag/v2.9.1

Go version (output of 'go version')

❯❯❯ go version
go version go1.16.7 darwin/amd64

Operating system / Environment

❯❯❯ uname -a
Darwin FVFZR19DLYWP 20.5.0 Darwin Kernel Version 20.5.0: Sat May  8 05:10:33 PDT 2021; root:xnu-7195.121.3~9/RELEASE_X86_64 x86_64

Expected behavior

No errors, or solve this error.

Actual behavior

gosec detects G307 (CWE-703).

[koddr]

Yes, after updating to gosec latest (v2.9.1), I always receive these issues for code with defer:

Results:


[/Users/koddr/Code/Project/api/platform/cdn/digitalocean_spaces.go:67-72] - G307 (CWE-703): Deferring unsafe method "Close" on type "*os.File" (Confidence: HIGH, Severity: MEDIUM)
    66:         }
  > 67:         defer func() {
  > 68:                 if errClose := file.Close(); errClose != nil {
  > 69:                         log.Fatal(errClose.Error())
  > 70:                         return
  > 71:                 }
  > 72:         }()
    73: 


Summary:
  Gosec  : 2.9.1
  Files  : 29
  Lines  : 2756
  Nosec  : 0
  Issues : 1

Out of the $ gosec -version:

Version: 2.9.1
Git tag: v2.9.1
Build date: 2021-10-15T09:00:44Z

Flag -exclude=G307 solve this problem, but it's so strange. Can you fix this or push us a better solution?

My system: macOS 11.6
Golang: 1.17.2

[xxgreg]

This is how I typically open and close files. It gets flagged even though I thought it was idiomatic. However I am new to Go, so I'm open to being told this is poor coding style.

        f, err := os.Create("foo.txt")
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }
        defer f.Close()

        _, err = f.Write([]byte("foo"))
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }

        err = f.Close()
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }

The defer f.Close() is used to close the file even if there is a panic before directly calling f.Close(). It is apparently safe to call Close() twice. If the code is already panicking then the close error is ignored.

More details:

$ go version
go version go1.17 darwin/amd64

$ go install github.com/securego/gosec/v2/cmd/gosec@latest
go: downloading github.com/securego/gosec v0.0.0-20200401082031-e946c8c39989
go: downloading github.com/securego/gosec/v2 v2.9.1
go: downloading github.com/gookit/color v1.4.2
go: downloading gopkg.in/yaml.v2 v2.4.0
go: downloading golang.org/x/tools v0.1.7
go: downloading github.com/google/uuid v1.3.0
go: downloading github.com/nbutton23/zxcvbn-go v0.0.0-20210217022336-fa2cb2858354
go: downloading github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778

Example source:

package main

import (
        "fmt"
        "os"
)

func main() {
        err := foo()
        fmt.Println(err)
}

func foo() error {
        f, err := os.Create("foo.txt")
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }
        defer f.Close()

        _, err = f.Write([]byte("foo"))
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }

        err = f.Close()
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }

        return nil
}

Result:

[/Users/greg/go/src/foo/main.go:18] - G307 (CWE-703): Deferring unsafe method "Close" on type "*os.File" (Confidence: HIGH, Severity: MEDIUM)
    17: 	}
  > 18: 	defer f.Close()
    19: 

[jsirianni]

I am seeing this as well. I would expect a defer func() to pass gosec without issue, because I am checking the error. This was working before, not sure what version.

G307 (CWE-703): Deferring unsafe method "Close" on type "*os.File" (Confidence: HIGH, Severity: MEDIUM)
    191: 			}
  > 192: 			defer func() {
  > 193: 				if err := f.Close(); err != nil {
  > 194: 					logger.Errorw("Failed to close file", zap.Error(err))
  > 195: 				}
  > 196: 			}()

[Aisuko]

We hit the same issue, we use the Github Action with the @master version. And I belive we can ignore this check first by add /* #nosec G307 */ (only in the situation below, you already check the error return by file close function)

// Github Action version of Gosec
 - name: Run Gosec Security Scanner
        uses: securego/gosec@master
        with:
          args: ./... -exclude=G301,G304,G107,G101,G110

The error

[/github/workspace/linkerd/install.go:274-278] - G307 (CWE-703): Deferring unsafe method "Close" on type "*os.File" (Confidence: HIGH, Severity: MEDIUM)
42
    273: 	}
43
  > 274: 	defer func() {
44
  > 275: 		if err := out.Close(); err != nil {
45
  > 276: 			fmt.Println(err)
46
  > 277: 		}
47
  > 278: 	}()
48
    279: 
49

[lootek]

This is not necessarily a false positive: https://www.joeshaw.org/dont-defer-close-on-writable-files/

Though gosec does not accept even the named parameters workaround so this issue here should be addressed anyway

[xxgreg]

The double defer pattern is mentioned in the update at the end of the post. That's actually the article I learnt about the pattern from. It is safe when close is idempotent. When code has multiple returns it is easy to miss adding a close. Also if you've already found an error, then call close, and get another error, what should you do? Return both errors? Most code I've seen ignores the close error in this scenario, and returns the first error.

…

On Tue, 16 Nov 2021, 05:54 Piotr Lucinski, ***@***.***> wrote: This is not necessarily a *false* positive: https://www.joeshaw.org/dont-defer-close-on-writable-files/ — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#714 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAFQFESI4SGMPLW3XWXU3UME3KTANCNFSM5GFRNOQQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[lootek]

Well, but calling just defer f.Close() where you don't even log the possible error (neither skip it explicitly with _ = f.Close() is IMO not a good practice as you'll never know if sth went wrong or not.

In that article, it's stated it's unclear/not stated in docs whether you actually should safely close the resource multiple times (and even if it's working now, I wouldn't rely on that).

Also if you've already found an error, then call close, and get another
error, what should you do? Return both errors? Most code I've seen ignores
the close error in this scenario, and returns the first error.

I'd either combine/wrap them both or return only the first one but log any other at the same time.

Anyway, I'd say this is not so good for the gosec to be that radical as people are now massively skipping the checks with /* #nosec */ (not even specifying the check number as I took a brief look at the linked PRs) and that's for sure not what should happen.
I strongly vote for reverting the changeset to make it work the way it did before - claiming about deferred actions with no error checks/skipped error checks and the simple error handling (like eg. logging) being enough to "fix"/shut up the validation rule.

[xxgreg]

@lootek It is stated in the docs. In fact the docs were specifically updated because of this pattern. See: golang/go#32427

// Close closes the File, rendering it unusable for I/O.
// On files that support SetDeadline, any pending I/O operations will
// be canceled and return immediately with an error.
// Close will return an error if it has already been called.
func (f *File) Close() error {

[lootek]

thanks @xxgreg , didn't notice that! Then the double-close pattern is even worse ;)

[danieljmt]

@ccojocar @elgohr do you have any updated guidance on this?

[elgohr]

I guess I already mentioned this #661 (comment)

[danieljmt]

@elgohr so essentially we always have to deal with a false positive, even when we're just reading a file or have handled syncing the file as suggested?

[elgohr]

My intention was to discuss this in the PR, before it got merged to avoid this false positive... Now it's merged - I have no idea

[elgohr]

To my point of view a solution would be to revert the change and start again with the initial PR (including the failing false-positive-test)

[danieljmt]

Yeah that sounds like it'd be reasonable to me! 😄

[ccojocar]

To my point of view a solution would be to revert the change and start again with the initial PR (including the failing false-positive-test)

@elgohr Please feel free to revert the pull request and to provide an updated solution. Thanks

[elgohr]

@ccojocar please see #733
Do you want to reopen the old PR?

[ccojocar]

@elgohr I merged your pull request.

Do you want to reopen the old PR?

You can reopen it if you want to rework it.

[danieljmt]

@ccojocar Please could we get a new release now the PR has been reverted?

[ccojocar]

@danieljmt new version released https://github.com/securego/gosec/releases/tag/v2.9.3