Skip to content

Commit

Permalink
kustomize/filesys: config of allowed prefixes
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@hiddeco
hiddeco committed Apr 20, 2022
1 parent c380964 commit 3abdbb3
Show file tree
Hide file tree
Showing 2 changed files with 161 additions and 26 deletions.
92 changes: 73 additions & 19 deletions kustomize/filesys/fs_secure.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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())}
}
Expand All @@ -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)
Expand All @@ -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)
}
}
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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 {
Expand All @@ -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())
}
Expand All @@ -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, ""
}
95 changes: 88 additions & 7 deletions kustomize/filesys/fs_secure_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -491,6 +535,20 @@ func Test_isSecurePath(t *testing.T) {
path: "<root>/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) {
Expand Down Expand Up @@ -530,12 +588,35 @@ func Test_isSecurePath(t *testing.T) {
path = strings.Replace(path, "<root>", 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))
}
Expand Down

0 comments on commit 3abdbb3

Please sign in to comment.