From 2bd0ffdde26b041b7375fc690503c7b1322eb4a7 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Mon, 2 Dec 2024 10:02:32 +0100 Subject: [PATCH] gosec: reorganize filepath validation We recently added filepath sanitization before to feed it in the `os.ReadFile`. Turns out the linter was still unhappy because it seems it can't track the variable sanitization across function calls bonundaries. With this change we rework the validation to have them in the same function block of the `ReadFile` call. This is arguably (much) more wasteful (in relative terms) because we redo multiple times redundant validations, for example when we handle the config file directory. Still, this makes the linter demonstrably happier and it's (hopefully) safer because we can't call ReadFile by mistake with unsanitized input. In addition, configfile reading is done once at runtime, so the extra cost should be amortized. X-Gosec-Scan: 2.21.4 Signed-off-by: Francesco Romani --- pkg/config/cfgfile.go | 26 ++++++++++---------------- pkg/config/flags.go | 14 +++----------- pkg/config/validation.go | 22 +++++++++------------- 3 files changed, 22 insertions(+), 40 deletions(-) diff --git a/pkg/config/cfgfile.go b/pkg/config/cfgfile.go index dfafc6bc..9a0bdbdf 100644 --- a/pkg/config/cfgfile.go +++ b/pkg/config/cfgfile.go @@ -44,19 +44,17 @@ func FixExtraConfigPath(configRoot string) string { } func FromFiles(pArgs *ProgArgs, configRoot, extraConfigPath string) error { - cfgRoot, err := validateConfigRootPath(configRoot) - if err != nil { - return err + if configRoot == "" { + return errors.New("configRoot is not allowed to be an empty string") } - extraCfgPath, err := validateConfigRootPath(extraConfigPath) - if err != nil { - return err + if extraConfigPath == "" { + return errors.New("extraConfigPath is not allowed to be an empty string") } - err = fromDaemonFiles(pArgs, cfgRoot) + err := fromDaemonFiles(pArgs, configRoot) if err != nil && !errors.Is(err, os.ErrNotExist) { return err } - return fromExtraFile(pArgs, extraCfgPath) + return fromExtraFile(pArgs, extraConfigPath) } func fromExtraFile(pArgs *ProgArgs, extraConfigPath string) error { @@ -108,11 +106,7 @@ func fromDaemonFiles(pArgs *ProgArgs, configPathRoot string) error { klog.Infof("configlet %q not regular file: ignored", configlet.Name()) continue } - configletPath, err := validateConfigPath(filepath.Join(configletDir, configlet.Name())) - if err != nil { - klog.Infof("could not load %q: %v", configlet.Name(), err) - continue - } + configletPath := filepath.Join(configletDir, configlet.Name()) if pArgs.Global.Debug { klog.Infof("loading configlet: %q", configletPath) } @@ -130,11 +124,11 @@ func fromDaemonFiles(pArgs *ProgArgs, configPathRoot string) error { } func loadConfiglet(confObj map[string]interface{}, configPath string) error { - obj := make(map[string]interface{}) - data, err := os.ReadFile(configPath) + data, err := ReadConfiglet(configPath) if err != nil { return err } + obj := make(map[string]interface{}) err = yaml.Unmarshal(data, &obj) if err != nil { return err @@ -191,7 +185,7 @@ type config struct { func readExtraConfig(configPath string) (config, error) { conf := config{} - data, err := os.ReadFile(configPath) + data, err := ReadConfiglet(configPath) if err != nil { // config is optional if errors.Is(err, os.ErrNotExist) { diff --git a/pkg/config/flags.go b/pkg/config/flags.go index 3c311e6f..8acdfed5 100644 --- a/pkg/config/flags.go +++ b/pkg/config/flags.go @@ -108,20 +108,12 @@ Special targets: } params := flags.Args() - extraConfigPath, err := validateConfigRootPath(configPath) - if err != nil { - return DefaultConfigRoot, LegacyExtraConfigPath, err - } - if len(params) > 1 { - return DefaultConfigRoot, extraConfigPath, fmt.Errorf("too many config roots given (%d), currently supported up to 1", len(params)) + return DefaultConfigRoot, configPath, fmt.Errorf("too many config roots given (%d), currently supported up to 1", len(params)) } if len(params) == 0 { - return DefaultConfigRoot, extraConfigPath, nil - } - configRoot, err := validateConfigRootPath(params[0]) - if err != nil { - return DefaultConfigRoot, LegacyExtraConfigPath, err + return DefaultConfigRoot, configPath, nil } + configRoot := params[0] return configRoot, FixExtraConfigPath(configRoot), nil } diff --git a/pkg/config/validation.go b/pkg/config/validation.go index 65340da6..632cb993 100644 --- a/pkg/config/validation.go +++ b/pkg/config/validation.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "io/fs" + "os" "path/filepath" "strings" @@ -43,35 +44,30 @@ func Validate(pArgs *ProgArgs) error { return nil } -func validateConfigPath(configletPath string) (string, error) { - configRoot := filepath.Clean(configletPath) +func ReadConfiglet(configPath string) ([]byte, error) { + // to make gosec happy, the validation must be in the same function on which we call `os.ReadFile`. + // IOW, the linter canno track variable sanitization across functions. + configRoot := filepath.Clean(configPath) cfgRoot, err := filepath.EvalSymlinks(configRoot) if err != nil { if errors.Is(err, fs.ErrNotExist) { // reset to original value, it somehow passed the symlink check cfgRoot = configRoot } else { - return "", fmt.Errorf("failed to validate configRoot path: %w", err) + return nil, fmt.Errorf("failed to validate config path: %w", err) } } // else either success or checking a non-existing path. Which can be still OK. pattern, err := IsConfigRootAllowed(cfgRoot, UserRunDir, UserHomeDir) if err != nil { - return "", err + return nil, err } if pattern == "" { - return "", fmt.Errorf("failed to validate configRoot path %q: does not match any allowed pattern", cfgRoot) + return nil, fmt.Errorf("failed to validate configRoot path %q: does not match any allowed pattern", cfgRoot) } - return cfgRoot, nil -} - -func validateConfigRootPath(configRoot string) (string, error) { - if configRoot == "" { - return "", fmt.Errorf("configRoot is not allowed to be an empty string") - } - return validateConfigPath(configRoot) + return os.ReadFile(cfgRoot) } // IsConfigRootAllowed checks if an *already cleaned and canonicalized* path is among the allowed list.