harden isAllowedDir

Author: sean-breen

internal/config/config_test.go

@@ -1037,8 +1037,8 @@ func agentConfig() *Config {
 			},
 		},
 		AllowedDirectories: []string{
-			"/etc/nginx/", "/etc/nginx-agent/", "/usr/local/etc/nginx/", "/var/run/nginx/", "/var/log/nginx/",
-			"/usr/share/nginx/modules/", "/etc/app_protect/",
+			"/etc/nginx/", "/etc/nginx-agent/", "/usr/local/etc/nginx", "/var/run/nginx", "/var/log/nginx",
+			"/usr/share/nginx/modules", "/etc/app_protect",
 		},
 		Collector: createDefaultCollectorConfig(),
 		Command: &Command{

internal/config/types.go

@@ -10,8 +10,9 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"os"
 	"path/filepath"
-	"regexp"
+	"slices"
 	"strings"
 	"time"
 
@@ -396,8 +397,9 @@ func (nr *NginxReceiver) Validate(allowedDirectories []string) error {
 	return err
 }
 
-func (c *Config) IsDirectoryAllowed(directory string) bool {
-	allow, err := isAllowedDir(directory, c.AllowedDirectories)
+// IsAllowedDirectory checks if the given path is in the list of allowed directories.
+func (c *Config) IsDirectoryAllowed(path string) bool {
+	allow, err := isAllowedDir(path, c.AllowedDirectories)
 	if err != nil {
 		slog.Warn("Unable to determine if directory is allowed", "error", err)
 		return false
@@ -480,29 +482,40 @@ func (c *Config) IsCommandServerProxyConfigured() bool {
 }
 
 // isAllowedDir checks if the given path is in the list of allowed directories.
-// It returns true if the path is allowed, false otherwise.
-// If the path is allowed but does not exist, it also logs a warning.
-// It also checks if the path is a file, in which case it checks the parent directory of the file.
+// It works on files and directories.
+// If the path is a file, it checks the directory of the file.
+// If the path is a directory, it checks the directory itself.
+// It also allows subdirectories within the allowed directories.
 func isAllowedDir(path string, allowedDirs []string) (bool, error) {
 	if len(allowedDirs) == 0 {
 		return false, errors.New("no allowed directories configured")
 	}
 
-	directoryPath := path
-	// Check if the path is a file, regex matches when end of string is /<filename>.<extension>
-	isFilePath, err := regexp.MatchString(`/(\w+)\.(\w+)$`, directoryPath)
-	if err != nil {
-		return false, errors.New("error matching path" + directoryPath)
+	// Check if the path is absolute
+	if !filepath.IsAbs(path) {
+		return false, fmt.Errorf("PathError: %s is not absolute", path)
 	}
 
-	if isFilePath {
-		directoryPath = filepath.Dir(directoryPath)
+	// Does the path exist?
+	info, err := os.Stat(path)
+	if err == nil && !info.IsDir() {
+		// Path exists and is not a directory
+		// It's a file so get the parent directory
+		path = filepath.Dir(path)
+	}
+	if err != nil {
+		if os.IsNotExist(err) {
+			// Path does not exist, we can still check if it's an allowed directory
+		} else {
+			// Some other error occurred
+			return false, fmt.Errorf("PathError %s %w", path, err)
+		}
 	}
 
-	for _, allowedDirectory := range allowedDirs {
-		// Check if the directoryPath starts with the allowedDirectory
-		// This allows for subdirectories within the allowed directories.
-		if strings.HasPrefix(directoryPath, allowedDirectory) {
+	for _, dir := range allowedDirs {
+		// Check if the path is a subdirectory of the allowed directory
+		if slices.Contains(allowedDirs, path) || strings.HasPrefix(path, dir+"/") {
+			slog.Info(fmt.Sprintf("Allowed directory %s is allowed", path))
 			return true, nil
 		}
 	}

internal/config/types_test.go

@@ -66,6 +66,22 @@ func TestTypes_isAllowedDir(t *testing.T) {
 			},
 			filePath: "/not-nginx-test/idontexist.conf",
 		},
+		{
+			name:    "Test 7: File is in allowed directory with trailing slash",
+			allowed: true,
+			allowedDirs: []string{
+				"/etc/nginx/",
+			},
+			filePath: "/etc/nginx/nginx.conf",
+		},
+		{
+			name:    "Test 8: Prefix match",
+			allowed: false,
+			allowedDirs: []string{
+				"/etc/nginx",
+			},
+			filePath: "/etc/nginx-test/nginx.conf",
+		},
 	}
 
 	for _, test := range tests {

test/types/config.go

@@ -62,7 +62,7 @@ func AgentConfig() *config.Config {
 				Multiplier:          commonMultiplier,
 			},
 		},
-		AllowedDirectories: []string{"/tmp/"},
+		AllowedDirectories: []string{"/tmp"},
 		Collector: &config.Collector{
 			ConfigPath: "/etc/nginx-agent/nginx-agent-otelcol.yaml",
 			Exporters: config.Exporters{