Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression of '#nosec' handling #1046

Closed
skabashnyuk opened this issue Oct 15, 2023 · 4 comments · Fixed by #1050
Closed

Regression of '#nosec' handling #1046

skabashnyuk opened this issue Oct 15, 2023 · 4 comments · Fixed by #1050
Labels

Comments

@skabashnyuk
Copy link

skabashnyuk commented Oct 15, 2023

Summary

//#nosec G101 does not work in =v2.18.1

Steps to reproduce the behavior

Create new project

$ mkdir gosec-check && cd gosec-check
$ touch main.go
$ touch labels.go
$ touch go.mod

main.go

package main

import (
	"fmt"
)

func main() {
	fmt.Printf("Label: %s ", TokenLabel)
}

labels.go

package main

const TokenLabel = "test/linked-access-token" //#nosec G101 -- false positive, this is not a private data

go.mod

module example.com/m

go 1.21.3

gosec version

  • 2.18.1 -github action

Go version (output of 'go version')

  • from image

Operating system / Environment

  • Github actions/docker image securego/gosec:2.18.1

Expected behavior

Line with #nosec G101 is not reported

Actual behavior

Line is reported as issue

@skabashnyuk
Copy link
Author

This could be related to #1043

@ccojocar ccojocar added the bug label Oct 16, 2023
@dataclouder
Copy link

Same error for me.
This code was skipped as expected up to version 2.17. It started failing with 2.18.1

	// Setting defaults
	// #nosec G402 -- InsecureSkipVerify: insecure - This allows connecting to VCDs with self-signed certificates
	vcdClient := &VCDClient{
		Client: Client{
			APIVersion: minVcdApiVersion,
			// UserAgent cannot embed exact version by default because this is source code and is supposed to be used by programs,
			// but any client can customize or disable it at all using WithHttpUserAgent() configuration options function.
			UserAgent: "go-vcloud-director",
			VCDHREF:   vcdEndpoint,
			Http: http.Client{
				Transport: &http.Transport{
					TLSClientConfig: &tls.Config{
						InsecureSkipVerify: insecure, // #nosec G402 -- InsecureSkipVerify: insecure - This allows connecting to VCDs with self-signed certificates
					},
					Proxy:               http.ProxyFromEnvironment,
					TLSHandshakeTimeout: 120 * time.Second, // Default timeout for TSL hand shake
				},
				Timeout: 600 * time.Second, // Default value for http request+response timeout
			},
			MaxRetryTimeout: 60, // Default timeout in seconds for retries calls in functions
		},
	}

Here's a sample:

[/home/runner/work/go-vcloud-director/go-vcloud-director/govcd/api_vcd.go:146] - G402 (CWE-295): TLS InsecureSkipVerify set true. (Confidence: HIGH, Severity: HIGH)
    145: 					TLSClientConfig: &tls.Config{
  > 146: 						InsecureSkipVerify: insecure,
    147: 					},

Adding the #nosec directive on line 146 itself doesn't help.

@tbailey
Copy link

tbailey commented Oct 16, 2023

@dataclouder I was able to work around this issue by breaking the setting out from the inline struct:

	tlsConfig := &tls.Config{
		MinVersion: tls.VersionTLS12,
	}
	tlsConfig.InsecureSkipVerify = cfg.Insecure // #nosec G402

@dataclouder
Copy link

dataclouder commented Oct 16, 2023

Thanks @tbailey
That would work. However, since we're dealing with a regression (and we have several places where a similar directive was ignored), I changed my CI workflow to use a specific gosec version until this bug is fixed.

thaJeztah added a commit to thaJeztah/cli that referenced this issue Oct 23, 2023
Latest gosec linter has a regression in parsing "nosec" comments;
see securego/gosec#1046

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this issue Oct 24, 2023
Latest gosec linter has a regression in parsing "nosec" comments;
see securego/gosec#1046

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants