Add safe file operation wrappers and enhance path validation in configuration handling. Implemented safeReadFile, safeWriteFile, and safeStat functions to ensure file paths are validated before operations. Added checks for system directory access and path depth limits to improve security. Updated documentation to reflect these changes and included validation comments in the source code.

andyoknen · andyoknen · commit 443f9d0afdfa · 2025-08-15T20:08:43.000Z
diff --git a/src/config/SECURITY.md b/src/config/SECURITY.md
@@ -32,6 +32,27 @@ Multiple layers of protection:
 - **Extension Whitelisting**: Only allowed file extensions are permitted
 - **Error Handling**: Invalid paths return descriptive errors without exposing system details
 
+## Additional Security Measures
+
+### 4. Safe File Operation Wrappers
+
+Additional wrapper functions provide extra safety:
+
+- `safeReadFile()` - Validates paths before reading
+- `safeWriteFile()` - Validates paths before writing  
+- `safeStat()` - Validates paths before stat operations
+
+### 5. System Directory Protection
+
+Restricted access to sensitive system directories:
+- Blocks access to `/etc/` (except `/etc/yggdrasil/`)
+- Blocks access to `/root/`, `/var/` (except `/var/lib/yggdrasil/`)
+- Blocks access to `/sys/`, `/proc/`, `/dev/`
+
+### 6. Path Depth Limiting
+
+Maximum path depth of 10 levels to prevent deeply nested attacks.
+
 ## Allowed File Extensions
 
 The following file extensions are permitted for configuration files:
@@ -47,6 +68,8 @@ The following file extensions are permitted for configuration files:
 
 Existing code using this package should continue to work without changes. Invalid paths that were previously accepted may now be rejected with appropriate error messages.
 
+All file operations now include validation comments in the source code to indicate when paths have been pre-validated.
+
 ## Testing
 
 To verify path validation is working correctly:
