10000 Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) by cpuguy83 · Pull Request #39292 · moby/moby · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) #39292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions daemon/archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it perhaps make sense to add root to archive.TarOptions instead of as a new argument?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, those are specific to tar format.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point; I was a bit in doubt; had a quick look at other options, and wasn't sure if all the existing ones were really Tar-specific, or just a mechanism to pass options for our specific purpose

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
Expand Down Expand Up @@ -238,7 +239,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)
data, err := archivePath(driver, sourceDir, opts, container.BaseFS.Path())
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -432,7 +433,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
}
Expand Down
2 changes: 1 addition & 1 deletion daemon/export.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 28 additions & 4 deletions pkg/chrootarchive/archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the 5th parameter is the same with 2nd parameter , both dest. is it by purpose ?
it will make the chroot folder always as "/" ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with Untar -- is there a reason to have the old functions around given that they're broken?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just didn't want to make a bunch of changes to code that isn't affected by this.
In reality there are only a couple of places this is used, I think it's safe to go ahead just change the method signatures rather than introduce a new one.

}

// 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")
}
Expand Down Expand Up @@ -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)
}
130 changes: 125 additions & 5 deletions pkg/chrootarchive/archive_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@ 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
Expand All @@ -23,18 +26,28 @@ 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)
}

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
Expand All @@ -45,7 +58,10 @@ 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 {
if root == "" {
return errors.New("must specify a root to chroot to")
}

// We can't pass a potentially large exclude list directly via cmd line
// because we easily overrun the kernel's max argument/environment size
Expand All @@ -57,7 +73,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)
Expand All @@ -69,6 +99,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()
Expand All @@ -86,3 +117,92 @@ 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
}
Loading
0