Skip to content

Commit

Permalink
Merge pull request #264 from fluxcd/kus-fs-allow-prefix
Browse files Browse the repository at this point in the history
  • Loading branch information
@hiddeco
hiddeco authored Apr 20, 2022
2 parents 9f5245b + a2505ae commit 42a63df
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 42a63df

Please sign in to comment.