From 9b03f386aa9e7b6401048fc272971ee7e02a6a4d Mon Sep 17 00:00:00 2001 From: Teppei Fukuda Date: Wed, 10 Jun 2026 15:02:23 +0400 Subject: [PATCH 1/4] fix(oci): validate artifact filename Reject any filename that is not a local relative path via filepath.IsLocal before opening the destination file. GHSA-mcj4-mphf-j9ff (cherry picked from commit a72d9a4d997c25fbb6534e231b4e206c9b202b31) --- pkg/oci/artifact.go | 6 ++++++ pkg/oci/artifact_test.go | 31 ++++++++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/pkg/oci/artifact.go b/pkg/oci/artifact.go index a6cf3b2c78c9..431d71ec3e43 100644 --- a/pkg/oci/artifact.go +++ b/pkg/oci/artifact.go @@ -144,6 +144,12 @@ func (a *Artifact) Download(ctx context.Context, dir string, opt DownloadOption) fileName = v } + // Reject filenames that are not local relative paths to prevent path traversal + // (e.g., "../foo", "/foo", or "C:\foo" on Windows). + if !filepath.IsLocal(fileName) { + return xerrors.Errorf("invalid filename %q: must be a local relative path", fileName) + } + layerMediaType, err := layer.MediaType() if err != nil { return xerrors.Errorf("media type error: %w", err) diff --git a/pkg/oci/artifact_test.go b/pkg/oci/artifact_test.go index 7faf17e9bea9..83e47e724b1a 100644 --- a/pkg/oci/artifact_test.go +++ b/pkg/oci/artifact_test.go @@ -1,6 +1,7 @@ package oci_test import ( + "cmp" "errors" "os" "path/filepath" @@ -42,6 +43,7 @@ func TestArtifact_Download(t *testing.T) { name string input string mediaType string + annotation string layersReturns layersReturns want string wantErr string @@ -90,6 +92,33 @@ func TestArtifact_Download(t *testing.T) { }, wantErr: "unacceptable media type", }, + { + name: "sad: filename with parent traversal", + mediaType: "application/vnd.cncf.openpolicyagent.layer.v1.tar+gzip", + annotation: "../../../etc/passwd", + layersReturns: layersReturns{ + layers: []v1.Layer{flayer}, + }, + wantErr: `invalid filename "../../../etc/passwd"`, + }, + { + name: "sad: filename with absolute path", + mediaType: "application/vnd.cncf.openpolicyagent.layer.v1.tar+gzip", + annotation: "/etc/passwd", + layersReturns: layersReturns{ + layers: []v1.Layer{flayer}, + }, + wantErr: `invalid filename "/etc/passwd"`, + }, + { + name: "sad: filename with subdirectory traversal", + mediaType: "application/vnd.cncf.openpolicyagent.layer.v1.tar+gzip", + annotation: "foo/../../bar", + layersReturns: layersReturns{ + layers: []v1.Layer{flayer}, + }, + wantErr: `invalid filename "foo/../../bar"`, + }, } for _, tt := range tests { @@ -109,7 +138,7 @@ func TestArtifact_Download(t *testing.T) { Hex: "cba33656188782852f58993f45b68bfb8577f64cdcf02a604e3fc2afbeb5f2d8", }, Annotations: map[string]string{ - "org.opencontainers.image.title": "bundle.tar.gz", + "org.opencontainers.image.title": cmp.Or(tt.annotation, "bundle.tar.gz"), }, }, }, From 9f55b13df82bb5bce04648fc4a0e9e713b22652b Mon Sep 17 00:00:00 2001 From: Teppei Fukuda Date: Tue, 30 Jun 2026 10:03:28 +0400 Subject: [PATCH 2/4] Merge commit from fork Adopt Go's os.Root for plugin file operations so that all path access derived from a plugin name stays within the plugin root (~/.trivy/plugins). Add pkg/x/os.Root, a small wrapper over os.Root with a Join helper, and route install, uninstall, and plugin loading through it instead of joining names onto the root path directly. NewManager opens the plugin root eagerly and returns an error, and exposes Close; the package-level Run/Start helpers wrap it for callers outside the plugin subcommands. (cherry picked from commit d4213d7735c74e57f06c02ccb39ebca67abc7959) --- integration/plugin_test.go | 4 +- pkg/commands/app.go | 39 +++- pkg/plugin/index_test.go | 12 +- pkg/plugin/manager.go | 188 +++++++++++------- pkg/plugin/manager_test.go | 75 +++++-- pkg/plugin/manager_unix_test.go | 14 +- pkg/plugin/testdata/absolute_name/plugin.yaml | 5 + pkg/plugin/testdata/escaping_name/plugin.yaml | 5 + pkg/x/os/root.go | 50 +++++ pkg/x/os/root_test.go | 66 ++++++ 10 files changed, 346 insertions(+), 112 deletions(-) create mode 100644 pkg/plugin/testdata/absolute_name/plugin.yaml create mode 100644 pkg/plugin/testdata/escaping_name/plugin.yaml create mode 100644 pkg/x/os/root.go create mode 100644 pkg/x/os/root_test.go diff --git a/integration/plugin_test.go b/integration/plugin_test.go index f63e3ca0d85e..3fefd98e9a5b 100644 --- a/integration/plugin_test.go +++ b/integration/plugin_test.go @@ -44,8 +44,8 @@ func TestPlugin(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // We can overwrite stdout for `_default_Manager` only once. - // So we need to clear the temporary stdout file before each test case. + // The output plugin appends to os.Stdout (redirected to tempStdOut), + // so clear it before each test case. clearFile(t, tempStdOut) t.Setenv("XDG_DATA_HOME", t.TempDir()) diff --git a/pkg/commands/app.go b/pkg/commands/app.go index c528c06afc3c..d3d579d7ed90 100644 --- a/pkg/commands/app.go +++ b/pkg/commands/app.go @@ -119,7 +119,17 @@ func loadPluginCommands() []*cobra.Command { ctx := context.Background() var commands []*cobra.Command - plugins, err := plugin.NewManager().LoadAll(ctx) + // Avoid creating the plugin directory on every run; nothing to load if it is absent. + if !plugin.DirExists() { + return nil + } + manager, err := plugin.NewManager() + if err != nil { + log.WarnContext(ctx, "Failed to initialize the plugin manager", log.Err(err)) + return nil + } + defer manager.Close() + plugins, err := manager.LoadAll(ctx) if err != nil { log.DebugContext(ctx, "No plugins loaded") return nil @@ -750,6 +760,7 @@ func NewConfigCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { func NewPluginCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { var pluginOptions flag.Options + var manager *plugin.Manager pluginFlags := &flag.Flags{ globalFlags, } @@ -766,6 +777,16 @@ func NewPluginCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { if err != nil { return err } + manager, err = plugin.NewManager() + if err != nil { + return xerrors.Errorf("plugin manager error: %w", err) + } + return nil + }, + PersistentPostRunE: func(_ *cobra.Command, _ []string) error { + if manager != nil { + return manager.Close() + } return nil }, } @@ -787,7 +808,7 @@ func NewPluginCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { DisableFlagsInUseLine: true, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - if _, err := plugin.Install(cmd.Context(), args[0], plugin.Options{Insecure: pluginOptions.Insecure}); err != nil { + if _, err := manager.Install(cmd.Context(), args[0], plugin.Options{Insecure: pluginOptions.Insecure}); err != nil { return xerrors.Errorf("plugin install error: %w", err) } return nil @@ -802,7 +823,7 @@ func NewPluginCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { SilenceUsage: true, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - if err := plugin.Uninstall(cmd.Context(), args[0]); err != nil { + if err := manager.Uninstall(cmd.Context(), args[0]); err != nil { return xerrors.Errorf("plugin uninstall error: %w", err) } return nil @@ -817,7 +838,7 @@ func NewPluginCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { Short: "List installed plugin", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { - if err := plugin.List(cmd.Context()); err != nil { + if err := manager.List(cmd.Context()); err != nil { return xerrors.Errorf("plugin list display error: %w", err) } return nil @@ -831,7 +852,7 @@ func NewPluginCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { SilenceUsage: true, Args: cobra.ExactArgs(1), RunE: func(_ *cobra.Command, args []string) error { - if err := plugin.Information(args[0]); err != nil { + if err := manager.Information(args[0]); err != nil { return xerrors.Errorf("plugin information display error: %w", err) } return nil @@ -846,7 +867,7 @@ func NewPluginCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { Short: "Run a plugin on the fly", Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return plugin.Run(cmd.Context(), args[0], plugin.Options{ + return manager.Run(cmd.Context(), args[0], plugin.Options{ Args: args[1:], Insecure: pluginOptions.Insecure, }) @@ -860,7 +881,7 @@ func NewPluginCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { SilenceUsage: true, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { - if err := plugin.Update(cmd.Context(), plugin.Options{Insecure: pluginOptions.Insecure}); err != nil { + if err := manager.Update(cmd.Context(), plugin.Options{Insecure: pluginOptions.Insecure}); err != nil { return xerrors.Errorf("plugin update error: %w", err) } return nil @@ -878,7 +899,7 @@ func NewPluginCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { if len(args) == 1 { keyword = args[0] } - return plugin.Search(cmd.Context(), keyword) + return manager.Search(cmd.Context(), keyword) }, }, &cobra.Command{ @@ -888,7 +909,7 @@ func NewPluginCommand(globalFlags *flag.GlobalFlagGroup) *cobra.Command { SilenceErrors: true, SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { - if err := plugin.Upgrade(cmd.Context(), args); err != nil { + if err := manager.Upgrade(cmd.Context(), args); err != nil { return xerrors.Errorf("plugin upgrade error: %w", err) } return nil diff --git a/pkg/plugin/index_test.go b/pkg/plugin/index_test.go index 1c5c6745a277..39027eac2af9 100644 --- a/pkg/plugin/index_test.go +++ b/pkg/plugin/index_test.go @@ -24,8 +24,9 @@ func TestManager_Update(t *testing.T) { })) t.Cleanup(ts.Close) - manager := plugin.NewManager(plugin.WithIndexURL(ts.URL + "/index.yaml")) - err := manager.Update(t.Context(), plugin.Options{}) + manager, err := plugin.NewManager(plugin.WithIndexURL(ts.URL + "/index.yaml")) + require.NoError(t, err) + err = manager.Update(t.Context(), plugin.Options{}) require.NoError(t, err) indexPath := filepath.Join(tempDir, ".trivy", "plugins", "index.yaml") @@ -65,7 +66,7 @@ bar A bar plugin { name: "no index", keyword: "", - dir: "unknown", + dir: t.TempDir(), // a fresh dir without index.yaml wantErr: "plugin index not found", }, } @@ -74,8 +75,9 @@ bar A bar plugin t.Setenv("XDG_DATA_HOME", tt.dir) var got bytes.Buffer - m := plugin.NewManager(plugin.WithWriter(&got)) - err := m.Search(t.Context(), tt.keyword) + m, err := plugin.NewManager(plugin.WithWriter(&got)) + require.NoError(t, err) + err = m.Search(t.Context(), tt.keyword) if tt.wantErr != "" { require.ErrorContains(t, err, tt.wantErr) return diff --git a/pkg/plugin/manager.go b/pkg/plugin/manager.go index 799481b98f14..1b778bc3e82d 100644 --- a/pkg/plugin/manager.go +++ b/pkg/plugin/manager.go @@ -2,8 +2,10 @@ package plugin import ( "context" + "errors" "fmt" "io" + "io/fs" "os" "path/filepath" "strings" @@ -18,15 +20,13 @@ import ( ftypes "github.com/aquasecurity/trivy/pkg/fanal/types" "github.com/aquasecurity/trivy/pkg/log" "github.com/aquasecurity/trivy/pkg/utils/fsutils" + xos "github.com/aquasecurity/trivy/pkg/x/os" + xslices "github.com/aquasecurity/trivy/pkg/x/slices" ) const configFile = "plugin.yaml" -var ( - pluginsDir = "plugins" - - _defaultManager *Manager -) +var pluginsDir = "plugins" type ManagerOption func(indexer *Manager) @@ -50,50 +50,69 @@ func WithIndexURL(indexURL string) ManagerOption { // Manager manages the plugins type Manager struct { - w io.Writer - indexURL string - logger *log.Logger - pluginRoot string - indexPath string + w io.Writer + indexURL string + logger *log.Logger + indexPath string + // root confines all plugin-name-derived file access to the plugin root. + root *xos.Root +} + +// dir returns the plugin root directory (e.g. ~/.trivy/plugins). +func dir() string { + return filepath.Join(fsutils.TrivyHomeDir(), pluginsDir) } -func NewManager(opts ...ManagerOption) *Manager { - root := filepath.Join(fsutils.TrivyHomeDir(), pluginsDir) +// DirExists reports whether the plugin root directory exists. It lets callers +// skip creating a Manager (which creates the directory) when no plugin is +// installed, e.g. on every Trivy run while building the command tree. +func DirExists() bool { + return fsutils.DirExists(dir()) +} + +func NewManager(opts ...ManagerOption) (*Manager, error) { + root, err := xos.NewRoot(dir()) + if err != nil { + return nil, xerrors.Errorf("failed to open the plugin root: %w", err) + } m := &Manager{ - w: os.Stdout, - indexURL: indexURL, - logger: log.WithPrefix("plugin"), - pluginRoot: root, - indexPath: filepath.Join(root, "index.yaml"), + w: os.Stdout, + indexURL: indexURL, + logger: log.WithPrefix("plugin"), + root: root, + indexPath: filepath.Join(root.Name(), "index.yaml"), } for _, opt := range opts { opt(m) } - return m + return m, nil } -func defaultManager() *Manager { - if _defaultManager == nil { - _defaultManager = NewManager() - } - return _defaultManager +// Close releases the plugin root handle. +func (m *Manager) Close() error { + return m.root.Close() } -func Install(ctx context.Context, name string, opts Options) (Plugin, error) { - return defaultManager().Install(ctx, name, opts) +// Run installs and runs the plugin with a one-off manager. It is a convenience +// for callers outside the plugin subcommands (e.g. run-as-plugin, output plugins). +func Run(ctx context.Context, name string, opts Options) error { + m, err := NewManager() + if err != nil { + return xerrors.Errorf("plugin manager init error: %w", err) + } + defer m.Close() + return m.Run(ctx, name, opts) } + +// Start starts the plugin with a one-off manager. See Run. func Start(ctx context.Context, name string, opts Options) (Wait, error) { - return defaultManager().Start(ctx, name, opts) -} -func Run(ctx context.Context, name string, opts Options) error { - return defaultManager().Run(ctx, name, opts) + m, err := NewManager() + if err != nil { + return nil, xerrors.Errorf("plugin manager init error: %w", err) + } + defer m.Close() + return m.Start(ctx, name, opts) } -func Upgrade(ctx context.Context, names []string) error { return defaultManager().Upgrade(ctx, names) } -func Uninstall(ctx context.Context, name string) error { return defaultManager().Uninstall(ctx, name) } -func Information(name string) error { return defaultManager().Information(name) } -func List(ctx context.Context) error { return defaultManager().List(ctx) } -func Update(ctx context.Context, opts Options) error { return defaultManager().Update(ctx, opts) } -func Search(ctx context.Context, keyword string) error { return defaultManager().Search(ctx, keyword) } // Install installs a plugin func (m *Manager) Install(ctx context.Context, arg string, opts Options) (Plugin, error) { @@ -120,29 +139,42 @@ func (m *Manager) install(ctx context.Context, src string, opts Options) (Plugin if entries, err := os.ReadDir(tempDir); err != nil { return Plugin{}, xerrors.Errorf("failed to read %s: %w", tempDir, err) } else if len(entries) == 1 && entries[0].IsDir() { - //　A single directory may be contained within an archive file. + // A single directory may be contained within an archive file. // e.g. https://github.com/aquasecurity/trivy-plugin-referrer/archive/refs/heads/main.zip tempDir = filepath.Join(tempDir, entries[0].Name()) } m.logger.DebugContext(ctx, "Loading the plugin metadata...") - plugin, err := m.loadMetadata(tempDir) + // tempDir is our own download dir, so a plain read is fine. + mf, err := os.Open(filepath.Join(tempDir, configFile)) + if err != nil { + return Plugin{}, xerrors.Errorf("file open error: %w", err) + } + plugin, err := decodeMetadata(mf) + _ = mf.Close() if err != nil { return Plugin{}, xerrors.Errorf("failed to load the plugin metadata: %w", err) } - if err = plugin.install(ctx, plugin.Dir(), tempDir, opts); err != nil { + // Resolve the destination; the downloader creates it. + dst, err := m.root.Join(plugin.Name) + if err != nil { + return Plugin{}, xerrors.Errorf("failed to create the plugin directory %q: %w", plugin.Name, err) + } + plugin.dir = dst + + if err = plugin.install(ctx, dst, tempDir, opts); err != nil { return Plugin{}, xerrors.Errorf("failed to install the plugin: %w", err) } - // Copy plugin.yaml into the plugin dir - f, err := os.Create(filepath.Join(plugin.Dir(), configFile)) + // Copy plugin.yaml into the plugin directory. + out, err := m.root.Create(filepath.Join(plugin.Name, configFile)) if err != nil { return Plugin{}, xerrors.Errorf("failed to create plugin.yaml: %w", err) } - defer f.Close() + defer out.Close() - if err = yaml.NewEncoder(f).Encode(plugin); err != nil { + if err = yaml.NewEncoder(out).Encode(plugin); err != nil { return Plugin{}, xerrors.Errorf("yaml encode error: %w", err) } @@ -152,14 +184,15 @@ func (m *Manager) install(ctx context.Context, src string, opts Options) (Plugin return plugin, nil } -// Uninstall installs the plugin +// Uninstall uninstalls the plugin func (m *Manager) Uninstall(ctx context.Context, name string) error { - pluginDir := filepath.Join(m.pluginRoot, name) - if !fsutils.DirExists(pluginDir) { + if _, err := m.root.Stat(name); errors.Is(err, fs.ErrNotExist) { m.logger.ErrorContext(ctx, "No such plugin") return nil + } else if err != nil { + return xerrors.Errorf("failed to check the plugin %q: %w", name, err) } - if err := os.RemoveAll(pluginDir); err != nil { + if err := m.root.RemoveAll(name); err != nil { return xerrors.Errorf("failed to uninstall the plugin: %w", err) } m.logger.InfoContext(ctx, "Plugin successfully uninstalled", log.String("name", name)) @@ -194,7 +227,7 @@ func (m *Manager) List(ctx context.Context) error { } func (m *Manager) list(ctx context.Context) (string, error) { - if _, err := os.Stat(m.pluginRoot); err != nil { + if _, err := os.Stat(m.root.Name()); err != nil { if os.IsNotExist(err) { return "No Installed Plugins", nil } @@ -266,9 +299,9 @@ func (m *Manager) upgrade(ctx context.Context, name string) error { // LoadAll loads all plugins func (m *Manager) LoadAll(ctx context.Context) ([]Plugin, error) { - dirs, err := os.ReadDir(m.pluginRoot) + dirs, err := os.ReadDir(m.root.Name()) if err != nil { - return nil, xerrors.Errorf("failed to read %s: %w", m.pluginRoot, err) + return nil, xerrors.Errorf("failed to read %s: %w", m.root.Name(), err) } var plugins []Plugin @@ -276,7 +309,7 @@ func (m *Manager) LoadAll(ctx context.Context) ([]Plugin, error) { if !d.IsDir() { continue } - plugin, err := m.loadMetadata(filepath.Join(m.pluginRoot, d.Name())) + plugin, err := m.loadMetadata(d.Name()) if err != nil { m.logger.WarnContext(ctx, "Plugin load error", log.Err(err)) continue @@ -314,47 +347,36 @@ func (m *Manager) Run(ctx context.Context, name string, opts Options) error { } func (m *Manager) load(name string) (Plugin, error) { - pluginDir := filepath.Join(m.pluginRoot, name) - if _, err := os.Stat(pluginDir); err != nil { - if os.IsNotExist(err) { - return Plugin{}, xerrors.Errorf("could not find a plugin called '%s', did you install it?", name) - } + if _, err := m.root.Stat(name); errors.Is(err, fs.ErrNotExist) { + return Plugin{}, xerrors.Errorf("could not find a plugin called '%s', did you install it?", name) + } else if err != nil { return Plugin{}, xerrors.Errorf("plugin stat error: %w", err) } - - plugin, err := m.loadMetadata(pluginDir) + plugin, err := m.loadMetadata(name) if err != nil { return Plugin{}, xerrors.Errorf("unable to load plugin metadata: %w", err) } - return plugin, nil } -func (m *Manager) loadMetadata(dir string) (Plugin, error) { - filePath := filepath.Join(dir, configFile) - f, err := os.Open(filePath) +// loadMetadata reads an installed plugin's metadata and records its directory. +// Both reads go through m.root, so name is confined to the root here. +func (m *Manager) loadMetadata(name string) (Plugin, error) { + f, err := m.root.Open(filepath.Join(name, configFile)) if err != nil { return Plugin{}, xerrors.Errorf("file open error: %w", err) } defer f.Close() - var plugin Plugin - if err = yaml.NewDecoder(f).Decode(&plugin); err != nil { - return Plugin{}, xerrors.Errorf("yaml decode error: %w", err) - } - - if plugin.Name == "" { - return Plugin{}, xerrors.Errorf("'name' is empty") + plugin, err := decodeMetadata(f) + if err != nil { + return Plugin{}, xerrors.Errorf("metadata decode error: %w", err) } - // e.g. ~/.trivy/plugins/kubectl - plugin.dir = filepath.Join(m.pluginRoot, plugin.Name) - - if plugin.Summary == "" && plugin.Usage != "" { - plugin.Summary = plugin.Usage // For backward compatibility - plugin.Usage = "" + plugin.dir, err = m.root.Join(name) + if err != nil { + return Plugin{}, xerrors.Errorf("failed to resolve the plugin directory: %w", err) } - return plugin, nil } @@ -400,3 +422,19 @@ func (m *Manager) parseArg(ctx context.Context, arg string) Input { version: after, } } + +// decodeMetadata decodes a plugin.yaml stream into a Plugin. +func decodeMetadata(r io.Reader) (Plugin, error) { + var plugin Plugin + if err := yaml.NewDecoder(r).Decode(&plugin); err != nil { + return Plugin{}, xerrors.Errorf("yaml decode error: %w", err) + } + if plugin.Name == "" { + return Plugin{}, xerrors.Errorf("'name' is empty") + } + if plugin.Summary == "" && plugin.Usage != "" { + plugin.Summary = plugin.Usage // For backward compatibility + plugin.Usage = "" + } + return plugin, nil +} diff --git a/pkg/plugin/manager_test.go b/pkg/plugin/manager_test.go index 610f361fd034..2306e0770ca5 100644 --- a/pkg/plugin/manager_test.go +++ b/pkg/plugin/manager_test.go @@ -51,6 +51,28 @@ installed: require.NoError(t, err) } +func TestNewManager(t *testing.T) { + t.Run("creates the plugin root", func(t *testing.T) { + home := t.TempDir() + t.Setenv("XDG_DATA_HOME", home) + + m, err := plugin.NewManager() + require.NoError(t, err) + t.Cleanup(func() { _ = m.Close() }) + + assert.DirExists(t, filepath.Join(home, ".trivy", "plugins")) + }) + + t.Run("data home is a file", func(t *testing.T) { + f := filepath.Join(t.TempDir(), "not-a-dir") + require.NoError(t, os.WriteFile(f, []byte("x"), 0o600)) + t.Setenv("XDG_DATA_HOME", f) + + _, err := plugin.NewManager() + require.ErrorContains(t, err, "failed to") + }) +} + func TestManager_Run(t *testing.T) { if runtime.GOOS == "windows" { // the test.sh script can't be run on windows so skipping @@ -229,7 +251,9 @@ func TestManager_Uninstall(t *testing.T) { require.NoError(t, err) // Uninstall the plugin - err = plugin.NewManager().Uninstall(ctx, pluginName) + m, err := plugin.NewManager() + require.NoError(t, err) + err = m.Uninstall(ctx, pluginName) require.NoError(t, err) assert.NoDirExists(t, pluginDir) }) @@ -239,10 +263,28 @@ func TestManager_Uninstall(t *testing.T) { buf := bytes.NewBuffer(nil) slog.SetDefault(slog.New(log.NewHandler(buf, &log.Options{Level: log.LevelInfo}))) - err := plugin.NewManager().Uninstall(ctx, pluginName) + m, err := plugin.NewManager() + require.NoError(t, err) + err = m.Uninstall(ctx, pluginName) require.NoError(t, err) assert.Equal(t, "2021-08-25T12:20:30Z\tERROR\t[plugin] No such plugin\n", buf.String()) }) + + t.Run("escaping name is rejected", func(t *testing.T) { + // A sentinel file outside the plugin root must not be removed. + sentinelDir := filepath.Join(tempDir, ".trivy", "secret") + require.NoError(t, os.MkdirAll(sentinelDir, 0o755)) + sentinel := filepath.Join(sentinelDir, "keep.txt") + require.NoError(t, os.WriteFile(sentinel, []byte("keep"), 0o600)) + + // "../secret" escapes the plugin root, so os.Root rejects it: Uninstall + // surfaces the error (not "No such plugin") and removes nothing. + m, err := plugin.NewManager() + require.NoError(t, err) + err = m.Uninstall(ctx, "../secret") + require.Error(t, err) + assert.FileExists(t, sentinel) + }) } func TestManager_Information(t *testing.T) { @@ -268,7 +310,8 @@ description: A simple test plugin` require.NoError(t, err) var got bytes.Buffer - manager := plugin.NewManager(plugin.WithWriter(&got)) + manager, err := plugin.NewManager(plugin.WithWriter(&got)) + require.NoError(t, err) // Get Information for the plugin err = manager.Information(pluginName) @@ -289,10 +332,9 @@ Plugin: test_plugin func TestManager_LoadAll(t *testing.T) { tests := []struct { - name string - dir string - want []plugin.Plugin - wantErr string + name string + dir string + want []plugin.Plugin }{ { name: "happy path", @@ -317,21 +359,13 @@ func TestManager_LoadAll(t *testing.T) { }, }, }, - { - name: "sad path", - dir: "sad", - wantErr: "failed to read", - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Setenv("XDG_DATA_HOME", tt.dir) - - got, err := plugin.NewManager().LoadAll(t.Context()) - if tt.wantErr != "" { - require.ErrorContains(t, err, tt.wantErr) - return - } + m, err := plugin.NewManager() + require.NoError(t, err) + got, err := m.LoadAll(t.Context()) require.NoError(t, err) require.Len(t, got, len(tt.want)) for i := range tt.want { @@ -358,13 +392,14 @@ func TestManager_Upgrade(t *testing.T) { }) ctx := t.Context() - m := plugin.NewManager() + m, err := plugin.NewManager() + require.NoError(t, err) // verify initial version verifyVersion(t, ctx, m, pluginName, pluginVersion) // Upgrade the existing plugin - err := m.Upgrade(ctx, nil) + err = m.Upgrade(ctx, nil) require.NoError(t, err) // verify plugin updated diff --git a/pkg/plugin/manager_unix_test.go b/pkg/plugin/manager_unix_test.go index 98a8baa01ab6..462de8780e0c 100644 --- a/pkg/plugin/manager_unix_test.go +++ b/pkg/plugin/manager_unix_test.go @@ -190,6 +190,16 @@ func TestManager_Install(t *testing.T) { pluginName: "testdata/no_yaml", wantErr: "file open error", }, + { + name: "name escapes the root", + pluginName: "testdata/escaping_name", + wantErr: "failed to create the plugin directory", + }, + { + name: "absolute name escapes the root", + pluginName: "testdata/absolute_name", + wantErr: "failed to create the plugin directory", + }, } for _, tt := range tests { @@ -214,7 +224,9 @@ func TestManager_Install(t *testing.T) { ctx := clock.With(t.Context(), time.Date(2021, 8, 25, 12, 20, 30, 5, time.UTC)) - got, err := plugin.NewManager(plugin.WithLogger(logger)).Install(ctx, tt.pluginName, plugin.Options{ + manager, err := plugin.NewManager(plugin.WithLogger(logger)) + require.NoError(t, err) + got, err := manager.Install(ctx, tt.pluginName, plugin.Options{ Platform: ftypes.Platform{ Platform: &v1.Platform{ Architecture: "amd64", diff --git a/pkg/plugin/testdata/absolute_name/plugin.yaml b/pkg/plugin/testdata/absolute_name/plugin.yaml new file mode 100644 index 000000000000..a35be3f1ab6b --- /dev/null +++ b/pkg/plugin/testdata/absolute_name/plugin.yaml @@ -0,0 +1,5 @@ +name: "/escaped_abs" +repository: testdata/absolute_name +version: "0.1.0" +summary: test +description: test diff --git a/pkg/plugin/testdata/escaping_name/plugin.yaml b/pkg/plugin/testdata/escaping_name/plugin.yaml new file mode 100644 index 000000000000..d43c3590e81e --- /dev/null +++ b/pkg/plugin/testdata/escaping_name/plugin.yaml @@ -0,0 +1,5 @@ +name: "../escaped_plugin" +repository: testdata/escaping_name +version: "0.1.0" +summary: test +description: test diff --git a/pkg/x/os/root.go b/pkg/x/os/root.go new file mode 100644 index 000000000000..257bb9d960e0 --- /dev/null +++ b/pkg/x/os/root.go @@ -0,0 +1,50 @@ +package os + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + + "golang.org/x/xerrors" +) + +// Root is a traversal-resistant handle to a directory. It embeds *os.Root +// (https://pkg.go.dev/os#Root), so every name passed to its methods is confined +// to the directory. Root adds NewRoot and Join. +type Root struct { + *os.Root +} + +// NewRoot creates dir (0700) if necessary and opens it as a Root. Like +// os.OpenRoot, it returns an error if the directory cannot be opened. +func NewRoot(dir string) (*Root, error) { + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, xerrors.Errorf("failed to create directory %q: %w", dir, err) + } + root, err := os.OpenRoot(dir) + if err != nil { + return nil, xerrors.Errorf("failed to open root %q: %w", dir, err) + } + return &Root{Root: root}, nil +} + +// Join validates name and returns it joined to the root directory. It rejects +// names that escape the root either literally (filepath.IsLocal handles "..", +// absolute paths, and escapes hidden behind a missing component) or through an +// existing symlink (the embedded os.Root). A name that does not exist yet is +// allowed, so the caller can resolve a destination before creating it. +// +// The result is a plain string, so it is not protected against a later symlink +// swap (TOCTOU): use it to hand a confined path to an external tool, and use the +// os.Root methods directly for ordinary I/O. +func (r *Root) Join(name string) (string, error) { + if !filepath.IsLocal(name) { + return "", xerrors.Errorf("name %q is not local", name) + } + // An existing symlink that escapes is rejected; a not-yet-existing name is fine. + if _, err := r.Stat(name); err != nil && !errors.Is(err, fs.ErrNotExist) { + return "", xerrors.Errorf("%q: %w", name, err) + } + return filepath.Join(r.Name(), name), nil +} diff --git a/pkg/x/os/root_test.go b/pkg/x/os/root_test.go new file mode 100644 index 000000000000..53a57bbc03bb --- /dev/null +++ b/pkg/x/os/root_test.go @@ -0,0 +1,66 @@ +package os_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + xos "github.com/aquasecurity/trivy/pkg/x/os" +) + +func TestNewRoot_CreatesAndConfines(t *testing.T) { + base := filepath.Join(t.TempDir(), "root") + + r, err := xos.NewRoot(base) + require.NoError(t, err) + t.Cleanup(func() { _ = r.Close() }) + + assert.DirExists(t, base) // NewRoot creates the directory + assert.Equal(t, base, r.Name()) // promoted os.Root.Name() + + // Confined operations (promoted from os.Root) reject escaping names. + _, err = r.Stat("../escape") + require.Error(t, err) + require.Error(t, r.MkdirAll("../escape", 0o700)) +} + +func TestRoot_Join(t *testing.T) { + base := filepath.Join(t.TempDir(), "root") + r, err := xos.NewRoot(base) + require.NoError(t, err) + t.Cleanup(func() { _ = r.Close() }) + + // An existing directory resolves to its path under the root. + require.NoError(t, r.MkdirAll("plugin", 0o700)) + got, err := r.Join("plugin") + require.NoError(t, err) + assert.Equal(t, filepath.Join(base, "plugin"), got) + + // A not-yet-existing local name is allowed (the caller creates it later). + got, err = r.Join("does-not-exist") + require.NoError(t, err) + assert.Equal(t, filepath.Join(base, "does-not-exist"), got) + + // Escaping names are rejected, including escapes hidden behind a missing + // component: os.Root.Stat reports those as ErrNotExist, so filepath.IsLocal + // is what catches them. + for _, name := range []string{"../escape", "/etc", "..", "a/../../etc", "missing/../../etc"} { + _, err := r.Join(name) + require.Error(t, err, name) + } + + // A symlink that stays within the root is allowed. + require.NoError(t, os.WriteFile(filepath.Join(base, "target.txt"), []byte("x"), 0o600)) + require.NoError(t, os.Symlink("target.txt", filepath.Join(base, "inside"))) + got, err = r.Join("inside") + require.NoError(t, err) + assert.Equal(t, filepath.Join(base, "inside"), got) + + // An existing symlink that points outside the root is rejected. + require.NoError(t, os.Symlink(t.TempDir(), filepath.Join(base, "link"))) + _, err = r.Join("link") + require.Error(t, err) +} From 56c6d9d459b38202924b4551ff4a2df5cef27d40 Mon Sep 17 00:00:00 2001 From: Kaiyong Chen Date: Tue, 8 Sep 2026 23:04:16 +0800 Subject: [PATCH 3/4] fix(security): bound Helm archive decompression (CVE-2026-54448) Upstream fixed this in 0.71.0 by replacing the custom Helm archive parser with the Helm SDK loaders (#10718) - 143 files, +1746/-6984. That is far too much to backport onto this branch, so this applies the same protection to the parser we have. unpackArchive read every regular entry with io.ReadAll(tr) and no limit, so a small crafted .tgz could expand to gigabytes and OOM the scanner. It now enforces the Helm SDK's own published limits, taken from the loader package this tree already depends on (helm.sh/helm/v3 v3.20.2): loader.MaxDecompressedFileSize 5 MiB per entry loader.MaxDecompressedChartSize 100 MiB per archive The per-archive budget is decremented by bytes actually read, via io.LimitReader(tr, remaining+1), so a header that understates the entry size is caught rather than trusted. A chart Helm itself would refuse to load is now refused here too. Also drops the unused xslices import that came in with the plugin-manager backport: upstream had refactored lo.Map to xslices.Map in unrelated cleanup, and this branch still uses lo.Map. Verified: go build ./pkg/... ./cmd/... passes, and go test for pkg/iac/scanners/helm/..., pkg/plugin/... and pkg/oci/... is green, including the escaping_name / absolute_name plugin cases the CVE-2026-63328 backport adds. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/iac/scanners/helm/parser/parser_tar.go | 21 ++++++++++++++++++++- pkg/plugin/manager.go | 1 - 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/pkg/iac/scanners/helm/parser/parser_tar.go b/pkg/iac/scanners/helm/parser/parser_tar.go index 63da1aa97d60..a18e609ba48f 100644 --- a/pkg/iac/scanners/helm/parser/parser_tar.go +++ b/pkg/iac/scanners/helm/parser/parser_tar.go @@ -11,6 +11,8 @@ import ( "path" "path/filepath" + "helm.sh/helm/v3/pkg/chart/loader" + "github.com/aquasecurity/trivy/pkg/iac/detection" "github.com/aquasecurity/trivy/pkg/log" "github.com/aquasecurity/trivy/pkg/mapfs" @@ -41,6 +43,11 @@ func (p *Parser) unpackArchive(srcFS fs.FS, targetFS *mapfs.FS, archivePath stri checkExistedChart := true symlinks := make(map[string]string) + // Bound how much the archive is allowed to decompress to. Without this a small + // crafted .tgz can expand to gigabytes and OOM the scanner. The limits are the + // Helm SDK's own, so a chart Helm would refuse to load is refused here too. + remainingSize := loader.MaxDecompressedChartSize + for { header, err := tr.Next() if err != nil { @@ -71,10 +78,22 @@ func (p *Parser) unpackArchive(srcFS fs.FS, targetFS *mapfs.FS, archivePath stri return err } case tar.TypeReg: - data, err := io.ReadAll(tr) + if header.Size > loader.MaxDecompressedFileSize { + return fmt.Errorf("decompressed chart file %q is larger than the maximum file size %d", + name, loader.MaxDecompressedFileSize) + } + + // Read at most the remaining budget plus one byte, so an entry whose real + // size exceeds the header (or the budget) is detected rather than trusted. + data, err := io.ReadAll(io.LimitReader(tr, remainingSize+1)) if err != nil { return fmt.Errorf("read file: %w", err) } + if int64(len(data)) > remainingSize { + return fmt.Errorf("decompressed chart is larger than the maximum size %d", + loader.MaxDecompressedChartSize) + } + remainingSize -= int64(len(data)) p.logger.Debug("Unpacking tar entry", log.FilePath(targetPath)) if err := writeFile(targetFS, data, targetPath); err != nil { diff --git a/pkg/plugin/manager.go b/pkg/plugin/manager.go index 1b778bc3e82d..dd76368273d1 100644 --- a/pkg/plugin/manager.go +++ b/pkg/plugin/manager.go @@ -21,7 +21,6 @@ import ( "github.com/aquasecurity/trivy/pkg/log" "github.com/aquasecurity/trivy/pkg/utils/fsutils" xos "github.com/aquasecurity/trivy/pkg/x/os" - xslices "github.com/aquasecurity/trivy/pkg/x/slices" ) const configFile = "plugin.yaml" From 7d12ae65064c1c0081abaf7848f5be1e6d8d4fe0 Mon Sep 17 00:00:00 2001 From: Kaiyong Chen Date: Tue, 8 Sep 2026 23:09:05 +0800 Subject: [PATCH 4/4] test: cover the Helm archive decompression limits The two cherry-picked backports carry upstream tests (pkg/oci/artifact_test.go for CVE-2026-55092; manager_test.go, manager_unix_test.go, pkg/x/os/root_test.go and the absolute_name / escaping_name plugin fixtures for CVE-2026-63328). The hand-written CVE-2026-54448 fix had none, so this adds one. The test builds gzipped tars whose entries are zero bytes on disk but declare large sizes - the shape of a decompression bomb - and asserts three things: an entry over the per-file limit is rejected, entries that together exceed the per-archive budget are rejected, and an archive within both limits still parses. Helm exports its limits as vars, so the test lowers them and restores them in t.Cleanup rather than building a 100 MiB fixture. Confirmed to be a real regression test: with parser_tar.go reverted to the pre-fix version both failing cases fail, and they pass again with the fix. Co-Authored-By: Claude Opus 5 (1M context) --- .../scanners/helm/parser/parser_tar_test.go | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 pkg/iac/scanners/helm/parser/parser_tar_test.go diff --git a/pkg/iac/scanners/helm/parser/parser_tar_test.go b/pkg/iac/scanners/helm/parser/parser_tar_test.go new file mode 100644 index 000000000000..39da0189ecca --- /dev/null +++ b/pkg/iac/scanners/helm/parser/parser_tar_test.go @@ -0,0 +1,81 @@ +package parser + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "testing" + "testing/fstest" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "helm.sh/helm/v3/pkg/chart/loader" +) + +// buildArchive returns a gzipped tar holding one regular entry per size in sizes. +// Entries are zero bytes, so the archive stays tiny however large they decompress +// to - the shape of a decompression bomb. +func buildArchive(t *testing.T, sizes ...int64) []byte { + t.Helper() + + var buf bytes.Buffer + zw := gzip.NewWriter(&buf) + tw := tar.NewWriter(zw) + + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: "chart/", Typeflag: tar.TypeDir, Mode: 0o755, + })) + + for i, size := range sizes { + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: "chart/file" + string(rune('a'+i)) + ".yaml", + Typeflag: tar.TypeReg, + Mode: 0o644, + Size: size, + })) + _, err := tw.Write(bytes.Repeat([]byte{'x'}, int(size))) + require.NoError(t, err) + } + + require.NoError(t, tw.Close()) + require.NoError(t, zw.Close()) + return buf.Bytes() +} + +func parseArchive(t *testing.T, data []byte) error { + t.Helper() + p, err := New(".") + require.NoError(t, err) + return p.ParseFS(t.Context(), fstest.MapFS{ + "chart.tar.gz": {Data: data}, + }, "chart.tar.gz") +} + +// CVE-2026-54448: unpackArchive used to read every entry with an unbounded +// io.ReadAll, so a small archive could decompress to gigabytes and OOM the +// scanner. It now enforces the Helm SDK limits. +func TestUnpackArchiveEnforcesDecompressionLimits(t *testing.T) { + // Shrink the Helm limits so the test does not have to build a 100 MiB archive. + origFile, origChart := loader.MaxDecompressedFileSize, loader.MaxDecompressedChartSize + loader.MaxDecompressedFileSize, loader.MaxDecompressedChartSize = 1024, 2048 + t.Cleanup(func() { + loader.MaxDecompressedFileSize, loader.MaxDecompressedChartSize = origFile, origChart + }) + + t.Run("single entry over the per-file limit is rejected", func(t *testing.T) { + err := parseArchive(t, buildArchive(t, loader.MaxDecompressedFileSize+1)) + require.Error(t, err) + assert.Contains(t, err.Error(), "larger than the maximum file size") + }) + + t.Run("entries summing over the per-archive limit are rejected", func(t *testing.T) { + // Each entry is within the per-file limit; together they blow the budget. + err := parseArchive(t, buildArchive(t, 1024, 1024, 1024)) + require.Error(t, err) + assert.Contains(t, err.Error(), "larger than the maximum size") + }) + + t.Run("archive within the limits still parses", func(t *testing.T) { + require.NoError(t, parseArchive(t, buildArchive(t, 512, 512))) + }) +}