From 3cf69c91d135ab88339f0a2c418c9007a00d7872 Mon Sep 17 00:00:00 2001 From: Ran Wurmbrand Date: Fri, 17 Jul 2026 16:46:30 +0300 Subject: [PATCH 1/2] fix: sanitize plugin manifest name in plugin-manager add to prevent path traversal and plugin override Signed-off-by: Ran Wurmbrand --- cmd/plugin-manager/add/add.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/cmd/plugin-manager/add/add.go b/cmd/plugin-manager/add/add.go index 8c708fd1..18746715 100644 --- a/cmd/plugin-manager/add/add.go +++ b/cmd/plugin-manager/add/add.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "syscall" "github.com/konveyor/crane/internal/flags" @@ -201,7 +202,23 @@ func (o *Options) run(args []string) error { return nil } +func validateFileInput(filepath, filename string) error { + if strings.Contains(filename, "/") || strings.Contains(filename, "\\") || strings.Contains(filename, "..") { + return fmt.Errorf("invalid plugin name %q: must not contain path separators or traversal sequences", filename) + } + + existingPath := filepath + "/" + filename + if _, err := os.Stat(existingPath); err == nil { + return fmt.Errorf("a plugin named %q already exists at %s, please remove it first before installing", filename, existingPath) + } + return nil +} + func downloadBinary(filepath string, filename string, url string, log *logrus.Logger) error { + if err := validateFileInput(filepath, filename); err != nil { + return err + } + var binaryContents io.Reader isUrl, url := plugin.IsUrl(url) if !isUrl { From 08c597055a9e55f7522a9ea767fd1dc6791a01a9 Mon Sep 17 00:00:00 2001 From: Ran Wurmbrand Date: Mon, 17 Aug 2026 13:17:32 +0300 Subject: [PATCH 2/2] fix: harden plugin name validation and add unit tests Use filepath.Base pattern instead of a character blocklist, reject backslashes explicitly, use os.Lstat so symlinks are not followed, fail closed on unexpected stat errors, create the binary with O_EXCL and 0755 perms, and add unit tests for validateFileInput. Signed-off-by: Ran Wurmbrand --- cmd/plugin-manager/add/add.go | 16 +++++---- cmd/plugin-manager/add/add_test.go | 58 ++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 7 deletions(-) create mode 100644 cmd/plugin-manager/add/add_test.go diff --git a/cmd/plugin-manager/add/add.go b/cmd/plugin-manager/add/add.go index 18746715..8f76b26d 100644 --- a/cmd/plugin-manager/add/add.go +++ b/cmd/plugin-manager/add/add.go @@ -202,14 +202,16 @@ func (o *Options) run(args []string) error { return nil } -func validateFileInput(filepath, filename string) error { - if strings.Contains(filename, "/") || strings.Contains(filename, "\\") || strings.Contains(filename, "..") { - return fmt.Errorf("invalid plugin name %q: must not contain path separators or traversal sequences", filename) +func validateFileInput(dir, filename string) error { + if filename == "" || filename == "." || filename == ".." || strings.ContainsRune(filename, '\\') || filename != filepath.Base(filename) { + return fmt.Errorf("invalid plugin name %q: must be a bare file name without path separators or traversal sequences", filename) } - existingPath := filepath + "/" + filename - if _, err := os.Stat(existingPath); err == nil { + existingPath := filepath.Join(dir, filename) + if _, err := os.Lstat(existingPath); err == nil { return fmt.Errorf("a plugin named %q already exists at %s, please remove it first before installing", filename, existingPath) + } else if !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("failed to check destination %s: %w", existingPath, err) } return nil } @@ -218,7 +220,7 @@ func downloadBinary(filepath string, filename string, url string, log *logrus.Lo if err := validateFileInput(filepath, filename); err != nil { return err } - + var binaryContents io.Reader isUrl, url := plugin.IsUrl(url) if !isUrl { @@ -246,7 +248,7 @@ func downloadBinary(filepath string, filename string, url string, log *logrus.Lo } // Create the file - pluginBinary, err := os.OpenFile(filepath+"/"+filename, syscall.O_RDWR|syscall.O_CREAT|syscall.O_TRUNC, 0777) + pluginBinary, err := os.OpenFile(filepath+"/"+filename, syscall.O_RDWR|syscall.O_CREAT|syscall.O_EXCL, 0755) if err != nil { return err } diff --git a/cmd/plugin-manager/add/add_test.go b/cmd/plugin-manager/add/add_test.go new file mode 100644 index 00000000..d8d2f164 --- /dev/null +++ b/cmd/plugin-manager/add/add_test.go @@ -0,0 +1,58 @@ +package add + +import ( + "os" + "path/filepath" + "testing" +) + +func TestValidateFileInput(t *testing.T) { + dir := t.TempDir() + + tests := []struct { + name string + filename string + wantErr bool + }{ + {name: "valid name", filename: "my-plugin", wantErr: false}, + {name: "empty name", filename: "", wantErr: true}, + {name: "dot", filename: ".", wantErr: true}, + {name: "dotdot", filename: "..", wantErr: true}, + {name: "forward slash", filename: "foo/bar", wantErr: true}, + {name: "back slash", filename: "foo\\bar", wantErr: true}, + {name: "traversal", filename: "../evil", wantErr: true}, + {name: "absolute path", filename: "/etc/passwd", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := validateFileInput(dir, tt.filename); (err != nil) != tt.wantErr { + t.Errorf("validateFileInput(%q, %q) error = %v, wantErr %v", dir, tt.filename, err, tt.wantErr) + } + }) + } +} + +func TestValidateFileInputExistingFile(t *testing.T) { + dir := t.TempDir() + filename := "existing-plugin" + if err := os.WriteFile(filepath.Join(dir, filename), []byte("x"), 0755); err != nil { + t.Fatal(err) + } + + if err := validateFileInput(dir, filename); err == nil { + t.Errorf("expected error for existing file, got nil") + } +} + +func TestValidateFileInputExistingSymlink(t *testing.T) { + dir := t.TempDir() + filename := "broken-link" + if err := os.Symlink(filepath.Join(dir, "does-not-exist"), filepath.Join(dir, filename)); err != nil { + t.Fatal(err) + } + + if err := validateFileInput(dir, filename); err == nil { + t.Errorf("expected error for existing broken symlink, got nil") + } +}