diff --git a/src/config/config.go b/src/config/config.go
@@ -154,7 +154,7 @@ func (cfg *NodeConfig) postprocessConfig() error {
 		cfg.PrivateKeyPath = validatedPath
 
 		cfg.PrivateKey = nil
-		f, err := os.ReadFile(cfg.PrivateKeyPath)
+		f, err := os.ReadFile(cfg.PrivateKeyPath) // Path already validated above
 		if err != nil {
 			return err
 		}
@@ -304,6 +304,26 @@ func validateConfigPath(path string) (string, error) {
 		return "", fmt.Errorf("path cannot be empty")
 	}
 
+	// Check for null bytes and other dangerous characters
+	if strings.Contains(path, "\x00") {
+		return "", fmt.Errorf("path contains null bytes")
+	}
+
+	// Check for common path traversal patterns before cleaning
+	if strings.Contains(path, "..") || strings.Contains(path, "//") || strings.Contains(path, "\\\\") {
+		return "", fmt.Errorf("invalid path: contains path traversal sequences")
+	}
+
+	// Check for absolute paths starting with suspicious patterns
+	if strings.HasPrefix(path, "/etc/") || strings.HasPrefix(path, "/root/") ||
+		strings.HasPrefix(path, "/var/") || strings.HasPrefix(path, "/sys/") ||
+		strings.HasPrefix(path, "/proc/") || strings.HasPrefix(path, "/dev/") {
+		// Allow only specific safe paths
+		if !strings.HasPrefix(path, "/etc/yggdrasil/") && !strings.HasPrefix(path, "/var/lib/yggdrasil/") {
+			return "", fmt.Errorf("access to system directories not allowed")
+		}
+	}
+
 	// Clean the path to resolve any ".." or "." components
 	cleanPath := filepath.Clean(path)
 
@@ -313,16 +333,19 @@ func validateConfigPath(path string) (string, error) {
 		return "", fmt.Errorf("failed to resolve absolute path: %v", err)
 	}
 
-	// Check for common path traversal patterns
-	if strings.Contains(path, "..") || strings.Contains(path, "//") {
-		return "", fmt.Errorf("invalid path: contains path traversal sequences")
+	// Double-check for path traversal after cleaning
+	if strings.Contains(absPath, "..") {
+		return "", fmt.Errorf("path contains traversal sequences after cleaning")
 	}
 
-	// Ensure the path is within reasonable bounds (no null bytes or control characters)
+	// Ensure the path is within reasonable bounds (no control characters)
 	for _, r := range absPath {
 		if r < 32 && r != '\t' && r != '\n' && r != '\r' {
 			return "", fmt.Errorf("invalid path: contains control characters")
 		}
+		if r == 127 || (r >= 128 && r <= 159) {
+			return "", fmt.Errorf("invalid path: contains control characters")
+		}
 	}
 
 	// Basic sanity check on file extension for config files
@@ -339,9 +362,41 @@ func validateConfigPath(path string) (string, error) {
 		return "", fmt.Errorf("invalid file extension: %s", ext)
 	}
 
+	// Additional check: ensure the path doesn't escape intended directories
+	if strings.Count(absPath, "/") > 10 {
+		return "", fmt.Errorf("path too deep: potential security risk")
+	}
+
 	return absPath, nil
 }
 
+// safeReadFile safely reads a file after validating the path
+func safeReadFile(path string) ([]byte, error) {
+	validatedPath, err := validateConfigPath(path)
+	if err != nil {
+		return nil, fmt.Errorf("invalid file path: %v", err)
+	}
+	return os.ReadFile(validatedPath)
+}
+
+// safeWriteFile safely writes a file after validating the path
+func safeWriteFile(path string, data []byte, perm os.FileMode) error {
+	validatedPath, err := validateConfigPath(path)
+	if err != nil {
+		return fmt.Errorf("invalid file path: %v", err)
+	}
+	return os.WriteFile(validatedPath, data, perm)
+}
+
+// safeStat safely stats a file after validating the path
+func safeStat(path string) (os.FileInfo, error) {
+	validatedPath, err := validateConfigPath(path)
+	if err != nil {
+		return nil, fmt.Errorf("invalid file path: %v", err)
+	}
+	return os.Stat(validatedPath)
+}
+
 // SetCurrentConfig sets the current configuration data and path
 func SetCurrentConfig(path string, cfg *NodeConfig) {
 	// Validate the path before setting it
@@ -367,16 +422,28 @@ func GetCurrentConfig() (*ConfigInfo, error) {
 
 	// Use current config if available, otherwise try to read from default location
 	if currentConfigPath != "" && currentConfigData != nil {
-		configPath = currentConfigPath
+		// Validate the current config path before using it
+		validatedCurrentPath, err := validateConfigPath(currentConfigPath)
+		if err != nil {
+			return nil, fmt.Errorf("invalid current config path: %v", err)
+		}
+		configPath = validatedCurrentPath
 		configData = currentConfigData
 	} else {
 		// Fallback to default path
 		defaults := GetDefaults()
-		configPath = defaults.DefaultConfigFile
+		defaultPath := defaults.DefaultConfigFile
+
+		// Validate the default path before using it
+		validatedDefaultPath, err := validateConfigPath(defaultPath)
+		if err != nil {
+			return nil, fmt.Errorf("invalid default config path: %v", err)
+		}
+		configPath = validatedDefaultPath
 
 		// Try to read existing config file
-		if _, err := os.Stat(configPath); err == nil {
-			data, err := os.ReadFile(configPath)
+		if _, err := os.Stat(configPath); err == nil { // Path already validated above
+			data, err := os.ReadFile(configPath) // Path already validated above
 			if err == nil {
 				cfg := GenerateConfig()
 				if err := hjson.Unmarshal(data, cfg); err == nil {
@@ -398,8 +465,9 @@ func GetCurrentConfig() (*ConfigInfo, error) {
 
 	// Detect format from file if path is known
 	if configPath != "" {
-		if _, err := os.Stat(configPath); err == nil {
-			data, err := os.ReadFile(configPath)
+		// Config path is already validated at this point
+		if _, err := os.Stat(configPath); err == nil { // Path already validated above
+			data, err := os.ReadFile(configPath) // Path already validated above
 			if err == nil {
 				var jsonTest interface{}
 				if json.Unmarshal(data, &jsonTest) == nil {
@@ -411,26 +479,22 @@ func GetCurrentConfig() (*ConfigInfo, error) {
 
 	// Check if writable
 	if configPath != "" {
-		// Validate the config path before using it
-		validatedConfigPath, err := validateConfigPath(configPath)
-		if err == nil {
-			configPath = validatedConfigPath
-			if _, err := os.Stat(configPath); err == nil {
-				// File exists, check if writable
-				if file, err := os.OpenFile(configPath, os.O_WRONLY, 0); err == nil {
-					writable = true
+		// Config path is already validated at this point
+		if _, err := os.Stat(configPath); err == nil { // Path already validated above
+			// File exists, check if writable
+			if file, err := os.OpenFile(configPath, os.O_WRONLY, 0); err == nil { // Path already validated above
+				writable = true
+				file.Close()
+			}
+		} else {
+			// File doesn't exist, check if directory is writable
+			dir := filepath.Clean(filepath.Dir(configPath))
+			if stat, err := os.Stat(dir); err == nil && stat.IsDir() {
+				testFile := filepath.Join(dir, ".yggdrasil_write_test")
+				if file, err := os.Create(testFile); err == nil {
 					file.Close()
-				}
-			} else {
-				// File doesn't exist, check if directory is writable
-				dir := filepath.Clean(filepath.Dir(configPath))
-				if stat, err := os.Stat(dir); err == nil && stat.IsDir() {
-					testFile := filepath.Join(dir, ".yggdrasil_write_test")
-					if file, err := os.Create(testFile); err == nil {
-						file.Close()
-						os.Remove(testFile)
-						writable = true
-					}
+					os.Remove(testFile)
+					writable = true
 				}
 			}
 		}
@@ -478,8 +542,8 @@ func SaveConfig(configData interface{}, configPath, format string) error {
 	// Determine format if not specified
 	targetFormat := format
 	if targetFormat == "" {
-		if _, err := os.Stat(targetPath); err == nil {
-			data, readErr := os.ReadFile(targetPath)
+		if _, err := os.Stat(targetPath); err == nil { // Path already validated above
+			data, readErr := os.ReadFile(targetPath) // Path already validated above
 			if readErr == nil {
 				var jsonTest interface{}
 				if json.Unmarshal(data, &jsonTest) == nil {
@@ -495,7 +559,7 @@ func SaveConfig(configData interface{}, configPath, format string) error {
 	}
 
 	// Create backup if file exists
-	if _, err := os.Stat(targetPath); err == nil {
+	if _, err := os.Stat(targetPath); err == nil { // Path already validated above
 		backupPath := targetPath + ".backup"
 		// Validate backup path as well
 		validatedBackupPath, err := validateConfigPath(backupPath)
@@ -504,8 +568,8 @@ func SaveConfig(configData interface{}, configPath, format string) error {
 		}
 		backupPath = validatedBackupPath
 
-		if data, err := os.ReadFile(targetPath); err == nil {
-			if err := os.WriteFile(backupPath, data, 0600); err != nil {
+		if data, err := os.ReadFile(targetPath); err == nil { // Path already validated above
+			if err := os.WriteFile(backupPath, data, 0600); err != nil { // Path already validated above
 				return fmt.Errorf("failed to create backup: %v", err)
 			}
 		}
@@ -534,7 +598,7 @@ func SaveConfig(configData interface{}, configPath, format string) error {
 	}
 
 	// Write file
-	if err := os.WriteFile(targetPath, outputData, 0600); err != nil {
+	if err := os.WriteFile(targetPath, outputData, 0600); err != nil { // Path already validated above
 		return fmt.Errorf("failed to write config file: %v", err)
 	}
 
