Merge pull request #2095 from jsco2t/CWE-23_internal/pkg/clean/clean.go

Fix for CWE-23 in internal/pkg/clean/clean.go

CHANGELOG.md

@@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - Bugfix for command-line arguments during single-stage image unpacking
 - Fix path traversal vulnerability (CWE-23) by validating overlay names in `overlay.Get()` and `overlay.Create()`
 - Upgrade golang version to 1.25 to resolve stdlib CVEs (CVE-2025-4673, CVE-2025-58187, CVE-2025-61723, CVE-2025-58188, CVE-2025-61725, CVE-2025-61726, CVE-2025-47907, CVE-2025-58189, CVE-2025-47906, CVE-2025-58186, CVE-2025-61727, CVE-2025-61724, CVE-2025-58185, CVE-2025-47912, CVE-2025-61729, CVE-2025-0913, CVE-2025-22873, CVE-2024-45336, CVE-2024-45341, CVE-2025-58183, CVE-2025-68121, CVE-2025-61730)
+- Fix path traversal vulnerability (CWE-23) in overlay cleanup function
 
 ### Dependencies
 

internal/pkg/clean/clean.go

@@ -1,8 +1,10 @@
 package clean
 
 import (
+	"fmt"
 	"os"
-	"path"
+	"path/filepath"
+	"strings"
 
 	warewulfconf "github.com/warewulf/warewulf/internal/pkg/config"
 	"github.com/warewulf/warewulf/internal/pkg/node"
@@ -26,7 +28,11 @@ func CleanOverlays() error {
 		return err
 	}
 	nodes := nodeDB.ListAllNodes()
-	dirList, err := os.ReadDir(warewulfconf.Paths.OverlayProvisiondir())
+
+	// Clean the base directory path FIRST
+	baseDir := filepath.Clean(warewulfconf.Paths.OverlayProvisiondir())
+
+	dirList, err := os.ReadDir(baseDir)
 	if err != nil {
 		return err
 	}
@@ -34,9 +40,42 @@ func CleanOverlays() error {
 		if !item.IsDir() {
 			continue
 		}
+
 		if !util.InSlice(nodes, item.Name()) {
-			wwlog.Verbose("removing overlays of delete node: %s", item.Name())
-			err = os.RemoveAll(path.Join(warewulfconf.Paths.OverlayProvisiondir(), item.Name()))
+
+			// Construct and validate the path (filepath.Join already calls Clean)
+			cleanTarget := filepath.Join(baseDir, item.Name())
+
+			// Compute the relative path to verify cleanTarget is within baseDir.
+			// This should never fail in normal operation since we just constructed cleanTarget
+			// using filepath.Join(baseDir, item.Name()). However, filepath.Rel() can fail in
+			// edge cases (different volumes on Windows, symlink anomalies, filesystem corruption).
+			// If we cannot determine the path relationship, we cannot safely validate whether
+			// deletion would stay within bounds, so we fail-secure and return an error.
+			rel, err := filepath.Rel(baseDir, cleanTarget)
+			if err != nil {
+				return fmt.Errorf(
+					"failed to compute relative path for '%s' with overlay working directory '%s': %w",
+					item.Name(),
+					baseDir,
+					err,
+				)
+			}
+
+			// Check for actual parent directory traversal (CWE-23 mitigation)
+			// This catches paths that escape baseDir like ".." or "../etc/passwd"
+			// but allows legitimate directory names like "..suspicious" or "...triple"
+			// which remain inside baseDir despite starting with dots.
+			if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
+				return fmt.Errorf(
+					"'%s' is not inside of overlay working directory: %s",
+					item.Name(),
+					baseDir,
+				)
+			}
+
+			wwlog.Verbose("removing overlays of deleted node: %s", item.Name())
+			err = os.RemoveAll(cleanTarget)
 			if err != nil {
 				return err
 			}

internal/pkg/clean/clean_test.go

@@ -0,0 +1,184 @@
+package clean
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/warewulf/warewulf/internal/pkg/testenv"
+)
+
+func Test_CleanOverlays(t *testing.T) {
+	tests := map[string]struct {
+		nodesConf        string   // nodes.conf content
+		overlayDirs      []string // directories to create under overlays/
+		overlayFiles     []string // files to create under overlays/ (should be skipped)
+		skipOverlaySetup bool     // skip creating overlay directory structure
+		wantPreserved    []string // overlay dirs that should exist after cleanup
+		wantDeleted      []string // overlay dirs that should not exist after cleanup
+		wantErr          bool     // expect error from CleanOverlays
+	}{
+		"preserves valid node, deletes orphaned node": {
+			nodesConf: `nodeprofiles: {}
+nodes:
+  node1: {}`,
+			overlayDirs:   []string{"node1", "node2"},
+			wantPreserved: []string{"node1"},
+			wantDeleted:   []string{"node2"},
+			wantErr:       false,
+		},
+		"empty overlay directory": {
+			nodesConf: `nodeprofiles: {}
+nodes:
+  node1: {}`,
+			overlayDirs:   []string{},
+			wantPreserved: []string{},
+			wantDeleted:   []string{},
+			wantErr:       false,
+		},
+		"skips files in overlay directory": {
+			nodesConf: `nodeprofiles: {}
+nodes:
+  node1: {}`,
+			overlayDirs:   []string{"node1"},
+			overlayFiles:  []string{"somefile.txt", "another.img"},
+			wantPreserved: []string{"node1"},
+			wantDeleted:   []string{},
+			wantErr:       false,
+		},
+		"multiple orphaned nodes deleted": {
+			nodesConf: `nodeprofiles: {}
+nodes:
+  node1: {}`,
+			overlayDirs:   []string{"node1", "node2", "node3", "node4"},
+			wantPreserved: []string{"node1"},
+			wantDeleted:   []string{"node2", "node3", "node4"},
+			wantErr:       false,
+		},
+		"all nodes preserved when all exist in database": {
+			nodesConf: `nodeprofiles: {}
+nodes:
+  node1: {}
+  node2: {}
+  node3: {}`,
+			overlayDirs:   []string{"node1", "node2", "node3"},
+			wantPreserved: []string{"node1", "node2", "node3"},
+			wantDeleted:   []string{},
+			wantErr:       false,
+		},
+		"empty node database deletes all overlays": {
+			nodesConf: `nodeprofiles: {}
+nodes: {}`,
+			overlayDirs:   []string{"node1", "node2"},
+			wantPreserved: []string{},
+			wantDeleted:   []string{"node1", "node2"},
+			wantErr:       false,
+		},
+		"error when nodes.conf is invalid": {
+			nodesConf:   `this is not valid YAML: [[[`,
+			overlayDirs: []string{"node1"},
+			wantErr:     true,
+		},
+		"error when overlay directory is missing": {
+			nodesConf: `nodeprofiles: {}
+nodes:
+  node1: {}`,
+			skipOverlaySetup: true,
+			wantErr:          true,
+		},
+		"cleans directories with names starting with .. (not actual traversal)": {
+			nodesConf: `nodeprofiles: {}
+nodes:
+  node1: {}`,
+			overlayDirs: []string{"node1", "..suspicious", "..hidden"},
+			// These are just oddly-named directories, not actual path traversal
+			// They remain inside baseDir and should be cleaned up normally
+			wantPreserved: []string{"node1"},
+			wantDeleted:   []string{"..suspicious", "..hidden"},
+			wantErr:       false,
+		},
+		"handles directory names with dots correctly": {
+			nodesConf: `nodeprofiles: {}
+nodes:
+  node1: {}
+  node.valid: {}`,
+			overlayDirs:   []string{"node1", "node.valid", "node.orphaned"},
+			wantPreserved: []string{"node1", "node.valid"},
+			wantDeleted:   []string{"node.orphaned"},
+			wantErr:       false,
+		},
+		"handles edge case directory names with multiple dots": {
+			nodesConf: `nodeprofiles: {}
+nodes:
+  validnode: {}`,
+			// Directory names like "..test" and "...triple" are just unusual names,
+			// not actual path traversal - they stay within baseDir and can be safely deleted
+			overlayDirs:   []string{"validnode", "..test", "...triple"},
+			wantPreserved: []string{"validnode"},
+			wantDeleted:   []string{"..test", "...triple"},
+			wantErr:       false,
+		},
+	}
+
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			env := testenv.New(t)
+			defer env.RemoveAll()
+
+			// Setup nodes.conf
+			if tt.nodesConf != "" {
+				env.WriteFile("etc/warewulf/nodes.conf", tt.nodesConf)
+			}
+
+			// Setup overlay directory structure (unless explicitly skipped)
+			if !tt.skipOverlaySetup {
+				// Ensure the overlay directory exists (even if empty)
+				env.MkdirAll("srv/warewulf/overlays")
+
+				// Setup overlay directories
+				for _, dir := range tt.overlayDirs {
+					// Create a file inside each directory to make it a proper overlay dir
+					env.WriteFile(filepath.Join("srv/warewulf/overlays", dir, "__SYSTEM__.img"), "Fake System")
+				}
+
+				// Setup overlay files (non-directories)
+				for _, file := range tt.overlayFiles {
+					env.WriteFile(filepath.Join("srv/warewulf/overlays", file), "test content")
+				}
+			} else {
+				// For tests that skip setup, remove the overlay directory
+				overlayPath := env.GetPath("srv/warewulf/overlays")
+				assert.NoError(t, os.RemoveAll(overlayPath))
+			}
+
+			// Execute
+			err := CleanOverlays()
+
+			// Assert
+			if tt.wantErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+
+				// Verify preserved directories exist
+				for _, dir := range tt.wantPreserved {
+					dirPath := env.GetPath(filepath.Join("srv/warewulf/overlays", dir))
+					assert.DirExists(t, dirPath, "expected directory %s to be preserved", dir)
+				}
+
+				// Verify deleted directories do not exist
+				for _, dir := range tt.wantDeleted {
+					dirPath := env.GetPath(filepath.Join("srv/warewulf/overlays", dir))
+					assert.NoDirExists(t, dirPath, "expected directory %s to be deleted", dir)
+				}
+
+				// Verify files were not deleted (when specified)
+				for _, file := range tt.overlayFiles {
+					filePath := env.GetPath(filepath.Join("srv/warewulf/overlays", file))
+					assert.FileExists(t, filePath, "expected file %s to be skipped", file)
+				}
+			}
+		})
+	}
+}