From afec478beb2fb1d15f7251bafe2242bc28db02ef Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Mon, 24 Jul 2017 17:51:47 -0700 Subject: [PATCH] Update tar path resolution Fixes bug for resolving symlinks which allowed fully resolving an existing symlink to a path, causing some symlinks to get overridden as symlinks to self. Updates logic to split name into parent path, resolve the parent path, then safely join back with the base name. Uses the split code to ensure parent directories are created in all cases. Replaces `rootJoin` with filepath.Join to the root, which already correctly cleans relative symlinks to the root. Signed-off-by: Derek McGowan --- archive/path.go | 20 +--- archive/tar.go | 40 ++++--- archive/tar_test.go | 276 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 291 insertions(+), 45 deletions(-) diff --git a/archive/path.go b/archive/path.go index fd3d0b108..32374459c 100644 --- a/archive/path.go +++ b/archive/path.go @@ -28,7 +28,7 @@ func rootPath(root, path string) (string, error) { } path = newpath if i == linksWalked { - newpath = rootJoin(newpath) + newpath = filepath.Join("/", newpath) if path == newpath { return filepath.Join(root, newpath), nil } @@ -37,28 +37,12 @@ func rootPath(root, path string) (string, error) { } } -// rootJoin joins a path with root, cleaning up any links that -// reference above root. -func rootJoin(path string) string { - if filepath.IsAbs(path) { - path = filepath.Clean(path) - } - - // Resolve any ".." or "/.." before joining to root - for !filepath.IsAbs(path) { - path = "/" + path - path = filepath.Clean(path) - } - - return path -} - func walkLink(root, path string, linksWalked *int) (newpath string, islink bool, err error) { if *linksWalked > 255 { return "", false, errTooManyLinks } - path = rootJoin(path) + path = filepath.Join("/", path) if path == "/" { return path, false, nil } diff --git a/archive/tar.go b/archive/tar.go index c9212e463..b29699e10 100644 --- a/archive/tar.go +++ b/archive/tar.go @@ -126,19 +126,30 @@ func Apply(ctx context.Context, root string, r io.Reader) (int64, error) { continue } - // Note as these operations are platform specific, so must the slash be. - if !strings.HasSuffix(hdr.Name, string(os.PathSeparator)) { - // Not the root directory, ensure that the parent directory exists. - // This happened in some tests where an image had a tarfile without any - // parent directories. - parent := filepath.Dir(hdr.Name) - parentPath, err := rootPath(root, parent) - if err != nil { - return 0, err - } + // Split name and resolve symlinks for root directory. + ppath, base := filepath.Split(hdr.Name) + ppath, err = rootPath(root, ppath) + if err != nil { + return 0, errors.Wrap(err, "failed to get root path") + } + // Join to root before joining to parent path to ensure relative links are + // already resolved based on the root before adding to parent. + path := filepath.Join(ppath, filepath.Join("/", base)) + if path == root { + log.G(ctx).Debugf("file %q ignored: resolved to root", hdr.Name) + continue + } + + // If file is not directly under root, ensure parent directory + // exists or is created. + if ppath != root { + parentPath := ppath + if base == "" { + parentPath = filepath.Dir(path) + } if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) { - err = mkdirAll(parentPath, 0600) + err = mkdirAll(parentPath, 0700) if err != nil { return 0, err } @@ -173,13 +184,6 @@ func Apply(ctx context.Context, root string, r io.Reader) (int64, error) { } } - path, err := rootPath(root, hdr.Name) - if err != nil { - return 0, errors.Wrap(err, "failed to get root path") - } - - base := filepath.Base(path) - if strings.HasPrefix(base, whiteoutPrefix) { dir := filepath.Dir(path) if base == whiteoutOpaqueDir { diff --git a/archive/tar_test.go b/archive/tar_test.go index 1cc9149e1..68cfeebee 100644 --- a/archive/tar_test.go +++ b/archive/tar_test.go @@ -89,6 +89,83 @@ func TestRelativeSymlinks(t *testing.T) { } } +func TestSymlinks(t *testing.T) { + links := [][2]fstest.Applier{ + { + fstest.Apply( + fstest.CreateDir("/bin/", 0755), + fstest.CreateFile("/bin/superbinary", []byte{0x00, 0x00}, 0755), + fstest.Symlink("../bin/superbinary", "/bin/other1"), + ), + fstest.Apply( + fstest.Remove("/bin/other1"), + fstest.Symlink("/bin/superbinary", "/bin/other1"), + fstest.Symlink("../bin/superbinary", "/bin/other2"), + fstest.Symlink("superbinary", "/bin/other3"), + ), + }, + { + fstest.Apply( + fstest.CreateDir("/bin/", 0755), + fstest.CreateDir("/sbin/", 0755), + fstest.CreateFile("/sbin/superbinary", []byte{0x00, 0x00}, 0755), + fstest.Symlink("/sbin/superbinary", "/bin/superbinary"), + fstest.Symlink("../bin/superbinary", "/bin/other1"), + ), + fstest.Apply( + fstest.Remove("/bin/other1"), + fstest.Symlink("/bin/superbinary", "/bin/other1"), + fstest.Symlink("superbinary", "/bin/other2"), + ), + }, + { + fstest.Apply( + fstest.CreateDir("/bin/", 0755), + fstest.CreateDir("/sbin/", 0755), + fstest.CreateFile("/sbin/superbinary", []byte{0x00, 0x00}, 0755), + fstest.Symlink("../sbin/superbinary", "/bin/superbinary"), + fstest.Symlink("../bin/superbinary", "/bin/other1"), + ), + fstest.Apply( + fstest.Remove("/bin/other1"), + fstest.Symlink("/bin/superbinary", "/bin/other1"), + ), + }, + { + fstest.Apply( + fstest.CreateDir("/bin/", 0755), + fstest.CreateFile("/bin/actualbinary", []byte{0x00, 0x00}, 0755), + fstest.Symlink("actualbinary", "/bin/superbinary"), + fstest.Symlink("../bin/superbinary", "/bin/other1"), + fstest.Symlink("superbinary", "/bin/other2"), + ), + fstest.Apply( + fstest.Remove("/bin/other1"), + fstest.Remove("/bin/other2"), + fstest.Symlink("/bin/superbinary", "/bin/other1"), + fstest.Symlink("superbinary", "/bin/other2"), + ), + }, + { + fstest.Apply( + fstest.CreateDir("/bin/", 0755), + fstest.CreateFile("/bin/actualbinary", []byte{0x00, 0x00}, 0755), + fstest.Symlink("actualbinary", "/bin/myapp"), + ), + fstest.Apply( + fstest.Remove("/bin/myapp"), + fstest.CreateDir("/bin/myapp", 0755), + ), + }, + } + + for i, l := range links { + if err := testDiffApply(l[0], l[1]); err != nil { + t.Fatalf("Test[%d] apply failed: %+v", i+1, err) + } + } +} + func TestBreakouts(t *testing.T) { tc := TarContext{}.WithUidGid(os.Getuid(), os.Getgid()).WithModTime(time.Now().UTC()) expected := "unbroken" @@ -102,10 +179,17 @@ func TestBreakouts(t *testing.T) { } return nil } + errFileDiff := errors.New("files differ") sameFile := func(f1, f2 string) func(string) error { return func(root string) error { - p1 := filepath.Join(root, f1) - p2 := filepath.Join(root, f2) + p1, err := rootPath(root, f1) + if err != nil { + return err + } + p2, err := rootPath(root, f2) + if err != nil { + return err + } s1, err := os.Stat(p1) if err != nil { return err @@ -115,7 +199,20 @@ func TestBreakouts(t *testing.T) { return err } if !os.SameFile(s1, s2) { - return errors.Errorf("files differ: %#v and %#v", s1, s2) + return errors.Wrapf(errFileDiff, "%#v and %#v", s1, s2) + } + return nil + } + } + notSameFile := func(f1, f2 string) func(string) error { + same := sameFile(f1, f2) + return func(root string) error { + err := same(root) + if err == nil { + return errors.New("files are the same, expected diff") + } + if errors.Cause(err) != errFileDiff { + return err } return nil } @@ -124,6 +221,7 @@ func TestBreakouts(t *testing.T) { breakouts := []struct { name string w WriterToTar + apply fstest.Applier validator func(string) error }{ { @@ -277,10 +375,103 @@ func TestBreakouts(t *testing.T) { ), validator: sameFile("/localpasswd", "/etc/passwd"), }, + { + name: "SymlinkParentDirectory", + w: TarAll( + tc.Dir("etc", 0770), + tc.File("/etc/passwd", []byte("inside"), 0644), + tc.Symlink("/etc/", ".."), + tc.Link("/etc/passwd", "localpasswd"), + ), + validator: sameFile("/localpasswd", "/etc/passwd"), + }, + { + name: "SymlinkEmptyFilename", + w: TarAll( + tc.Dir("etc", 0770), + tc.File("/etc/passwd", []byte("inside"), 0644), + tc.Symlink("/etc/", ""), + tc.Link("/etc/passwd", "localpasswd"), + ), + validator: sameFile("/localpasswd", "/etc/passwd"), + }, + { + name: "SymlinkParentRelative", + w: TarAll( + tc.Dir("etc", 0770), + tc.File("/etc/passwd", []byte("inside"), 0644), + tc.Symlink("/etc/", "localetc/sub/.."), + tc.Link("/etc/passwd", "/localetc/localpasswd"), + ), + validator: sameFile("/localetc/localpasswd", "/etc/passwd"), + }, + { + name: "SymlinkSlashEnded", + w: TarAll( + tc.Dir("etc", 0770), + tc.File("/etc/passwd", []byte("inside"), 0644), + tc.Dir("localetc/", 0770), + tc.Link("/etc/passwd", "/localetc/localpasswd"), + ), + validator: sameFile("/localetc/localpasswd", "/etc/passwd"), + }, + { + name: "SymlinkOverrideDirectory", + apply: fstest.Apply( + fstest.CreateDir("/etc/", 0755), + fstest.CreateFile("/etc/passwd", []byte("inside"), 0644), + fstest.CreateDir("/localetc/", 0755), + ), + w: TarAll( + tc.Symlink("/etc", "localetc"), + tc.Link("/etc/passwd", "/localetc/localpasswd"), + ), + validator: sameFile("/localetc/localpasswd", "/etc/passwd"), + }, + { + name: "SymlinkOverrideDirectoryRelative", + apply: fstest.Apply( + fstest.CreateDir("/etc/", 0755), + fstest.CreateFile("/etc/passwd", []byte("inside"), 0644), + fstest.CreateDir("/localetc/", 0755), + ), + w: TarAll( + tc.Symlink("../../etc", "localetc"), + tc.Link("/etc/passwd", "/localetc/localpasswd"), + ), + validator: sameFile("/localetc/localpasswd", "/etc/passwd"), + }, + { + name: "DirectoryOverrideSymlink", + apply: fstest.Apply( + fstest.CreateDir("/etc/", 0755), + fstest.CreateFile("/etc/passwd", []byte("inside"), 0644), + fstest.Symlink("/etc", "localetc"), + ), + w: TarAll( + tc.Dir("/localetc/", 0755), + tc.Link("/etc/passwd", "/localetc/localpasswd"), + ), + validator: sameFile("/localetc/localpasswd", "/etc/passwd"), + }, + { + name: "DirectoryOverrideSymlinkAndHardlink", + apply: fstest.Apply( + fstest.CreateDir("/etc/", 0755), + fstest.CreateFile("/etc/passwd", []byte("inside"), 0644), + fstest.Symlink("etc", "localetc"), + fstest.Link("/etc/passwd", "/localetc/localpasswd"), + ), + w: TarAll( + tc.Dir("/localetc/", 0755), + tc.File("/localetc/localpasswd", []byte("different"), 0644), + ), + validator: notSameFile("/localetc/localpasswd", "/etc/passwd"), + }, } for _, bo := range breakouts { - t.Run(bo.name, makeWriterToTarTest(bo.w, bo.validator)) + t.Run(bo.name, makeWriterToTarTest(bo.w, bo.apply, bo.validator)) } } @@ -288,6 +479,56 @@ func TestDiffApply(t *testing.T) { fstest.FSSuite(t, diffApplier{}) } +func TestApplyTar(t *testing.T) { + tc := TarContext{}.WithUidGid(os.Getuid(), os.Getgid()).WithModTime(time.Now().UTC()) + directoriesExist := func(dirs ...string) func(string) error { + return func(root string) error { + for _, d := range dirs { + p, err := rootPath(root, d) + if err != nil { + return err + } + if _, err := os.Stat(p); err != nil { + return errors.Wrapf(err, "failure checking existance for %v", d) + } + } + return nil + } + } + + tests := []struct { + name string + w WriterToTar + apply fstest.Applier + validator func(string) error + }{ + { + name: "DirectoryCreation", + apply: fstest.Apply( + fstest.CreateDir("/etc/", 0755), + ), + w: TarAll( + tc.Dir("/etc/subdir", 0755), + tc.Dir("/etc/subdir2/", 0755), + tc.Dir("/etc/subdir2/more", 0755), + tc.Dir("/other/noparent-1/1", 0755), + tc.Dir("/other/noparent-2/2/", 0755), + ), + validator: directoriesExist( + "etc/subdir", + "etc/subdir2", + "etc/subdir2/more", + "other/noparent-1/1", + "other/noparent-2/2", + ), + }, + } + + for _, at := range tests { + t.Run(at.name, makeWriterToTarTest(at.w, at.apply, at.validator)) + } +} + func testApply(a fstest.Applier) error { td, err := ioutil.TempDir("", "test-apply-") if err != nil { @@ -356,7 +597,7 @@ func testBaseDiff(a fstest.Applier) error { return fstest.CheckDirectoryEqual(td, dest) } -func testDiffApply(a fstest.Applier) error { +func testDiffApply(appliers ...fstest.Applier) error { td, err := ioutil.TempDir("", "test-diff-apply-") if err != nil { return errors.Wrap(err, "failed to create temp dir") @@ -368,11 +609,22 @@ func testDiffApply(a fstest.Applier) error { } defer os.RemoveAll(dest) - if err := a.Apply(td); err != nil { - return errors.Wrap(err, "failed to apply filesystem changes") + for _, a := range appliers { + if err := a.Apply(td); err != nil { + return errors.Wrap(err, "failed to apply filesystem changes") + } } - diffBytes, err := ioutil.ReadAll(Diff(context.Background(), "", td)) + // Apply base changes before diff + if len(appliers) > 1 { + for _, a := range appliers[:len(appliers)-1] { + if err := a.Apply(dest); err != nil { + return errors.Wrap(err, "failed to apply base filesystem changes") + } + } + } + + diffBytes, err := ioutil.ReadAll(Diff(context.Background(), dest, td)) if err != nil { return errors.Wrap(err, "failed to create diff") } @@ -384,7 +636,7 @@ func testDiffApply(a fstest.Applier) error { return fstest.CheckDirectoryEqual(td, dest) } -func makeWriterToTarTest(wt WriterToTar, validate func(string) error) func(*testing.T) { +func makeWriterToTarTest(wt WriterToTar, a fstest.Applier, validate func(string) error) func(*testing.T) { return func(t *testing.T) { td, err := ioutil.TempDir("", "test-writer-to-tar-") if err != nil { @@ -392,6 +644,12 @@ func makeWriterToTarTest(wt WriterToTar, validate func(string) error) func(*test } defer os.RemoveAll(td) + if a != nil { + if err := a.Apply(td); err != nil { + t.Fatalf("Failed to apply filesystem to directory: %v", err) + } + } + tr := TarFromWriterTo(wt) if _, err := Apply(context.Background(), td, tr); err != nil {