From 0afcda1a50daff85656691a3ceef053a351c351c Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Thu, 4 Jun 2026 21:23:56 +0300 Subject: [PATCH] Validate plugin binary path Signed-off-by: Stefan Prodan --- internal/plugin/catalog.go | 12 +++ internal/plugin/catalog_test.go | 57 +++++++++++++ internal/plugin/install.go | 6 ++ internal/plugin/install_test.go | 141 ++++++++++++++++++++++++++++++++ 4 files changed, 216 insertions(+) diff --git a/internal/plugin/catalog.go b/internal/plugin/catalog.go index 0cb76dbd..d1da4f9b 100644 --- a/internal/plugin/catalog.go +++ b/internal/plugin/catalog.go @@ -20,6 +20,8 @@ import ( "fmt" "io" "net/http" + "path/filepath" + "strings" "time" "github.com/hashicorp/go-retryablehttp" @@ -83,6 +85,16 @@ func (c *CatalogClient) FetchManifest(name string) (*plugintypes.Manifest, error return nil, fmt.Errorf("plugin %q has unexpected kind %q (expected %q)", name, manifest.Kind, plugintypes.PluginKind) } + // Bin becomes the on-disk binary path during install. Require a plain + // flux-prefixed filename: no separators or traversal, matching what the + // discovery layer surfaces. + if manifest.Bin == "" || + manifest.Bin != filepath.Base(manifest.Bin) || + !filepath.IsLocal(manifest.Bin) || + !strings.HasPrefix(manifest.Bin, pluginPrefix) { + return nil, fmt.Errorf("plugin %q has invalid bin %q (must be a plain filename prefixed with %q)", name, manifest.Bin, pluginPrefix) + } + return &manifest, nil } diff --git a/internal/plugin/catalog_test.go b/internal/plugin/catalog_test.go index 6ffdffa3..e3483b52 100644 --- a/internal/plugin/catalog_test.go +++ b/internal/plugin/catalog_test.go @@ -17,8 +17,10 @@ limitations under the License. package plugin import ( + "fmt" "net/http" "net/http/httptest" + "strings" "testing" plugintypes "github.com/fluxcd/flux2/v2/pkg/plugin" @@ -90,6 +92,61 @@ func TestFetchManifestNotFound(t *testing.T) { } } +func TestFetchManifestRejectsInvalidBin(t *testing.T) { + cases := []struct { + name string + bin string + }{ + {"parent traversal", "../evil"}, + {"nested traversal", "../../bin/evil"}, + {"absolute path", "/tmp/evil"}, + {"subdirectory", "sub/flux-evil"}, + {"missing prefix", "evil"}, + {"empty", ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + manifest := fmt.Sprintf(` +apiVersion: cli.fluxcd.io/v1beta1 +kind: Plugin +name: operator +description: Flux Operator CLI +bin: %q +versions: + - version: 0.45.0 + platforms: + - os: linux + arch: amd64 + url: https://example.com/archive.tar.gz + checksum: sha256:abc123 +`, tc.bin) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/operator.yaml" { + w.Write([]byte(manifest)) + return + } + http.NotFound(w, r) + })) + defer server.Close() + + client := &CatalogClient{ + BaseURL: server.URL + "/", + HTTPClient: server.Client(), + GetEnv: func(key string) string { return "" }, + } + + _, err := client.FetchManifest("operator") + if err == nil { + t.Fatal("expected error for invalid bin, got nil") + } + if !strings.Contains(err.Error(), "invalid bin") { + t.Errorf("expected 'invalid bin' error, got: %v", err) + } + }) + } +} + func TestFetchCatalog(t *testing.T) { catalog := ` apiVersion: cli.fluxcd.io/v1beta1 diff --git a/internal/plugin/install.go b/internal/plugin/install.go index 7b2129e6..1222d77b 100644 --- a/internal/plugin/install.go +++ b/internal/plugin/install.go @@ -84,7 +84,13 @@ func (inst *Installer) Install(pluginDir string, manifest *plugintypes.Manifest, // name (e.g. "flux-validate"). On Windows we always append ".exe". // For archives it's also the entry name we look up; for raw binaries // it's the rename target regardless of the URL's filename. + // + // Bin is remote-controlled and joined to pluginDir below, so it must be a + // plain filename: reject separators and traversal so the write can't escape. binName := manifest.Bin + if binName == "" || binName != filepath.Base(binName) || !filepath.IsLocal(binName) { + return fmt.Errorf("invalid plugin binary name %q: must be a plain filename", manifest.Bin) + } if runtime.GOOS == "windows" { binName += ".exe" } diff --git a/internal/plugin/install_test.go b/internal/plugin/install_test.go index dc2dd587..ccfeaf83 100644 --- a/internal/plugin/install_test.go +++ b/internal/plugin/install_test.go @@ -239,6 +239,147 @@ func TestInstallChecksumMismatch(t *testing.T) { } } +func TestInstallRejectsUnsafeBinName(t *testing.T) { + // A malicious manifest must not be able to write the binary outside the + // plugin directory via path traversal or an absolute path in Bin. + cases := []struct { + name string + bin string + }{ + {"parent traversal", "../evil"}, + {"nested traversal", "../../bin/evil"}, + {"absolute path", "/tmp/evil"}, + {"subdirectory", "sub/evil"}, + {"empty", ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + binaryContent := []byte("#!/bin/sh\necho pwned") + archive, err := createTestTarGz("flux-operator", binaryContent) + if err != nil { + t.Fatalf("failed to create test archive: %v", err) + } + checksum := fmt.Sprintf("sha256:%x", sha256.Sum256(archive)) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(archive) + })) + defer server.Close() + + pluginDir := t.TempDir() + + manifest := &plugintypes.Manifest{Name: "operator", Bin: tc.bin} + pv := &plugintypes.Version{Version: "0.45.0"} + plat := &plugintypes.Platform{ + OS: "linux", + Arch: "amd64", + URL: server.URL + "/archive.tar.gz", + Checksum: checksum, + } + + installer := &Installer{HTTPClient: server.Client()} + err = installer.Install(pluginDir, manifest, pv, plat) + if err == nil { + t.Fatal("expected error for unsafe binary name, got nil") + } + if !bytes.Contains([]byte(err.Error()), []byte("invalid plugin binary name")) { + t.Errorf("expected 'invalid plugin binary name' error, got: %v", err) + } + + // Nothing must have been written outside the plugin directory. + if _, statErr := os.Stat(filepath.Join(filepath.Dir(pluginDir), "evil")); statErr == nil { + t.Fatal("binary was written outside the plugin directory") + } + }) + } +} + +// Raw-binary write path (copyPluginBinary): a traversing Bin must be rejected. +func TestInstallRejectsUnsafeBinNameRawBinary(t *testing.T) { + // Bytes that don't match zip/gzip/tar magic — treated as a raw binary. + binaryContent := []byte("#!/bin/sh\necho pwned") + checksum := fmt.Sprintf("sha256:%x", sha256.Sum256(binaryContent)) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(binaryContent) + })) + defer server.Close() + + pluginDir := t.TempDir() + + manifest := &plugintypes.Manifest{Name: "validate", Bin: "../../evil"} + pv := &plugintypes.Version{Version: "1.2.3"} + plat := &plugintypes.Platform{ + OS: runtime.GOOS, + Arch: runtime.GOARCH, + URL: server.URL + "/download/flux-validate", + Checksum: checksum, + } + + installer := &Installer{HTTPClient: server.Client()} + err := installer.Install(pluginDir, manifest, pv, plat) + if err == nil { + t.Fatal("expected error for unsafe binary name, got nil") + } + if !bytes.Contains([]byte(err.Error()), []byte("invalid plugin binary name")) { + t.Errorf("expected 'invalid plugin binary name' error, got: %v", err) + } + if _, statErr := os.Stat(filepath.Join(filepath.Dir(pluginDir), "evil")); statErr == nil { + t.Fatal("binary was written outside the plugin directory") + } +} + +// Archive write path with ExtractPath set: the entry is a legitimate local +// path, but the destination derives from Bin, so a traversing Bin is rejected. +func TestInstallRejectsUnsafeBinNameWithExtractPath(t *testing.T) { + binaryContent := []byte("#!/bin/sh\necho pwned") + entries := []tarEntry{ + { + header: tar.Header{ + Name: "subdir/flux-operator", + Typeflag: tar.TypeReg, + Mode: 0o755, + }, + content: binaryContent, + }, + } + archive, err := createTestTarGzMulti(entries) + if err != nil { + t.Fatalf("failed to create archive: %v", err) + } + checksum := fmt.Sprintf("sha256:%x", sha256.Sum256(archive)) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(archive) + })) + defer server.Close() + + pluginDir := t.TempDir() + + manifest := &plugintypes.Manifest{Name: "operator", Bin: "../../evil"} + pv := &plugintypes.Version{Version: "0.45.0"} + plat := &plugintypes.Platform{ + OS: "linux", + Arch: "amd64", + URL: server.URL + "/archive.tar.gz", + Checksum: checksum, + ExtractPath: "subdir/flux-operator", + } + + installer := &Installer{HTTPClient: server.Client()} + err = installer.Install(pluginDir, manifest, pv, plat) + if err == nil { + t.Fatal("expected error for unsafe binary name, got nil") + } + if !bytes.Contains([]byte(err.Error()), []byte("invalid plugin binary name")) { + t.Errorf("expected 'invalid plugin binary name' error, got: %v", err) + } + if _, statErr := os.Stat(filepath.Join(filepath.Dir(pluginDir), "evil")); statErr == nil { + t.Fatal("binary was written outside the plugin directory") + } +} + func TestInstallBinaryNotInArchive(t *testing.T) { // Archive contains "wrong-name" instead of "flux-operator". archive, err := createTestTarGz("wrong-name", []byte("content"))