From 7b7d54242c2aa0846766f2063e3bd4fe72999a3b Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 28 May 2019 12:43:09 -0400 Subject: [PATCH 1/9] Use securejoin to merge paths in `podman cp` Securejoin ensures that paths are resolved in the container, not on the host. Fixes #3211 Signed-off-by: Matthew Heon --- cmd/podman/cp.go | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/cmd/podman/cp.go b/cmd/podman/cp.go index 8240cc1930..5addf88d3d 100644 --- a/cmd/podman/cp.go +++ b/cmd/podman/cp.go @@ -17,6 +17,7 @@ import ( "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/chrootarchive" "github.com/containers/storage/pkg/idtools" + securejoin "github.com/cyphar/filepath-securejoin" digest "github.com/opencontainers/go-digest" specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" @@ -112,19 +113,38 @@ func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest strin var glob []string if isFromHostToCtr { if filepath.IsAbs(destPath) { - destPath = filepath.Join(mountPoint, destPath) - + cleanedPath, err := securejoin.SecureJoin(mountPoint, destPath) + if err != nil { + return err + } + destPath = cleanedPath } else { - if err = idtools.MkdirAllAndChownNew(filepath.Join(mountPoint, ctr.WorkingDir()), 0755, hostOwner); err != nil { + ctrWorkDir, err := securejoin.SecureJoin(mountPoint, ctr.WorkingDir()) + if err != nil { + return err + } + if err = idtools.MkdirAllAndChownNew(ctrWorkDir, 0755, hostOwner); err != nil { return errors.Wrapf(err, "error creating directory %q", destPath) } - destPath = filepath.Join(mountPoint, ctr.WorkingDir(), destPath) + cleanedPath, err := securejoin.SecureJoin(mountPoint, filepath.Join(ctr.WorkingDir(), destPath)) + if err != nil { + return err + } + destPath = cleanedPath } } else { if filepath.IsAbs(srcPath) { - srcPath = filepath.Join(mountPoint, srcPath) + cleanedPath, err := securejoin.SecureJoin(mountPoint, srcPath) + if err != nil { + return err + } + srcPath = cleanedPath } else { - srcPath = filepath.Join(mountPoint, ctr.WorkingDir(), srcPath) + cleanedPath, err := securejoin.SecureJoin(mountPoint, filepath.Join(ctr.WorkingDir(), srcPath)) + if err != nil { + return err + } + srcPath = cleanedPath } } glob, err = filepath.Glob(srcPath) From 49dc18552a13ee76dc012c35ff073ed07aaeb05b Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 28 May 2019 13:11:55 -0400 Subject: [PATCH 2/9] Pause containers while copying into them Should fix CVE-2018-15664 for Podman. Signed-off-by: Matthew Heon --- cmd/podman/cliconfig/create.go | 1 + cmd/podman/cp.go | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/cmd/podman/cliconfig/create.go b/cmd/podman/cliconfig/create.go index 49ab3d8279..5fb2eed10f 100644 --- a/cmd/podman/cliconfig/create.go +++ b/cmd/podman/cliconfig/create.go @@ -24,4 +24,5 @@ type BuildValues struct { type CpValues struct { PodmanCommand Extract bool + Pause bool } diff --git a/cmd/podman/cp.go b/cmd/podman/cp.go index 5addf88d3d..7092da5e74 100644 --- a/cmd/podman/cp.go +++ b/cmd/podman/cp.go @@ -50,6 +50,7 @@ func init() { cpCommand.Command = _cpCommand flags := cpCommand.Flags() flags.BoolVar(&cpCommand.Extract, "extract", false, "Extract the tar file into the destination directory.") + flags.BoolVar(&cpCommand.Pause, "pause", true, "Pause the container while copying") cpCommand.SetHelpTemplate(HelpTemplate()) cpCommand.SetUsageTemplate(UsageTemplate()) rootCmd.AddCommand(cpCommand.Command) @@ -67,11 +68,10 @@ func cpCmd(c *cliconfig.CpValues) error { } defer runtime.Shutdown(false) - extract := c.Flag("extract").Changed - return copyBetweenHostAndContainer(runtime, args[0], args[1], extract) + return copyBetweenHostAndContainer(runtime, args[0], args[1], c.Extract, c.Pause) } -func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest string, extract bool) error { +func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest string, extract bool, pause bool) error { srcCtr, srcPath := parsePath(runtime, src) destCtr, destPath := parsePath(runtime, dest) @@ -94,6 +94,18 @@ func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest strin return err } defer ctr.Unmount(false) + + if pause { + if err := ctr.Pause(); err != nil { + return err + } + defer func() { + if err := ctr.Unpause(); err != nil { + logrus.Errorf("Error unpausing container after copying: %v", err) + } + }() + } + user, err := getUser(mountPoint, ctr.User()) if err != nil { return err From 32fc6b906cf6641ecdb64be04e15b0abf93ecde4 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 28 May 2019 13:22:15 -0400 Subject: [PATCH 3/9] Add --pause to podman cp manpage and bash completions Signed-off-by: Matthew Heon --- completions/bash/podman | 1 + docs/podman-cp.1.md | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/completions/bash/podman b/completions/bash/podman index 98038d19cd..49c8c0e52c 100644 --- a/completions/bash/podman +++ b/completions/bash/podman @@ -2388,6 +2388,7 @@ _podman_cp() { --help -h --extract + --pause " _complete_ "$boolean_options" } diff --git a/docs/podman-cp.1.md b/docs/podman-cp.1.md index 406dd51df2..76fe57a9e7 100644 --- a/docs/podman-cp.1.md +++ b/docs/podman-cp.1.md @@ -61,6 +61,10 @@ If you use a : in a local machine path, you must be explicit with a relative or Extract the tar file into the destination directory. If the destination directory is not provided, extract the tar file into the root directory. +**--pause** + +Pause the container while copying into it to avoid potential security issues around symlinks. Defaults to *true*. + ## ALTERNATIVES Podman has much stronger capabilities than just `podman cp` to achieve copy files between host and container. From 431e633b48fbfc486c332c8374d14fd6e0073840 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 28 May 2019 13:53:25 -0400 Subject: [PATCH 4/9] Add test to ensure symlinks are resolved in ctr scope Signed-off-by: Matthew Heon --- test/e2e/cp_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/e2e/cp_test.go b/test/e2e/cp_test.go index f8df5d3d09..529e01c99e 100644 --- a/test/e2e/cp_test.go +++ b/test/e2e/cp_test.go @@ -158,4 +158,27 @@ var _ = Describe("Podman cp", func() { os.Remove("file.tar") os.RemoveAll(testDirPath) }) + + It("podman cp symlink", func() { + srcPath := filepath.Join(podmanTest.RunRoot, "cp_test.txt") + fromHostToContainer := []byte("copy from host to container") + err := ioutil.WriteFile(srcPath, fromHostToContainer, 0644) + Expect(err).To(BeNil()) + + session := podmanTest.Podman([]string{"create", ALPINE, "cat", "foo"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + name := session.OutputToString() + + session = podmanTest.Podman([]string{"exec", name, "ln", "-s", "/tmp", "/test"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"cp", srcPath, name + ":/test"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + _, err = os.Stat("/tmp/cp_test.txt") + Expect(err).To(Not(BeNil())) + }) }) From 79990b7364310d256e4dbaf7adc2b451adda854f Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 29 May 2019 16:35:16 -0400 Subject: [PATCH 5/9] Tolerate non-running containers in paused cp Signed-off-by: Matthew Heon --- cmd/podman/cp.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/cmd/podman/cp.go b/cmd/podman/cp.go index 7092da5e74..a0677071d6 100644 --- a/cmd/podman/cp.go +++ b/cmd/podman/cp.go @@ -97,13 +97,22 @@ func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest strin if pause { if err := ctr.Pause(); err != nil { - return err - } - defer func() { - if err := ctr.Unpause(); err != nil { - logrus.Errorf("Error unpausing container after copying: %v", err) + // An invalid state error is fine. + // The container isn't running or is already paused. + // TODO: We can potentially start the container while + // the copy is running, which still allows a race where + // malicious code could mess with the symlink. + if errors.Cause(err) != libpod.ErrCtrStateInvalid { + return err } - }() + } else if err == nil { + // Only add the defer if we actually paused + defer func() { + if err := ctr.Unpause(); err != nil { + logrus.Errorf("Error unpausing container after copying: %v", err) + } + }() + } } user, err := getUser(mountPoint, ctr.User()) From f456825fe8783d84e23a5cd291c637b3c3ed53dd Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 29 May 2019 18:08:05 -0400 Subject: [PATCH 6/9] Fix bug in e2e tests for podman cp Signed-off-by: Matthew Heon --- test/e2e/cp_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/cp_test.go b/test/e2e/cp_test.go index 529e01c99e..e08d05eaea 100644 --- a/test/e2e/cp_test.go +++ b/test/e2e/cp_test.go @@ -165,7 +165,7 @@ var _ = Describe("Podman cp", func() { err := ioutil.WriteFile(srcPath, fromHostToContainer, 0644) Expect(err).To(BeNil()) - session := podmanTest.Podman([]string{"create", ALPINE, "cat", "foo"}) + session := podmanTest.Podman([]string{"run", "-d", ALPINE, "top"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) name := session.OutputToString() From 48e35f7da70c24edd339bf29b83c427c48afcaab Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 29 May 2019 22:43:07 -0400 Subject: [PATCH 7/9] We can't pause rootless containers during cp Rootless containers can't be paused (no CGroups, so no freezer). We could try and emulate this with a SIGSTOP to all PIDs in the container, but that's inherently racy, so let's avoid it for now. Signed-off-by: Matthew Heon --- cmd/podman/cp.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/podman/cp.go b/cmd/podman/cp.go index a0677071d6..406337955e 100644 --- a/cmd/podman/cp.go +++ b/cmd/podman/cp.go @@ -13,6 +13,7 @@ import ( "github.com/containers/libpod/cmd/podman/cliconfig" "github.com/containers/libpod/cmd/podman/libpodruntime" "github.com/containers/libpod/libpod" + "github.com/containers/libpod/pkg/rootless" "github.com/containers/storage" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/chrootarchive" @@ -95,7 +96,12 @@ func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest strin } defer ctr.Unmount(false) - if pause { + // We can't pause rootless containers. + if pause && rootless.IsRootless() { + logrus.Warnf("Cannot pause rootless containers - pause option will be ignored") + } + + if pause && !rootless.IsRootless() { if err := ctr.Pause(); err != nil { // An invalid state error is fine. // The container isn't running or is already paused. From 57d40939792719e6dc54701226049e2b4c9aadc2 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 30 May 2019 09:20:04 -0400 Subject: [PATCH 8/9] Error when trying to copy into a running rootless ctr We can't pause them, so if that's requested, throw an error. Signed-off-by: Matthew Heon --- cmd/podman/cp.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/podman/cp.go b/cmd/podman/cp.go index 406337955e..907bde4b9b 100644 --- a/cmd/podman/cp.go +++ b/cmd/podman/cp.go @@ -98,7 +98,13 @@ func copyBetweenHostAndContainer(runtime *libpod.Runtime, src string, dest strin // We can't pause rootless containers. if pause && rootless.IsRootless() { - logrus.Warnf("Cannot pause rootless containers - pause option will be ignored") + state, err := ctr.State() + if err != nil { + return err + } + if state == libpod.ContainerStateRunning { + return errors.Errorf("cannot copy into running rootless container with pause set - pass --pause=false to force copying") + } } if pause && !rootless.IsRootless() { From 5a07311d9e8f6c66d23b77a3b01abe1d3aa0676d Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 30 May 2019 10:53:52 -0400 Subject: [PATCH 9/9] Fix podman cp tests Signed-off-by: Matthew Heon --- test/e2e/cp_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/cp_test.go b/test/e2e/cp_test.go index e08d05eaea..3c0288b195 100644 --- a/test/e2e/cp_test.go +++ b/test/e2e/cp_test.go @@ -174,7 +174,7 @@ var _ = Describe("Podman cp", func() { session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) - session = podmanTest.Podman([]string{"cp", srcPath, name + ":/test"}) + session = podmanTest.Podman([]string{"cp", "--pause=false", srcPath, name + ":/test"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0))