Merge pull request #275 from tiborvass/19.03-revert-chroot-tar-untar

[19.03] Revert Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664)

daemon/archive.go

@@ -31,19 +31,18 @@ type archiver interface {
 }
 
 // helper functions to extract or archive
-func extractArchive(i interface{}, src io.Reader, dst string, opts *archive.TarOptions, root string) error {
+func extractArchive(i interface{}, src io.Reader, dst string, opts *archive.TarOptions) error {
 	if ea, ok := i.(extractor); ok {
 		return ea.ExtractArchive(src, dst, opts)
 	}
-
-	return chrootarchive.UntarWithRoot(src, dst, opts, root)
+	return chrootarchive.Untar(src, dst, opts)
 }
 
-func archivePath(i interface{}, src string, opts *archive.TarOptions, root string) (io.ReadCloser, error) {
+func archivePath(i interface{}, src string, opts *archive.TarOptions) (io.ReadCloser, error) {
 	if ap, ok := i.(archiver); ok {
 		return ap.ArchivePath(src, opts)
 	}
-	return chrootarchive.Tar(src, opts, root)
+	return archive.TarWithOptions(src, opts)
 }
 
 // ContainerCopy performs a deprecated operation of archiving the resource at
@@ -239,7 +238,7 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path
 	sourceDir, sourceBase := driver.Dir(resolvedPath), driver.Base(resolvedPath)
 	opts := archive.TarResourceRebaseOpts(sourceBase, driver.Base(absPath))
 
-	data, err := archivePath(driver, sourceDir, opts, container.BaseFS.Path())
+	data, err := archivePath(driver, sourceDir, opts)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -368,7 +367,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path
 		}
 	}
 
-	if err := extractArchive(driver, content, resolvedPath, options, container.BaseFS.Path()); err != nil {
+	if err := extractArchive(driver, content, resolvedPath, options); err != nil {
 		return err
 	}
 
@@ -433,7 +432,7 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str
 	archive, err := archivePath(driver, basePath, &archive.TarOptions{
 		Compression:  archive.Uncompressed,
 		IncludeFiles: filter,
-	}, container.BaseFS.Path())
+	})
 	if err != nil {
 		return nil, err
 	}

daemon/export.go

@@ -70,7 +70,7 @@ func (daemon *Daemon) containerExport(container *container.Container) (arch io.R
 		Compression: archive.Uncompressed,
 		UIDMaps:     daemon.idMapping.UIDs(),
 		GIDMaps:     daemon.idMapping.GIDs(),
-	}, basefs.Path())
+	})
 	if err != nil {
 		rwlayer.Unmount()
 		return nil, err

pkg/chrootarchive/archive.go

@@ -27,34 +27,18 @@ func NewArchiver(idMapping *idtools.IdentityMapping) *archive.Archiver {
 // The archive may be compressed with one of the following algorithms:
 //  identity (uncompressed), gzip, bzip2, xz.
 func Untar(tarArchive io.Reader, dest string, options *archive.TarOptions) error {
-	return untarHandler(tarArchive, dest, options, true, dest)
-}
-
-// UntarWithRoot is the same as `Untar`, but allows you to pass in a root directory
-// The root directory is the directory that will be chrooted to.
-// `dest` must be a path within `root`, if it is not an error will be returned.
-//
-// `root` should set to a directory which is not controlled by any potentially
-// malicious process.
-//
-// This should be used to prevent a potential attacker from manipulating `dest`
-// such that it would provide access to files outside of `dest` through things
-// like symlinks. Normally `ResolveSymlinksInScope` would handle this, however
-// sanitizing symlinks in this manner is inherrently racey:
-// ref: CVE-2018-15664
-func UntarWithRoot(tarArchive io.Reader, dest string, options *archive.TarOptions, root string) error {
-	return untarHandler(tarArchive, dest, options, true, root)
+	return untarHandler(tarArchive, dest, options, true)
 }
 
 // UntarUncompressed reads a stream of bytes from `archive`, parses it as a tar archive,
 // and unpacks it into the directory at `dest`.
 // The archive must be an uncompressed stream.
 func UntarUncompressed(tarArchive io.Reader, dest string, options *archive.TarOptions) error {
-	return untarHandler(tarArchive, dest, options, false, dest)
+	return untarHandler(tarArchive, dest, options, false)
 }
 
 // Handler for teasing out the automatic decompression
-func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions, decompress bool, root string) error {
+func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions, decompress bool) error {
 	if tarArchive == nil {
 		return fmt.Errorf("Empty archive")
 	}
@@ -85,13 +69,5 @@ func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions
 		r = decompressedArchive
 	}
 
-	return invokeUnpack(r, dest, options, root)
-}
-
-// Tar tars the requested path while chrooted to the specified root.
-func Tar(srcPath string, options *archive.TarOptions, root string) (io.ReadCloser, error) {
-	if options == nil {
-		options = &archive.TarOptions{}
-	}
-	return invokePack(srcPath, options, root)
+	return invokeUnpack(r, dest, options)
 }

