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/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/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))) + }) +} 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"), }, }, }, 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..dd76368273d1 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,12 @@ 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" ) const configFile = "plugin.yaml" -var ( - pluginsDir = "plugins" - - _defaultManager *Manager -) +var pluginsDir = "plugins" type ManagerOption func(indexer *Manager) @@ -50,50 +49,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 +138,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 +183,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 +226,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 +298,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 +308,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 +346,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 +421,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) +}