Fix isAllowedDir matching by prefix (#1276)

* harden isAllowedDir

* fix lint issue

* simplify check to direct matches or subdirectories

* simplify check

* recursively check parent directory when determining if a path is allowed

* add test for allowing root directory

* remove logs

* fix relative paths in benchmark test

* fails config apply when deleting a file outside allowed dirs, update manifest UT

* fix fieldalignment

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

@@ -9,9 +9,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"log/slog"
 	"path/filepath"
-	"regexp"
+	"slices"
 	"strings"
 	"time"
 
@@ -356,10 +355,7 @@ func (col *Collector) Validate(allowedDirectories []string) error {
 	var err error
 	cleanedConfPath := filepath.Clean(col.ConfigPath)
 
-	allowed, err := isAllowedDir(cleanedConfPath, allowedDirectories)
-	if err != nil {
-		return err
-	}
+	allowed := isAllowedDir(cleanedConfPath, allowedDirectories)
 	if !allowed {
 		err = errors.Join(err, fmt.Errorf("collector path %s not allowed", col.ConfigPath))
 	}
@@ -378,10 +374,7 @@ func (nr *NginxReceiver) Validate(allowedDirectories []string) error {
 	}
 
 	for _, al := range nr.AccessLogs {
-		allowed, allowedError := isAllowedDir(al.FilePath, allowedDirectories)
-		if allowedError != nil {
-			err = errors.Join(err, fmt.Errorf("invalid nginx receiver access log path: %s", al.FilePath))
-		}
+		allowed := isAllowedDir(al.FilePath, allowedDirectories)
 		if !allowed {
 			err = errors.Join(err, fmt.Errorf("nginx receiver access log path %s not allowed", al.FilePath))
 		}
@@ -396,13 +389,9 @@ func (nr *NginxReceiver) Validate(allowedDirectories []string) error {
 	return err
 }
 
-func (c *Config) IsDirectoryAllowed(directory string) bool {
-	allow, err := isAllowedDir(directory, c.AllowedDirectories)
-	if err != nil {
-		slog.Warn("Unable to determine if directory is allowed", "error", err)
-		return false
-	}
-
+// IsAllowedDirectory checks if the given path is in the list of allowed directories.
+func (c *Config) IsDirectoryAllowed(path string) bool {
+	allow := isAllowedDir(path, c.AllowedDirectories)
 	return allow
 }
 
@@ -480,32 +469,19 @@ 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.
-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)
-	}
+// It recursively checks the parent directories of the path, until it finds a match or reaches the root directory.
+func isAllowedDir(path string, allowedDirs []string) bool {
+	return checkDirIsAllowed(filepath.Clean(path), allowedDirs)
+}
 
-	if isFilePath {
-		directoryPath = filepath.Dir(directoryPath)
+func checkDirIsAllowed(path string, allowedDirs []string) bool {
+	if slices.Contains(allowedDirs, path) {
+		return true
 	}
 
-	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) {
-			return true, nil
-		}
+	if path == "/" || !strings.HasPrefix(path, "/") { // root directory reached with no match, path is not allowed
+		return false
 	}
 
-	return false, nil
+	return checkDirIsAllowed(filepath.Dir(path), allowedDirs)
 }

internal/config/types_test.go

@@ -66,12 +66,69 @@ func TestTypes_isAllowedDir(t *testing.T) {
 			},
 			filePath: "/not-nginx-test/idontexist.conf",
 		},
+		{
+			name:    "Test 7: Prefix match not allowed",
+			allowed: false,
+			allowedDirs: []string{
+				"/etc/nginx",
+			},
+			filePath: "/etc/nginx-test/nginx.conf",
+		},
+		{
+			name:    "Test 8: Empty allowed directories",
+			allowed: false,
+			allowedDirs: []string{
+				"",
+			},
+			filePath: "/etc/nginx/nginx.conf",
+		},
+		{
+			name:    "Test 9: Multiple allowed directories, file in one of them",
+			allowed: true,
+			allowedDirs: []string{
+				"/etc/nginx",
+				"/usr/local/nginx",
+			},
+			filePath: "/usr/local/nginx/nginx.conf",
+		},
+		{
+			name:    "Test 10: Multiple allowed directories, file not in any of them",
+			allowed: false,
+			allowedDirs: []string{
+				"/etc/nginx",
+				"/usr/local/nginx",
+			},
+			filePath: "/opt/nginx/nginx.conf",
+		},
+		{
+			name:    "Test 11: Path is root directory, not in allowed directories",
+			allowed: false,
+			allowedDirs: []string{
+				"/etc/nginx",
+			},
+			filePath: "/",
+		},
+		{
+			name:    "Test 12: File is in directory nested 5 levels deep within allowed directory",
+			allowed: true,
+			allowedDirs: []string{
+				"/etc/nginx",
+			},
+			filePath: "/etc/nginx/level1/level2/level3/level4/nginx.conf",
+		},
+		{
+			name:    "Test 13: Root directory is allowed, file in subdirectory",
+			allowed: true,
+			allowedDirs: []string{
+				"/",
+			},
+			filePath: "/etc/nginx/nginx.conf",
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			result, err := isAllowedDir(test.filePath, test.allowedDirs)
-			require.NoError(t, err)
+			result := isAllowedDir(test.filePath, test.allowedDirs)
 			require.Equal(t, test.allowed, result)
 		})
 	}

