Skip to content

Commit

Permalink
Ensure that --userns=keep-id sets user in config
Browse files Browse the repository at this point in the history 
One of the side-effects of the `--userns=keep-id` command is
switching the default user of the container to the UID of the
user running Podman (though this can still be overridden by the
`--user` flag). However, it did this by setting the UID and GID
in the OCI spec, and not by informing Libpod of its intention to
switch users via the `WithUser()` option. Because of this, a lot
of the code that should have triggered when the container ran
with a non-root user was not triggering. In the case of the issue
that this fixed, the code to remove capabilities from non-root
users was not triggering. Adjust the keep-id code to properly
inform Libpod of our intention to use a non-root user to fix
this.

Also, fix an annoying race around short-running exec sessions
where Podman would always print a warning that the exec session
had already stopped.

Fixes containers#9919

Signed-off-by: Matthew Heon <[email protected]>
  • Loading branch information
@mheon
mheon committed Apr 6, 2021
1 parent 3fae801 commit 541252a
Show file tree
Hide file tree
Showing 6 changed files with 178 additions and 123 deletions.
27 changes: 25 additions & 2 deletions libpod/container_exec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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
}
}
Expand Down
6 changes: 6 additions & 0 deletions libpod/container_validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
124 changes: 124 additions & 0 deletions libpod/oci_conmon_exec_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
120 changes: 0 additions & 120 deletions libpod/oci_conmon_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,13 @@ 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"
"github.com/containers/podman/v3/libpod/logs"
"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"
Expand Down Expand Up @@ -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) {
Expand Down
10 changes: 10 additions & 0 deletions pkg/specgen/generate/namespaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 13 additions & 1 deletion test/e2e/exec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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() {
Expand Down

0 comments on commit 541252a

Please sign in to comment.