From efd16d27645420cbda4a0d53720b4ca5dc66fdc5 Mon Sep 17 00:00:00 2001 From: unclejack Date: Tue, 28 Oct 2014 23:18:45 +0200 Subject: [PATCH 01/13] pkg/symlink: avoid following out of scope Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) Upstream-commit: faab87cc36fb6f02ddd53e1be09f10623a40773a Component: engine --- components/engine/pkg/symlink/fs.go | 47 ++++-- components/engine/pkg/symlink/fs_test.go | 150 ++++++++++++++++-- components/engine/pkg/symlink/testdata/fs/j/k | 1 + 3 files changed, 171 insertions(+), 27 deletions(-) create mode 120000 components/engine/pkg/symlink/testdata/fs/j/k diff --git a/components/engine/pkg/symlink/fs.go b/components/engine/pkg/symlink/fs.go index d761732571..6ce99c6bda 100644 --- a/components/engine/pkg/symlink/fs.go +++ b/components/engine/pkg/symlink/fs.go @@ -12,6 +12,12 @@ const maxLoopCounter = 100 // FollowSymlink will follow an existing link and scope it to the root // path provided. +// The role of this function is to return an absolute path in the root +// or normalize to the root if the symlink leads to a path which is +// outside of the root. +// Errors encountered while attempting to follow the symlink in path +// will be reported. +// Normalizations to the root don't constitute errors. func FollowSymlinkInScope(link, root string) (string, error) { root, err := filepath.Abs(root) if err != nil { @@ -60,25 +66,36 @@ func FollowSymlinkInScope(link, root string) (string, error) { } return "", err } - if stat.Mode()&os.ModeSymlink == os.ModeSymlink { - dest, err := os.Readlink(prev) - if err != nil { - return "", err - } - if path.IsAbs(dest) { - prev = filepath.Join(root, dest) - } else { - prev, _ = filepath.Abs(prev) - - if prev = filepath.Join(filepath.Dir(prev), dest); len(prev) < len(root) { - prev = filepath.Join(root, filepath.Base(dest)) - } - } - } else { + // let's break if we're not dealing with a symlink + if stat.Mode()&os.ModeSymlink != os.ModeSymlink { break } + + // process the symlink + dest, err := os.Readlink(prev) + if err != nil { + return "", err + } + + if path.IsAbs(dest) { + prev = filepath.Join(root, dest) + } else { + prev, _ = filepath.Abs(prev) + + dir := filepath.Dir(prev) + prev = filepath.Join(dir, dest) + if dir == root && !strings.HasPrefix(prev, root) { + prev = root + } + if len(prev) < len(root) || (len(prev) == len(root) && prev != root) { + prev = filepath.Join(root, filepath.Base(dest)) + } + } } } + if prev == "/" { + prev = root + } return prev, nil } diff --git a/components/engine/pkg/symlink/fs_test.go b/components/engine/pkg/symlink/fs_test.go index cc0d82d1a3..0e2f948b6a 100644 --- a/components/engine/pkg/symlink/fs_test.go +++ b/components/engine/pkg/symlink/fs_test.go @@ -98,25 +98,151 @@ func TestFollowSymLinkRelativeLink(t *testing.T) { } func TestFollowSymLinkRelativeLinkScope(t *testing.T) { - link := "testdata/fs/a/f" + // avoid letting symlink f lead us out of the "testdata" scope + // we don't normalize because symlink f is in scope and there is no + // information leak + { + link := "testdata/fs/a/f" - rewrite, err := FollowSymlinkInScope(link, "testdata") - if err != nil { - t.Fatal(err) + rewrite, err := FollowSymlinkInScope(link, "testdata") + if err != nil { + t.Fatal(err) + } + + if expected := abs(t, "testdata/test"); expected != rewrite { + t.Fatalf("Expected %s got %s", expected, rewrite) + } } - if expected := abs(t, "testdata/test"); expected != rewrite { - t.Fatalf("Expected %s got %s", expected, rewrite) + // avoid letting symlink f lead us out of the "testdata/fs" scope + // we don't normalize because symlink f is in scope and there is no + // information leak + { + link := "testdata/fs/a/f" + + rewrite, err := FollowSymlinkInScope(link, "testdata/fs") + if err != nil { + t.Fatal(err) + } + + if expected := abs(t, "testdata/fs/test"); expected != rewrite { + t.Fatalf("Expected %s got %s", expected, rewrite) + } } - link = "testdata/fs/b/h" + // avoid letting symlink g (pointed at by symlink h) take out of scope + // TODO: we should probably normalize to scope here because ../[....]/root + // is out of scope and we leak information + { + link := "testdata/fs/b/h" - rewrite, err = FollowSymlinkInScope(link, "testdata") - if err != nil { - t.Fatal(err) + rewrite, err := FollowSymlinkInScope(link, "testdata") + if err != nil { + t.Fatal(err) + } + + if expected := abs(t, "testdata/root"); expected != rewrite { + t.Fatalf("Expected %s got %s", expected, rewrite) + } } - if expected := abs(t, "testdata/root"); expected != rewrite { - t.Fatalf("Expected %s got %s", expected, rewrite) + // avoid letting allowing symlink e lead us to ../b + // normalize to the "testdata/fs/a" + { + link := "testdata/fs/a/e" + + rewrite, err := FollowSymlinkInScope(link, "testdata/fs/a") + if err != nil { + t.Fatal(err) + } + + if expected := abs(t, "testdata/fs/a"); expected != rewrite { + t.Fatalf("Expected %s got %s", expected, rewrite) + } + } + + // avoid letting symlink -> ../directory/file escape from scope + // normalize to "testdata/fs/j" + { + link := "testdata/fs/j/k" + + rewrite, err := FollowSymlinkInScope(link, "testdata/fs/j") + if err != nil { + t.Fatal(err) + } + + if expected := abs(t, "testdata/fs/j"); expected != rewrite { + t.Fatalf("Expected %s got %s", expected, rewrite) + } + } + + // make sure we don't allow escaping to / + // normalize to dir + { + dir, err := ioutil.TempDir("", "docker-fs-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + linkFile := filepath.Join(dir, "foo") + os.Mkdir(filepath.Join(dir, ""), 0700) + os.Symlink("/", linkFile) + + rewrite, err := FollowSymlinkInScope(linkFile, dir) + if err != nil { + t.Fatal(err) + } + + if rewrite != dir { + t.Fatalf("Expected %s got %s", dir, rewrite) + } + } + + // make sure we don't allow escaping to / + // normalize to dir + { + dir, err := ioutil.TempDir("", "docker-fs-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + linkFile := filepath.Join(dir, "foo") + os.Mkdir(filepath.Join(dir, ""), 0700) + os.Symlink("/../../", linkFile) + + rewrite, err := FollowSymlinkInScope(linkFile, dir) + if err != nil { + t.Fatal(err) + } + + if rewrite != dir { + t.Fatalf("Expected %s got %s", dir, rewrite) + } + } + + // make sure we stay in scope without leaking information + // this also checks for escaping to / + // normalize to dir + { + dir, err := ioutil.TempDir("", "docker-fs-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + linkFile := filepath.Join(dir, "foo") + os.Mkdir(filepath.Join(dir, ""), 0700) + os.Symlink("../../", linkFile) + + rewrite, err := FollowSymlinkInScope(linkFile, dir) + if err != nil { + t.Fatal(err) + } + + if rewrite != dir { + t.Fatalf("Expected %s got %s", dir, rewrite) + } } } diff --git a/components/engine/pkg/symlink/testdata/fs/j/k b/components/engine/pkg/symlink/testdata/fs/j/k new file mode 120000 index 0000000000..f559e8fda2 --- /dev/null +++ b/components/engine/pkg/symlink/testdata/fs/j/k @@ -0,0 +1 @@ +../i/a \ No newline at end of file From 8c7a6654fa8d768fa9440fd9bd0c95378e09c1e7 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 3 Nov 2014 22:57:18 +0000 Subject: [PATCH 02/13] Move security opts to HostConfig These settings need to be in the HostConfig so that they are not committed to an image and cannot introduce a security issue. We can safely move this field from the Config to the HostConfig without any regressions because these settings are consumed at container created and used to populate fields on the Container struct. Because of this, existing settings will be honored for containers already created on a daemon with custom security settings and prevent values being consumed via an Image. Signed-off-by: Michael Crosby Conflicts: daemon/create.go changing config to hostConfig was required to fix the build Upstream-commit: 294843ef23fcff3c080d9fbd12df17ae7006a9f8 Component: engine --- components/engine/daemon/create.go | 4 ++-- components/engine/daemon/daemon.go | 11 +++++------ components/engine/daemon/daemon_unit_test.go | 2 +- components/engine/daemon/start.go | 3 +++ components/engine/runconfig/config.go | 2 -- components/engine/runconfig/hostconfig.go | 2 ++ components/engine/runconfig/parse.go | 2 +- 7 files changed, 14 insertions(+), 12 deletions(-) diff --git a/components/engine/daemon/create.go b/components/engine/daemon/create.go index 3a71a8ac7e..e666e6f6ff 100644 --- a/components/engine/daemon/create.go +++ b/components/engine/daemon/create.go @@ -83,8 +83,8 @@ func (daemon *Daemon) Create(config *runconfig.Config, hostConfig *runconfig.Hos if warnings, err = daemon.mergeAndVerifyConfig(config, img); err != nil { return nil, nil, err } - if hostConfig != nil && config.SecurityOpt == nil { - config.SecurityOpt, err = daemon.GenerateSecurityOpt(hostConfig.IpcMode) + if hostConfig != nil && hostConfig.SecurityOpt == nil { + hostConfig.SecurityOpt, err = daemon.GenerateSecurityOpt(hostConfig.IpcMode) if err != nil { return nil, nil, err } diff --git a/components/engine/daemon/daemon.go b/components/engine/daemon/daemon.go index 84628be729..93cb101f61 100644 --- a/components/engine/daemon/daemon.go +++ b/components/engine/daemon/daemon.go @@ -531,10 +531,10 @@ func (daemon *Daemon) getEntrypointAndArgs(configEntrypoint, configCmd []string) return entrypoint, args } -func parseSecurityOpt(container *Container, config *runconfig.Config) error { +func parseSecurityOpt(container *Container, config *runconfig.HostConfig) error { var ( - label_opts []string - err error + labelOpts []string + err error ) for _, opt := range config.SecurityOpt { @@ -544,7 +544,7 @@ func parseSecurityOpt(container *Container, config *runconfig.Config) error { } switch con[0] { case "label": - label_opts = append(label_opts, con[1]) + labelOpts = append(labelOpts, con[1]) case "apparmor": container.AppArmorProfile = con[1] default: @@ -552,7 +552,7 @@ func parseSecurityOpt(container *Container, config *runconfig.Config) error { } } - container.ProcessLabel, container.MountLabel, err = label.InitLabels(label_opts) + container.ProcessLabel, container.MountLabel, err = label.InitLabels(labelOpts) return err } @@ -586,7 +586,6 @@ func (daemon *Daemon) newContainer(name string, config *runconfig.Config, img *i execCommands: newExecStore(), } container.root = daemon.containerRoot(container.ID) - err = parseSecurityOpt(container, config) return container, err } diff --git a/components/engine/daemon/daemon_unit_test.go b/components/engine/daemon/daemon_unit_test.go index f3b899ec8d..fbc3302aaa 100644 --- a/components/engine/daemon/daemon_unit_test.go +++ b/components/engine/daemon/daemon_unit_test.go @@ -8,7 +8,7 @@ import ( func TestParseSecurityOpt(t *testing.T) { container := &Container{} - config := &runconfig.Config{} + config := &runconfig.HostConfig{} // test apparmor config.SecurityOpt = []string{"apparmor:test_profile"} diff --git a/components/engine/daemon/start.go b/components/engine/daemon/start.go index f2c375ddc9..f72407e3f3 100644 --- a/components/engine/daemon/start.go +++ b/components/engine/daemon/start.go @@ -44,6 +44,9 @@ func (daemon *Daemon) ContainerStart(job *engine.Job) engine.Status { } func (daemon *Daemon) setHostConfig(container *Container, hostConfig *runconfig.HostConfig) error { + if err := parseSecurityOpt(container, hostConfig); err != nil { + return err + } // Validate the HostConfig binds. Make sure that: // the source exists for _, bind := range hostConfig.Binds { diff --git a/components/engine/runconfig/config.go b/components/engine/runconfig/config.go index 29c54a4d6d..ca5c3240b6 100644 --- a/components/engine/runconfig/config.go +++ b/components/engine/runconfig/config.go @@ -33,7 +33,6 @@ type Config struct { NetworkDisabled bool MacAddress string OnBuild []string - SecurityOpt []string } func ContainerConfigFromJob(job *engine.Job) *Config { @@ -58,7 +57,6 @@ func ContainerConfigFromJob(job *engine.Job) *Config { } job.GetenvJson("ExposedPorts", &config.ExposedPorts) job.GetenvJson("Volumes", &config.Volumes) - config.SecurityOpt = job.GetenvList("SecurityOpt") if PortSpecs := job.GetenvList("PortSpecs"); PortSpecs != nil { config.PortSpecs = PortSpecs } diff --git a/components/engine/runconfig/hostconfig.go b/components/engine/runconfig/hostconfig.go index 01388ad727..b619e9c31c 100644 --- a/components/engine/runconfig/hostconfig.go +++ b/components/engine/runconfig/hostconfig.go @@ -95,6 +95,7 @@ type HostConfig struct { CapAdd []string CapDrop []string RestartPolicy RestartPolicy + SecurityOpt []string } // This is used by the create command when you want to set both the @@ -130,6 +131,7 @@ func ContainerHostConfigFromJob(job *engine.Job) *HostConfig { job.GetenvJson("PortBindings", &hostConfig.PortBindings) job.GetenvJson("Devices", &hostConfig.Devices) job.GetenvJson("RestartPolicy", &hostConfig.RestartPolicy) + hostConfig.SecurityOpt = job.GetenvList("SecurityOpt") if Binds := job.GetenvList("Binds"); Binds != nil { hostConfig.Binds = Binds } diff --git a/components/engine/runconfig/parse.go b/components/engine/runconfig/parse.go index 2bd8cf969e..0d682f35d3 100644 --- a/components/engine/runconfig/parse.go +++ b/components/engine/runconfig/parse.go @@ -273,7 +273,6 @@ func Parse(cmd *flag.FlagSet, args []string) (*Config, *HostConfig, *flag.FlagSe MacAddress: *flMacAddress, Entrypoint: entrypoint, WorkingDir: *flWorkingDir, - SecurityOpt: flSecurityOpt.GetAll(), } hostConfig := &HostConfig{ @@ -294,6 +293,7 @@ func Parse(cmd *flag.FlagSet, args []string) (*Config, *HostConfig, *flag.FlagSe CapAdd: flCapAdd.GetAll(), CapDrop: flCapDrop.GetAll(), RestartPolicy: restartPolicy, + SecurityOpt: flSecurityOpt.GetAll(), } // When allocating stdin in attached mode, close stdin at client disconnect From 3527869dfc3f1e07da11e30e063ce7e5cba662b6 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 3 Nov 2014 23:00:49 +0000 Subject: [PATCH 03/13] Add AppArmorProfile to container inspect json Signed-off-by: Michael Crosby Upstream-commit: fa1484d12c5b66f7db03a9c93002ba3df56cdb4e Component: engine --- components/engine/daemon/inspect.go | 1 + 1 file changed, 1 insertion(+) diff --git a/components/engine/daemon/inspect.go b/components/engine/daemon/inspect.go index 396ca0227f..cf2ed644d0 100644 --- a/components/engine/daemon/inspect.go +++ b/components/engine/daemon/inspect.go @@ -47,6 +47,7 @@ func (daemon *Daemon) ContainerInspect(job *engine.Job) engine.Status { out.Set("ProcessLabel", container.ProcessLabel) out.SetJson("Volumes", container.Volumes) out.SetJson("VolumesRW", container.VolumesRW) + out.SetJson("AppArmorProfile", container.AppArmorProfile) if children, err := daemon.Children(container.Name); err == nil { for linkAlias, child := range children { From dd4a9e5429fc226f61074a1aed95de2c44470bd6 Mon Sep 17 00:00:00 2001 From: unclejack Date: Wed, 29 Oct 2014 21:06:51 +0200 Subject: [PATCH 04/13] add pkg/chrootarchive and use it on the daemon Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) Conflicts: builder/internals.go daemon/graphdriver/aufs/aufs.go daemon/volumes.go fixed conflicts in imports Upstream-commit: 1cb17f03d0b217acf2d2c289b4946d367f9d3e80 Component: engine --- components/engine/builder/internals.go | 11 ++- .../engine/daemon/graphdriver/aufs/aufs.go | 3 +- .../engine/daemon/graphdriver/fsdiff.go | 3 +- .../engine/daemon/graphdriver/vfs/driver.go | 4 +- components/engine/daemon/volumes.go | 4 +- .../engine/pkg/chrootarchive/archive.go | 76 +++++++++++++++++++ components/engine/pkg/chrootarchive/diff.go | 38 ++++++++++ components/engine/pkg/chrootarchive/init.go | 18 +++++ components/engine/pkg/reexec/command_linux.go | 18 +++++ .../engine/pkg/reexec/command_unsupported.go | 11 +++ components/engine/pkg/reexec/reexec.go | 3 - 11 files changed, 176 insertions(+), 13 deletions(-) create mode 100644 components/engine/pkg/chrootarchive/archive.go create mode 100644 components/engine/pkg/chrootarchive/diff.go create mode 100644 components/engine/pkg/chrootarchive/init.go create mode 100644 components/engine/pkg/reexec/command_linux.go create mode 100644 components/engine/pkg/reexec/command_unsupported.go diff --git a/components/engine/builder/internals.go b/components/engine/builder/internals.go index f6083e7918..a894dd0b6b 100644 --- a/components/engine/builder/internals.go +++ b/components/engine/builder/internals.go @@ -24,6 +24,7 @@ import ( "github.com/docker/docker/daemon" imagepkg "github.com/docker/docker/image" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/parsers" "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/pkg/system" @@ -46,7 +47,9 @@ func (b *Builder) readContext(context io.Reader) error { if b.context, err = tarsum.NewTarSum(decompressedStream, true, tarsum.Version0); err != nil { return err } - if err := archive.Untar(b.context, tmpdirPath, nil); err != nil { + + os.MkdirAll(tmpdirPath, 0700) + if err := chrootarchive.Untar(b.context, tmpdirPath, nil); err != nil { return err } @@ -627,7 +630,7 @@ func (b *Builder) addContext(container *daemon.Container, orig, dest string, dec } // try to successfully untar the orig - if err := archive.UntarPath(origPath, tarDest); err == nil { + if err := chrootarchive.UntarPath(origPath, tarDest); err == nil { return nil } else if err != io.EOF { log.Debugf("Couldn't untar %s to %s: %s", origPath, tarDest, err) @@ -637,7 +640,7 @@ func (b *Builder) addContext(container *daemon.Container, orig, dest string, dec if err := os.MkdirAll(path.Dir(destPath), 0755); err != nil { return err } - if err := archive.CopyWithTar(origPath, destPath); err != nil { + if err := chrootarchive.CopyWithTar(origPath, destPath); err != nil { return err } @@ -650,7 +653,7 @@ func (b *Builder) addContext(container *daemon.Container, orig, dest string, dec } func copyAsDirectory(source, destination string, destinationExists bool) error { - if err := archive.CopyWithTar(source, destination); err != nil { + if err := chrootarchive.CopyWithTar(source, destination); err != nil { return err } diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index da3c720d16..55cfd00c1f 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -33,6 +33,7 @@ import ( log "github.com/Sirupsen/logrus" "github.com/docker/docker/daemon/graphdriver" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" mountpk "github.com/docker/docker/pkg/mount" "github.com/docker/docker/utils" "github.com/docker/libcontainer/label" @@ -305,7 +306,7 @@ func (a *Driver) Diff(id, parent string) (archive.Archive, error) { } func (a *Driver) applyDiff(id string, diff archive.ArchiveReader) error { - return archive.Untar(diff, path.Join(a.rootPath(), "diff", id), nil) + return chrootarchive.Untar(diff, path.Join(a.rootPath(), "diff", id), nil) } // DiffSize calculates the changes between the specified id diff --git a/components/engine/daemon/graphdriver/fsdiff.go b/components/engine/daemon/graphdriver/fsdiff.go index 3569cf910e..48852a5631 100644 --- a/components/engine/daemon/graphdriver/fsdiff.go +++ b/components/engine/daemon/graphdriver/fsdiff.go @@ -8,6 +8,7 @@ import ( log "github.com/Sirupsen/logrus" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/utils" ) @@ -122,7 +123,7 @@ func (gdw *naiveDiffDriver) ApplyDiff(id, parent string, diff archive.ArchiveRea start := time.Now().UTC() log.Debugf("Start untar layer") - if err = archive.ApplyLayer(layerFs, diff); err != nil { + if err = chrootarchive.ApplyLayer(layerFs, diff); err != nil { return } log.Debugf("Untar time: %vs", time.Now().UTC().Sub(start).Seconds()) diff --git a/components/engine/daemon/graphdriver/vfs/driver.go b/components/engine/daemon/graphdriver/vfs/driver.go index 1076eb38dd..aa104500bc 100644 --- a/components/engine/daemon/graphdriver/vfs/driver.go +++ b/components/engine/daemon/graphdriver/vfs/driver.go @@ -8,7 +8,7 @@ import ( "path" "github.com/docker/docker/daemon/graphdriver" - "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/libcontainer/label" ) @@ -66,7 +66,7 @@ func (d *Driver) Create(id, parent string) error { if err != nil { return fmt.Errorf("%s: %s", parent, err) } - if err := archive.CopyWithTar(parentDir, dir); err != nil { + if err := chrootarchive.CopyWithTar(parentDir, dir); err != nil { return err } return nil diff --git a/components/engine/daemon/volumes.go b/components/engine/daemon/volumes.go index 6523dae853..a2cf3af33a 100644 --- a/components/engine/daemon/volumes.go +++ b/components/engine/daemon/volumes.go @@ -12,7 +12,7 @@ import ( log "github.com/Sirupsen/logrus" "github.com/docker/docker/daemon/execdriver" - "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/volumes" ) @@ -320,7 +320,7 @@ func copyExistingContents(source, destination string) error { if len(srcList) == 0 { // If the source volume is empty copy files from the root into the volume - if err := archive.CopyWithTar(source, destination); err != nil { + if err := chrootarchive.CopyWithTar(source, destination); err != nil { return err } } diff --git a/components/engine/pkg/chrootarchive/archive.go b/components/engine/pkg/chrootarchive/archive.go new file mode 100644 index 0000000000..f1df57ca59 --- /dev/null +++ b/components/engine/pkg/chrootarchive/archive.go @@ -0,0 +1,76 @@ +package chrootarchive + +import ( + "flag" + "fmt" + "io" + "os" + "runtime" + "syscall" + + "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/reexec" +) + +func untar() { + runtime.LockOSThread() + flag.Parse() + + if err := syscall.Chroot(flag.Arg(0)); err != nil { + fatal(err) + } + if err := syscall.Chdir("/"); err != nil { + fatal(err) + } + if err := archive.Untar(os.Stdin, "/", nil); err != nil { + fatal(err) + } + os.Exit(0) +} + +var ( + chrootArchiver = &archive.Archiver{Untar} +) + +func Untar(archive io.Reader, dest string, options *archive.TarOptions) error { + if _, err := os.Stat(dest); os.IsNotExist(err) { + if err := os.MkdirAll(dest, 0777); err != nil { + return err + } + } + cmd := reexec.Command("docker-untar", dest) + cmd.Stdin = archive + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("Untar %s %s", err, out) + } + return nil +} + +func TarUntar(src, dst string) error { + return chrootArchiver.TarUntar(src, dst) +} + +// CopyWithTar creates a tar archive of filesystem path `src`, and +// unpacks it at filesystem path `dst`. +// The archive is streamed directly with fixed buffering and no +// intermediary disk IO. +func CopyWithTar(src, dst string) error { + return chrootArchiver.CopyWithTar(src, dst) +} + +// CopyFileWithTar emulates the behavior of the 'cp' command-line +// for a single file. It copies a regular file from path `src` to +// path `dst`, and preserves all its metadata. +// +// If `dst` ends with a trailing slash '/', the final destination path +// will be `dst/base(src)`. +func CopyFileWithTar(src, dst string) (err error) { + return chrootArchiver.CopyFileWithTar(src, dst) +} + +// UntarPath is a convenience function which looks for an archive +// at filesystem path `src`, and unpacks it at `dst`. +func UntarPath(src, dst string) error { + return chrootArchiver.UntarPath(src, dst) +} diff --git a/components/engine/pkg/chrootarchive/diff.go b/components/engine/pkg/chrootarchive/diff.go new file mode 100644 index 0000000000..2133200c68 --- /dev/null +++ b/components/engine/pkg/chrootarchive/diff.go @@ -0,0 +1,38 @@ +package chrootarchive + +import ( + "flag" + "fmt" + "os" + "runtime" + "syscall" + + "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/reexec" +) + +func applyLayer() { + runtime.LockOSThread() + flag.Parse() + + if err := syscall.Chroot(flag.Arg(0)); err != nil { + fatal(err) + } + if err := syscall.Chdir("/"); err != nil { + fatal(err) + } + if err := archive.ApplyLayer("/", os.Stdin); err != nil { + fatal(err) + } + os.Exit(0) +} + +func ApplyLayer(dest string, layer archive.ArchiveReader) error { + cmd := reexec.Command("docker-applyLayer", dest) + cmd.Stdin = layer + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("ApplyLayer %s %s", err, out) + } + return nil +} diff --git a/components/engine/pkg/chrootarchive/init.go b/components/engine/pkg/chrootarchive/init.go new file mode 100644 index 0000000000..b548e9fe72 --- /dev/null +++ b/components/engine/pkg/chrootarchive/init.go @@ -0,0 +1,18 @@ +package chrootarchive + +import ( + "fmt" + "os" + + "github.com/docker/docker/pkg/reexec" +) + +func init() { + reexec.Register("docker-untar", untar) + reexec.Register("docker-applyLayer", applyLayer) +} + +func fatal(err error) { + fmt.Fprint(os.Stderr, err) + os.Exit(1) +} diff --git a/components/engine/pkg/reexec/command_linux.go b/components/engine/pkg/reexec/command_linux.go new file mode 100644 index 0000000000..8dc3f3a4a6 --- /dev/null +++ b/components/engine/pkg/reexec/command_linux.go @@ -0,0 +1,18 @@ +// +build linux + +package reexec + +import ( + "os/exec" + "syscall" +) + +func Command(args ...string) *exec.Cmd { + return &exec.Cmd{ + Path: Self(), + Args: args, + SysProcAttr: &syscall.SysProcAttr{ + Pdeathsig: syscall.SIGTERM, + }, + } +} diff --git a/components/engine/pkg/reexec/command_unsupported.go b/components/engine/pkg/reexec/command_unsupported.go new file mode 100644 index 0000000000..a579318e82 --- /dev/null +++ b/components/engine/pkg/reexec/command_unsupported.go @@ -0,0 +1,11 @@ +// +build !linux + +package reexec + +import ( + "os/exec" +) + +func Command(args ...string) *exec.Cmd { + return nil +} diff --git a/components/engine/pkg/reexec/reexec.go b/components/engine/pkg/reexec/reexec.go index 136b905bd2..774e71c76d 100644 --- a/components/engine/pkg/reexec/reexec.go +++ b/components/engine/pkg/reexec/reexec.go @@ -27,19 +27,16 @@ func Init() bool { return true } - return false } // Self returns the path to the current processes binary func Self() string { name := os.Args[0] - if filepath.Base(name) == name { if lp, err := exec.LookPath(name); err == nil { name = lp } } - return name } From 9d0c84c559bec8ed11d37b7204d473441e3bba15 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Sat, 8 Nov 2014 10:38:42 -0500 Subject: [PATCH 05/13] pkg/chrootarchive: pass TarOptions via CLI arg Signed-off-by: Tibor Vass Conflicts: graph/load.go fixed conflict in imports Upstream-commit: 9c01bc249dc628280f3fc019d5f0e0ace71be248 Component: engine --- components/engine/builder/internals.go | 1 - components/engine/graph/load.go | 3 +- .../engine/pkg/chrootarchive/archive.go | 18 ++++++++- .../engine/pkg/chrootarchive/archive_test.go | 39 +++++++++++++++++++ components/engine/pkg/chrootarchive/init.go | 1 + 5 files changed, 58 insertions(+), 4 deletions(-) create mode 100644 components/engine/pkg/chrootarchive/archive_test.go diff --git a/components/engine/builder/internals.go b/components/engine/builder/internals.go index a894dd0b6b..0a2432f144 100644 --- a/components/engine/builder/internals.go +++ b/components/engine/builder/internals.go @@ -48,7 +48,6 @@ func (b *Builder) readContext(context io.Reader) error { return err } - os.MkdirAll(tmpdirPath, 0700) if err := chrootarchive.Untar(b.context, tmpdirPath, nil); err != nil { return err } diff --git a/components/engine/graph/load.go b/components/engine/graph/load.go index 875741ecf7..18c83c07de 100644 --- a/components/engine/graph/load.go +++ b/components/engine/graph/load.go @@ -11,6 +11,7 @@ import ( "github.com/docker/docker/engine" "github.com/docker/docker/image" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" ) // Loads a set of images into the repository. This is the complementary of ImageExport. @@ -53,7 +54,7 @@ func (s *TagStore) CmdLoad(job *engine.Job) engine.Status { excludes[i] = k i++ } - if err := archive.Untar(repoFile, repoDir, &archive.TarOptions{Excludes: excludes}); err != nil { + if err := chrootarchive.Untar(repoFile, repoDir, &archive.TarOptions{Excludes: excludes}); err != nil { return job.Error(err) } diff --git a/components/engine/pkg/chrootarchive/archive.go b/components/engine/pkg/chrootarchive/archive.go index f1df57ca59..fc2bea2c40 100644 --- a/components/engine/pkg/chrootarchive/archive.go +++ b/components/engine/pkg/chrootarchive/archive.go @@ -1,11 +1,14 @@ package chrootarchive import ( + "bytes" + "encoding/json" "flag" "fmt" "io" "os" "runtime" + "strings" "syscall" "github.com/docker/docker/pkg/archive" @@ -22,7 +25,12 @@ func untar() { if err := syscall.Chdir("/"); err != nil { fatal(err) } - if err := archive.Untar(os.Stdin, "/", nil); err != nil { + options := new(archive.TarOptions) + dec := json.NewDecoder(strings.NewReader(flag.Arg(1))) + if err := dec.Decode(options); err != nil { + fatal(err) + } + if err := archive.Untar(os.Stdin, "/", options); err != nil { fatal(err) } os.Exit(0) @@ -33,12 +41,18 @@ var ( ) func Untar(archive io.Reader, dest string, options *archive.TarOptions) error { + var buf bytes.Buffer + enc := json.NewEncoder(&buf) + if err := enc.Encode(options); err != nil { + return fmt.Errorf("Untar json encode: %v", err) + } if _, err := os.Stat(dest); os.IsNotExist(err) { if err := os.MkdirAll(dest, 0777); err != nil { return err } } - cmd := reexec.Command("docker-untar", dest) + + cmd := reexec.Command("docker-untar", dest, buf.String()) cmd.Stdin = archive out, err := cmd.CombinedOutput() if err != nil { diff --git a/components/engine/pkg/chrootarchive/archive_test.go b/components/engine/pkg/chrootarchive/archive_test.go new file mode 100644 index 0000000000..aeac448743 --- /dev/null +++ b/components/engine/pkg/chrootarchive/archive_test.go @@ -0,0 +1,39 @@ +package chrootarchive + +import ( + "io/ioutil" + "os" + "path/filepath" + "testing" + + "github.com/docker/docker/pkg/archive" +) + +func TestChrootTarUntar(t *testing.T) { + tmpdir, err := ioutil.TempDir("", "docker-TestChrootTarUntar") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpdir) + src := filepath.Join(tmpdir, "src") + if err := os.MkdirAll(src, 0700); err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(filepath.Join(src, "toto"), []byte("hello toto"), 0644); err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(filepath.Join(src, "lolo"), []byte("hello lolo"), 0644); err != nil { + t.Fatal(err) + } + stream, err := archive.Tar(src, archive.Uncompressed) + if err != nil { + t.Fatal(err) + } + dest := filepath.Join(tmpdir, "src") + if err := os.MkdirAll(dest, 0700); err != nil { + t.Fatal(err) + } + if err := Untar(stream, dest, &archive.TarOptions{Excludes: []string{"lolo"}}); err != nil { + t.Fatal(err) + } +} diff --git a/components/engine/pkg/chrootarchive/init.go b/components/engine/pkg/chrootarchive/init.go index b548e9fe72..f05698f65b 100644 --- a/components/engine/pkg/chrootarchive/init.go +++ b/components/engine/pkg/chrootarchive/init.go @@ -10,6 +10,7 @@ import ( func init() { reexec.Register("docker-untar", untar) reexec.Register("docker-applyLayer", applyLayer) + reexec.Init() } func fatal(err error) { From 8fc694c8198da619878bc89be1d3faf6ad89303e Mon Sep 17 00:00:00 2001 From: unclejack Date: Tue, 11 Nov 2014 13:02:14 +0200 Subject: [PATCH 06/13] don't call reexec.Init from chrootarchive Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) Conflicts: daemon/graphdriver/aufs/aufs_test.go fixed conflict caused by imports Upstream-commit: 209deff9633b82198925846ebcb0a02191553005 Component: engine --- components/engine/daemon/graphdriver/aufs/aufs_test.go | 5 +++++ components/engine/daemon/graphdriver/vfs/vfs_test.go | 9 ++++++++- components/engine/graph/pools_test.go | 10 +++++++++- components/engine/pkg/chrootarchive/archive_test.go | 5 +++++ components/engine/pkg/chrootarchive/init.go | 1 - 5 files changed, 27 insertions(+), 3 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs_test.go b/components/engine/daemon/graphdriver/aufs/aufs_test.go index e1ed64985f..c17a5dcce6 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs_test.go +++ b/components/engine/daemon/graphdriver/aufs/aufs_test.go @@ -11,6 +11,7 @@ import ( "github.com/docker/docker/daemon/graphdriver" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/reexec" ) var ( @@ -18,6 +19,10 @@ var ( tmp = path.Join(tmpOuter, "aufs") ) +func init() { + reexec.Init() +} + func testInit(dir string, t *testing.T) graphdriver.Driver { d, err := Init(dir, nil) if err != nil { diff --git a/components/engine/daemon/graphdriver/vfs/vfs_test.go b/components/engine/daemon/graphdriver/vfs/vfs_test.go index eaf70f59d3..1ee6ae4a90 100644 --- a/components/engine/daemon/graphdriver/vfs/vfs_test.go +++ b/components/engine/daemon/graphdriver/vfs/vfs_test.go @@ -1,10 +1,17 @@ package vfs import ( - "github.com/docker/docker/daemon/graphdriver/graphtest" "testing" + + "github.com/docker/docker/daemon/graphdriver/graphtest" + + "github.com/docker/docker/pkg/reexec" ) +func init() { + reexec.Init() +} + // This avoids creating a new driver for each test if all tests are run // Make sure to put new tests between TestVfsSetup and TestVfsTeardown func TestVfsSetup(t *testing.T) { diff --git a/components/engine/graph/pools_test.go b/components/engine/graph/pools_test.go index 785a4bd122..129a5e1fec 100644 --- a/components/engine/graph/pools_test.go +++ b/components/engine/graph/pools_test.go @@ -1,6 +1,14 @@ package graph -import "testing" +import ( + "testing" + + "github.com/docker/docker/pkg/reexec" +) + +func init() { + reexec.Init() +} func TestPools(t *testing.T) { s := &TagStore{ diff --git a/components/engine/pkg/chrootarchive/archive_test.go b/components/engine/pkg/chrootarchive/archive_test.go index aeac448743..69e18e3199 100644 --- a/components/engine/pkg/chrootarchive/archive_test.go +++ b/components/engine/pkg/chrootarchive/archive_test.go @@ -7,8 +7,13 @@ import ( "testing" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/reexec" ) +func init() { + reexec.Init() +} + func TestChrootTarUntar(t *testing.T) { tmpdir, err := ioutil.TempDir("", "docker-TestChrootTarUntar") if err != nil { diff --git a/components/engine/pkg/chrootarchive/init.go b/components/engine/pkg/chrootarchive/init.go index f05698f65b..b548e9fe72 100644 --- a/components/engine/pkg/chrootarchive/init.go +++ b/components/engine/pkg/chrootarchive/init.go @@ -10,7 +10,6 @@ import ( func init() { reexec.Register("docker-untar", untar) reexec.Register("docker-applyLayer", applyLayer) - reexec.Init() } func fatal(err error) { From 588421ad7ae0bd3a6321e39ce3da854b69ffab71 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Mon, 20 Oct 2014 15:35:48 -0400 Subject: [PATCH 07/13] archive: add breakout tests Signed-off-by: Tibor Vass Conflicts: pkg/archive/archive.go fixed conflict which git couldn't fix with the added BreakoutError Conflicts: pkg/archive/archive_test.go fixed conflict in imports Upstream-commit: 221617dbcd9431f14a3779d8bac9aba52f78ea21 Component: engine --- components/engine/pkg/archive/archive.go | 5 + components/engine/pkg/archive/archive_test.go | 192 +++++++++++++++++- components/engine/pkg/archive/diff_test.go | 191 +++++++++++++++++ components/engine/pkg/archive/utils_test.go | 166 +++++++++++++++ 4 files changed, 553 insertions(+), 1 deletion(-) create mode 100644 components/engine/pkg/archive/diff_test.go create mode 100644 components/engine/pkg/archive/utils_test.go diff --git a/components/engine/pkg/archive/archive.go b/components/engine/pkg/archive/archive.go index 995668104d..d90dfcffcf 100644 --- a/components/engine/pkg/archive/archive.go +++ b/components/engine/pkg/archive/archive.go @@ -42,6 +42,11 @@ type ( Archiver struct { Untar func(io.Reader, string, *TarOptions) error } + + // breakoutError is used to differentiate errors related to breaking out + // When testing archive breakout in the unit tests, this error is expected + // in order for the test to pass. + breakoutError error ) var ( diff --git a/components/engine/pkg/archive/archive_test.go b/components/engine/pkg/archive/archive_test.go index 3516aca8f0..36abdb958b 100644 --- a/components/engine/pkg/archive/archive_test.go +++ b/components/engine/pkg/archive/archive_test.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path" + "path/filepath" "syscall" "testing" "time" @@ -214,7 +215,12 @@ func TestTarWithOptions(t *testing.T) { // Failing prevents the archives from being uncompressed during ADD func TestTypeXGlobalHeaderDoesNotFail(t *testing.T) { hdr := tar.Header{Typeflag: tar.TypeXGlobalHeader} - err := createTarFile("pax_global_header", "some_dir", &hdr, nil, true) + tmpDir, err := ioutil.TempDir("", "docker-test-archive-pax-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + err = createTarFile(filepath.Join(tmpDir, "pax_global_header"), tmpDir, &hdr, nil, true) if err != nil { t.Fatal(err) } @@ -403,3 +409,187 @@ func BenchmarkTarUntarWithLinks(b *testing.B) { os.RemoveAll(target) } } + +func TestUntarInvalidFilenames(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { + { + Name: "../victim/dotdot", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { + { + // Note the leading slash + Name: "/../victim/slash-dotdot", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("untar", "docker-TestUntarInvalidFilenames", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} + +func TestUntarInvalidHardlink(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { // try reading victim/hello (../) + { + Name: "dotdot", + Typeflag: tar.TypeLink, + Linkname: "../victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (/../) + { + Name: "slash-dotdot", + Typeflag: tar.TypeLink, + // Note the leading slash + Linkname: "/../victim/hello", + Mode: 0644, + }, + }, + { // try writing victim/file + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim/file", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { // try reading victim/hello (hardlink, symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "symlink", + Typeflag: tar.TypeSymlink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // Try reading victim/hello (hardlink, hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "hardlink", + Typeflag: tar.TypeLink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // Try removing victim directory (hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("untar", "docker-TestUntarInvalidHardlink", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} + +func TestUntarInvalidSymlink(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { // try reading victim/hello (../) + { + Name: "dotdot", + Typeflag: tar.TypeSymlink, + Linkname: "../victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (/../) + { + Name: "slash-dotdot", + Typeflag: tar.TypeSymlink, + // Note the leading slash + Linkname: "/../victim/hello", + Mode: 0644, + }, + }, + { // try writing victim/file + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim/file", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { // try reading victim/hello (symlink, symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "symlink", + Typeflag: tar.TypeSymlink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (symlink, hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "hardlink", + Typeflag: tar.TypeLink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // try removing victim directory (symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("untar", "docker-TestUntarInvalidSymlink", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} diff --git a/components/engine/pkg/archive/diff_test.go b/components/engine/pkg/archive/diff_test.go new file mode 100644 index 0000000000..758c4115d5 --- /dev/null +++ b/components/engine/pkg/archive/diff_test.go @@ -0,0 +1,191 @@ +package archive + +import ( + "testing" + + "github.com/docker/docker/vendor/src/code.google.com/p/go/src/pkg/archive/tar" +) + +func TestApplyLayerInvalidFilenames(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { + { + Name: "../victim/dotdot", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { + { + // Note the leading slash + Name: "/../victim/slash-dotdot", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("applylayer", "docker-TestApplyLayerInvalidFilenames", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} + +func TestApplyLayerInvalidHardlink(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { // try reading victim/hello (../) + { + Name: "dotdot", + Typeflag: tar.TypeLink, + Linkname: "../victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (/../) + { + Name: "slash-dotdot", + Typeflag: tar.TypeLink, + // Note the leading slash + Linkname: "/../victim/hello", + Mode: 0644, + }, + }, + { // try writing victim/file + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim/file", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { // try reading victim/hello (hardlink, symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "symlink", + Typeflag: tar.TypeSymlink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // Try reading victim/hello (hardlink, hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "hardlink", + Typeflag: tar.TypeLink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // Try removing victim directory (hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("applylayer", "docker-TestApplyLayerInvalidHardlink", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} + +func TestApplyLayerInvalidSymlink(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { // try reading victim/hello (../) + { + Name: "dotdot", + Typeflag: tar.TypeSymlink, + Linkname: "../victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (/../) + { + Name: "slash-dotdot", + Typeflag: tar.TypeSymlink, + // Note the leading slash + Linkname: "/../victim/hello", + Mode: 0644, + }, + }, + { // try writing victim/file + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim/file", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { // try reading victim/hello (symlink, symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "symlink", + Typeflag: tar.TypeSymlink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (symlink, hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "hardlink", + Typeflag: tar.TypeLink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // try removing victim directory (symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("applylayer", "docker-TestApplyLayerInvalidSymlink", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} diff --git a/components/engine/pkg/archive/utils_test.go b/components/engine/pkg/archive/utils_test.go new file mode 100644 index 0000000000..3624fe5afa --- /dev/null +++ b/components/engine/pkg/archive/utils_test.go @@ -0,0 +1,166 @@ +package archive + +import ( + "bytes" + "fmt" + "io" + "io/ioutil" + "os" + "path/filepath" + "time" + + "github.com/docker/docker/vendor/src/code.google.com/p/go/src/pkg/archive/tar" +) + +var testUntarFns = map[string]func(string, io.Reader) error{ + "untar": func(dest string, r io.Reader) error { + return Untar(r, dest, nil) + }, + "applylayer": func(dest string, r io.Reader) error { + return ApplyLayer(dest, ArchiveReader(r)) + }, +} + +// testBreakout is a helper function that, within the provided `tmpdir` directory, +// creates a `victim` folder with a generated `hello` file in it. +// `untar` extracts to a directory named `dest`, the tar file created from `headers`. +// +// Here are the tested scenarios: +// - removed `victim` folder (write) +// - removed files from `victim` folder (write) +// - new files in `victim` folder (write) +// - modified files in `victim` folder (write) +// - file in `dest` with same content as `victim/hello` (read) +// +// When using testBreakout make sure you cover one of the scenarios listed above. +func testBreakout(untarFn string, tmpdir string, headers []*tar.Header) error { + tmpdir, err := ioutil.TempDir("", tmpdir) + if err != nil { + return err + } + defer os.RemoveAll(tmpdir) + + dest := filepath.Join(tmpdir, "dest") + if err := os.Mkdir(dest, 0755); err != nil { + return err + } + + victim := filepath.Join(tmpdir, "victim") + if err := os.Mkdir(victim, 0755); err != nil { + return err + } + hello := filepath.Join(victim, "hello") + helloData, err := time.Now().MarshalText() + if err != nil { + return err + } + if err := ioutil.WriteFile(hello, helloData, 0644); err != nil { + return err + } + helloStat, err := os.Stat(hello) + if err != nil { + return err + } + + reader, writer := io.Pipe() + go func() { + t := tar.NewWriter(writer) + for _, hdr := range headers { + t.WriteHeader(hdr) + } + t.Close() + }() + + untar := testUntarFns[untarFn] + if untar == nil { + return fmt.Errorf("could not find untar function %q in testUntarFns", untarFn) + } + if err := untar(dest, reader); err != nil { + if _, ok := err.(breakoutError); !ok { + // If untar returns an error unrelated to an archive breakout, + // then consider this an unexpected error and abort. + return err + } + // Here, untar detected the breakout. + // Let's move on verifying that indeed there was no breakout. + fmt.Printf("breakoutError: %v\n", err) + } + + // Check victim folder + f, err := os.Open(victim) + if err != nil { + // codepath taken if victim folder was removed + return fmt.Errorf("archive breakout: error reading %q: %v", victim, err) + } + defer f.Close() + + // Check contents of victim folder + // + // We are only interested in getting 2 files from the victim folder, because if all is well + // we expect only one result, the `hello` file. If there is a second result, it cannot + // hold the same name `hello` and we assume that a new file got created in the victim folder. + // That is enough to detect an archive breakout. + names, err := f.Readdirnames(2) + if err != nil { + // codepath taken if victim is not a folder + return fmt.Errorf("archive breakout: error reading directory content of %q: %v", victim, err) + } + for _, name := range names { + if name != "hello" { + // codepath taken if new file was created in victim folder + return fmt.Errorf("archive breakout: new file %q", name) + } + } + + // Check victim/hello + f, err = os.Open(hello) + if err != nil { + // codepath taken if read permissions were removed + return fmt.Errorf("archive breakout: could not lstat %q: %v", hello, err) + } + defer f.Close() + b, err := ioutil.ReadAll(f) + if err != nil { + return err + } + fi, err := f.Stat() + if err != nil { + return err + } + if helloStat.IsDir() != fi.IsDir() || + // TODO: cannot check for fi.ModTime() change + helloStat.Mode() != fi.Mode() || + helloStat.Size() != fi.Size() || + !bytes.Equal(helloData, b) { + // codepath taken if hello has been modified + return fmt.Errorf("archive breakout: file %q has been modified. Contents: expected=%q, got=%q. FileInfo: expected=%#v, got=%#v.", hello, helloData, b, helloStat, fi) + } + + // Check that nothing in dest/ has the same content as victim/hello. + // Since victim/hello was generated with time.Now(), it is safe to assume + // that any file whose content matches exactly victim/hello, managed somehow + // to access victim/hello. + return filepath.Walk(dest, func(path string, info os.FileInfo, err error) error { + if info.IsDir() { + if err != nil { + // skip directory if error + return filepath.SkipDir + } + // enter directory + return nil + } + if err != nil { + // skip file if error + return nil + } + b, err := ioutil.ReadFile(path) + if err != nil { + // Houston, we have a problem. Aborting (space)walk. + return err + } + if bytes.Equal(helloData, b) { + return fmt.Errorf("archive breakout: file %q has been accessed via %q", hello, path) + } + return nil + }) +} From 89c9d8d1a23e79268a8ea9141c6830066e8e123f Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Mon, 20 Oct 2014 15:36:28 -0400 Subject: [PATCH 08/13] archive: prevent breakout in Untar Signed-off-by: Tibor Vass Upstream-commit: 1852cc38415c3d63d18c2938af9c112fbc4dfc10 Component: engine --- components/engine/pkg/archive/archive.go | 22 +++++++++++++++++++++- components/engine/pkg/symlink/fs.go | 4 +++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/components/engine/pkg/archive/archive.go b/components/engine/pkg/archive/archive.go index d90dfcffcf..67eb0be8ad 100644 --- a/components/engine/pkg/archive/archive.go +++ b/components/engine/pkg/archive/archive.go @@ -22,6 +22,7 @@ import ( "github.com/docker/docker/pkg/fileutils" "github.com/docker/docker/pkg/pools" "github.com/docker/docker/pkg/promise" + "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/pkg/system" ) @@ -292,11 +293,23 @@ func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, L } case tar.TypeLink: - if err := os.Link(filepath.Join(extractDir, hdr.Linkname), path); err != nil { + targetPath := filepath.Join(extractDir, hdr.Linkname) + // check for hardlink breakout + if !strings.HasPrefix(targetPath, extractDir) { + return breakoutError(fmt.Errorf("invalid hardlink %q -> %q", targetPath, hdr.Linkname)) + } + if err := os.Link(targetPath, path); err != nil { return err } case tar.TypeSymlink: + // check for symlink breakout + if _, err := symlink.FollowSymlinkInScope(filepath.Join(filepath.Dir(path), hdr.Linkname), extractDir); err != nil { + if _, ok := err.(symlink.ErrBreakout); ok { + return breakoutError(fmt.Errorf("invalid symlink %q -> %q", path, hdr.Linkname)) + } + return err + } if err := os.Symlink(hdr.Linkname, path); err != nil { return err } @@ -456,6 +469,8 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) // identity (uncompressed), gzip, bzip2, xz. // FIXME: specify behavior when target path exists vs. doesn't exist. func Untar(archive io.Reader, dest string, options *TarOptions) error { + dest = filepath.Clean(dest) + if options == nil { options = &TarOptions{} } @@ -493,6 +508,7 @@ loop: } // Normalize name, for safety and for a simple is-root check + // This keeps "../" as-is, but normalizes "/../" to "/" hdr.Name = filepath.Clean(hdr.Name) for _, exclude := range options.Excludes { @@ -513,7 +529,11 @@ loop: } } + // Prevent symlink breakout path := filepath.Join(dest, hdr.Name) + if !strings.HasPrefix(path, dest) { + return breakoutError(fmt.Errorf("%q is outside of %q", path, dest)) + } // If path exits we almost always just want to remove and replace it // The only exception is when it is a directory *and* the file from diff --git a/components/engine/pkg/symlink/fs.go b/components/engine/pkg/symlink/fs.go index 6ce99c6bda..09271ffc4b 100644 --- a/components/engine/pkg/symlink/fs.go +++ b/components/engine/pkg/symlink/fs.go @@ -10,6 +10,8 @@ import ( const maxLoopCounter = 100 +type ErrBreakout error + // FollowSymlink will follow an existing link and scope it to the root // path provided. // The role of this function is to return an absolute path in the root @@ -34,7 +36,7 @@ func FollowSymlinkInScope(link, root string) (string, error) { } if !strings.HasPrefix(filepath.Dir(link), root) { - return "", fmt.Errorf("%s is not within %s", link, root) + return "", ErrBreakout(fmt.Errorf("%s is not within %s", link, root)) } prev := "/" From bc5bfe8ae5f7074bf2483afe43ccb7f635858781 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Fri, 31 Oct 2014 13:18:39 -0400 Subject: [PATCH 09/13] archive: prevent breakout in ApplyLayer Signed-off-by: Tibor Vass Upstream-commit: 31d1d733037b22591e2dd2edfe6c4d2d4b8086cc Component: engine --- components/engine/pkg/archive/diff.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/components/engine/pkg/archive/diff.go b/components/engine/pkg/archive/diff.go index eabb7c48ff..856cedcead 100644 --- a/components/engine/pkg/archive/diff.go +++ b/components/engine/pkg/archive/diff.go @@ -18,6 +18,8 @@ import ( // ApplyLayer parses a diff in the standard layer format from `layer`, and // applies it to the directory `dest`. func ApplyLayer(dest string, layer ArchiveReader) error { + dest = filepath.Clean(dest) + // We need to be able to set any perms oldmask, err := system.Umask(0) if err != nil { @@ -91,6 +93,12 @@ func ApplyLayer(dest string, layer ArchiveReader) error { path := filepath.Join(dest, hdr.Name) base := filepath.Base(path) + + // Prevent symlink breakout + if !strings.HasPrefix(path, dest) { + return breakoutError(fmt.Errorf("%q is outside of %q", path, dest)) + } + if strings.HasPrefix(base, ".wh.") { originalBase := base[len(".wh."):] originalPath := filepath.Join(filepath.Dir(path), originalBase) From 05d1551428e9f23a8888502ca6e08e9d44809164 Mon Sep 17 00:00:00 2001 From: unclejack Date: Tue, 18 Nov 2014 23:33:13 +0200 Subject: [PATCH 10/13] pkg/chrootarchive: provide TMPDIR for ApplyLayer Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) Upstream-commit: 330171e1d9ec537d7f691fd63c697a0540589053 Component: engine --- components/engine/pkg/chrootarchive/diff.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/components/engine/pkg/chrootarchive/diff.go b/components/engine/pkg/chrootarchive/diff.go index 2133200c68..2653aefe9d 100644 --- a/components/engine/pkg/chrootarchive/diff.go +++ b/components/engine/pkg/chrootarchive/diff.go @@ -3,6 +3,7 @@ package chrootarchive import ( "flag" "fmt" + "io/ioutil" "os" "runtime" "syscall" @@ -21,9 +22,16 @@ func applyLayer() { if err := syscall.Chdir("/"); err != nil { fatal(err) } - if err := archive.ApplyLayer("/", os.Stdin); err != nil { + tmpDir, err := ioutil.TempDir("/", "temp-docker-extract") + if err != nil { fatal(err) } + os.Setenv("TMPDIR", tmpDir) + if err := archive.ApplyLayer("/", os.Stdin); err != nil { + os.RemoveAll(tmpDir) + fatal(err) + } + os.RemoveAll(tmpDir) os.Exit(0) } From 0a5e96f3d3e545b940594dd1f3c87cd7e000f2bb Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Wed, 19 Nov 2014 11:27:34 -0500 Subject: [PATCH 11/13] archive: do not call FollowSymlinkInScope in createTarFile Signed-off-by: Tibor Vass Upstream-commit: f6d9780229bfa52c86762d49a7a7e644dcd8f6df Component: engine --- components/engine/pkg/archive/archive.go | 15 ++++++++------- components/engine/pkg/archive/archive_test.go | 14 ++++++++++++++ components/engine/pkg/symlink/fs.go | 4 +--- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/components/engine/pkg/archive/archive.go b/components/engine/pkg/archive/archive.go index 67eb0be8ad..aaeed31981 100644 --- a/components/engine/pkg/archive/archive.go +++ b/components/engine/pkg/archive/archive.go @@ -22,7 +22,6 @@ import ( "github.com/docker/docker/pkg/fileutils" "github.com/docker/docker/pkg/pools" "github.com/docker/docker/pkg/promise" - "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/pkg/system" ) @@ -303,12 +302,14 @@ func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, L } case tar.TypeSymlink: - // check for symlink breakout - if _, err := symlink.FollowSymlinkInScope(filepath.Join(filepath.Dir(path), hdr.Linkname), extractDir); err != nil { - if _, ok := err.(symlink.ErrBreakout); ok { - return breakoutError(fmt.Errorf("invalid symlink %q -> %q", path, hdr.Linkname)) - } - return err + // path -> hdr.Linkname = targetPath + // e.g. /extractDir/path/to/symlink -> ../2/file = /extractDir/path/2/file + targetPath := filepath.Join(filepath.Dir(path), hdr.Linkname) + + // the reason we don't need to check symlinks in the path (with FollowSymlinkInScope) is because + // that symlink would first have to be created, which would be caught earlier, at this very check: + if !strings.HasPrefix(targetPath, extractDir) { + return breakoutError(fmt.Errorf("invalid symlink %q -> %q", path, hdr.Linkname)) } if err := os.Symlink(hdr.Linkname, path); err != nil { return err diff --git a/components/engine/pkg/archive/archive_test.go b/components/engine/pkg/archive/archive_test.go index 36abdb958b..05362a21c9 100644 --- a/components/engine/pkg/archive/archive_test.go +++ b/components/engine/pkg/archive/archive_test.go @@ -587,6 +587,20 @@ func TestUntarInvalidSymlink(t *testing.T) { Mode: 0644, }, }, + { // try writing to victim/newdir/newfile with a symlink in the path + { + // this header needs to be before the next one, or else there is an error + Name: "dir/loophole", + Typeflag: tar.TypeSymlink, + Linkname: "../../victim", + Mode: 0755, + }, + { + Name: "dir/loophole/newdir/newfile", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, } { if err := testBreakout("untar", "docker-TestUntarInvalidSymlink", headers); err != nil { t.Fatalf("i=%d. %v", i, err) diff --git a/components/engine/pkg/symlink/fs.go b/components/engine/pkg/symlink/fs.go index 09271ffc4b..6ce99c6bda 100644 --- a/components/engine/pkg/symlink/fs.go +++ b/components/engine/pkg/symlink/fs.go @@ -10,8 +10,6 @@ import ( const maxLoopCounter = 100 -type ErrBreakout error - // FollowSymlink will follow an existing link and scope it to the root // path provided. // The role of this function is to return an absolute path in the root @@ -36,7 +34,7 @@ func FollowSymlinkInScope(link, root string) (string, error) { } if !strings.HasPrefix(filepath.Dir(link), root) { - return "", ErrBreakout(fmt.Errorf("%s is not within %s", link, root)) + return "", fmt.Errorf("%s is not within %s", link, root) } prev := "/" From e2ca84e5a6c5ad602003e62d4229030be30a3a5f Mon Sep 17 00:00:00 2001 From: unclejack Date: Tue, 25 Nov 2014 01:28:20 +0200 Subject: [PATCH 12/13] graph/load: add build tags to fix make cross Signed-off-by: Cristian Staretu Upstream-commit: 2ec2237909ba51d3fe10a2ee6cfb81f315408f68 Component: engine --- components/engine/graph/load.go | 2 ++ components/engine/graph/load_unsupported.go | 11 +++++++++++ 2 files changed, 13 insertions(+) create mode 100644 components/engine/graph/load_unsupported.go diff --git a/components/engine/graph/load.go b/components/engine/graph/load.go index 18c83c07de..76172d2555 100644 --- a/components/engine/graph/load.go +++ b/components/engine/graph/load.go @@ -1,3 +1,5 @@ +// +build linux + package graph import ( diff --git a/components/engine/graph/load_unsupported.go b/components/engine/graph/load_unsupported.go new file mode 100644 index 0000000000..164e9176a1 --- /dev/null +++ b/components/engine/graph/load_unsupported.go @@ -0,0 +1,11 @@ +// +build !linux + +package graph + +import ( + "github.com/docker/docker/engine" +) + +func (s *TagStore) CmdLoad(job *engine.Job) engine.Status { + return job.Errorf("CmdLoad is not supported on this platform") +} From f5cd7e99486d64eadaa0012f48203e60001418b5 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Fri, 14 Nov 2014 14:47:37 -0800 Subject: [PATCH 13/13] Add v1.3.2 changelog & bump version to 1.3.2-dev Signed-off-by: Tibor Vass Signed-off-by: Cristian Staretu Upstream-commit: b21e1d4a00f2687ef24aa47039ac2f0281294365 Component: engine --- components/engine/CHANGELOG.md | 16 ++++++++++++++++ components/engine/VERSION | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/components/engine/CHANGELOG.md b/components/engine/CHANGELOG.md index b9d4370517..2d8f5cce8c 100644 --- a/components/engine/CHANGELOG.md +++ b/components/engine/CHANGELOG.md @@ -1,5 +1,21 @@ # Changelog +## 1.3.2 (2014-11-20) + +#### Security +- Fix tar breakout vulnerability +* Extractions are now sandboxed chroot +- Security options are no longer committed to images + +#### Runtime +- Fix deadlock in `docker ps -f exited=1` +- Fix a bug when `--volumes-from` references a container that failed to start + +#### Registry ++ `--insecure-registry` now accepts CIDR notation such as 10.1.0.0/16 +* Private registries whose IPs fall in the 127.0.0.0/8 range do no need the `--insecure-registry` flag +- Skip the experimental registry v2 API when mirroring is enabled + ## 1.3.1 (2014-10-28) #### Security diff --git a/components/engine/VERSION b/components/engine/VERSION index 625610ece8..259bb263c9 100644 --- a/components/engine/VERSION +++ b/components/engine/VERSION @@ -1 +1 @@ -1.3.1-dev +1.3.2-dev