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

v2.16.0 - #nosec and -nosec-tag seems to not work like before #961

Closed
chinchila opened this issue May 23, 2023 · 7 comments · Fixed by #962
Closed

v2.16.0 - #nosec and -nosec-tag seems to not work like before #961

chinchila opened this issue May 23, 2023 · 7 comments · Fixed by #962

Comments

@chinchila
Copy link

chinchila commented May 23, 2023

Hello friends, in the newest versions it seems that the #nohusky does not work when a comment has other strings in it for example, same applies for a custom tag set with -nosec-tag, is this intended behaviour? A version which this works is v2.3.0. Examples of output:

[file1.go:6] - G505 (CWE-327): Blocklisted import crypto/sha1: weak cryptographic primitive (Confidence: HIGH, Severity: MEDIUM)
    4: 	"crypto/hmac"
  > 5: 	"crypto/sha1" // nolint:gosec #nosec G505
    6: 	"encoding/base64"
[file2.go:5] - G501 (CWE-327): Blocklisted import crypto/md5: weak cryptographic primitive (Confidence: HIGH, Severity: MEDIUM)

    3: import (
  > 4: 	"crypto/md5" // #nohusky #nosec
    5: 	b64 "encoding/base64"

the command that I'm running is gosec -nosec-tag nohusky -quiet -log=log.txt ./....

@ccojocar
Copy link
Member

@chinchila Does the #nonsec directive works directly without prepending any other custom directive?

@chinchila
Copy link
Author

chinchila commented May 24, 2023

Yes, if the comment is #nosec only, it works

@ccojocar
Copy link
Member

The --nosec-tag option is to set an alternative directive name instead of nonsec. So basically in the case when you scan using the nohusky nonsec tag instead of default nosec, you need to have something like:

  3: import (
  > 4: 	"crypto/md5" // #nohusky
    5: 	b64 "encoding/base64"

@chinchila
Copy link
Author

chinchila commented May 24, 2023

So I can't comment like #nosec this is not vulnerability because md5 is still secure with HMAC see line #xx in my code, I need to put only #nosec and comment anywhere else? Does this change of behaviour from other previous versions makes sense in a scan that runs on pipelines and precommit hooks? Can we have some coverage tests run on this to not alter how this happens in next releases?

@ccojocar
Copy link
Member

Normally the tags prepended before #nonsec tag are added by other tools which integrated the gosec. Such as nolint:gosec is added by golangci-lint tool. If your security exceptions are managed by gosec directly, there isn't a need to add them.

The #nosec tag should be already covered by unit tests.

@chinchila
Copy link
Author

chinchila commented May 24, 2023

The -nosec-tag argument does not work at all, here is an example with only #nohusky in it:

➜  security git:(master) ✗ gosec -nosec-tag nohusky -quiet -log=log.txt ./...
Results:


[file1:117] - G401 (CWE-326): Use of weak cryptographic primitive (Confidence: HIGH, Severity: MEDIUM)
    116: func getMD5Hash(text string) string {
  > 117: 	hash := md5.Sum([]byte(text)) // #nohusky #nosec
    118: 	return urlsafeB64EncodeToString(hash[:])



[file2:5] - G505 (CWE-327): Blocklisted import crypto/sha1: weak cryptographic primitive (Confidence: HIGH, Severity: MEDIUM)
    4: 	"crypto/hmac"
  > 5: 	"crypto/sha1" // nolint:gosec #nosec G505
    6: 	"encoding/base64"



[file3:4] - G501 (CWE-327): Blocklisted import crypto/md5: weak cryptographic primitive (Confidence: HIGH, Severity: MEDIUM)
    3: import (
  > 4: 	"crypto/md5" // #nohusky
    5: 	b64 "encoding/base64"



Summary:
  Gosec  : dev
  Files  : 16
  Lines  : 1028
  Nosec  : 0
  Issues : 3

It seems that currently gosec is checking only the start of the string, if it starts with #nosec then it will ignore the line, but this does not make sense if this is a tag.

@chinchila
Copy link
Author

I highly appreciate your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants