Adding mitigation for CWE-23 + verification test cases
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jonathon Anderson <janderson@ciq.com>
This commit is contained in:
jason scott
committed by
Jonathon Anderson
Jonathon Anderson
parent
a19bee3777
commit
f39c71f5c2
@@ -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
|
||||
}
|
||||
|
||||
184
internal/pkg/clean/clean_test.go
Normal file
184
internal/pkg/clean/clean_test.go
Normal file
@@ -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)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user