Merge pull request #9942 from mheon/fix_9919

Ensure that `--userns=keep-id` sets user in config

6 files changed, 178 insertions, 123 deletions

diff --git a/libpod/container_exec.go b/libpod/container_exec.go
index bb43287d9..8d8ed14aa 100644
--- a/libpod/container_exec.go
+++ b/libpod/container_exec.go
@@ -696,6 +696,24 @@ func (c *Container) ExecResize(sessionID string, newSize define.TerminalSize) er
 		return errors.Wrapf(define.ErrExecSessionStateInvalid, "cannot resize container %s exec session %s as it is not running", c.ID(), session.ID())
 	}
 
+	// The exec session may have exited since we last updated.
+	// Needed to prevent race conditions around short-running exec sessions.
+	running, err := c.ociRuntime.ExecUpdateStatus(c, session.ID())
+	if err != nil {
+		return err
+	}
+	if !running {
+		session.State = define.ExecStateStopped
+
+		if err := c.save(); err != nil {
+			logrus.Errorf("Error saving state of container %s: %v", c.ID(), err)
+		}
+
+		return errors.Wrapf(define.ErrExecSessionStateInvalid, "cannot resize container %s exec session %s as it has stopped", c.ID(), session.ID())
+	}
+
+	// Make sure the exec session is still running.
+
 	return c.ociRuntime.ExecAttachResize(c, sessionID, newSize)
 }
 
@@ -720,8 +738,13 @@ func (c *Container) Exec(config *ExecConfig, streams *define.AttachStreams, resi
 			logrus.Debugf("Sending resize events to exec session %s", sessionID)
 			for resizeRequest := range resize {
 				if err := c.ExecResize(sessionID, resizeRequest); err != nil {
-					// Assume the exec session went down.
-					logrus.Warnf("Error resizing exec session %s: %v", sessionID, err)
+					if errors.Cause(err) == define.ErrExecSessionStateInvalid {
+						// The exec session stopped
+						// before we could resize.
+						logrus.Infof("Missed resize on exec session %s, already stopped", sessionID)
+					} else {
+						logrus.Warnf("Error resizing exec session %s: %v", sessionID, err)
+					}
 					return
 				}
 			}
diff --git a/libpod/container_validate.go b/libpod/container_validate.go
index 245121a91..aae96ae85 100644
--- a/libpod/container_validate.go
+++ b/libpod/container_validate.go
@@ -126,5 +126,11 @@ func (c *Container) validate() error {
 		}
 	}
 
+	// If User in the OCI spec is set, require that c.config.User is set for
+	// security reasons (a lot of our code relies on c.config.User).
+	if c.config.User == "" && (c.config.Spec.Process.User.UID != 0 || c.config.Spec.Process.User.GID != 0) {
+		return errors.Wrapf(define.ErrInvalidArg, "please set User explicitly via WithUser() instead of in OCI spec directly")
+	}
+
 	return nil
 }
diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go
index 173edba2b..b43316951 100644
--- a/libpod/oci_conmon_exec_linux.go
+++ b/libpod/oci_conmon_exec_linux.go
@@ -2,18 +2,23 @@ package libpod
 
 import (
 	"fmt"
+	"io/ioutil"
 	"net/http"
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 	"syscall"
 	"time"
 
+	"github.com/containers/common/pkg/capabilities"
 	"github.com/containers/common/pkg/config"
 	"github.com/containers/podman/v3/libpod/define"
 	"github.com/containers/podman/v3/pkg/errorhandling"
+	"github.com/containers/podman/v3/pkg/lookup"
 	"github.com/containers/podman/v3/pkg/util"
 	"github.com/containers/podman/v3/utils"
+	spec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
@@ -654,3 +659,122 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 		}
 	}
 }
