Skip to content

Commit

Permalink
Restrict loading to root or below.
Browse files Browse the repository at this point in the history
  • Loading branch information
@monopole
monopole committed Jan 16, 2019
1 parent 4d60f92 commit 905125d
Show file tree
Hide file tree
Showing 2 changed files with 216 additions and 59 deletions.
171 changes: 112 additions & 59 deletions pkg/loader/fileloader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,54 +26,56 @@ import (
"sigs.k8s.io/kustomize/pkg/ifc"
)

// fileLoader loads files, returning an array of bytes.
// It also enforces two kustomization requirements:
//
// 1) relocatable
//
// A kustomization and the resources, bases,
// patches, etc. that it depends on should be
// relocatable, so all path specifications
// must be relative, not absolute. The paths
// are taken relative to the location of the
// kusttomization file.
//
// 2) acyclic
//
// There should be no cycles in overlay to base
// relationships, including no cycles between
// git repositories.
//
// The loader has a notion of a current working directory
// (CWD), called 'root', that is independent of the CWD
// of the process. When `Load` is called with a file path
// argument, the load is done relative to this root,
// not relative to the process CWD.
//
// The loader offers a `New` method returning a new loader
// with a new root. The new root can be one of two things,
// a remote git repo URL, or a directory specified relative
// to the current root. In the former case, the remote
// repository is locally cloned, and the new loader is
// rooted on a path in that clone.
//
// Crucially, a root history is used to so that New fails
// if its argument either matches or is a parent of the
// current or any previously used root.
//
// This disallows:
//
// * A base that is a git repository that, in turn,
// specifies a base repository seen previously
// in the loading process (a cycle).
//
// * An overlay depending on a base positioned at or
// above it. I.e. '../foo' is OK, but '.', '..',
// '../..', etc. are disallowed. Allowing such a
// base has no advantages and encourage cycles,
// particularly if some future change were to
// introduce globbing to file specifications in
// the kustomization file.
// fileLoader is a kustomization's interface to files.
//
// The directory in which a kustomization file sits
// is referred to below as the kustomization's root.
//
// An instance of fileLoader has an immutable root,
// and offers a `New` method returning a new loader
// with a new root.
//
// A kustomization file refers to two kinds of files:
//
// * supplemental data paths
//
// `Load` is used to visit these paths.
//
// They must terminate in or below the root.
//
// They hold things like resources, patches,
// data for ConfigMaps, etc.
//
// * bases; other kustomizations
//
// `New` is used to load bases.
//
// A base can be either a remote git repo URL, or
// a directory specified relative to the current
// root. In the former case, the repo is locally
// cloned, and the new loader is rooted on a path
// in that clone.
//
// As loaders create new loaders, a root history
// is established, and used to disallow:
//
// - A base that is a repository that, in turn,
// specifies a base repository seen previously
// in the loading stack (a cycle).
//
// - An overlay depending on a base positioned at
// or above it. I.e. '../foo' is OK, but '.',
// '..', '../..', etc. are disallowed. Allowing
// such a base has no advantages and encourages
// cycles, particularly if some future change
// were to introduce globbing to file
// specifications in the kustomization file.
//
// These restrictions assure that kustomizations
// are self-contained and relocatable, and impose
// some safety when relying on remote kustomizations,
// e.g. a ConfigMap generator specified to read
// from /etc/passwd will fail.
//
type fileLoader struct {
// Previously visited absolute directory paths.
Expand Down Expand Up @@ -121,7 +123,7 @@ func newFileLoaderAt(
return nil, fmt.Errorf(
"loader root cannot be empty")
}
absRoot, err := filepath.Abs(root)
absRoot, err := cleanedAbs(root, fSys)
if err != nil {
return nil, fmt.Errorf(
"absolute path error in '%s' : %v", root, err)
Expand All @@ -141,6 +143,22 @@ func newFileLoaderAt(
}, nil
}

// cleanedAbs returns a cleaned, absolute path
// with no symbolic links.
func cleanedAbs(path string, fSys fs.FileSystem) (string, error) {
absRoot, err := filepath.Abs(path)
if err != nil {
return "", fmt.Errorf(
"abs path error on '%s' : %v", path, err)
}
deLinked, err := fSys.EvalSymlinks(absRoot)
if err != nil {
return "", fmt.Errorf(
"evalsymlink failure on '%s' : %v", path, err)
}
return deLinked, nil
}

// New returns a new Loader, rooted relative to current loader,
// or rooted in a temp directory holding a git repo clone.
func (l *fileLoader) New(path string) (ifc.Loader, error) {
Expand All @@ -158,12 +176,8 @@ func (l *fileLoader) New(path string) (ifc.Loader, error) {
if filepath.IsAbs(path) {
return nil, fmt.Errorf("new root '%s' cannot be absolute", path)
}
absRoot, err := filepath.Abs(filepath.Join(l.Root(), path))
if err != nil {
return nil, fmt.Errorf(
"problem joining '%s' to '%s': %v", l.Root(), path, err)
}
return newFileLoaderAt(absRoot, l.fSys, l.roots, l.cloner)
return newFileLoaderAt(
filepath.Join(l.Root(), path), l.fSys, l.roots, l.cloner)
}

// newGitLoader returns a new Loader pinned to a temporary
Expand Down Expand Up @@ -205,13 +219,52 @@ func isPathEqualToOrAbove(path string, roots []string) error {
return nil
}

// Load returns content of file at the given relative path.
// Load returns content of file at the given relative path,
// else an error. The path must refer to a file in or
// below the current Root().
func (l *fileLoader) Load(path string) ([]byte, error) {
if filepath.IsAbs(path) {
return nil, fmt.Errorf(
"must use relative path; '%s' is absolute", path)
return nil, l.loadOutOfBounds(path)
}
return l.fSys.ReadFile(filepath.Join(l.Root(), path))
path, err := cleanedAbs(
filepath.Join(l.Root(), path), l.fSys)
if err != nil {
return nil, err
}
if !l.isInOrBelowRoot(path) {
return nil, l.loadOutOfBounds(path)
}
return l.fSys.ReadFile(path)
}

// isInOrBelowRoot is true if the argument is in or
// below Root() from purely a path perspective (no
// check for actual file existence). For this to work,
// both the given argument "path" and l.Root() must
// be cleaned, absolute paths, and l.Root() must be
// a directory. Both conditions enforced elsewhere.
//
// This is tested on linux, but will have trouble
// on other operating systems. As soon as related
// work is completed in the core filepath package,
// this code should be refactored to use it.
// See:
// https://github.com/golang/go/issues/18355
// https://github.com/golang/dep/issues/296
// https://github.com/golang/dep/blob/master/internal/fs/fs.go#L33
// https://codereview.appspot.com/5712045
func (l *fileLoader) isInOrBelowRoot(path string) bool {
if l.Root() == string(filepath.Separator) {
return true
}
return strings.HasPrefix(
path, l.Root()+string(filepath.Separator))
}

func (l *fileLoader) loadOutOfBounds(path string) error {
return fmt.Errorf(
"security; file '%s' must be in or below '%s'",
path, l.Root())
}

// Cleanup runs the cleaner.
Expand Down
104 changes: 104 additions & 0 deletions pkg/loader/fileloader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,16 @@ limitations under the License.
package loader

import (
"io/ioutil"
"os"
"path"
"path/filepath"
"reflect"
"strings"
"testing"

"sigs.k8s.io/kustomize/pkg/fs"
"sigs.k8s.io/kustomize/pkg/ifc"
)

type testData struct {
Expand Down Expand Up @@ -198,3 +203,102 @@ func TestLoaderMisc(t *testing.T) {
t.Fatalf("Expected error")
}
}

func TestRestrictedLoadingInRealLoader(t *testing.T) {
// Create a structure like this
//
// /tmp/kustomize-test-SZwCZYjySj
// ├── base
// │ ├── okayData
// │ ├── symLinkToOkayData -> okayData
// │ └── symLinkToForbiddenData -> ../forbiddenData
// └── forbiddenData
//
dir, err := ioutil.TempDir("", "kustomize-test-")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
defer os.RemoveAll(dir)

fSys := fs.MakeRealFS()
fSys.Mkdir(filepath.Join(dir, "base"))

contentOk := "hi there, i'm OK data"
fSys.WriteFile(
filepath.Join(dir, "base", "okayData"), []byte(contentOk))

contentForbidden := "don't be looking at me"
fSys.WriteFile(
filepath.Join(dir, "forbiddenData"), []byte(contentForbidden))

os.Symlink(
filepath.Join(dir, "base", "okayData"),
filepath.Join(dir, "base", "symLinkToOkayData"))
os.Symlink(
filepath.Join(dir, "forbiddenData"),
filepath.Join(dir, "base", "symLinkToForbiddenData"))

var l ifc.Loader

l = newLoaderOrDie(fSys, dir)

// Sanity checks - loading from perspective of "dir".
// Everything works, including reading from a subdirectory.
data, err := l.Load(path.Join("base", "okayData"))
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if string(data) != contentOk {
t.Fatalf("unexpected content: %v", data)
}
data, err = l.Load("forbiddenData")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if string(data) != contentForbidden {
t.Fatalf("unexpected content: %v", data)
}

// Drop in.
l, err = l.New("base")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

// Reading okayData works.
data, err = l.Load("okayData")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if string(data) != contentOk {
t.Fatalf("unexpected content: %v", data)
}

// Reading local symlink to okayData works.
data, err = l.Load("symLinkToOkayData")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if string(data) != contentOk {
t.Fatalf("unexpected content: %v", data)
}

// Reading symlink to forbiddenData fails.
_, err = l.Load("symLinkToForbiddenData")
if err == nil {
t.Fatalf("expected error")
}
if !strings.Contains(err.Error(), "must be in or below") {
t.Fatalf("unexpected err: %v", err)
}

// Attempt to read "up" fails, though earlier we were
// able to read this file when root was "..".
_, err = l.Load("../forbiddenData")
if err == nil {
t.Fatalf("expected error")
}
if !strings.Contains(err.Error(), "must be in or below") {
t.Fatalf("unexpected err: %v", err)
}
}

0 comments on commit 905125d

Please sign in to comment.