gosec: G104 -> ignoring functions doesn't work for types containing `.`

[zak-pawel]

Welcome

• Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
• Yes, I've searched similar issues on GitHub and didn't find any.
• Yes, I've included all information below (version, config, etc.).
• Yes, I've tried with the standalone linter if available (e.g., gocritic, go vet, etc.). (https://golangci-lint.run/usage/linters/)

Description of the problem

I can ignore fmt.Fscanf but ignoring net.Conn.Write doesn't work.

When I run gosec directly with the following configuration:

{
  "G104": {
    "fmt": ["Fscanf"],
    "net.Conn": ["Write"]
  }
}

I get no issues found:

$ gosec -conf config.json .
[gosec] 2023/03/31 21:36:16 Including rules: default
[gosec] 2023/03/31 21:36:16 Excluding rules: default
[gosec] 2023/03/31 21:36:16 Import directory: /home/pzak/work/gosectest
[gosec] 2023/03/31 21:36:16 Checking package: main
[gosec] 2023/03/31 21:36:16 Checking file: /home/pzak/work/gosectest/main.go
Results:


Summary:
  Gosec  : dev
  Files  : 1
  Lines  : 17
  Nosec  : 0
  Issues : 0

Version of golangci-lint

$ golangci-lint --version
golangci-lint has version v1.52.2 built with go1.20.2 from (unknown, mod sum: "h1:FrPElUUI5rrHXg1mQ7KxI1MXPAw5lBVskiz7U7a8a1A=") on (unknown)

Configuration file

$ cat .golangci.yml
linters:
  disable-all: true
  enable:
    - gosec

linters-settings:
  gosec:
    includes:
      - G104
    config:
      G104:
        fmt:
          - Fscanf
        net.Conn:
          - Write

issues:
  exclude-use-default: false

Go environment

$ go version && go env
go version go1.20.2 linux/amd64
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/pzak/.cache/go-build"
GOENV="/home/pzak/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/pzak/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/pzak/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/pzak/work/gosectest/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build731263030=/tmp/go-build -gno-record-gcc-switches"

Verbose output of running

$ golangci-lint cache clean
$ golangci-lint run -v
INFO [config_reader] Config search paths: [./ /home/pzak/work/gosectest /home/pzak/work /home/pzak /home /] 
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 1 linters: [gosec]        
INFO [loader] Go packages loading at mode 575 (deps|types_sizes|compiled_files|exports_file|files|imports|name) took 52.74387ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 146.301µs 
INFO [linters_context/goanalysis] analyzers took 80.996µs with top 10 stages: gosec: 80.996µs 
INFO [runner] Processors filtering stat (out/in): skip_dirs: 1/1, exclude: 1/1, max_from_linter: 1/1, source_code: 1/1, cgo: 1/1, skip_files: 1/1, severity-rules: 1/1, sort_results: 1/1, uniq_by_line: 1/1, path_shortener: 1/1, identifier_marker: 1/1, exclude-rules: 1/1, diff: 1/1, max_per_file_from_linter: 1/1, fixer: 1/1, path_prefixer: 1/1, filename_unadjuster: 1/1, path_prettifier: 1/1, max_same_issues: 1/1, autogenerated_exclude: 1/1, nolint: 1/1 
INFO [runner] processing took 183.653µs with stages: identifier_marker: 90.848µs, nolint: 37.973µs, skip_dirs: 15.515µs, autogenerated_exclude: 13.277µs, path_prettifier: 10.437µs, source_code: 8.962µs, uniq_by_line: 1.232µs, cgo: 1.022µs, max_same_issues: 852ns, path_shortener: 512ns, filename_unadjuster: 477ns, max_from_linter: 404ns, max_per_file_from_linter: 398ns, skip_files: 347ns, fixer: 342ns, exclude-rules: 249ns, severity-rules: 233ns, diff: 204ns, exclude: 190ns, sort_results: 106ns, path_prefixer: 73ns 
INFO [runner] linters took 34.512091ms with stages: gosec: 34.256374ms 
main.go:16:2: G104: Errors unhandled. (gosec)
        conn.Write([]byte("test"))
        ^
INFO File cache stats: 1 entries of total size 237B 
INFO Memory: 2 samples, avg is 32.4MB, max is 38.4MB 
INFO Execution took 91.933964ms                 

Code example or link to a public repository

package main

import (
	"fmt"
	"net"
	"strings"
)

func main() {
	var number int
	r := strings.NewReader("42")
	fmt.Fscanf(r, "%d", &number)

	var conn net.Conn
	conn, _ = net.Dial("tcp", "127.0.0.1:12345")
	conn.Write([]byte("test"))
}

[boring-cyborg]

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

[ldez]

Hello,

it's a problem related to YAML and Viper: . is used as a separator.

In YAML, to use . inside a key you have to quote it:

linters-settings:
  gosec:
    includes:
      - G104
    config:
      G104:
        fmt:
          - Fscanf
        "net.Conn":
          - Write

But the way that viper read YAML doesn't follow the YAML spec, at the end you have something like that:

{"G104":{"fmt":["Fscanf"],"net":{"conn":["Write"]}},"global":{}}

Unfortunately, this parsing cannot be controlled only for a field in Viper.
spf13/viper#324

[zak-pawel]

I suspect that there might be some problem when configuration is parsed.

Look at rules/errors.go -> NewNoErrorCheck in gosec.
This is for direct gosec run:

This is for golangci-lint run:

net.Conn -> Write is never added to whitelist

[ldez]

Read my message #3749 (comment)

[zak-pawel]

{"G104":{"fmt":["Fscanf"],"net":{"conn":["Write"]}},"global":{}}

Might it be "corrected" in golangci-lint after Viper reads it? Just for this particular case/rule?

[ldez]

Not really because it is not just an "expanded" parsing but there is also a modification of the case.
As you can see in my example Conn has been converted to conn (Upper/Lower case of the C)

Another example to illustrate the problem:

linters:
  disable-all: true
  enable:
    - gosec

linters-settings:
  gosec:
    includes:
      - G104
    config:
      G104:
        fmt:
          - Fscanf
        "net.FooBar":
          - Write

issues:
  exclude-use-default: false

{"G104":{"fmt":["Fscanf"],"net":{"foobar":["Write"]}},"global":{}}

So it's impossible to "correct" that.

[zak-pawel]

AFAIU, changing config type to json or toml will not change anything. I got the same results for:

.golangci.json:

{
  "linters": {
    "disable-all": true,
    "enable": [
      "gosec"
    ]
  },
  "linters-settings": {
    "gosec": {
      "includes": [
        "G104"
      ],
      "config": {
        "G104": {
          "fmt": [
            "Fscanf"
          ],
          "net.Conn": [
            "Write"
          ]
        }
      }
    }
  },
  "issues": {
    "exclude-use-default": false
  }
}

or

.golangci.toml:

[linters]
disable-all = true
enable = [ "gosec" ]

[linters-settings.gosec]
includes = [ "G104" ]

[linters-settings.gosec.config.G104]
fmt = [ "Fscanf" ]
"net.Conn" = [ "Write" ]

[issues]
exclude-use-default = false

[nickajacks1]

@ldez would it be acceptable to change the viper key delimiter in the config to something else, like "::"? If this has happened twice already, I see it happening again with another linter down the road.

[ldez]

@nickajacks1 I already tried that and this creates regressions on other elements, so it's not acceptable.

#3749 (comment)

[nickajacks1]

Do you recall what broke when you changed the delimiter? I don't see viper.Get being used anywhere, and if the delimiter is something obscure enough, we could all but guarantee there is no more errant splitting.