+
+// prepareProcessExec returns the path of the process.json used in runc exec -p
+// caller is responsible to close the returned *os.File if needed.
+func prepareProcessExec(c *Container, options *ExecOptions, env []string, sessionID string) (*os.File, error) {
+	f, err := ioutil.TempFile(c.execBundlePath(sessionID), "exec-process-")
+	if err != nil {
+		return nil, err
+	}
+	pspec := new(spec.Process)
+	if err := JSONDeepCopy(c.config.Spec.Process, pspec); err != nil {
+		return nil, err
+	}
+	pspec.SelinuxLabel = c.config.ProcessLabel
+	pspec.Args = options.Cmd
+
+	// We need to default this to false else it will inherit terminal as true
+	// from the container.
+	pspec.Terminal = false
+	if options.Terminal {
+		pspec.Terminal = true
+	}
+	if len(env) > 0 {
+		pspec.Env = append(pspec.Env, env...)
+	}
+
+	if options.Cwd != "" {
+		pspec.Cwd = options.Cwd
+	}
+
+	var addGroups []string
+	var sgids []uint32
+
+	// if the user is empty, we should inherit the user that the container is currently running with
+	user := options.User
+	if user == "" {
+		logrus.Debugf("Set user to %s", c.config.User)
+		user = c.config.User
+		addGroups = c.config.Groups
+	}
+
+	overrides := c.getUserOverrides()
+	execUser, err := lookup.GetUserGroupInfo(c.state.Mountpoint, user, overrides)
+	if err != nil {
+		return nil, err
+	}
+
+	if len(addGroups) > 0 {
+		sgids, err = lookup.GetContainerGroups(addGroups, c.state.Mountpoint, overrides)
+		if err != nil {
+			return nil, errors.Wrapf(err, "error looking up supplemental groups for container %s exec session %s", c.ID(), sessionID)
+		}
+	}
+
+	// If user was set, look it up in the container to get a UID to use on
+	// the host
+	if user != "" || len(sgids) > 0 {
+		if user != "" {
+			for _, sgid := range execUser.Sgids {
+				sgids = append(sgids, uint32(sgid))
+			}
+		}
+		processUser := spec.User{
+			UID:            uint32(execUser.Uid),
+			GID:            uint32(execUser.Gid),
+			AdditionalGids: sgids,
+		}
+
+		pspec.User = processUser
+	}
+
+	ctrSpec, err := c.specFromState()
+	if err != nil {
+		return nil, err
+	}
+
+	allCaps, err := capabilities.BoundingSet()
+	if err != nil {
+		return nil, err
+	}
+	if options.Privileged {
+		pspec.Capabilities.Bounding = allCaps
+	} else {
+		pspec.Capabilities.Bounding = ctrSpec.Process.Capabilities.Bounding
+	}
+	if execUser.Uid == 0 {
+		pspec.Capabilities.Effective = pspec.Capabilities.Bounding
+		pspec.Capabilities.Inheritable = pspec.Capabilities.Bounding
+		pspec.Capabilities.Permitted = pspec.Capabilities.Bounding
+		pspec.Capabilities.Ambient = pspec.Capabilities.Bounding
+	} else {
+		if user == c.config.User {
+			pspec.Capabilities.Effective = ctrSpec.Process.Capabilities.Effective
+			pspec.Capabilities.Inheritable = ctrSpec.Process.Capabilities.Effective
+			pspec.Capabilities.Permitted = ctrSpec.Process.Capabilities.Effective
+			pspec.Capabilities.Ambient = ctrSpec.Process.Capabilities.Effective
+		}
+	}
+
+	hasHomeSet := false
+	for _, s := range pspec.Env {
+		if strings.HasPrefix(s, "HOME=") {
+			hasHomeSet = true
+			break
+		}
+	}
+	if !hasHomeSet {
+		pspec.Env = append(pspec.Env, fmt.Sprintf("HOME=%s", execUser.Home))
+	}
+
+	processJSON, err := json.Marshal(pspec)
+	if err != nil {
+		return nil, err
+	}
+
+	if err := ioutil.WriteFile(f.Name(), processJSON, 0644); err != nil {
+		return nil, err
+	}
+	return f, nil
+}
diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go
index 1c7089e5d..f26ca67ce 100644
--- a/libpod/oci_conmon_linux.go
+++ b/libpod/oci_conmon_linux.go
@@ -22,7 +22,6 @@ import (
 	"text/template"
 	"time"
 
-	"github.com/containers/common/pkg/capabilities"
 	"github.com/containers/common/pkg/config"
 	conmonConfig "github.com/containers/conmon/runner/config"
 	"github.com/containers/podman/v3/libpod/define"
@@ -30,7 +29,6 @@ import (
 	"github.com/containers/podman/v3/pkg/cgroups"
 	"github.com/containers/podman/v3/pkg/checkpoint/crutils"
 	"github.com/containers/podman/v3/pkg/errorhandling"
-	"github.com/containers/podman/v3/pkg/lookup"
 	"github.com/containers/podman/v3/pkg/rootless"
 	"github.com/containers/podman/v3/pkg/util"
 	"github.com/containers/podman/v3/utils"
@@ -1195,124 +1193,6 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co
 	return nil
 }
 
-// prepareProcessExec returns the path of the process.json used in runc exec -p
-// caller is responsible to close the returned *os.File if needed.
-func prepareProcessExec(c *Container, options *ExecOptions, env []string, sessionID string) (*os.File, error) {
-	f, err := ioutil.TempFile(c.execBundlePath(sessionID), "exec-process-")
-	if err != nil {
-		return nil, err
-	}
-	pspec := new(spec.Process)
-	if err := JSONDeepCopy(c.config.Spec.Process, pspec); err != nil {
-		return nil, err
-	}
-	pspec.SelinuxLabel = c.config.ProcessLabel
-	pspec.Args = options.Cmd
-
-	// We need to default this to false else it will inherit terminal as true
-	// from the container.
-	pspec.Terminal = false
-	if options.Terminal {
-		pspec.Terminal = true
-	}
-	if len(env) > 0 {
-		pspec.Env = append(pspec.Env, env...)
-	}
-
-	if options.Cwd != "" {
-		pspec.Cwd = options.Cwd
-	}
-
-	var addGroups []string
-	var sgids []uint32
-
-	// if the user is empty, we should inherit the user that the container is currently running with
-	user := options.User
-	if user == "" {
-		user = c.config.User
-		addGroups = c.config.Groups
-	}
-
-	overrides := c.getUserOverrides()
-	execUser, err := lookup.GetUserGroupInfo(c.state.Mountpoint, user, overrides)
-	if err != nil {
-		return nil, err
-	}
-
-	if len(addGroups) > 0 {
-		sgids, err = lookup.GetContainerGroups(addGroups, c.state.Mountpoint, overrides)
-		if err != nil {
-			return nil, errors.Wrapf(err, "error looking up supplemental groups for container %s exec session %s", c.ID(), sessionID)
-		}
-	}
-
-	// If user was set, look it up in the container to get a UID to use on
-	// the host
-	if user != "" || len(sgids) > 0 {
-		if user != "" {
-			for _, sgid := range execUser.Sgids {
-				sgids = append(sgids, uint32(sgid))
-			}
-		}
-		processUser := spec.User{
-			UID:            uint32(execUser.Uid),
-			GID:            uint32(execUser.Gid),
-			AdditionalGids: sgids,
-		}
-
-		pspec.User = processUser
-	}
-
-	ctrSpec, err := c.specFromState()
-	if err != nil {
-		return nil, err
-	}
-
-	allCaps, err := capabilities.BoundingSet()
-	if err != nil {
-		return nil, err
-	}
-	if options.Privileged {
-		pspec.Capabilities.Bounding = allCaps
-	} else {
-		pspec.Capabilities.Bounding = ctrSpec.Process.Capabilities.Bounding
-	}
-	if execUser.Uid == 0 {
-		pspec.Capabilities.Effective = pspec.Capabilities.Bounding
-		pspec.Capabilities.Inheritable = pspec.Capabilities.Bounding
-		pspec.Capabilities.Permitted = pspec.Capabilities.Bounding
-		pspec.Capabilities.Ambient = pspec.Capabilities.Bounding
-	} else {
-		if user == c.config.User {
-			pspec.Capabilities.Effective = ctrSpec.Process.Capabilities.Effective
-			pspec.Capabilities.Inheritable = ctrSpec.Process.Capabilities.Effective
-			pspec.Capabilities.Permitted = ctrSpec.Process.Capabilities.Effective
-			pspec.Capabilities.Ambient = ctrSpec.Process.Capabilities.Effective
-		}
-	}
-
-	hasHomeSet := false
-	for _, s := range pspec.Env {
-		if strings.HasPrefix(s, "HOME=") {
-			hasHomeSet = true
-			break
-		}
-	}
-	if !hasHomeSet {
-		pspec.Env = append(pspec.Env, fmt.Sprintf("HOME=%s", execUser.Home))
-	}
-
-	processJSON, err := json.Marshal(pspec)
-	if err != nil {
-		return nil, err
-	}
-
-	if err := ioutil.WriteFile(f.Name(), processJSON, 0644); err != nil {
-		return nil, err
-	}
-	return f, nil
-}
-
 // configureConmonEnv gets the environment values to add to conmon's exec struct
 // TODO this may want to be less hardcoded/more configurable in the future
 func (r *ConmonOCIRuntime) configureConmonEnv(ctr *Container, runtimeDir string) ([]string, []*os.File) {
diff --git a/pkg/specgen/generate/namespaces.go b/pkg/specgen/generate/namespaces.go
index 845dfdad7..b52e8d100 100644
--- a/pkg/specgen/generate/namespaces.go
+++ b/pkg/specgen/generate/namespaces.go
@@ -157,6 +157,16 @@ func namespaceOptions(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.
 	case specgen.KeepID:
 		if rootless.IsRootless() {
 			toReturn = append(toReturn, libpod.WithAddCurrentUserPasswdEntry())
+
+			// If user is not overridden, set user in the container
+			// to user running Podman.
+			if s.User == "" {
+				_, uid, gid, err := util.GetKeepIDMapping()
+				if err != nil {
+					return nil, err
+				}
+				toReturn = append(toReturn, libpod.WithUser(fmt.Sprintf("%d:%d", uid, gid)))
+			}
 		} else {
 			// keep-id as root doesn't need a user namespace
 			s.UserNS.NSMode = specgen.Host
diff --git a/test/e2e/exec_test.go b/test/e2e/exec_test.go
index df86eab15..e6f63a391 100644
--- a/test/e2e/exec_test.go
+++ b/test/e2e/exec_test.go
@@ -119,6 +119,19 @@ var _ = Describe("Podman exec", func() {
 		Expect(session.ExitCode()).To(Equal(100))
 	})
 
+	It("podman exec in keep-id container drops privileges", func() {
+		SkipIfNotRootless("This function is not enabled for rootful podman")
+		ctrName := "testctr1"
+		testCtr := podmanTest.Podman([]string{"run", "-d", "--name", ctrName, "--userns=keep-id", ALPINE, "top"})
+		testCtr.WaitWithDefaultTimeout()
+		Expect(testCtr.ExitCode()).To(Equal(0))
+
+		session := podmanTest.Podman([]string{"exec", ctrName, "grep", "CapEff", "/proc/self/status"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.OutputToString()).To(ContainSubstring("0000000000000000"))
+	})
+
 	It("podman exec --privileged", func() {
 		session := podmanTest.Podman([]string{"run", "--privileged", "--rm", ALPINE, "sh", "-c", "grep ^CapBnd /proc/self/status | cut -f 2"})
 		session.WaitWithDefaultTimeout()
@@ -143,7 +156,6 @@ var _ = Describe("Podman exec", func() {
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 		Expect(session.OutputToString()).To(ContainSubstring(bndPerms))
-
 	})
 
 	It("podman exec --privileged", func() {