From b9063d7362595fb01ea2905810daa06443b4b0df Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Sat, 9 Apr 2022 02:25:38 +0200 Subject: [PATCH] helm: add more test coverage for secureloader Signed-off-by: Hidde Beydals --- internal/helm/chart/secureloader/directory.go | 254 +++++++------ .../helm/chart/secureloader/directory_test.go | 347 +++++++++++++++++- internal/helm/chart/secureloader/loader.go | 15 +- .../helm/chart/secureloader/loader_test.go | 68 +++- 4 files changed, 556 insertions(+), 128 deletions(-) diff --git a/internal/helm/chart/secureloader/directory.go b/internal/helm/chart/secureloader/directory.go index 6b342a68..1c4b5f4b 100644 --- a/internal/helm/chart/secureloader/directory.go +++ b/internal/helm/chart/secureloader/directory.go @@ -26,7 +26,9 @@ package secureloader import ( "bytes" + "errors" "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -51,158 +53,204 @@ var ( // symlinks without including files outside root. type SecureDirLoader struct { root string - dir string + path string maxSize int } // NewSecureDirLoader returns a new SecureDirLoader, configured to the scope of the // root and provided dir. Max size configures the maximum size a file must not -// exceed to be loaded. If 0 it defaults to defaultMaxFileSize, it can be +// exceed to be loaded. If 0 it defaults to DefaultMaxFileSize, it can be // disabled using a negative integer. -func NewSecureDirLoader(root string, dir string, maxSize int) SecureDirLoader { +func NewSecureDirLoader(root string, path string, maxSize int) SecureDirLoader { if maxSize == 0 { maxSize = DefaultMaxFileSize } return SecureDirLoader{ root: root, - dir: dir, + path: path, maxSize: maxSize, } } // Load loads and returns the chart.Chart, or an error. func (l SecureDirLoader) Load() (*chart.Chart, error) { - return SecureLoadDir(l.root, l.dir, l.maxSize) + return SecureLoadDir(l.root, l.path, l.maxSize) } -// SecureLoadDir securely loads from a directory, without going outside root. -func SecureLoadDir(root, dir string, maxSize int) (*chart.Chart, error) { +// SecureLoadDir securely loads a chart from the path relative to root, without +// traversing outside root. When maxSize >= 0, files are not allowed to exceed +// this size, or an error is returned. +func SecureLoadDir(root, path string, maxSize int) (*chart.Chart, error) { root, err := filepath.Abs(root) if err != nil { return nil, err } - topDir, err := filepath.Abs(dir) + // Ensure path is relative + if filepath.IsAbs(path) { + relChartPath, err := filepath.Rel(root, path) + if err != nil { + return nil, err + } + path = relChartPath + } + + // Resolve secure absolute path + absChartName, err := securejoin.SecureJoin(root, path) if err != nil { return nil, err } - // Confirm topDir is actually relative to root - if _, err = isSecureSymlinkPath(root, topDir); err != nil { - return nil, fmt.Errorf("cannot load chart from dir: %w", err) - } - - // Just used for errors - c := &chart.Chart{} - - // Get the absolute location of the .helmignore file - relDirPath, err := filepath.Rel(root, topDir) + // Load ignore rules + rules, err := secureLoadIgnoreRules(root, path) if err != nil { - // We are not expected to be returning this error, as the above call to - // isSecureSymlinkPath already does the same. However, especially - // because we are dealing with security aspects here, we check it - // anyway in case this assumption changes. - return nil, err + return nil, fmt.Errorf("cannot load ignore rules for chart: %w", err) } - iFile, err := securejoin.SecureJoin(root, filepath.Join(relDirPath, ignore.HelmIgnore)) - // Load the .helmignore rules - rules := ignore.Empty() - if _, err = os.Stat(iFile); err == nil { - r, err := ignore.ParseFile(iFile) - if err != nil { - return c, err - } - rules = r + // Lets go for a walk... + fileWalker := newSecureFileWalker(root, absChartName, maxSize, rules) + if err = sympath.Walk(fileWalker.absChartPath, fileWalker.walk); err != nil { + return nil, fmt.Errorf("failed to load files from %s: %w", strings.TrimPrefix(fileWalker.absChartPath, fileWalker.root), err) } - rules.AddDefaults() - var files []*loader.BufferedFile - topDir += string(filepath.Separator) - - walk := func(name, absoluteName string, fi os.FileInfo, err error) error { - n := strings.TrimPrefix(name, topDir) - if n == "" { - // No need to process top level. Avoid bug with helmignore .* matching - // empty names. See issue 1779. - return nil - } - - // Normalize to / since it will also work on Windows - n = filepath.ToSlash(n) - - if err != nil { - return err - } - if fi.IsDir() { - // Directory-based ignore rules should involve skipping the entire - // contents of that directory. - if rules.Ignore(n, fi) { - return filepath.SkipDir - } - // Check after excluding ignores to provide the user with an option - // to opt-out from including certain paths. - if _, err := isSecureSymlinkPath(root, absoluteName); err != nil { - return fmt.Errorf("cannot load '%s' directory: %w", n, err) - } - return nil - } - - // If a .helmignore file matches, skip this file. - if rules.Ignore(n, fi) { - return nil - } - - // Check after excluding ignores to provide the user with an option - // to opt-out from including certain paths. - if _, err := isSecureSymlinkPath(root, absoluteName); err != nil { - return fmt.Errorf("cannot load '%s' file: %w", n, err) - } - - // Irregular files include devices, sockets, and other uses of files that - // are not regular files. In Go they have a file mode type bit set. - // See https://golang.org/pkg/os/#FileMode for examples. - if !fi.Mode().IsRegular() { - return fmt.Errorf("cannot load irregular file %s as it has file mode type bits set", n) - } - - if fileSize := fi.Size(); maxSize > 0 && fileSize > int64(maxSize) { - return fmt.Errorf("cannot load file %s as file size (%d) exceeds limit (%d)", n, fileSize, maxSize) - } - - data, err := os.ReadFile(name) - if err != nil { - return fmt.Errorf("error reading %s: %w", n, err) - } - data = bytes.TrimPrefix(data, utf8bom) - - files = append(files, &loader.BufferedFile{Name: n, Data: data}) - return nil + loaded, err := loader.LoadFiles(fileWalker.files) + if err != nil { + return nil, fmt.Errorf("failed to load chart from %s: %w", strings.TrimPrefix(fileWalker.absChartPath, fileWalker.root), err) } - if err = sympath.Walk(topDir, walk); err != nil { - return c, err - } - return loader.LoadFiles(files) + return loaded, nil } -// isSecureSymlinkPath attempts to make the given absolute path relative to +// secureLoadIgnoreRules attempts to load the ignore.HelmIgnore file from the +// chart path relative to root. If the file is a symbolic link, it is evaluated +// with the given root treated as root of the filesystem. +// If the ignore file does not exist, or points to a location outside of root, +// default ignore.Rules are returned. Any error other than fs.ErrNotExist is +// returned. +func secureLoadIgnoreRules(root, chartPath string) (*ignore.Rules, error) { + rules := ignore.Empty() + + iFile, err := securejoin.SecureJoin(root, filepath.Join(chartPath, ignore.HelmIgnore)) + if err != nil { + return nil, err + } + _, err = os.Stat(iFile) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return nil, err + } + if err == nil { + if rules, err = ignore.ParseFile(iFile); err != nil { + return nil, err + } + } + + rules.AddDefaults() + return rules, nil +} + +// secureFileWalker does the actual walking over the directory, any file loaded +// by walk is appended to files. +type secureFileWalker struct { + root string + absChartPath string + maxSize int + rules *ignore.Rules + files []*loader.BufferedFile +} + +func newSecureFileWalker(root, absChartPath string, maxSize int, rules *ignore.Rules) *secureFileWalker { + absChartPath = filepath.Clean(absChartPath) + string(filepath.Separator) + return &secureFileWalker{ + root: root, + absChartPath: absChartPath, + maxSize: maxSize, + rules: rules, + files: make([]*loader.BufferedFile, 0), + } +} + +func (w *secureFileWalker) walk(name, absName string, fi os.FileInfo, err error) error { + n := strings.TrimPrefix(name, w.absChartPath) + if n == "" { + // No need to process top level. Avoid bug with helmignore .* matching + // empty names. See issue 1779. + return nil + } + + if err != nil { + return err + } + + // Normalize to / since it will also work on Windows + n = filepath.ToSlash(n) + + if fi.IsDir() { + // Directory-based ignore rules should involve skipping the entire + // contents of that directory. + if w.rules.Ignore(n, fi) { + return filepath.SkipDir + } + // Check after excluding ignores to provide the user with an option + // to opt-out from including certain paths. + if _, err := isSecureAbsolutePath(w.root, absName); err != nil { + return fmt.Errorf("cannot load '%s' directory: %w", n, err) + } + return nil + } + + // If a .helmignore file matches, skip this file. + if w.rules.Ignore(n, fi) { + return nil + } + + // Check after excluding ignores to provide the user with an option + // to opt-out from including certain paths. + if _, err := isSecureAbsolutePath(w.root, absName); err != nil { + return fmt.Errorf("cannot load '%s' file: %w", n, err) + } + + // Irregular files include devices, sockets, and other uses of files that + // are not regular files. In Go they have a file mode type bit set. + // See https://golang.org/pkg/os/#FileMode for examples. + if !fi.Mode().IsRegular() { + return fmt.Errorf("cannot load irregular file %s as it has file mode type bits set", n) + } + + // Confirm size it not outside boundaries + if fileSize := fi.Size(); w.maxSize > 0 && fileSize > int64(w.maxSize) { + return fmt.Errorf("cannot load file %s as file size (%d) exceeds limit (%d)", n, fileSize, w.maxSize) + } + + data, err := os.ReadFile(absName) + if err != nil { + if pathErr := new(fs.PathError); errors.As(err, &pathErr) { + err = &fs.PathError{Op: pathErr.Op, Path: strings.TrimPrefix(absName, w.root), Err: pathErr.Err} + } + return fmt.Errorf("error reading %s: %w", n, err) + } + data = bytes.TrimPrefix(data, utf8bom) + + w.files = append(w.files, &loader.BufferedFile{Name: n, Data: data}) + return nil +} + +// isSecureAbsolutePath attempts to make the given absolute path relative to // root and securely joins this with root. If the result equals absolute path, // it is safe to use. -func isSecureSymlinkPath(root, absPath string) (bool, error) { +func isSecureAbsolutePath(root, absPath string) (bool, error) { root, absPath = filepath.Clean(root), filepath.Clean(absPath) if root == "/" { return true, nil } unsafePath, err := filepath.Rel(root, absPath) if err != nil { - return false, fmt.Errorf("cannot calculate path relative to root for resolved symlink") + return false, fmt.Errorf("cannot calculate path relative to root for absolute path") } safePath, err := securejoin.SecureJoin(root, unsafePath) if err != nil { - return false, fmt.Errorf("cannot securely join root with resolved relative symlink path") + return false, fmt.Errorf("cannot securely join root with resolved relative path") } if safePath != absPath { - return false, fmt.Errorf("symlink traverses outside root boundary: relative path to root %s", unsafePath) + return false, fmt.Errorf("absolute path traverses outside root boundary: relative path to root %s", unsafePath) } return true, nil } diff --git a/internal/helm/chart/secureloader/directory_test.go b/internal/helm/chart/secureloader/directory_test.go index e2031062..063b559c 100644 --- a/internal/helm/chart/secureloader/directory_test.go +++ b/internal/helm/chart/secureloader/directory_test.go @@ -17,12 +17,349 @@ limitations under the License. package secureloader import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" "testing" + "testing/fstest" + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader/ignore" . "github.com/onsi/gomega" + "helm.sh/helm/v3/pkg/chart" + "sigs.k8s.io/yaml" ) -func Test_isSecureSymlinkPath(t *testing.T) { +func TestSecureDirLoader_Load(t *testing.T) { + metadata := chart.Metadata{ + Name: "test", + APIVersion: "v2", + Version: "1.0", + Type: "application", + } + + t.Run("chart", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + m := metadata + b, err := yaml.Marshal(&m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) + + got, err := (NewSecureDirLoader(tmpDir, "", DefaultMaxFileSize)).Load() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).ToNot(BeNil()) + g.Expect(got.Name()).To(Equal(m.Name)) + }) + + t.Run("chart with absolute path", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + m := metadata + b, err := yaml.Marshal(&m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) + + got, err := (NewSecureDirLoader(tmpDir, tmpDir, DefaultMaxFileSize)).Load() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).ToNot(BeNil()) + g.Expect(got.Name()).To(Equal(m.Name)) + }) + + t.Run("chart with illegal path", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + + m := metadata + b, err := yaml.Marshal(&m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) + + root := filepath.Join(tmpDir, "root") + g.Expect(os.Mkdir(root, 0o700)).To(Succeed()) + + got, err := (NewSecureDirLoader(root, "../", DefaultMaxFileSize)).Load() + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to load chart from /: Chart.yaml file is missing")) + g.Expect(got).To(BeNil()) + + got, err = (NewSecureDirLoader(root, tmpDir, DefaultMaxFileSize)).Load() + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to load chart from /: Chart.yaml file is missing")) + g.Expect(got).To(BeNil()) + }) + + t.Run("chart with .helmignore", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + m := metadata + b, err := yaml.Marshal(&m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, ignore.HelmIgnore), []byte("file.txt"), 0o644)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "file.txt"), []byte("not included"), 0o644)).To(Succeed()) + + got, err := (NewSecureDirLoader(tmpDir, "", DefaultMaxFileSize)).Load() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).ToNot(BeNil()) + g.Expect(got.Name()).To(Equal(m.Name)) + g.Expect(got.Raw).To(HaveLen(2)) + }) +} + +func Test_secureLoadIgnoreRules(t *testing.T) { + t.Run("defaults", func(t *testing.T) { + g := NewWithT(t) + + r, err := secureLoadIgnoreRules("/workdir", "") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(r.Ignore("file.txt", nil)).To(BeFalse()) + g.Expect(r.Ignore("templates/.dotfile", nil)).To(BeTrue()) + }) + + t.Run("with "+ignore.HelmIgnore, func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + g.Expect(os.WriteFile(filepath.Join(tmpDir, ignore.HelmIgnore), []byte("file.txt"), 0o644)).To(Succeed()) + + r, err := secureLoadIgnoreRules(tmpDir, "") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(r.Ignore("file.txt", nil)).To(BeTrue()) + g.Expect(r.Ignore("templates/.dotfile", nil)).To(BeTrue()) + g.Expect(r.Ignore("other.txt", nil)).To(BeFalse()) + }) + + t.Run("with chart path and "+ignore.HelmIgnore, func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + chartPath := "./sub/chart" + g.Expect(os.MkdirAll(filepath.Join(tmpDir, chartPath), 0o700)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, chartPath, ignore.HelmIgnore), []byte("file.txt"), 0o644)).To(Succeed()) + + r, err := secureLoadIgnoreRules(tmpDir, chartPath) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(r.Ignore("file.txt", nil)).To(BeTrue()) + }) + + t.Run("with relative "+ignore.HelmIgnore+" symlink", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + chartPath := "sub/chart" + g.Expect(os.MkdirAll(filepath.Join(tmpDir, chartPath), 0o700)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "symlink"), []byte("file.txt"), 0o644)).To(Succeed()) + g.Expect(os.Symlink("../../symlink", filepath.Join(tmpDir, chartPath, ignore.HelmIgnore))) + + r, err := secureLoadIgnoreRules(tmpDir, chartPath) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(r.Ignore("file.txt", nil)).To(BeTrue()) + }) + + t.Run("with illegal "+ignore.HelmIgnore+" symlink", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + chartPath := "/sub/chart" + g.Expect(os.MkdirAll(filepath.Join(tmpDir, chartPath), 0o700)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "symlink"), []byte("file.txt"), 0o644)).To(Succeed()) + g.Expect(os.Symlink("../../symlink", filepath.Join(tmpDir, chartPath, ignore.HelmIgnore))) + + r, err := secureLoadIgnoreRules(filepath.Join(tmpDir, chartPath), "") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(r.Ignore("templates/.dotfile", nil)).To(BeTrue()) + g.Expect(r.Ignore("file.txt", nil)).To(BeFalse()) + }) + + t.Run("with "+ignore.HelmIgnore+" parsing error", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + g.Expect(os.WriteFile(filepath.Join(tmpDir, ignore.HelmIgnore), []byte("**"), 0o644)).To(Succeed()) + + _, err := secureLoadIgnoreRules(tmpDir, "") + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("syntax is not supported")) + }) +} + +func Test_secureFileWalker_walk(t *testing.T) { + g := NewWithT(t) + + const ( + root = "/fake/root" + chartPath = "/fake/root/dir" + ) + + fakeDirName := "fake-dir" + fakeFileName := "fake-file" + fakeDeviceFileName := "fake-device" + fakeFS := fstest.MapFS{ + fakeDirName: &fstest.MapFile{Mode: fs.ModeDir}, + fakeFileName: &fstest.MapFile{Data: []byte("a couple bytes")}, + fakeDeviceFileName: &fstest.MapFile{Mode: fs.ModeDevice}, + } + + // Safe to further re-use this for other paths + fakeDirInfo, err := fakeFS.Stat(fakeDirName) + g.Expect(err).ToNot(HaveOccurred()) + fakeFileInfo, err := fakeFS.Stat(fakeFileName) + g.Expect(err).ToNot(HaveOccurred()) + fakeDeviceInfo, err := fakeFS.Stat(fakeDeviceFileName) + g.Expect(err).ToNot(HaveOccurred()) + + t.Run("given name equals top dir", func(t *testing.T) { + g := NewWithT(t) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + g.Expect(w.walk(chartPath+"/", chartPath, nil, nil)).To(BeNil()) + }) + + t.Run("given error is returned", func(t *testing.T) { + g := NewWithT(t) + + err := errors.New("error argument") + got := (&secureFileWalker{}).walk("name", "/name", nil, err) + g.Expect(got).To(HaveOccurred()) + g.Expect(got).To(Equal(err)) + }) + + t.Run("ignore rule matches dir", func(t *testing.T) { + g := NewWithT(t) + + rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/")) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(Equal(fs.SkipDir)) + }) + + t.Run("absolute path match ignored", func(t *testing.T) { + g := NewWithT(t) + + rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/")) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + g.Expect(w.walk(filepath.Join(w.absChartPath, "symlink"), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(BeNil()) + }) + + t.Run("ignore rule not applicable to dir", func(t *testing.T) { + g := NewWithT(t) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(BeNil()) + }) + + t.Run("absolute path outside root", func(t *testing.T) { + g := NewWithT(t) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + err := w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join("/fake/another/root/", fakeDirName), fakeDirInfo, nil) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("cannot load 'fake-dir' directory: absolute path traverses outside root boundary")) + }) + + t.Run("dir ignore rules before secure path check", func(t *testing.T) { + g := NewWithT(t) + + rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/")) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join("/fake/another/root/", fakeDirName), fakeDirInfo, nil)).To(Equal(fs.SkipDir)) + }) + + t.Run("ignore rule matches file", func(t *testing.T) { + g := NewWithT(t) + + rules, err := ignore.Parse(strings.NewReader(fakeFileName)) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + g.Expect(w.walk(filepath.Join(w.absChartPath, fakeFileName), filepath.Join(w.absChartPath, fakeFileName), fakeFileInfo, nil)).To(BeNil()) + }) + + t.Run("file path outside root", func(t *testing.T) { + g := NewWithT(t) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + err := w.walk(filepath.Join(w.absChartPath, fakeFileName), filepath.Join("/fake/another/root/", fakeFileName), fakeFileInfo, nil) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("cannot load 'fake-file' file: absolute path traverses outside root boundary")) + }) + + t.Run("irregular file", func(t *testing.T) { + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + err := w.walk(fakeDeviceFileName, filepath.Join(w.absChartPath), fakeDeviceInfo, nil) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("cannot load irregular file fake-device as it has file mode type bits set")) + }) + + t.Run("file exceeds max size", func(t *testing.T) { + w := newSecureFileWalker(root, chartPath, 5, ignore.Empty()) + err := w.walk(fakeFileName, filepath.Join(w.absChartPath), fakeFileInfo, nil) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal(fmt.Sprintf("cannot load file fake-file as file size (%d) exceeds limit (%d)", fakeFileInfo.Size(), w.maxSize))) + }) + + t.Run("file is appended", func(t *testing.T) { + g := NewWithT(t) + tmpDir := t.TempDir() + + fileName := "append-file" + fileData := []byte("append-file-data") + absFilePath := filepath.Join(tmpDir, fileName) + g.Expect(os.WriteFile(absFilePath, fileData, 0o644)).To(Succeed()) + fileInfo, err := os.Lstat(absFilePath) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty()) + g.Expect(w.walk(fileName, absFilePath, fileInfo, nil)).To(Succeed()) + g.Expect(w.files).To(HaveLen(1)) + g.Expect(w.files[0].Name).To(Equal(fileName)) + g.Expect(w.files[0].Data).To(Equal(fileData)) + }) + + t.Run("utf8bom is removed from file data", func(t *testing.T) { + g := NewWithT(t) + tmpDir := t.TempDir() + + fileName := "append-file" + fileData := []byte("append-file-data") + fileDataWithBom := append(utf8bom, fileData...) + absFilePath := filepath.Join(tmpDir, fileName) + g.Expect(os.WriteFile(absFilePath, fileDataWithBom, 0o644)).To(Succeed()) + fileInfo, err := os.Lstat(absFilePath) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty()) + g.Expect(w.walk(fileName, absFilePath, fileInfo, nil)).To(Succeed()) + g.Expect(w.files).To(HaveLen(1)) + g.Expect(w.files[0].Name).To(Equal(fileName)) + g.Expect(w.files[0].Data).To(Equal(fileData)) + }) + + t.Run("file does not exist", func(t *testing.T) { + g := NewWithT(t) + tmpDir := t.TempDir() + + w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty()) + err := w.walk(filepath.Join(w.absChartPath, "invalid"), filepath.Join(w.absChartPath, "invalid"), fakeFileInfo, nil) + g.Expect(err).To(HaveOccurred()) + g.Expect(errors.Is(err, fs.ErrNotExist)).To(BeTrue()) + g.Expect(err.Error()).To(ContainSubstring("error reading invalid: open /invalid: no such file or directory")) + }) +} + +func Test_isSecureAbsolutePath(t *testing.T) { tests := []struct { name string root string @@ -42,7 +379,7 @@ func Test_isSecureSymlinkPath(t *testing.T) { root: "/working/dir", absPath: "/working/in/another/dir", safe: false, - wantErr: "symlink traverses outside root boundary", + wantErr: "absolute path traverses outside root boundary", }, { name: "abs path relative to root", @@ -55,21 +392,21 @@ func Test_isSecureSymlinkPath(t *testing.T) { root: "/working/dir", absPath: "/working/dir/../but/not/really", safe: false, - wantErr: "symlink traverses outside root boundary", + wantErr: "absolute path traverses outside root boundary", }, { name: "illegal root", root: "working/dir/", absPath: "/working/dir", safe: false, - wantErr: "cannot calculate path relative to root for resolved symlink", + wantErr: "cannot calculate path relative to root for absolute path", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got, err := isSecureSymlinkPath(tt.root, tt.absPath) + got, err := isSecureAbsolutePath(tt.root, tt.absPath) g.Expect(got).To(Equal(tt.safe)) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) diff --git a/internal/helm/chart/secureloader/loader.go b/internal/helm/chart/secureloader/loader.go index 86ff2cf6..25bce34b 100644 --- a/internal/helm/chart/secureloader/loader.go +++ b/internal/helm/chart/secureloader/loader.go @@ -22,6 +22,7 @@ import ( "io/fs" "os" "path/filepath" + "strings" securejoin "github.com/cyphar/filepath-securejoin" "helm.sh/helm/v3/pkg/chart" @@ -34,14 +35,19 @@ import ( // Name can be an absolute or relative path, but always has to be inside // root. func Loader(root, name string) (loader.ChartLoader, error) { - root, name = filepath.Clean(root), filepath.Clean(name) - relName := name + root, err := filepath.Abs(root) + if err != nil { + return nil, err + } + + relName := filepath.Clean(name) if filepath.IsAbs(relName) { var err error if relName, err = filepath.Rel(root, name); err != nil { return nil, err } } + secureName, err := securejoin.SecureJoin(root, relName) if err != nil { return nil, err @@ -49,12 +55,13 @@ func Loader(root, name string) (loader.ChartLoader, error) { fi, err := os.Lstat(secureName) if err != nil { if pathErr := new(fs.PathError); errors.As(err, &pathErr) { - return nil, &fs.PathError{Op: pathErr.Op, Path: name, Err: pathErr.Err} + return nil, &fs.PathError{Op: pathErr.Op, Path: strings.TrimPrefix(secureName, root), Err: pathErr.Err} } return nil, err } + if fi.IsDir() { - return NewSecureDirLoader(root, secureName, 0), nil + return NewSecureDirLoader(root, relName, DefaultMaxFileSize), nil } return FileLoader(secureName), nil } diff --git a/internal/helm/chart/secureloader/loader_test.go b/internal/helm/chart/secureloader/loader_test.go index d0b69a84..d5032de6 100644 --- a/internal/helm/chart/secureloader/loader_test.go +++ b/internal/helm/chart/secureloader/loader_test.go @@ -23,7 +23,9 @@ import ( "testing" . "github.com/onsi/gomega" + "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" + "sigs.k8s.io/yaml" ) func TestLoader(t *testing.T) { @@ -33,22 +35,56 @@ func TestLoader(t *testing.T) { fakeChart := filepath.Join(tmpDir, "fake.tgz") g.Expect(os.WriteFile(fakeChart, []byte(""), 0o644)).To(Succeed()) - got, err := Loader(tmpDir, fakeChart) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(loader.FileLoader(fakeChart))) + t.Run("file loader", func(t *testing.T) { + g := NewWithT(t) - fakeChartPath := filepath.Join(tmpDir, "fake") - g.Expect(os.Mkdir(fakeChartPath, 0o700)).To(Succeed()) - got, err = Loader(tmpDir, "fake") - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(SecureDirLoader{root: tmpDir, dir: fakeChartPath, maxSize: DefaultMaxFileSize})) + got, err := Loader(tmpDir, fakeChart) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).To(Equal(loader.FileLoader(fakeChart))) + }) - symlinkRoot := filepath.Join(tmpDir, "symlink") - g.Expect(os.Mkdir(symlinkRoot, 0o700)).To(Succeed()) - symlinkPath := filepath.Join(symlinkRoot, "fake.tgz") - g.Expect(os.Symlink(fakeChart, symlinkPath)) - got, err = Loader(symlinkRoot, symlinkPath) - g.Expect(err).To(HaveOccurred()) - g.Expect(err).To(BeAssignableToTypeOf(&fs.PathError{})) - g.Expect(got).To(BeNil()) + t.Run("dir loader", func(t *testing.T) { + g := NewWithT(t) + + fakeChartPath := filepath.Join(tmpDir, "fake") + g.Expect(os.Mkdir(fakeChartPath, 0o700)).To(Succeed()) + + got, err := Loader(tmpDir, "fake") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).To(Equal(SecureDirLoader{root: tmpDir, path: "fake", maxSize: DefaultMaxFileSize})) + }) + + t.Run("illegal path", func(t *testing.T) { + g := NewWithT(t) + + symlinkRoot := filepath.Join(tmpDir, "symlink") + g.Expect(os.Mkdir(symlinkRoot, 0o700)).To(Succeed()) + symlinkPath := filepath.Join(symlinkRoot, "fake.tgz") + g.Expect(os.Symlink(fakeChart, symlinkPath)) + + got, err := Loader(symlinkRoot, symlinkPath) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(BeAssignableToTypeOf(&fs.PathError{})) + g.Expect(got).To(BeNil()) + }) +} + +func TestLoad(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + metadata := chart.Metadata{ + Name: "test", + APIVersion: "v2", + Version: "1.0", + Type: "application", + } + b, err := yaml.Marshal(&metadata) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) + + got, err := Load(tmpDir, "") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).ToNot(BeNil()) + g.Expect(got.Name()).To(Equal(metadata.Name)) }