From a2505ae518437cd2bfbee994ba0954fd49a7fa44 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 20 Apr 2022 16:05:22 +0200 Subject: [PATCH] kustomize/filesys: config of allowed prefixes This introduction is required for the build of Kustomizations which refer to remote bases, as the internal load process creates new temporary directories to fetch these into. By ensuring the `root` of the FS does not start with an allowed prefix, it is not possible for a FS to reach into another FS if the program which creates them uses a static list. This solution is not optimal, and is a signal we need to fork Kustomize (and advocate upstream), to simply allow a more diverse configuration of loader restrictions. Making this FS implementation obsolete. Signed-off-by: Hidde Beydals --- kustomize/filesys/fs_secure.go | 92 ++++++++++++++++++++++------ kustomize/filesys/fs_secure_test.go | 95 ++++++++++++++++++++++++++--- 2 files changed, 161 insertions(+), 26 deletions(-) diff --git a/kustomize/filesys/fs_secure.go b/kustomize/filesys/fs_secure.go index 05f30811..267d7261 100644 --- a/kustomize/filesys/fs_secure.go +++ b/kustomize/filesys/fs_secure.go @@ -20,26 +20,65 @@ import ( "fmt" "os" "path/filepath" + "strings" "sigs.k8s.io/kustomize/kyaml/filesys" ) +var ( + // tmpConfirmedDirPrefix is the prefix used by filesys.NewTmpConfirmedDir(), + // the absolute prefix is the value prefixed with os.TempDir(). + tmpConfirmedDirPrefix = "kustomize-" +) + // MakeFsOnDiskSecure returns a secure file system which asserts any paths it -// handles to be inside root. -func MakeFsOnDiskSecure(root string) (filesys.FileSystem, error) { +// handles to be inside root. When allowed prefixes are provided, followed +// absolute links are allowed to traverse into these directories. +// The root is not allowed to match an allowed prefix, this to ensure an FS +// instance can not reach into another FS when the allowed prefix configuration +// is static. +func MakeFsOnDiskSecure(root string, allowPrefixes ...string) (filesys.FileSystem, error) { unsafeFS := filesys.MakeFsOnDisk() cleanedAbs, _, err := unsafeFS.CleanedAbs(root) if err != nil { return nil, err } - return fsSecure{root: cleanedAbs, unsafeFS: unsafeFS}, nil + if ok, prefix := hasOneOfPrefixes(cleanedAbs.String(), allowPrefixes); ok { + return nil, fmt.Errorf("root '%s' cannot be prefixed with '%s'", root, prefix) + } + return fsSecure{root: cleanedAbs, unsafeFS: unsafeFS, allowPrefixes: allowPrefixes}, nil +} + +// TmpConfirmedDirPrefix returns the absolute prefix path as constructed by +// filesys.NewTmpConfirmedDir(). +func TmpConfirmedDirPrefix() (string, error) { + // Some OS-es seem to symlink TempDir. + evaluated, err := filepath.EvalSymlinks(os.TempDir()) + if err != nil { + return "", err + } + return filepath.Join(evaluated, tmpConfirmedDirPrefix), nil +} + +// MakeFsOnDiskSecureBuild calls MakeFsOnDiskSecure, but configures the +// TmpConfirmedDirPrefix as an allowed prefix. +// NOTE: When e.g. being able to load remote bases is not a concern, opt to use +// MakeFsOnDiskSecure instead, unless running into specific traversal issues. +func MakeFsOnDiskSecureBuild(root string, allowPrefixes ...string) (filesys.FileSystem, error) { + dirPrefix, err := TmpConfirmedDirPrefix() + if err != nil { + return nil, err + } + allowPrefixes = append([]string{dirPrefix}, allowPrefixes...) + return MakeFsOnDiskSecure(root, allowPrefixes...) } // fsSecure wraps an unsafe FileSystem implementation, and secures it // by confirming paths are inside root. type fsSecure struct { - root filesys.ConfirmedDir - unsafeFS filesys.FileSystem + root filesys.ConfirmedDir + unsafeFS filesys.FileSystem + allowPrefixes []string } // ConstraintError records an error and the operation and file that @@ -60,7 +99,7 @@ func (e *ConstraintError) Unwrap() error { return e.Err } // to be inside root. If the provided path violates this constraint, an error // of type ConstraintError is returned. func (fs fsSecure) Create(path string) (filesys.File, error) { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return nil, &ConstraintError{Op: "create", Path: path, Err: err} } return fs.unsafeFS.Create(path) @@ -70,7 +109,7 @@ func (fs fsSecure) Create(path string) (filesys.File, error) { // to be inside root. If the provided path violates this constraint, an error // of type ConstraintError is returned. func (fs fsSecure) Mkdir(path string) error { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return &ConstraintError{Op: "mkdir", Path: path, Err: err} } return fs.unsafeFS.Mkdir(path) @@ -80,7 +119,7 @@ func (fs fsSecure) Mkdir(path string) error { // to be inside root. If the provided path violates this constraint, an error // type ConstraintError is returned. func (fs fsSecure) MkdirAll(path string) error { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return &ConstraintError{Op: "mkdir", Path: path, Err: err} } return fs.unsafeFS.MkdirAll(path) @@ -90,7 +129,7 @@ func (fs fsSecure) MkdirAll(path string) error { // path to be inside root. If the provided path violates this constraint, an // error of type ConstraintError is returned. func (fs fsSecure) RemoveAll(path string) error { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return &ConstraintError{Op: "remove", Path: path, Err: err} } return fs.unsafeFS.RemoveAll(path) @@ -100,7 +139,7 @@ func (fs fsSecure) RemoveAll(path string) error { // to be inside root. If the provided path violates this constraint, an error // of type ConstraintError is returned. func (fs fsSecure) Open(path string) (filesys.File, error) { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return nil, &ConstraintError{Op: "open", Path: path, Err: err} } return fs.unsafeFS.Open(path) @@ -110,7 +149,7 @@ func (fs fsSecure) Open(path string) (filesys.File, error) { // to be inside root. If the provided path violates this constraint, it returns // false. func (fs fsSecure) IsDir(path string) bool { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return false } return fs.unsafeFS.IsDir(path) @@ -120,7 +159,7 @@ func (fs fsSecure) IsDir(path string) bool { // to be inside root. If the provided path violates this constraint, an error // of type ConstraintError is returned. func (fs fsSecure) ReadDir(path string) ([]string, error) { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return nil, &ConstraintError{Op: "open", Path: path, Err: err} } return fs.unsafeFS.ReadDir(path) @@ -136,6 +175,9 @@ func (fs fsSecure) CleanedAbs(path string) (filesys.ConfirmedDir, string, error) if err != nil { return d, f, err } + if ok, _ := hasOneOfPrefixes(d.String(), fs.allowPrefixes); ok { + return d, f, err + } if !d.HasPrefix(fs.root) { return "", "", &ConstraintError{Op: "abs", Path: path, Err: rootConstraintErr(path, fs.root.String())} } @@ -146,7 +188,7 @@ func (fs fsSecure) CleanedAbs(path string) (filesys.ConfirmedDir, string, error) // to be inside root. If the provided path violates this constraint, it returns // false. func (fs fsSecure) Exists(path string) bool { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return false } return fs.unsafeFS.Exists(path) @@ -161,7 +203,7 @@ func (fs fsSecure) Glob(pattern string) ([]string, error) { } var securePaths []string for _, p := range paths { - if err := isSecurePath(fs.unsafeFS, fs.root, p); err == nil { + if err := isSecurePath(fs.unsafeFS, fs.root, p, fs.allowPrefixes...); err == nil { securePaths = append(securePaths, p) } } @@ -172,7 +214,7 @@ func (fs fsSecure) Glob(pattern string) ([]string, error) { // to be inside root. If the provided path violates this constraint, an error // of type ConstraintError is returned. func (fs fsSecure) ReadFile(path string) ([]byte, error) { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return nil, &ConstraintError{Op: "read", Path: path, Err: err} } return fs.unsafeFS.ReadFile(path) @@ -182,7 +224,7 @@ func (fs fsSecure) ReadFile(path string) ([]byte, error) { // path to be inside root. If the provided path violates this constraint, an // error of type ConstraintError is returned. func (fs fsSecure) WriteFile(path string, data []byte) error { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return &ConstraintError{Op: "write", Path: path, Err: err} } return fs.unsafeFS.WriteFile(path, data) @@ -193,7 +235,7 @@ func (fs fsSecure) WriteFile(path string, data []byte) error { // an error of type ConstraintError is returned and walkFn is not called. func (fs fsSecure) Walk(path string, walkFn filepath.WalkFunc) error { wrapWalkFn := func(path string, info os.FileInfo, err error) error { - if err := isSecurePath(fs.unsafeFS, fs.root, path); err != nil { + if err := isSecurePath(fs.unsafeFS, fs.root, path, fs.allowPrefixes...); err != nil { return &ConstraintError{Op: "walk", Path: path, Err: err} } return walkFn(path, info, err) @@ -204,12 +246,12 @@ func (fs fsSecure) Walk(path string, walkFn filepath.WalkFunc) error { // isSecurePath confirms the given path is inside root using the provided file // system. At present, it assumes the file system implementation to be on disk // and makes use of filepath.EvalSymlinks. -func isSecurePath(fs filesys.FileSystem, root filesys.ConfirmedDir, path string) error { +func isSecurePath(fs filesys.FileSystem, root filesys.ConfirmedDir, path string, allowedPrefixes ...string) error { absRoot, err := filepath.Abs(path) if err != nil { return fmt.Errorf("abs path error on '%s': %v", path, err) } - d := filesys.ConfirmedDir(filepath.Dir(absRoot)) + d := filesys.ConfirmedDir(absRoot) if fs.Exists(absRoot) { evaluated, err := filepath.EvalSymlinks(absRoot) if err != nil { @@ -221,6 +263,9 @@ func isSecurePath(fs filesys.FileSystem, root filesys.ConfirmedDir, path string) } d = filesys.ConfirmedDir(evaluatedDir) } + if ok, _ := hasOneOfPrefixes(d.String(), allowedPrefixes); ok { + return nil + } if !d.HasPrefix(root) { return rootConstraintErr(path, root.String()) } @@ -230,3 +275,12 @@ func isSecurePath(fs filesys.FileSystem, root filesys.ConfirmedDir, path string) func rootConstraintErr(path, root string) error { return fmt.Errorf("path '%s' is not in or below '%s'", path, root) } + +func hasOneOfPrefixes(s string, prefixes []string) (bool, string) { + for _, p := range prefixes { + if strings.HasPrefix(s, p) { + return true, p + } + } + return false, "" +} diff --git a/kustomize/filesys/fs_secure_test.go b/kustomize/filesys/fs_secure_test.go index 65e46be4..b57c5fc1 100644 --- a/kustomize/filesys/fs_secure_test.go +++ b/kustomize/filesys/fs_secure_test.go @@ -30,6 +30,21 @@ import ( "sigs.k8s.io/kustomize/kyaml/filesys" ) +func TestMakeFsOnDiskSecure(t *testing.T) { + t.Run("error on root prefixed with allowed prefix", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + matchingDir := filepath.Join(tmpDir, "subdir") + g.Expect(os.Mkdir(matchingDir, 0o644)).To(Succeed()) + + got, err := MakeFsOnDiskSecure(filepath.Join(tmpDir, "subdir"), tmpDir) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("cannot be prefixed with")) + g.Expect(got).To(BeNil()) + }) +} + func Test_fsSecure_Create(t *testing.T) { g := NewWithT(t) @@ -264,6 +279,25 @@ func Test_fsSecure_CleanedAbs(t *testing.T) { g.Expect(d).To(BeEmpty()) g.Expect(f).To(BeEmpty()) }) + + t.Run("prefix allowed cleaned abs", func(t *testing.T) { + g := NewWithT(t) + + allowedPrefix, err := TmpConfirmedDirPrefix() + g.Expect(err).ToNot(HaveOccurred()) + + fs, err := MakeFsOnDiskSecureBuild(root, allowedPrefix) + g.Expect(err).ToNot(HaveOccurred()) + + prefixedDir, err := os.MkdirTemp("", tmpConfirmedDirPrefix) + g.Expect(err).ToNot(HaveOccurred()) + t.Cleanup(func() { _ = os.RemoveAll(prefixedDir) }) + + d, f, err := fs.CleanedAbs(prefixedDir) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(d).To(Equal(filesys.ConfirmedDir(prefixedDir))) + g.Expect(f).To(BeEmpty()) + }) } func Test_fsSecure_Exists(t *testing.T) { @@ -415,17 +449,27 @@ func Test_fsSecure_Walk(t *testing.T) { } func Test_isSecurePath(t *testing.T) { + g := NewWithT(t) + + prefixedDir, err := os.MkdirTemp("", tmpConfirmedDirPrefix) + g.Expect(err).ToNot(HaveOccurred()) + t.Cleanup(func() { _ = os.RemoveAll(prefixedDir) }) + + allowPrefix, err := TmpConfirmedDirPrefix() + g.Expect(err).ToNot(HaveOccurred()) + type file struct { name string symlink string } tests := []struct { - name string - fs filesys.FileSystem - rootSuffix string - files []file - path string - wantErr types.GomegaMatcher + name string + fs filesys.FileSystem + rootSuffix string + files []file + path string + allowedPrefixes []string + wantErr types.GomegaMatcher }{ { name: "secure non existing path", @@ -491,6 +535,20 @@ func Test_isSecurePath(t *testing.T) { path: "/subdir/symlink", wantErr: HaveOccurred(), }, + { + name: "allowed prefix", + fs: filesys.MakeFsOnDisk(), + path: prefixedDir, + allowedPrefixes: []string{allowPrefix}, + wantErr: Succeed(), + }, + { + name: "illegal prefix", + fs: filesys.MakeFsOnDisk(), + path: filepath.Join(os.TempDir(), "illegal-path"), + allowedPrefixes: []string{allowPrefix}, + wantErr: HaveOccurred(), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -530,12 +588,35 @@ func Test_isSecurePath(t *testing.T) { path = strings.Replace(path, "", root, 1) } - err := isSecurePath(tt.fs, realRoot, path) + err := isSecurePath(tt.fs, realRoot, path, tt.allowedPrefixes...) g.Expect(err).To(tt.wantErr) }) } } +func Test_hasOneOfPrefixes(t *testing.T) { + tests := []struct { + name string + s string + prefixes []string + want bool + wantPrefix string + }{ + {name: "match", s: "/tmp/kustomize-3828348", prefixes: []string{"/tmp/kustomize-"}, want: true, wantPrefix: "/tmp/kustomize-"}, + {name: "not a match", s: "/tmp/workdir-6845913", prefixes: []string{"/tmp/kustomize-"}, want: false}, + {name: "match list", s: "/tmp/workdir-6845913", prefixes: []string{"/tmp/kustomize-", "/tmp/workdir-"}, want: true, wantPrefix: "/tmp/workdir-"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + has, prefix := hasOneOfPrefixes(tt.s, tt.prefixes) + g.Expect(has).To(Equal(tt.want)) + g.Expect(prefix).To(Equal(tt.wantPrefix)) + }) + } +} + func newTemp() string { return filepath.Join(os.TempDir(), "securefs-"+randStringBytes(5)) }