Merge pull request #5927 from fluxcd/fix-plugin-path

Validate plugin binary path

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
 }
 

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

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"
 	}

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"))