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 {