search

influxdata / telegraf

Agent for collecting, processing, aggregating, and writing metrics, logs, and other arbitrary data.
https://influxdata.com/telegraf
MIT License
14.6k stars 5.56k forks source link

Linter: gosec, Rule: G304 - File path provided as taint input. Should we enable it? #12943

Closed zak-pawel closed 1 year ago

zak-pawel commented 1 year ago

Use Case

This issue starts discussion about enabling:

Rule is mapped to CWE-22: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

Expected behavior

Decision if rule should be enabled or not.

Actual behavior

For this rule no findings were found in current code.

Additional info

For this rule no additional configuration can be provided.

zak-pawel commented 1 year ago

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

config/config.go:714:17                                                           gosec  G304: Potential file inclusion via variable
internal/internal.go:75:12                                                        gosec  G304: Potential file inclusion via variable
internal/rotate/file_writer.go:63:9                                               gosec  G304: Potential file inclusion via variable
internal/rotate/file_writer_test.go:119:19                                        gosec  G304: Potential file inclusion via variable
plugins/common/opcua/opcua_util.go:93:18                                          gosec  G304: Potential file inclusion via variable
plugins/common/opcua/opcua_util.go:104:17                                         gosec  G304: Potential file inclusion via variable
plugins/common/tls/config.go:209:15                                               gosec  G304: Potential file inclusion via variable
plugins/inputs/bcache/bcache.go:70:15                                             gosec  G304: Potential file inclusion via variable
plugins/inputs/bcache/bcache.go:82:16                                             gosec  G304: Potential file inclusion via variable
plugins/inputs/beat/beat_test.go:34:16                                            gosec  G304: Potential file inclusion via variable
plugins/inputs/beat/beat_test.go:178:16                                           gosec  G304: Potential file inclusion via variable
plugins/inputs/bond/bond.go:56:16                                                 gosec  G304: Potential file inclusion via variable
plugins/inputs/bond/bond.go:154:15                                                gosec  G304: Potential file inclusion via variable
plugins/inputs/bond/bond.go:159:14                                                gosec  G304: Potential file inclusion via variable
plugins/inputs/bond/bond.go:165:15                                                gosec  G304: Potential file inclusion via variable
plugins/inputs/burrow/burrow_test.go:30:10                                        gosec  G304: Potential file inclusion via variable
plugins/inputs/cloudwatch_metric_streams/cloudwatch_metric_streams_test.go:83:15  gosec  G304: Potential file inclusion via variable
plugins/inputs/conntrack/conntrack.go:90:21                                       gosec  G304: Potential file inclusion via variable
plugins/inputs/couchbase/couchbase_test.go:175:15                                 gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor.go:223:15                      gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor.go:332:20                      gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor.go:337:21                      gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:66:12                  gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:133:12                 gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:200:12                 gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:251:12                 gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:315:12                 gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:386:12                 gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:455:12                 gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:573:12                 gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:583:11                 gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:651:12                 gosec  G304: Potential file inclusion via variable
plugins/inputs/directory_monitor/directory_monitor_test.go:661:11                 gosec  G304: Potential file inclusion via variable
plugins/inputs/diskio/diskio_linux.go:63:12                                       gosec  G304: Potential file inclusion via variable
plugins/inputs/diskio/diskio_linux.go:128:14                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/file/file.go:86:15                                                 gosec  G304: Potential file inclusion via variable
plugins/inputs/filecount/filesystem_helpers.go:32:61                              gosec  G304: Potential file inclusion via variable
plugins/inputs/filestat/filestat.go:121:13                                        gosec  G304: Potential file inclusion via variable
plugins/inputs/gnmi/gnmi_test.go:1043:16                                          gosec  G304: Potential file inclusion via variable
plugins/inputs/google_cloud_storage/google_cloud_storage_test.go:412:15           gosec  G304: Potential file inclusion via variable
plugins/inputs/hugepages/hugepages.go:191:24                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_dlb/ras_reader.go:36:9                                       gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_pmu/intel_pmu.go:37:9                                        gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_powerstat/file.go:108:14                                     gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_powerstat/msr.go:86:22                                       gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_powerstat/msr.go:105:25                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_powerstat/msr.go:145:18                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_powerstat/msr.go:164:18                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_powerstat/rapl.go:58:29                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_powerstat/rapl.go:70:27                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_powerstat/rapl.go:81:32                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_powerstat/rapl.go:92:30                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/intel_powerstat/rapl.go:108:29                                     gosec  G304: Potential file inclusion via variable
plugins/inputs/interrupts/interrupts.go:111:12                                    gosec  G304: Potential file inclusion via variable
plugins/inputs/linux_cpu/linux_cpu.go:184:12                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/linux_cpu/linux_cpu.go:199:12                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/linux_sysctl_fs/linux_sysctl_fs.go:25:13                           gosec  G304: Potential file inclusion via variable
plugins/inputs/linux_sysctl_fs/linux_sysctl_fs.go:54:13                           gosec  G304: Potential file inclusion via variable
plugins/inputs/logparser/logparser_test.go:141:16                                 gosec  G304: Potential file inclusion via variable
plugins/inputs/lustre2/lustre2.go:385:21                                          gosec  G304: Potential file inclusion via variable
plugins/inputs/mdstat/mdstat.go:287:15                                            gosec  G304: Potential file inclusion via variable
plugins/inputs/netflow/netflow_test.go:156:15                                     gosec  G304: Potential file inclusion via variable
plugins/inputs/ntpq/ntpq_test.go:145:14                                           gosec  G304: Potential file inclusion via variable
plugins/inputs/postgresql_extensible/postgresql_extensible.go:72:15               gosec  G304: Potential file inclusion via variable
plugins/inputs/processes/processes_notwindows.go:196:15                           gosec  G304: Potential file inclusion via variable
plugins/inputs/procstat/native_finder.go:46:20                                    gosec  G304: Potential file inclusion via variable
plugins/inputs/procstat/pgrep.go:28:20                                            gosec  G304: Potential file inclusion via variable
plugins/inputs/procstat/procstat.go:473:14                                        gosec  G304: Potential file inclusion via variable
plugins/inputs/rabbitmq/rabbitmq_test.go:40:16                                    gosec  G304: Potential file inclusion via variable
plugins/inputs/rabbitmq/rabbitmq_test.go:251:16                                   gosec  G304: Potential file inclusion via variable
plugins/inputs/ravendb/ravendb_test.go:33:16                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/ravendb/ravendb_test.go:228:16                                     gosec  G304: Potential file inclusion via variable
plugins/inputs/socket_listener/socket_listener_test.go:312:18                     gosec  G304: Potential file inclusion via variable
plugins/inputs/socket_listener/socket_listener_test.go:330:20                     gosec  G304: Potential file inclusion via variable
plugins/inputs/solr/solr_test.go:194:15                                           gosec  G304: Potential file inclusion via variable
plugins/inputs/syslog/rfc5426_test.go:308:12                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/syslog/rfc5426_test.go:321:12                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/webhooks/filestack/filestack_webhooks_test.go:81:15                gosec  G304: Potential file inclusion via variable
plugins/inputs/webhooks/mandrill/mandrill_webhooks_test.go:92:15                  gosec  G304: Potential file inclusion via variable
plugins/inputs/wireless/wireless_linux.go:47:16                                   gosec  G304: Potential file inclusion via variable
plugins/inputs/x509_cert/x509_cert_test.go:509:12                                 gosec  G304: Potential file inclusion via variable
plugins/inputs/x509_cert/x509_cert_test.go:562:11                                 gosec  G304: Potential file inclusion via variable
plugins/inputs/xtremio/xtremio_test.go:87:36                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/xtremio/xtremio_test.go:93:35                                      gosec  G304: Potential file inclusion via variable
plugins/inputs/zipkin/cmd/thrift_serialize/thrift_serialize.go:54:19              gosec  G304: Potential file inclusion via variable
plugins/inputs/zipkin/zipkin_test.go:643:14                                       gosec  G304: Potential file inclusion via variable
plugins/outputs/file/file_test.go:194:14                                          gosec  G304: Potential file inclusion via variable
plugins/parsers/collectd/parser.go:194:17                                         gosec  G304: Potential file inclusion via variable
plugins/parsers/grok/parser.go:175:20                                             gosec  G304: Potential file inclusion via variable
plugins/parsers/xpath/parser_test.go:1344:20                                      gosec  G304: Potential file inclusion via variable
plugins/parsers/xpath/parser_test.go:1481:14                                      gosec  G304: Potential file inclusion via variable
plugins/processors/lookup/lookup.go:88:15                                         gosec  G304: Potential file inclusion via variable
plugins/processors/lookup/lookup.go:117:12                                        gosec  G304: Potential file inclusion via variable
plugins/processors/lookup/lookup.go:165:12                                        gosec  G304: Potential file inclusion via variable
plugins/processors/starlark/starlark_test.go:3277:15                              gosec  G304: Potential file inclusion via variable
plugins/serializers/csv/csv_test.go:168:14                                        gosec  G304: Potential file inclusion via variable
plugins/serializers/csv/csv_test.go:186:9                                         gosec  G304: Potential file inclusion via variable
plugins/serializers/json/json_test.go:466:14                                      gosec  G304: Potential file inclusion via variable
plugins/serializers/json/json_test.go:484:14                                      gosec  G304: Potential file inclusion via variable
testutil/file.go:92:12                                                            gosec  G304: Potential file inclusion via variable
testutil/file.go:121:12                                                           gosec  G304: Potential file inclusion via variable
testutil/tls.go:108:15                                                            gosec  G304: Potential file inclusion via variable
tools/custom_builder/packages.go:340:16                                           gosec  G304: Potential file inclusion via variable
tools/license_checker/main.go:80:17                                               gosec  G304: Potential file inclusion via variable
tools/license_checker/main.go:96:27                                               gosec  G304: Potential file inclusion via variable
tools/license_checker/whitelist.go:25:15                                          gosec  G304: Potential file inclusion via variable
tools/package_lxd_test/lxd.go:154:12                                              gosec  G304: Potential file inclusion via variable
tools/readme_config_includer/generator.go:80:15                                   gosec  G304: Potential file inclusion via variable
tools/readme_config_includer/generator.go:254:15                                  gosec  G304: Potential file inclusion via variable
tools/readme_linter/main.go:89:13                                                 gosec  G304: Potential file inclusion via variable

Currently they are excluded by default by:

# EXC0010 gosec: False positive is triggered by 'src, err := ioutil.ReadFile(filename)'
- Potential file inclusion via variable
powersj commented 1 year ago

-1 per discussion in our call

zak-pawel commented 1 year ago

@powersj It seems that there is way to fix that using filepath.Clean. Please, see: https://securego.io/docs/rules/g304.html and https://www.geeksforgeeks.org/filepath-clean-function-in-golang-with-examples/

Although, even after using filepath.Clean everywhere, every case should be deeply analyzed to understand how plugin should behave with cleaned path.

powersj commented 1 year ago

As telegraf can and should read arbitrary files on the filesystem what would "safe path" ever get set to? :) I'm still thinking -1 on this.

srebhan commented 1 year ago

I agree that there is no safe setting to choose. The best practice is to use a separate user and let the OS to the heavy lifting. So do not enable this.