From 7b7d54242c2aa0846766f2063e3bd4fe72999a3b Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 28 May 2019 12:43:09 -0400 Subject: 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(-) (limited to 'cmd/podman') diff --git a/cmd/podman/cp.go b/cmd/podman/cp.go index 8240cc193..5addf88d3 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) -- cgit v1.2.3-54-g00ecf From 49dc18552a13ee76dc012c35ff073ed07aaeb05b Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 28 May 2019 13:11:55 -0400 Subject: 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(-) (limited to 'cmd/podman') diff --git a/cmd/podman/cliconfig/create.go b/cmd/podman/cliconfig/create.go index 49ab3d827..5fb2eed10 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 5addf88d3..7092da5e7 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 -- cgit v1.2.3-54-g00ecf From 79990b7364310d256e4dbaf7adc2b451adda854f Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 29 May 2019 16:35:16 -0400 Subject: Tolerate non-running containers in paused cp Signed-off-by: Matthew Heon --- cmd/podman/cp.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) (limited to 'cmd/podman') diff --git a/cmd/podman/cp.go b/cmd/podman/cp.go index 7092da5e7..a0677071d 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()) -- cgit v1.2.3-54-g00ecf From 48e35f7da70c24edd339bf29b83c427c48afcaab Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 29 May 2019 22:43:07 -0400 Subject: 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(-) (limited to 'cmd/podman') diff --git a/cmd/podman/cp.go b/cmd/podman/cp.go index a0677071d..406337955 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. -- cgit v1.2.3-54-g00ecf From 57d40939792719e6dc54701226049e2b4c9aadc2 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 30 May 2019 09:20:04 -0400 Subject: 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(-) (limited to 'cmd/podman') diff --git a/cmd/podman/cp.go b/cmd/podman/cp.go index 406337955..907bde4b9 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() { -- cgit v1.2.3-54-g00ecf