Linter: gosec, Rule: G204 - Audit use of command execution. Should we enable it?

[zak-pawel]

Use Case

This issue starts discussion about enabling:

• linter: gosec - Inspects source code for security problems by scanning the Go AST.
• rule: G204 - Audit use of command execution

Rule is mapped to CWE-78: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

Expected behavior

Decision if rule should be enabled or not.

Actual behavior

For this rule following findings were found in current code:

internal/process/process.go:91:10                        gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/amd_rocm_smi/amd_rocm_smi.go:61:9         gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/ceph/ceph.go:296:9                        gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/chrony/chrony_test.go:63:9                gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/exec/exec.go:75:9                         gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/fail2ban/fail2ban_test.go:91:9            gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/intel_rdt/intel_rdt.go:241:9              gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/ipmi_sensor/ipmi_sensor_test.go:222:9     gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/ipmi_sensor/ipmi_sensor_test.go:545:9     gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/lvm/lvm_test.go:68:9                      gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/lvm/lvm_test.go:155:9                     gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/nvidia_smi/nvidia_smi.go:74:45            gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/processes/processes_notwindows.go:226:14  gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/procstat/procstat_test.go:25:9            gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/sensors/sensors_test.go:298:9             gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/slab/slab.go:96:13                        gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/smart/smart.go:529:9                      gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/snmp/snmp_mocks_test.go:19:9              gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/sysstat/sysstat_test.go:269:9             gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/inputs/unbound/unbound.go:77:9                   gosec  G204: Subprocess launched with a potential tainted input or cmd arguments
plugins/outputs/exec/exec.go:92:9                        gosec  G204: Subprocess launched with a potential tainted input or cmd arguments

Additional info

For this rule no additional configuration can be provided.

[powersj]

-1

This one feels like noise. Calling commands is pretty fundamental to telegraf, so these would be ignored. While I see some value in making us be explicit about ignoring them, I don't think it adds enough to justify the noise.

[srebhan]

I think this might be useful as it refers to "you could inject commands" I think. E.g. by passing a config param device; rm -rf / which is then executed by Telegraf (potentially with elevated privileges).

[zak-pawel]

With exclude-use-default: false set in .golangci.yml additional findings were found for this rule:

config/config_test.go:47:9                               gosec  G204: Subprocess launched with variable
internal/internal_test.go:58:9                           gosec  G204: Subprocess launched with variable
internal/internal_test.go:76:9                           gosec  G204: Subprocess launched with variable
internal/internal_test.go:98:9                           gosec  G204: Subprocess launched with variable
internal/internal_test.go:112:9                          gosec  G204: Subprocess launched with variable
internal/internal_test.go:125:9                          gosec  G204: Subprocess launched with variable
internal/internal_test.go:129:10                         gosec  G204: Subprocess launched with variable
internal/internal_test.go:140:9                          gosec  G204: Subprocess launched with variable
plugins/inputs/ceph/ceph.go:155:9                        gosec  G204: Subprocess launched with variable
plugins/inputs/ipset/ipset.go:108:9                      gosec  G204: Subprocess launched with variable
plugins/inputs/iptables/iptables.go:78:7                 gosec  G204: Subprocess launched with variable
plugins/inputs/leofs/leofs_test.go:137:21                gosec  G204: Subprocess launched with variable
plugins/inputs/ntpq/ntpq.go:107:11                       gosec  G204: Subprocess launched with variable
plugins/inputs/passenger/passenger.go:144:14             gosec  G204: Subprocess launched with variable
plugins/inputs/ping/ping.go:328:7                        gosec  G204: Subprocess launched with variable
plugins/inputs/socketstat/socketstat.go:66:9             gosec  G204: Subprocess launched with variable
plugins/inputs/systemd_units/systemd_units.go:205:9      gosec  G204: Subprocess launched with variable

Currently they are excluded by default by:

# EXC0007 gosec: Too many false-positives for parametrized shell calls
- Subprocess launch(ed with variable|ing should be audited)

[zak-pawel]

We can enable this one (with findings in tests filtered out) if we find a generic way to sanitize input.
Otherwise, it doesn't make sense to enable it (probably that's why this rule was partially excluded by default).

I haven't found any good solution. @powersj @srebhan have you?

[powersj]

This comment was interesting, stating this rule was a way of auditing code.

I'm still -1 one this one :)