False alarm for G101

[samirsss]

Summary

• G101 (CWE-798): Potential hardcoded credentials (Confidence: LOW, Severity: HIGH) 56: // PropertyNameSnmpGroupWriteView - Property name sent in response if group write view in snmp group is drifted

  57: PropertyNameSnmpGroupWriteView = "Snmp Group Write View" 58: // PropertyNameSnmpGroupNotifyView - Property name sent in response if group notify view in snmp group is drifted

Steps to reproduce the behavior

Just have this as a const in a file

gosec version

Version: 2.12.0 Git tag: v2.12.0 Build date: 2022-06-13T19:36:07Z

Go version (output of 'go version')

go version go version go1.17.2 darwin/amd64

Operating system / Environment

Macos

Expected behavior

Should not be flagged as an issue - since these are not credentials

Actual behavior

[Pai-Po]

gosec use entroy algorithm to verify the password string. You can customize the scanning using the following config: { "G101": { "pattern": "(?i)passwd|pass|password|pwd|secret|private_key|token", "ignore_entropy": false, "entropy_threshold": "80.0", "per_char_threshold": "3.0", "truncate": "32" } }

[TimonOmsk]

The documentation is slightly outdated. The pattern was changed in https://github.com/securego/gosec/pull/666 Now the pattern is: (?i)passwd|pass|password|pwd|secret|token|pw|apiKey|bearer|cred

As you can see the pattern starts with (?i) which forces case ignoring. So, because the var name contains "pw" in the middle PropertyNameSnmpGroupWriteView, gosec treats it as a target for the hardcoded credentials check.

In the SAST false positives is better than missing true issue, but in that case we need to limit somehow the check. I don't know a good solution. Some options:

1. Remove "pw" from the list. The easiest one. We'll remove tons of the false positives, but, at the same time, we can miss some real issues.
2. Make this list configurable. Near the same as the previous one, but all the responsibility will be taken by the end user :)
3. Invent some "smart" checks for variable names. For example, we can split the pattern into "hard" and "soft" triggers. In our case, all items except "pw" goes to "hard" category which unconditionally fires the check. In case the variable's name matches "soft" pattern, we can introduce additional checks like "case sensitivity", "where it is located(in the beginning, at the middle, at the end)", and so on. It will filter some false positives.

As I said, I don't know a good solution. I would prefer a combination of first and second options: remove "pw" from the list and give an opportunity to configure patterns.

[adamdecaf]

We're seeing false positives with the following chunk of names. I'm not sure what part of the regex is triggering secrets. Perhaps "cred" of "credit"? That seems too sensitive.

const (
	TelemetrySpan_create_credit_transfer          = "create-credit-transfer"
	TelemetrySpan_resend_credit_transfer_timer    = "resend-credit-transfer-timer"
	TelemetrySpan_payment_status_report           = "payment-status-report"
	TelemetrySpan_credit_transfer_acknowledgement = "credit-transfer-acknowledgement"
)

[kishorevaishnav]

Yeah, after googling I thought its better to add this comment here

• may be a temporary / permanent (depends on you) solution is to add //#nosec G101 at the end of line & executing gosec will ignore them.
• eg. based on the above code:

const (
	TelemetrySpan_create_credit_transfer          = "create-credit-transfer" 	//#nosec G101
	TelemetrySpan_resend_credit_transfer_timer    = "resend-credit-transfer-timer" 	//#nosec G101
	TelemetrySpan_payment_status_report           = "payment-status-report" 	//#nosec G101
	TelemetrySpan_credit_transfer_acknowledgement = "credit-transfer-acknowledgement" //#nosec G101
)

[adamdecaf]

I would argue that none of the TelemetrySpan_* lines constitute a legitimate finding and are false positives within gosec.

[kishorevaishnav]

I would argue that none of the TelemetrySpan_* lines constitute a legitimate finding and are false positives within gosec.

Absolutely, I can't disagree - I was trying to figure out how to avoid those false +ve's for my use case.

[ccojocar]

You can try to increase the entropy or change the matching pattern via configuration, maybe it helps to rule out these false positives.

The approach used by gosec is not bullet proof anyhow. It tries to do the best effort combining pattern matching with entropy to identify when a detected string has enough randomness.

[ccojocar]

This should be now mitigated by the custom patterns supported by the G101 rule.

[kishorevaishnav]

This should be now mitigated by the custom patterns supported by the G101 rule.

@ccojocar can you point me the docs/commit to understand the custom patterns.

[ccojocar]

It is documented in the README, see the configuration for G101 rule https://github.com/securego/gosec#configuration. That is the pattern for variable name, and now also there is a set with predefined patterns which match against the value https://github.com/securego/gosec/blob/5567ac4cfe9acae14f516b500e2cf229b6a8d16f/rules/hardcoded_credentials.go#L35.