config: add path validation

add validation and restrictions about configuration paths, to reduce the chance of path traversal vulnerabilities. Most notably: - config root path must be one of the allowed subpaths - configlet are expected not to escape the configuratio directories Signed-off-by: Francesco Romani <[email protected]>
k8stopologyawareschedwg · Nov 28, 2024 · 74312b1 · 74312b1
1 parent 7cbddad
commit 74312b1
Show file tree

Hide file tree

Showing 7 changed files with 209 additions and 12 deletions.
diff --git a/pkg/config/cfgfile.go b/pkg/config/cfgfile.go
@@ -94,17 +94,21 @@ func fromDaemonFiles(pArgs *ProgArgs, configPathRoot string) error {
 
 	// this directory may be missing, that's expected and fine
 	configletDir := filepath.Join(configPathRoot, "daemon", "config.yaml.d")
-	if configLets, err := os.ReadDir(configletDir); err == nil {
-		for _, configLet := range configLets {
-			if !configLet.Type().IsRegular() {
-				klog.Infof("configlet %q not regular file: ignored", configLet.Name())
+	if configlets, err := os.ReadDir(configletDir); err == nil {
+		for _, configlet := range configlets {
+			if !configlet.Type().IsRegular() {
+				klog.Infof("configlet %q not regular file: ignored", configlet.Name())
+				continue
+			}
+			configletPath, err := validateConfigLetPath(configletDir, configlet.Name())
+			if err != nil {
+				klog.Infof("could not load: %v", err)
 				continue
 			}
-			configLetPath := filepath.Join(configletDir, configLet.Name())
 			if pArgs.Global.Debug {
-				klog.Infof("loading configlet: %q", configLetPath)
+				klog.Infof("loading configlet: %q", configletPath)
 			}
-			err = loadConfiglet(confObj, configLetPath)
+			err = loadConfiglet(confObj, configletPath)
 			if err != nil {
 				return err
 			}

diff --git a/pkg/config/cfgfile_test.go b/pkg/config/cfgfile_test.go
@@ -69,6 +69,9 @@ func TestReadResourceExclude(t *testing.T) {
 }
 
 func TestFromFiles(t *testing.T) {
+	closer := setupTest(t)
+	t.Cleanup(closer)
+
 	type testCase struct {
 		name string
 	}

diff --git a/pkg/config/config.go b/pkg/config/config.go
@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"path/filepath"
 
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/yaml"
@@ -107,13 +108,19 @@ func LoadArgs(args ...string) (ProgArgs, error) {
 		return pArgs, err
 	}
 
+	// this needs to be done as early as possible
+	cfgRoot, err := validateConfigRootPath(configRoot)
+	if err != nil {
+		return pArgs, err
+	}
+
 	// now the real processing begins. From now on we waste nothing
 	if pArgs.Global.Debug {
-		klog.Infof("configRoot=%q extraConfigPath=%q", configRoot, extraConfigPath)
+		klog.Infof("configRoot=%q extraConfigPath=%q", cfgRoot, extraConfigPath)
 		klog.Infof("config defaults:{{\n%s}}", pArgs.ToYAMLString())
 	}
 
-	err = FromFiles(&pArgs, configRoot, extraConfigPath)
+	err = FromFiles(&pArgs, cfgRoot, extraConfigPath)
 	if err != nil {
 		return pArgs, err
 	}
@@ -150,3 +157,15 @@ func Finalize(pArgs *ProgArgs) error {
 	}
 	return err
 }
+
+func UserRunDir() (string, error) {
+	return filepath.Join("/run", "user", fmt.Sprintf("%d", os.Getuid()), "rte"), nil
+}
+
+func UserHomeDir() (string, error) {
+	homeDir, err := os.UserHomeDir()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(homeDir, ".rte"), nil
+}
diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
@@ -26,6 +26,9 @@ import (
 )
 
 func TestLoadArgs(t *testing.T) {
+	closer := setupTest(t)
+	t.Cleanup(closer)
+
 	type testCase struct {
 		name string
 	}
@@ -39,7 +42,8 @@ func TestLoadArgs(t *testing.T) {
 		},
 	} {
 		t.Run(tcase.name, func(t *testing.T) {
-			confRoot := filepath.Join(testDataDir, "conftree", tcase.name)
+			userDir, _ := UserRunDir()
+			confRoot := filepath.Join(userDir, "conftree", tcase.name)
 
 			environ := filepath.Join(confRoot, "_env", "vars.yaml")
 			setupEnviron(t, environ)

diff --git a/pkg/config/flags_test.go b/pkg/config/flags_test.go
@@ -19,6 +19,7 @@ package config
 import (
 	"flag"
 	"fmt"
+	"log"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -137,10 +138,24 @@ func setupTestWithEnv(t *testing.T, envs map[string]string) func() {
 	baseDir = filepath.Dir(file)
 	testDataDir = filepath.Clean(filepath.Join(baseDir, "..", "..", "test", "data"))
 
-	return envSetter(envs)
+	userDir, err := UserRunDir()
+	if err == nil {
+		err2 := os.CopyFS(userDir, os.DirFS(testDataDir))
+		log.Printf("copyfs %q -> %q err=%v", testDataDir, userDir, err2)
+	}
+
+	closer := envSetter(envs)
+	return func() {
+		closer()
+		if err != nil {
+			return
+		}
+		err2 := os.Remove(userDir)
+		log.Printf("unlinked %q -> %q err=%v", testDataDir, userDir, err2)
+	}
 }
 
-func envSetter(envs map[string]string) (closer func()) {
+func envSetter(envs map[string]string) func() {
 	originalEnvs := map[string]string{}
 
 	for name, value := range envs {

diff --git a/pkg/config/validation.go b/pkg/config/validation.go
@@ -17,6 +17,12 @@ limitations under the License.
 package config
 
 import (
+	"errors"
+	"fmt"
+	"io/fs"
+	"path/filepath"
+	"strings"
+
 	metricssrv "github.com/k8stopologyawareschedwg/resource-topology-exporter/pkg/metrics/server"
 	"github.com/k8stopologyawareschedwg/resource-topology-exporter/pkg/resourcemonitor"
 )
@@ -36,3 +42,67 @@ func Validate(pArgs *ProgArgs) error {
 
 	return nil
 }
+
+func validateConfigLetPath(configletDir, configletName string) (string, error) {
+	configletPath, err := filepath.EvalSymlinks(filepath.Clean(filepath.Join(configletDir, configletName)))
+	if err != nil {
+		return "", err
+	}
+	if filepath.Dir(configletPath) != configletDir {
+		return "", fmt.Errorf("configlet %q is not within %q", configletName, configletDir)
+	}
+	return configletPath, nil
+}
+
+func validateConfigRootPath(configRoot string) (string, error) {
+	if configRoot == "" {
+		return "", fmt.Errorf("configRoot is not allowed to be an empty string")
+	}
+
+	configRoot = filepath.Clean(configRoot)
+	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)
+		}
+	}
+	// 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
+	}
+	if pattern == "" {
+		return "", errors.New("failed to validate configRoot path: not matches any allowed pattern")
+	}
+	return cfgRoot, nil
+}
+
+// IsConfigRootAllowed checks if an *already cleaned and canonicalized* path is among the allowed list.
+// use `addDirFns` to inject user-dependent paths (e.g. $HOME). Returns the matched pattern, if any,
+// and error describing the failure. The error is only relevant if failed to inject user-provided paths.
+func IsConfigRootAllowed(cfgPath string, addDirFns ...func() (string, error)) (string, error) {
+	allowedPatterns := []string{
+		"/etc/rte",
+		"/run/rte",
+		"/var/rte",
+		"/usr/local/etc/rte",
+	}
+	for _, addDirFn := range addDirFns {
+		userDir, err := addDirFn()
+		if err != nil {
+			return "", err
+		}
+		allowedPatterns = append(allowedPatterns, userDir)
+	}
+
+	for _, pattern := range allowedPatterns {
+		if strings.HasPrefix(cfgPath, pattern) {
+			return pattern, nil
+		}
+	}
+	return "", nil
+}
diff --git a/pkg/config/validation_test.go b/pkg/config/validation_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package config
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/k8stopologyawareschedwg/resource-topology-exporter/pkg/resourcemonitor"
@@ -94,3 +95,84 @@ func TestValidation(t *testing.T) {
 		})
 	}
 }
+
+func TestIsConfigRootAllowed(t *testing.T) {
+	type testCase struct {
+		name            string
+		cfgPath         string
+		addDirFns       []func() (string, error)
+		expectedError   bool
+		expectedPattern string
+	}
+
+	// important note: this functions *expects* canonicalized paths
+	for _, tcase := range []testCase{
+		{
+			name:            "base path",
+			cfgPath:         "/etc/rte",
+			addDirFns:       []func() (string, error){},
+			expectedPattern: "/etc/rte",
+		},
+		{
+			name:            "obvious misuses",
+			cfgPath:         "/etc/passwd",
+			addDirFns:       []func() (string, error){},
+			expectedPattern: "",
+		},
+		{
+			name:            "slightly less obvious misuse",
+			cfgPath:         "/usr/local/etc/shadow",
+			addDirFns:       []func() (string, error){},
+			expectedPattern: "",
+		},
+		{
+			name:    "injection fails on err",
+			cfgPath: "/var/rte/config",
+			addDirFns: []func() (string, error){
+				func() (string, error) {
+					return "/var/rte/config", errors.New("bogus error")
+				},
+			},
+			expectedPattern: "",
+		},
+		{
+			name:    "injected run",
+			cfgPath: "/run/user/12345/rte",
+			addDirFns: []func() (string, error){
+				func() (string, error) {
+					return "/run/user/12345/rte", nil
+				},
+			},
+			expectedPattern: "/run/user/12345/rte",
+		},
+		{
+			name:            "run not allowed unless injected",
+			cfgPath:         "/run/user/12345/rte",
+			addDirFns:       []func() (string, error){},
+			expectedPattern: "",
+		},
+		{
+			name:    "injected home",
+			cfgPath: "/home/foobar/.rte",
+			addDirFns: []func() (string, error){
+				func() (string, error) {
+					return "/home/foobar/.rte", nil
+				},
+			},
+			expectedPattern: "/home/foobar/.rte",
+		},
+		{
+			name:            "home not allowed unless injected",
+			cfgPath:         "/home/foobar/.rte",
+			addDirFns:       []func() (string, error){},
+			expectedPattern: "",
+		},
+	} {
+		t.Run(tcase.name, func(t *testing.T) {
+			pattern, _ := IsConfigRootAllowed(tcase.cfgPath, tcase.addDirFns...)
+			if pattern != tcase.expectedPattern {
+				t.Errorf("validation mismatch: got pattern %q expected %q", pattern, tcase.expectedPattern)
+			}
+		})
+	}
+}