[19.03] Add chroot back to Tar/Untar without the previously introduced regression

daemon/archive.go

@@ -31,18 +31,19 @@ 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) {
+func archivePath(i interface{}, src string, opts *archive.TarOptions, root string) (io.ReadCloser, error) {
 	if ap, ok := i.(archiver); ok {
 		return ap.ArchivePath(src, opts)
 	}
-	return archive.TarWithOptions(src, opts)
+	return chrootarchive.Tar(src, opts, root)
 }
 
 // ContainerCopy performs a deprecated operation of archiving the resource at
@@ -235,10 +236,16 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path
 	if driver.Base(resolvedPath) == "." {
 		resolvedPath += string(driver.Separator()) + "."
 	}
-	sourceDir, sourceBase := driver.Dir(resolvedPath), driver.Base(resolvedPath)
+
+	sourceDir := resolvedPath
+	sourceBase := "."
+
+	if stat.Mode&os.ModeDir == 0 { // not dir
+		sourceDir, sourceBase = driver.Split(resolvedPath)
+	}
 	opts := archive.TarResourceRebaseOpts(sourceBase, driver.Base(absPath))
 
-	data, err := archivePath(driver, sourceDir, opts)
+	data, err := archivePath(driver, sourceDir, opts, container.BaseFS.Path())
 	if err != nil {
 		return nil, nil, err
 	}
@@ -367,7 +374,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
 	}
 
@@ -425,14 +432,11 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str
 		d, f := driver.Split(basePath)
 		basePath = d
 		filter = []string{f}
-	} else {
-		filter = []string{driver.Base(basePath)}
-		basePath = driver.Dir(basePath)
 	}
 	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

integration/container/copy_test.go

@@ -1,13 +1,20 @@
 package container // import "github.com/docker/docker/integration/container"
 
 import (
+	"archive/tar"
 	"context"
+	"encoding/json"
 	"fmt"
+	"io"
+	"io/ioutil"
+	"os"
 	"testing"
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/client"
 	"github.com/docker/docker/integration/internal/container"
+	"github.com/docker/docker/internal/test/fakecontext"
+	"github.com/docker/docker/pkg/jsonmessage"
 	"gotest.tools/assert"
 	is "gotest.tools/assert/cmp"
 	"gotest.tools/skip"
@@ -64,3 +71,93 @@ func TestCopyToContainerPathIsNotDir(t *testing.T) {
 	err := apiclient.CopyToContainer(ctx, cid, "/etc/passwd/", nil, types.CopyToContainerOptions{})
 	assert.Assert(t, is.ErrorContains(err, "not a directory"))
 }
+
+func TestCopyFromContainer(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows")
+	defer setupTest(t)()
+
+	ctx := context.Background()
+	apiClient := testEnv.APIClient()
+
+	dir, err := ioutil.TempDir("", t.Name())
+	assert.NilError(t, err)
+	defer os.RemoveAll(dir)
+
+	buildCtx := fakecontext.New(t, dir, fakecontext.WithFile("foo", "hello"), fakecontext.WithFile("baz", "world"), fakecontext.WithDockerfile(`
+		FROM busybox
+		COPY foo /foo
+		COPY baz /bar/quux/baz
+		RUN ln -s notexist /bar/notarget && ln -s quux/baz /bar/filesymlink && ln -s quux /bar/dirsymlink && ln -s / /bar/root
+		CMD /fake
+	`))
+	defer buildCtx.Close()
+
+	resp, err := apiClient.ImageBuild(ctx, buildCtx.AsTarReader(t), types.ImageBuildOptions{})
+	assert.NilError(t, err)
+	defer resp.Body.Close()
+
+	var imageID string
+	err = jsonmessage.DisplayJSONMessagesStream(resp.Body, ioutil.Discard, 0, false, func(msg jsonmessage.JSONMessage) {
+		var r types.BuildResult
+		assert.NilError(t, json.Unmarshal(*msg.Aux, &r))
+		imageID = r.ID
+	})
+	assert.NilError(t, err)
+	assert.Assert(t, imageID != "")
+
+	cid := container.Create(t, ctx, apiClient, container.WithImage(imageID))
+
+	for _, x := range []struct {
+		src    string
+		expect map[string]string
+	}{
+		{"/", map[string]string{"/": "", "/foo": "hello", "/bar/quux/baz": "world", "/bar/filesymlink": "", "/bar/dirsymlink": "", "/bar/notarget": ""}},
+		{"/bar/root", map[string]string{"root": ""}},
+		{"/bar/root/", map[string]string{"root/": "", "root/foo": "hello", "root/bar/quux/baz": "world", "root/bar/filesymlink": "", "root/bar/dirsymlink": "", "root/bar/notarget": ""}},
+
+		{"bar/quux", map[string]string{"quux/": "", "quux/baz": "world"}},
+		{"bar/quux/", map[string]string{"quux/": "", "quux/baz": "world"}},
+		{"bar/quux/baz", map[string]string{"baz": "world"}},
+
+		{"bar/filesymlink", map[string]string{"filesymlink": ""}},
+		{"bar/dirsymlink", map[string]string{"dirsymlink": ""}},
+		{"bar/dirsymlink/", map[string]string{"dirsymlink/": "", "dirsymlink/baz": "world"}},
+		{"bar/notarget", map[string]string{"notarget": ""}},
+	} {
+		t.Run(x.src, func(t *testing.T) {
+			rdr, _, err := apiClient.CopyFromContainer(ctx, cid, x.src)
+			assert.NilError(t, err)
+			defer rdr.Close()
+
+			found := make(map[string]bool, len(x.expect))
+			var numFound int
+			tr := tar.NewReader(rdr)
+			for numFound < len(x.expect) {
+				h, err := tr.Next()
+				if err == io.EOF {
+					break
+				}
+				assert.NilError(t, err)
+
+				expected, exists := x.expect[h.Name]
+				if !exists {
+					// this archive will have extra stuff in it since we are copying from root
+					// and docker adds a bunch of stuff
+					continue
+				}
+
+				numFound++
+				found[h.Name] = true
+
+				buf, err := ioutil.ReadAll(tr)
+				if err == nil {
+					assert.Check(t, is.Equal(string(buf), expected))
+				}
+			}
+
+			for f := range x.expect {
+				assert.Check(t, found[f], f+" not found in archive")
+			}
+		})
+	}
+}

pkg/archive/archive_unix.go

@@ -7,6 +7,7 @@ import (
 	"errors"
 	"os"
 	"path/filepath"
+	"strings"
 	"syscall"
 
 	"github.com/docker/docker/pkg/idtools"
@@ -26,7 +27,7 @@ func fixVolumePathPrefix(srcPath string) string {
 // can't use filepath.Join(srcPath,include) because this will clean away
 // a trailing "." or "/" which may be important.
 func getWalkRoot(srcPath string, include string) string {
-	return srcPath + string(filepath.Separator) + include
+	return strings.TrimSuffix(srcPath, string(filepath.Separator)) + string(filepath.Separator) + include
 }
 
 // CanonicalTarNameForPath returns platform-specific filepath

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,13 @@ func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions
 		r = decompressedArchive
 	}
 
-	return invokeUnpack(r, dest, options)
+	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)
 }