From f39c71f5c2e0d7918643874fdb3265a76153c6b3 Mon Sep 17 00:00:00 2001 From: jason scott Date: Thu, 22 Jan 2026 17:28:34 -0700 Subject: [PATCH] Adding mitigation for CWE-23 + verification test cases Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jonathon Anderson --- CHANGELOG.md | 1 + internal/pkg/clean/clean.go | 47 +++++++- internal/pkg/clean/clean_test.go | 184 +++++++++++++++++++++++++++++++ 3 files changed, 228 insertions(+), 4 deletions(-) create mode 100644 internal/pkg/clean/clean_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 13da7ad8..6e561ba9 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/pkg/clean/clean.go b/internal/pkg/clean/clean.go index f37e0d9d..7b44a943 100644 --- a/internal/pkg/clean/clean.go +++ b/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 } diff --git a/internal/pkg/clean/clean_test.go b/internal/pkg/clean/clean_test.go new file mode 100644 index 00000000..f161f2ca --- /dev/null +++ b/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) + } + } + }) + } +}