internal/datasource/config/nginx_config_parser.go

@@ -74,7 +74,7 @@ func NewNginxConfigParser(agentConfig *config.Config) *NginxConfigParser {
 }
 
 func (ncp *NginxConfigParser) Parse(ctx context.Context, instance *mpi.Instance) (*model.NginxConfigContext, error) {
-	configPath := instance.GetInstanceRuntime().GetConfigPath()
+	configPath, _ := filepath.Abs(instance.GetInstanceRuntime().GetConfigPath())
 
 	if !ncp.agentConfig.IsDirectoryAllowed(configPath) {
 		return nil, fmt.Errorf("config path %s is not in allowed directories", configPath)

internal/datasource/config/nginx_config_parser_benchmark_test.go

@@ -35,8 +35,9 @@ func BenchmarkNginxConfigParser_Parse(b *testing.B) {
 	for _, configFilePath := range configFilePaths {
 		func(configFilePath string) {
 			b.Run(configFilePath, func(bb *testing.B) {
+				absPath, _ := filepath.Abs(configFilePath)
 				agentConfig.AllowedDirectories = []string{
-					filepath.Dir(configFilePath),
+					filepath.Dir(absPath),
 				}
 
 				nginxConfigParser := NewNginxConfigParser(

internal/file/file_manager_service.go

@@ -344,6 +344,12 @@ func (fms *FileManagerService) DetermineFileActions(
 			}
 			fileContents[fileName] = fileContent
 
+			// Allowed directories could have been modified since file was created,
+			// so we should check before marking for deletion
+			if !fms.agentConfig.IsDirectoryAllowed(fileName) {
+				return nil, nil, fmt.Errorf("error deleting file %s: file not in allowed directories", fileName)
+			}
+
 			fileDiff[fileName] = &model.FileCache{
 				File:   manifestFile,
 				Action: model.Delete,

internal/file/file_manager_service_test.go

@@ -462,7 +462,7 @@ func TestFileManagerService_Rollback(t *testing.T) {
 
 func TestFileManagerService_DetermineFileActions(t *testing.T) {
 	ctx := context.Background()
-	tempDir := os.TempDir()
+	tempDir := filepath.Clean(os.TempDir())
 
 	deleteTestFile := helpers.CreateFileWithErrorCheck(t, tempDir, "nginx_delete.conf")
 	defer helpers.RemoveFileWithErrorCheck(t, deleteTestFile.Name())
@@ -498,9 +498,11 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
 		expectedCache   map[string]*model.FileCache
 		expectedContent map[string][]byte
 		name            string
+		allowedDirs     []string
 	}{
 		{
-			name: "Test 1: Add, Update & Delete Files",
+			name:        "Test 1: Add, Update & Delete Files",
+			allowedDirs: []string{tempDir},
 			modifiedFiles: map[string]*model.FileCache{
 				addTestFileName: {
 					File: &mpi.File{
@@ -563,7 +565,8 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
 			expectedError: nil,
 		},
 		{
-			name: "Test 2: Files same as on disk",
+			name:        "Test 2: Files same as on disk",
+			allowedDirs: []string{tempDir},
 			modifiedFiles: map[string]*model.FileCache{
 				addTestFile.Name(): {
 					File: &mpi.File{
@@ -598,6 +601,7 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
 		},
 		{
 			name:          "Test 3: File being deleted already doesn't exist",
+			allowedDirs:   []string{tempDir},
 			modifiedFiles: make(map[string]*model.FileCache),
 			currentFiles: map[string]*mpi.File{
 				"/unknown/file.conf": {
@@ -620,6 +624,7 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
 
 			fakeFileServiceClient := &v1fakes.FakeFileServiceClient{}
 			fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{})
+			fileManagerService.agentConfig.AllowedDirectories = test.allowedDirs
 			fileManagerService.agentConfig.LibDir = manifestDirPath
 			fileManagerService.manifestFilePath = manifestFilePath
 
@@ -794,6 +799,7 @@ func TestFileManagerService_UpdateManifestFile(t *testing.T) {
 
 			fakeFileServiceClient := &v1fakes.FakeFileServiceClient{}
 			fileManagerService := NewFileManagerService(fakeFileServiceClient, types.AgentConfig(), &sync.RWMutex{})
+			fileManagerService.agentConfig.AllowedDirectories = []string{"manifestDirPath"}
 			fileManagerService.agentConfig.LibDir = manifestDirPath
 			fileManagerService.manifestFilePath = file.Name()
 

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{