Add validation to allowed directories on config load

internal/config/config.go

@@ -14,6 +14,7 @@ import (
 	"log/slog"
 	"os"
 	"path/filepath"
+	"regexp"
 	"slices"
 	"strconv"
 	"strings"
@@ -36,11 +37,15 @@ const (
 	EnvPrefix                    = "NGINX_AGENT"
 	KeyDelimiter                 = "_"
 	KeyValueNumber               = 2
-	AgentDirName                 = "/etc/nginx-agent/"
+	AgentDirName                 = "/etc/nginx-agent"
 	DefaultMetricsBatchProcessor = "default_metrics"
 	DefaultLogsBatchProcessor    = "default_logs"
 	DefaultExporter              = "default"
 	DefaultPipeline              = "default"
+
+	// Regular expression to match invalid characters in paths.
+	// It matches whitespace, control characters, non-printable characters, and specific Unicode characters.
+	regexInvalidPath = "\\s|[[:cntrl:]]|[[:space:]]|[[^:print:]]|ㅤ|\\.\\.|\\*"
 )
 
 var viperInstance = viper.NewWithOptions(viper.KeyDelimiter(KeyDelimiter))
@@ -83,27 +88,13 @@ func RegisterConfigFile() error {
 }
 
 func ResolveConfig() (*Config, error) {
-	// Collect allowed directories, so that paths in the config can be validated.
-	directories := viperInstance.GetStringSlice(AllowedDirectoriesKey)
-	allowedDirs := []string{AgentDirName}
-
 	log := resolveLog()
 	slogger := logger.New(log.Path, log.Level)
 	slog.SetDefault(slogger)
 
-	// Check directories in allowed_directories are valid
-	for _, dir := range directories {
-		if dir == "" || !filepath.IsAbs(dir) {
-			slog.Warn("Invalid directory: ", "dir", dir)
-			continue
-		}
-
-		if !strings.HasSuffix(dir, "/") {
-			dir += "/"
-		}
-		allowedDirs = append(allowedDirs, dir)
-	}
-
+	// Collect allowed directories, so that paths in the config can be validated.
+	directories := viperInstance.GetStringSlice(AllowedDirectoriesKey)
+	allowedDirs := resolveAllowedDirectories(directories)
 	slog.Info("Configured allowed directories", "allowed_directories", allowedDirs)
 
 	// Collect all parsing errors before returning the error, so the user sees all issues with config
@@ -142,6 +133,29 @@ func ResolveConfig() (*Config, error) {
 	return config, nil
 }
 
+// resolveAllowedDirectories checks if the provided directories are valid and returns a slice of cleaned absolute paths.
+// It ignores empty paths, paths that are not absolute, and paths containing invalid characters.
+// Invalid paths are logged as warnings.
+func resolveAllowedDirectories(dirs []string) []string {
+	allowed := []string{AgentDirName}
+	for _, dir := range dirs {
+		re := regexp.MustCompile(regexInvalidPath)
+		invalidChars := re.MatchString(dir)
+		if dir == "" || dir == "/" || !filepath.IsAbs(dir) || invalidChars {
+			slog.Warn("Ignoring invalid directory", "dir", dir)
+			continue
+		}
+		dir = filepath.Clean(dir)
+		if dir == AgentDirName {
+			// If the directory is the default agent directory, we skip adding it again.
+			continue
+		}
+		allowed = append(allowed, dir)
+	}
+
+	return allowed
+}
+
 func defaultCollector(collector *Collector, config *Config) {
 	// Always add default host metric receiver and default processor
 	addDefaultHostMetricsReceiver(collector)
@@ -909,13 +923,14 @@ func resolveClient() *Client {
 }
 
 func resolveCollector(allowedDirs []string) (*Collector, error) {
+	// Collect receiver configurations
 	var receivers Receivers
-
 	err := resolveMapStructure(CollectorReceiversKey, &receivers)
 	if err != nil {
 		return nil, fmt.Errorf("unmarshal collector receivers config: %w", err)
 	}
 
+	// Collect exporter configurations
 	exporters, err := resolveExporters()
 	if err != nil {
 		return nil, fmt.Errorf("unmarshal collector exporters config: %w", err)

internal/config/config_test.go

@@ -163,6 +163,84 @@ func TestNormalizeFunc(t *testing.T) {
 	assert.Equal(t, expected, result)
 }
 
+func TestResolveAllowedDirectories(t *testing.T) {
+	tests := []struct {
+		name           string
+		configuredDirs []string
+		expected       []string
+	}{
+		{
+			name:           "Test 1: Empty path",
+			configuredDirs: []string{""},
+			expected:       []string{"/etc/nginx-agent"},
+		},
+		{
+			name:           "Test 2: Absolute path",
+			configuredDirs: []string{"/etc/agent/"},
+			expected:       []string{"/etc/nginx-agent", "/etc/agent"},
+		},
+		{
+			name:           "Test 3: Absolute paths",
+			configuredDirs: []string{"/etc/nginx/"},
+			expected:       []string{"/etc/nginx-agent", "/etc/nginx"},
+		},
+		{
+			name:           "Test 4: Absolute path with multiple slashes",
+			configuredDirs: []string{"/etc///////////nginx-agent/"},
+			expected:       []string{"/etc/nginx-agent"},
+		},
+		{
+			name:           "Test 5: Absolute path with directory traversal",
+			configuredDirs: []string{"/etc/nginx/../nginx-agent"},
+			expected:       []string{"/etc/nginx-agent"},
+		},
+		{
+			name:           "Test 6: Absolute path with repeat directory traversal",
+			configuredDirs: []string{"/etc/nginx-agent/../../../../../nginx-agent"},
+			expected:       []string{"/etc/nginx-agent"},
+		},
+		{
+			name:           "Test 7: Absolute path with control characters",
+			configuredDirs: []string{"/etc/nginx-agent/\\x08../tmp/"},
+			expected:       []string{"/etc/nginx-agent"},
+		},
+		{
+			name:           "Test 8: Absolute path with invisible characters",
+			configuredDirs: []string{"/etc/nginx-agent/ㅤㅤㅤ/tmp/"},
+			expected:       []string{"/etc/nginx-agent"},
+		},
+		{
+			name:           "Test 9: Absolute path with escaped invisible characters",
+			configuredDirs: []string{"/etc/nginx-agent/\\\\ㅤ/tmp/"},
+			expected:       []string{"/etc/nginx-agent"},
+		},
+		{
+			name: "Test 10: Mixed paths",
+			configuredDirs: []string{
+				"nginx-agent",
+				"",
+				"..",
+				"/",
+				"\\/",
+				".",
+				"/etc/nginx/",
+			},
+			expected: []string{"/etc/nginx-agent", "/etc/nginx"},
+		},
+		{
+			name:           "Test 11: Relative path",
+			configuredDirs: []string{"nginx-agent"},
+			expected:       []string{"/etc/nginx-agent"},
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			allowed := resolveAllowedDirectories(test.configuredDirs)
+			assert.Equal(t, test.expected, allowed)
+		})
+	}
+}
+
 func TestResolveLog(t *testing.T) {
 	viperInstance = viper.NewWithOptions(viper.KeyDelimiter(KeyDelimiter))
 	viperInstance.Set(LogLevelKey, "error")
@@ -867,89 +945,7 @@ func agentConfig() *Config {
 			"/etc/nginx/", "/etc/nginx-agent/", "/usr/local/etc/nginx/", "/var/run/nginx/", "/var/log/nginx/",
 			"/usr/share/nginx/modules/", "/etc/app_protect/",
 		},
-		Collector: &Collector{
-			ConfigPath: "/etc/nginx-agent/nginx-agent-otelcol.yaml",
-			Exporters: Exporters{
-				OtlpExporters: map[string]*OtlpExporter{
-					"default": {
-						Server: &ServerConfig{
-							Host: "127.0.0.1",
-							Port: 1234,
-							Type: Grpc,
-						},
-						TLS: &TLSConfig{
-							Cert:       "/path/to/server-cert.pem",
-							Key:        "/path/to/server-cert.pem",
-							Ca:         "/path/to/server-cert.pem",
-							SkipVerify: true,
-							ServerName: "remote-saas-server",
-						},
-					},
-				},
-			},
-			Processors: Processors{
-				Batch: map[string]*Batch{
-					"default_logs": {
-						SendBatchMaxSize: DefCollectorLogsBatchProcessorSendBatchMaxSize,
-						SendBatchSize:    DefCollectorLogsBatchProcessorSendBatchSize,
-						Timeout:          DefCollectorLogsBatchProcessorTimeout,
-					},
-				},
-				LogsGzip: map[string]*LogsGzip{
-					"default": {},
-				},
-			},
-			Receivers: Receivers{
-				OtlpReceivers: map[string]*OtlpReceiver{
-					"default": {
-						Server: &ServerConfig{
-							Host: "localhost",
-							Port: 4317,
-							Type: Grpc,
-						},
-						Auth: &AuthConfig{
-							Token: "even-secreter-token",
-						},
-						OtlpTLSConfig: &OtlpTLSConfig{
-							GenerateSelfSignedCert: false,
-							Cert:                   "/path/to/server-cert.pem",
-							Key:                    "/path/to/server-cert.pem",
-							Ca:                     "/path/to/server-cert.pem",
-							SkipVerify:             true,
-							ServerName:             "local-data-plane-server",
-						},
-					},
-				},
-				NginxReceivers: []NginxReceiver{
-					{
-						InstanceID: "cd7b8911-c2c5-4daf-b311-dbead151d938",
-						StubStatus: APIDetails{
-							URL:    "http://localhost:4321/status",
-							Listen: "",
-						},
-						AccessLogs: []AccessLog{
-							{
-								LogFormat: accessLogFormat,
-								FilePath:  "/var/log/nginx/access-custom.conf",
-							},
-						},
-					},
-				},
-			},
-			Extensions: Extensions{
-				Health: &Health{
-					Server: &ServerConfig{
-						Host: "localhost",
-						Port: 1337,
-					},
-					Path: "/",
-				},
-			},
-			Log: &Log{
-				Level: "INFO",
-				Path:  "/var/log/nginx-agent/opentelemetry-collector-agent.log",
-			},
-		},
+		Collector: createDefaultCollectorConfig(),
 		Command: &Command{
 			Server: &ServerConfig{
 				Host: "127.0.0.1",
@@ -1002,8 +998,8 @@ func createConfig() *Config {
 			},
 		},
 		AllowedDirectories: []string{
-			"/etc/nginx-agent/", "/etc/nginx/", "/usr/local/etc/nginx/", "/var/run/nginx/",
-			"/usr/share/nginx/modules/", "/var/log/nginx/",
+			"/etc/nginx-agent", "/etc/nginx", "/usr/local/etc/nginx", "/var/run/nginx",
+			"/usr/share/nginx/modules", "/var/log/nginx",
 		},
 		DataPlaneConfig: &DataPlaneConfig{
 			Nginx: &NginxDataPlaneConfig{
@@ -1226,3 +1222,89 @@ func createConfig() *Config {
 		},
 	}
 }
+
+func createDefaultCollectorConfig() *Collector {
+	return &Collector{
+		ConfigPath: "/etc/nginx-agent/nginx-agent-otelcol.yaml",
+		Exporters: Exporters{
+			OtlpExporters: map[string]*OtlpExporter{
+				"default": {
+					Server: &ServerConfig{
+						Host: "127.0.0.1",
+						Port: 1234,
+						Type: Grpc,
+					},
+					TLS: &TLSConfig{
+						Cert:       "/path/to/server-cert.pem",
+						Key:        "/path/to/server-cert.pem",
+						Ca:         "/path/to/server-cert.pem",
+						SkipVerify: true,
+						ServerName: "remote-saas-server",
+					},
+				},
+			},
+		},
+		Processors: Processors{
+			Batch: map[string]*Batch{
+				"default_logs": {
+					SendBatchMaxSize: DefCollectorLogsBatchProcessorSendBatchMaxSize,
+					SendBatchSize:    DefCollectorLogsBatchProcessorSendBatchSize,
+					Timeout:          DefCollectorLogsBatchProcessorTimeout,
+				},
+			},
+			LogsGzip: map[string]*LogsGzip{
+				"default": {},
+			},
+		},
+		Receivers: Receivers{
+			OtlpReceivers: map[string]*OtlpReceiver{
+				"default": {
+					Server: &ServerConfig{
+						Host: "localhost",
+						Port: 4317,
+						Type: Grpc,
+					},
+					Auth: &AuthConfig{
+						Token: "even-secreter-token",
+					},
+					OtlpTLSConfig: &OtlpTLSConfig{
+						GenerateSelfSignedCert: false,
+						Cert:                   "/path/to/server-cert.pem",
+						Key:                    "/path/to/server-cert.pem",
+						Ca:                     "/path/to/server-cert.pem",
+						SkipVerify:             true,
+						ServerName:             "local-data-plane-server",
+					},
+				},
+			},
+			NginxReceivers: []NginxReceiver{
+				{
+					InstanceID: "cd7b8911-c2c5-4daf-b311-dbead151d938",
+					StubStatus: APIDetails{
+						URL:    "http://localhost:4321/status",
+						Listen: "",
+					},
+					AccessLogs: []AccessLog{
+						{
+							LogFormat: accessLogFormat,
+							FilePath:  "/var/log/nginx/access-custom.conf",
+						},
+					},
+				},
+			},
+		},
+		Extensions: Extensions{
+			Health: &Health{
+				Server: &ServerConfig{
+					Host: "localhost",
+					Port: 1337,
+				},
+				Path: "/",
+			},
+		},
+		Log: &Log{
+			Level: "INFO",
+			Path:  "/var/log/nginx-agent/opentelemetry-collector-agent.log",
+		},
+	}
+}