Revert "Revert "Pass root to chroot to for chroot Untar""

This reverts commit 60013ba69b.

Signed-off-by: Tibor Vass <tibor@docker.com>

daemon/archive.go

@@ -31,11 +31,12 @@ type archiver interface {
 }
 
 // helper functions to extract or archive
-func extractArchive(i interface{}, src io.Reader, dst string, opts *archive.TarOptions) error {
+func extractArchive(i interface{}, src io.Reader, dst string, opts *archive.TarOptions, root string) error {
 	if ea, ok := i.(extractor); ok {
 		return ea.ExtractArchive(src, dst, opts)
 	}
-	return chrootarchive.Untar(src, dst, opts)
+
+	return chrootarchive.UntarWithRoot(src, dst, opts, root)
 }
 
 func archivePath(i interface{}, src string, opts *archive.TarOptions) (io.ReadCloser, error) {
@@ -367,7 +368,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path
 		}
 	}
 
-	if err := extractArchive(driver, content, resolvedPath, options); err != nil {
+	if err := extractArchive(driver, content, resolvedPath, options, container.BaseFS.Path()); err != nil {
 		return err
 	}
 

pkg/chrootarchive/archive.go

@@ -27,18 +27,34 @@ 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)
+	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)
 }
 
 // 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)
+	return untarHandler(tarArchive, dest, options, false, dest)
 }
 
 // Handler for teasing out the automatic decompression
-func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions, decompress bool) error {
+func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions, decompress bool, root string) error {
 	if tarArchive == nil {
 		return fmt.Errorf("Empty archive")
 	}
@@ -69,5 +85,5 @@ func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions
 		r = decompressedArchive
 	}
 
-	return invokeUnpack(r, dest, options)
+	return invokeUnpack(r, dest, options, root)
 }

pkg/chrootarchive/archive_unix.go

@@ -10,6 +10,7 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"runtime"
 
 	"github.com/docker/docker/pkg/archive"
@@ -30,11 +31,21 @@ func untar() {
 		fatal(err)
 	}
 
-	if err := chroot(flag.Arg(0)); err != nil {
+	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 {
 		fatal(err)
 	}
 
-	if err := archive.Unpack(os.Stdin, "/", options); err != nil {
+	if err := archive.Unpack(os.Stdin, dst, options); err != nil {
 		fatal(err)
 	}
 	// fully consume stdin in case it is zero padded
@@ -45,7 +56,7 @@ func untar() {
 	os.Exit(0)
 }
 
-func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.TarOptions) error {
+func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.TarOptions, root string) 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
@@ -57,7 +68,21 @@ func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.T
 		return fmt.Errorf("Untar pipe failure: %v", err)
 	}
 
-	cmd := reexec.Command("docker-untar", dest)
+	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.Stdin = decompressedArchive
 
 	cmd.ExtraFiles = append(cmd.ExtraFiles, r)
@@ -69,6 +94,7 @@ 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()

pkg/chrootarchive/archive_unix_test.go

@@ -0,0 +1,77 @@
+// +build !windows
+
+package chrootarchive
+
+import (
+	"bytes"
+	"io"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"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")
+}

pkg/chrootarchive/archive_windows.go

@@ -14,7 +14,7 @@ func chroot(path string) error {
 
 func invokeUnpack(decompressedArchive io.ReadCloser,
 	dest string,
-	options *archive.TarOptions) error {
+	options *archive.TarOptions, root string) 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.