pkg/chrootarchive/archive_unix.go

@@ -10,13 +10,10 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"path/filepath"
 	"runtime"
-	"strings"
 
 	"github.com/docker/docker/pkg/archive"
 	"github.com/docker/docker/pkg/reexec"
-	"github.com/pkg/errors"
 )
 
 // untar is the entry-point for docker-untar on re-exec. This is not used on
@@ -26,28 +23,18 @@ func untar() {
 	runtime.LockOSThread()
 	flag.Parse()
 
-	var options archive.TarOptions
+	var options *archive.TarOptions
 
 	//read the options from the pipe "ExtraFiles"
 	if err := json.NewDecoder(os.NewFile(3, "options")).Decode(&options); err != nil {
 		fatal(err)
 	}
 
-	dst := flag.Arg(0)
-	var root string
-	if len(flag.Args()) > 1 {
-		root = flag.Arg(1)
-	}
-
-	if root == "" {
-		root = dst
-	}
-
-	if err := chroot(root); err != nil {
+	if err := chroot(flag.Arg(0)); err != nil {
 		fatal(err)
 	}
 
-	if err := archive.Unpack(os.Stdin, dst, &options); err != nil {
+	if err := archive.Unpack(os.Stdin, "/", options); err != nil {
 		fatal(err)
 	}
 	// fully consume stdin in case it is zero padded
@@ -58,10 +45,7 @@ func untar() {
 	os.Exit(0)
 }
 
-func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.TarOptions, root string) error {
-	if root == "" {
-		return errors.New("must specify a root to chroot to")
-	}
+func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.TarOptions) error {
 
 	// We can't pass a potentially large exclude list directly via cmd line
 	// because we easily overrun the kernel's max argument/environment size
@@ -73,21 +57,7 @@ func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.T
 		return fmt.Errorf("Untar pipe failure: %v", err)
 	}
 
-	if root != "" {
-		relDest, err := filepath.Rel(root, dest)
-		if err != nil {
-			return err
-		}
-		if relDest == "." {
-			relDest = "/"
-		}
-		if relDest[0] != '/' {
-			relDest = "/" + relDest
-		}
-		dest = relDest
-	}
-
-	cmd := reexec.Command("docker-untar", dest, root)
+	cmd := reexec.Command("docker-untar", dest)
 	cmd.Stdin = decompressedArchive
 
 	cmd.ExtraFiles = append(cmd.ExtraFiles, r)
@@ -99,7 +69,6 @@ func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.T
 		w.Close()
 		return fmt.Errorf("Untar error on re-exec cmd: %v", err)
 	}
-
 	//write the options to the pipe for the untar exec to read
 	if err := json.NewEncoder(w).Encode(options); err != nil {
 		w.Close()
@@ -117,92 +86,3 @@ func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.T
 	}
 	return nil
 }
-
-func tar() {
-	runtime.LockOSThread()
-	flag.Parse()
-
-	src := flag.Arg(0)
-	var root string
-	if len(flag.Args()) > 1 {
-		root = flag.Arg(1)
-	}
-
-	if root == "" {
-		root = src
-	}
-
-	if err := realChroot(root); err != nil {
-		fatal(err)
-	}
-
-	var options archive.TarOptions
-	if err := json.NewDecoder(os.Stdin).Decode(&options); err != nil {
-		fatal(err)
-	}
-
-	rdr, err := archive.TarWithOptions(src, &options)
-	if err != nil {
-		fatal(err)
-	}
-	defer rdr.Close()
-
-	if _, err := io.Copy(os.Stdout, rdr); err != nil {
-		fatal(err)
-	}
-
-	os.Exit(0)
-}
-
-func invokePack(srcPath string, options *archive.TarOptions, root string) (io.ReadCloser, error) {
-	if root == "" {
-		return nil, errors.New("root path must not be empty")
-	}
-
-	relSrc, err := filepath.Rel(root, srcPath)
-	if err != nil {
-		return nil, err
-	}
-	if relSrc == "." {
-		relSrc = "/"
-	}
-	if relSrc[0] != '/' {
-		relSrc = "/" + relSrc
-	}
-
-	// make sure we didn't trim a trailing slash with the call to `Rel`
-	if strings.HasSuffix(srcPath, "/") && !strings.HasSuffix(relSrc, "/") {
-		relSrc += "/"
-	}
-
-	cmd := reexec.Command("docker-tar", relSrc, root)
-
-	errBuff := bytes.NewBuffer(nil)
-	cmd.Stderr = errBuff
-
-	tarR, tarW := io.Pipe()
-	cmd.Stdout = tarW
-
-	stdin, err := cmd.StdinPipe()
-	if err != nil {
-		return nil, errors.Wrap(err, "error getting options pipe for tar process")
-	}
-
-	if err := cmd.Start(); err != nil {
-		return nil, errors.Wrap(err, "tar error on re-exec cmd")
-	}
-
-	go func() {
-		err := cmd.Wait()
-		err = errors.Wrapf(err, "error processing tar file: %s", errBuff)
-		tarW.CloseWithError(err)
-	}()
-
-	if err := json.NewEncoder(stdin).Encode(options); err != nil {
-		stdin.Close()
-		return nil, errors.Wrap(err, "tar json encode to pipe failed")
-	}
-	stdin.Close()
-
-	return tarR, nil
-}

pkg/chrootarchive/archive_unix_test.go

@@ -1,171 +0,0 @@
-// +build !windows
-
-package chrootarchive
-
-import (
-	gotar "archive/tar"
-	"bytes"
-	"io"
-	"io/ioutil"
-	"os"
-	"path"
-	"path/filepath"
-	"strings"
-	"testing"
-
-	"github.com/docker/docker/pkg/archive"
-	"golang.org/x/sys/unix"
-	"gotest.tools/assert"
-)
-
-// Test for CVE-2018-15664
-// Assures that in the case where an "attacker" controlled path is a symlink to
-// some path outside of a container's rootfs that we do not copy data to a
-// container path that will actually overwrite data on the host
-func TestUntarWithMaliciousSymlinks(t *testing.T) {
-	dir, err := ioutil.TempDir("", t.Name())
-	assert.NilError(t, err)
-	defer os.RemoveAll(dir)
-
-	root := filepath.Join(dir, "root")
-
-	err = os.MkdirAll(root, 0755)
-	assert.NilError(t, err)
-
-	// Add a file into a directory above root
-	// Ensure that we can't access this file while tarring.
-	err = ioutil.WriteFile(filepath.Join(dir, "host-file"), []byte("I am a host file"), 0644)
-	assert.NilError(t, err)
-
-	// Create some data which which will be copied into the "container" root into
-	// the symlinked path.
-	// Before this change, the copy would overwrite the "host" content.
-	// With this change it should not.
-	data := filepath.Join(dir, "data")
-	err = os.MkdirAll(data, 0755)
-	assert.NilError(t, err)
-	err = ioutil.WriteFile(filepath.Join(data, "local-file"), []byte("pwn3d"), 0644)
-	assert.NilError(t, err)
-
-	safe := filepath.Join(root, "safe")
-	err = unix.Symlink(dir, safe)
-	assert.NilError(t, err)
-
-	rdr, err := archive.TarWithOptions(data, &archive.TarOptions{IncludeFiles: []string{"local-file"}, RebaseNames: map[string]string{"local-file": "host-file"}})
-	assert.NilError(t, err)
-
-	// Use tee to test both the good case and the bad case w/o recreating the archive
-	bufRdr := bytes.NewBuffer(nil)
-	tee := io.TeeReader(rdr, bufRdr)
-
-	err = UntarWithRoot(tee, safe, nil, root)
-	assert.Assert(t, err != nil)
-	assert.ErrorContains(t, err, "open /safe/host-file: no such file or directory")
-
-	// Make sure the "host" file is still in tact
-	// Before the fix the host file would be overwritten
-	hostData, err := ioutil.ReadFile(filepath.Join(dir, "host-file"))
-	assert.NilError(t, err)
-	assert.Equal(t, string(hostData), "I am a host file")
-
-	// Now test by chrooting to an attacker controlled path
-	// This should succeed as is and overwrite a "host" file
-	// Note that this would be a mis-use of this function.
-	err = UntarWithRoot(bufRdr, safe, nil, safe)
-	assert.NilError(t, err)
-
-	hostData, err = ioutil.ReadFile(filepath.Join(dir, "host-file"))
-	assert.NilError(t, err)
-	assert.Equal(t, string(hostData), "pwn3d")
-}
-
-// Test for CVE-2018-15664
-// Assures that in the case where an "attacker" controlled path is a symlink to
-// some path outside of a container's rootfs that we do not unwittingly leak
-// host data into the archive.
-func TestTarWithMaliciousSymlinks(t *testing.T) {
-	dir, err := ioutil.TempDir("", t.Name())
-	assert.NilError(t, err)
-	// defer os.RemoveAll(dir)
-	t.Log(dir)
-
-	root := filepath.Join(dir, "root")
-
-	err = os.MkdirAll(root, 0755)
-	assert.NilError(t, err)
-
-	hostFileData := []byte("I am a host file")
-
-	// Add a file into a directory above root
-	// Ensure that we can't access this file while tarring.
-	err = ioutil.WriteFile(filepath.Join(dir, "host-file"), hostFileData, 0644)
-	assert.NilError(t, err)
-
-	safe := filepath.Join(root, "safe")
-	err = unix.Symlink(dir, safe)
-	assert.NilError(t, err)
-
-	data := filepath.Join(dir, "data")
-	err = os.MkdirAll(data, 0755)
-	assert.NilError(t, err)
-
-	type testCase struct {
-		p        string
-		includes []string
-	}
-
-	cases := []testCase{
-		{p: safe, includes: []string{"host-file"}},
-		{p: safe + "/", includes: []string{"host-file"}},
-		{p: safe, includes: nil},
-		{p: safe + "/", includes: nil},
-		{p: root, includes: []string{"safe/host-file"}},
-		{p: root, includes: []string{"/safe/host-file"}},
-		{p: root, includes: nil},
-	}
-
-	maxBytes := len(hostFileData)
-
-	for _, tc := range cases {
-		t.Run(path.Join(tc.p+"_"+strings.Join(tc.includes, "_")), func(t *testing.T) {
-			// Here if we use archive.TarWithOptions directly or change the "root" parameter
-			// to be the same as "safe", data from the host will be leaked into the archive
-			var opts *archive.TarOptions
-			if tc.includes != nil {
-				opts = &archive.TarOptions{
-					IncludeFiles: tc.includes,
-				}
-			}
-			rdr, err := Tar(tc.p, opts, root)
-			assert.NilError(t, err)
-			defer rdr.Close()
-
-			tr := gotar.NewReader(rdr)
-			assert.Assert(t, !isDataInTar(t, tr, hostFileData, int64(maxBytes)), "host data leaked to archive")
-		})
-	}
-}
-
-func isDataInTar(t *testing.T, tr *gotar.Reader, compare []byte, maxBytes int64) bool {
-	for {
-		h, err := tr.Next()
-		if err == io.EOF {
-			break
-		}
-		assert.NilError(t, err)
-
-		if h.Size == 0 {
-			continue
-		}
-		assert.Assert(t, h.Size <= maxBytes, "%s: file size exceeds max expected size %d: %d", h.Name, maxBytes, h.Size)
-
-		data := make([]byte, int(h.Size))
-		_, err = io.ReadFull(tr, data)
-		assert.NilError(t, err)
-		if bytes.Contains(data, compare) {
-			return true
-		}
-	}
-
-	return false
-}

pkg/chrootarchive/archive_windows.go

@@ -14,16 +14,9 @@ func chroot(path string) error {
 
 func invokeUnpack(decompressedArchive io.ReadCloser,
 	dest string,
-	options *archive.TarOptions, root string) error {
+	options *archive.TarOptions) error {
 	// Windows is different to Linux here because Windows does not support
 	// chroot. Hence there is no point sandboxing a chrooted process to
 	// do the unpack. We call inline instead within the daemon process.
 	return archive.Unpack(decompressedArchive, longpath.AddPrefix(dest), options)
 }
-
-func invokePack(srcPath string, options *archive.TarOptions, root string) (io.ReadCloser, error) {
-	// Windows is different to Linux here because Windows does not support
-	// chroot. Hence there is no point sandboxing a chrooted process to
-	// do the pack. We call inline instead within the daemon process.
-	return archive.TarWithOptions(srcPath, options)
-}

pkg/chrootarchive/init_unix.go

@@ -14,7 +14,6 @@ import (
 func init() {
 	reexec.Register("docker-applyLayer", applyLayer)
 	reexec.Register("docker-untar", untar)
-	reexec.Register("docker-tar", tar)
 }
 
 func fatal(err error) {