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

nosec statement ignored when additionally defined in front of function, const or variable segment #1105

Open
joernlenoch opened this issue Feb 14, 2024 · 4 comments

Comments

@joernlenoch
Copy link

joernlenoch commented Feb 14, 2024

Summary

Adding a #nosec marker in front of this const or var segment causes the parser to ignore the actual nosec statements.

Steps to reproduce the behavior

// #nosec G101
const (
    ConfigLearnerTokenAuth string = "learner_auth_token_config" // #nosec G101
)

will create this error message.

Results:

[...:9] - G101 (CWE-798): Potential hardcoded credentials (Confidence: LOW, Severity: HIGH)
    8: const (
  > 9:  ConfigLearnerTokenAuthTemp string = "learner_auth_token_config" // #nosec G101
    10: )

When removed, no error is shown.

const (
	ConfigLearnerTokenAuthTemp string = "learner_auth_token_config" // #nosec G101
)
Results:


Summary: [...]

gosec version

2.19.0 (latest on 14th February 2024)

Works fine with 2.18.

Go version (output of 'go version')

go version go1.21.4 windows/amd64

Operating system / Environment

Windows 11

Expected behavior

Should not show the ignored security errors.

Actual behavior

Ignores the #nosec statements and displays an error.

@joernlenoch joernlenoch changed the title nosec statement ignored when additionally defined in front of const or variable segment nosec statement ignored when additionally defined in front of function, const or variable segment Feb 14, 2024
@joernlenoch
Copy link
Author

joernlenoch commented Feb 14, 2024

Same issue is true for functions

// RandomizeRuntime ...
// #nosec G404
func (p *passwordResetter) RandomizeRuntime() {
	sleepTime := rand.Intn(1000) + 1000 // #nosec G404
	time.Sleep(time.Duration(sleepTime) * time.Millisecond)
}

will cause the same behaviour. Removing the first nosec-marker will solve this issue as well..

@rd-bruno-lueders
Copy link

rd-bruno-lueders commented Feb 15, 2024

It seems like block-wide nosec statements are broken in general. Example:

//#nosec G404
fmt.Printf("%d\n",
    rand.Int())

In v2.18.2, the nosec directive suppresses the G404 warning, but in v2.19.0, that doesn't work anymore. To make it work in v2.19.0, I have to move the nosec right next to the rand.Int call:

fmt.Printf("%d\n",
    //#nosec G404
    rand.Int())

Interestingly, there has to be a line-feed in the Printf call. This works in both versions:

//#nosec G404
fmt.Printf("%d\n", rand.Int())

@ccojocar
Copy link
Member

The nonsec directive was refactored to be more fined-grained instead of ignoring an entire AST node.

@MaehlerChristopher
Copy link

MaehlerChristopher commented Feb 16, 2024

Unfortunately, the use of #nosec is no longer predictable in the current version (2.19.0). Multiple tests show a different number of issues with the same code. With version 2.18.2, the problem does not occur at all.

example Code:

...
                _, _, err := procPathNameVolume.Call(
                        uintptr(unsafe.Pointer(volumeName)),     //#nosec
                        uintptr(unsafe.Pointer(&nameBuffer[0])), //#nosec
                        uintptr(nameSize),
                        uintptr(unsafe.Pointer(&retSize))) //#nosec
...

first run with 2.19.0:

utils.go:84] - G103 (CWE-242): Use of unsafe calls should be audited (Confidence: HIGH, Severity: LOW)
    83:                         uintptr(nameSize),
  > 84:                         uintptr(unsafe.Pointer(&retSize))) //#nosec
    85:                 if err == windows.ERROR_MORE_DATA {



Summary:
  Gosec  : dev
  Files  : 4
  Lines  : 1497
  Nosec  : 4
  Issues : 1

second run with 2.19.0:

utils.go:81] - G103 (CWE-242): Use of unsafe calls should be audited (Confidence: HIGH, Severity: LOW)
    80:                 _, _, err := procPathNameVolume.Call(
  > 81:                         uintptr(unsafe.Pointer(volumeName)),     //#nosec
    82:                         uintptr(unsafe.Pointer(&nameBuffer[0])), //#nosec



Summary:
  Gosec  : dev
  Files  : 4
  Lines  : 1497
  Nosec  : 4
  Issues : 3

third run with 2.19.0:

utils.go:81] - G103 (CWE-242): Use of unsafe calls should be audited (Confidence: HIGH, Severity: LOW)
    80:                 _, _, err := procPathNameVolume.Call(
  > 81:                         uintptr(unsafe.Pointer(volumeName)),     //#nosec
    82:                         uintptr(unsafe.Pointer(&nameBuffer[0])), //#nosec



Summary:
  Gosec  : dev
  Files  : 4
  Lines  : 1497
  Nosec  : 4
  Issues : 2

run with 2.18.0:

Summary:
  Gosec  : dev
  Files  : 4
  Lines  : 1497
  Nosec  : 4
  Issues : 0

GabrielNagy added a commit to ubuntu/adsys that referenced this issue Feb 21, 2024
The easiest and safest approach to satisfy the revive linter, as these
functions must have this exact signature. The occurrences were mostly
split between Cobra and tests, hence the separate commits for easier
review.

Fixes UDENG-2287

Note that the latest version of golangci-lint (currently 1.56.2) pulls
in a version of gosec where the `#nosec` directive is sort of
[broken](securego/gosec#1105), hence the
additional influx of warnings in the [dependabot
PR](#915).
GabrielNagy added a commit to ubuntu/adsys that referenced this issue Mar 20, 2024
The `nosec` directive [is
broken](securego/gosec#1105) as of gosec 2.19
-- we can work around this by using the `nolint` directive from
golangci-lint. The main drawback is that this disables the entire
linter, not just one rule, but I still kept the rule name for reference.

After this is merged we can bump golangci-lint to ~1.56.2~ 1.57.0.
silvestre added a commit to cloudfoundry/app-autoscaler-release that referenced this issue Mar 28, 2024
Due to securego/gosec#1105 we need to use
`nolint` instead of `nosec` for `gosec`.
silvestre added a commit to cloudfoundry/app-autoscaler-release that referenced this issue Mar 28, 2024
Due to securego/gosec#1105 we need to use
`nolint` instead of `nosec` for `gosec`.
silvestre added a commit to cloudfoundry/app-autoscaler-release that referenced this issue Apr 2, 2024
Due to securego/gosec#1105 we need to use
`nolint` instead of `nosec` for `gosec`.
silvestre added a commit to cloudfoundry/app-autoscaler-release that referenced this issue Apr 2, 2024
Due to securego/gosec#1105 we need to use
`nolint` instead of `nosec` for `gosec`.
app-autoscaler-ci-bot pushed a commit to cloudfoundry/app-autoscaler-release that referenced this issue Apr 3, 2024
* chore(deps): update dependency golangci-lint to v1.57.1

* chore(gosec): use `nolint` instead of `nosec` for `gosec`

Due to securego/gosec#1105 we need to use
`nolint` instead of `nosec` for `gosec`.

* fix deprecation warning

> WARN [config_reader] The configuration option `govet.check-shadowing` is deprecated. Please enable `shadow` instead, if you are not using `enable-all`.

* fix local timeout issue

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Silvestre Zabala <silvestre.zabala@sap.com>
nscuro added a commit to CycloneDX/cyclonedx-gomod that referenced this issue Apr 6, 2024
See securego/gosec#1105

Signed-off-by: nscuro <nscuro@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants