Skip to content

Commit

Permalink
capabilities: always set ambient and inheritable
Browse files Browse the repository at this point in the history 
change capabilities handling to reflect what docker does.

Bounding: set to caplist
Inheritable: set to caplist
Effective: if uid != 0 then clear; else set to caplist
Permitted: if uid != 0 then clear; else set to caplist
Ambient: clear

Signed-off-by: Giuseppe Scrivano <[email protected]>
  • Loading branch information
@giuseppe
giuseppe committed Sep 30, 2020
1 parent 2ee415b commit 703381b
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 7 deletions.
3 changes: 2 additions & 1 deletion pkg/specgen/generate/security.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,13 @@ func securityConfigureGenerator(s *specgen.SpecGenerator, g *generate.Generator,
}

configSpec := g.Config
configSpec.Process.Capabilities.Ambient = []string{}
configSpec.Process.Capabilities.Bounding = caplist
configSpec.Process.Capabilities.Inheritable = caplist

if s.User == "" || s.User == "root" || s.User == "0" {
configSpec.Process.Capabilities.Effective = caplist
configSpec.Process.Capabilities.Permitted = caplist
configSpec.Process.Capabilities.Inheritable = caplist
} else {
userCaps, err := capabilities.NormalizeCapabilities(s.CapAdd)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/cp_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,11 +269,11 @@ var _ = Describe("Podman cp", func() {
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))

session = podmanTest.Podman([]string{"exec", "-u", "testuser", "testctr", "touch", "testfile"})
session = podmanTest.Podman([]string{"exec", "-u", "testuser", "testctr", "touch", "/tmp/testfile"})
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))

session = podmanTest.Podman([]string{"cp", "--pause=false", "testctr:testfile", "testfile1"})
session = podmanTest.Podman([]string{"cp", "--pause=false", "testctr:/tmp/testfile", "testfile1"})
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))

Expand Down
4 changes: 2 additions & 2 deletions test/system/030-run.bats
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ echo $rand | 0 | $rand

# #6991 : /etc/passwd is modifiable
@test "podman run : --userns=keep-id: passwd file is modifiable" {
run_podman run -d --userns=keep-id $IMAGE sh -c 'while ! test -e /stop; do sleep 0.1; done'
run_podman run -d --userns=keep-id --cap-add=dac_override $IMAGE sh -c 'while ! test -e /tmp/stop; do sleep 0.1; done'
cid="$output"

# Assign a UID that is (a) not in our image /etc/passwd and (b) not
Expand All @@ -325,7 +325,7 @@ echo $rand | 0 | $rand
is "$output" "newuser3:x:$uid:999:$gecos:/home/newuser3:/bin/sh" \
"newuser3 added to /etc/passwd in container"

run_podman exec $cid touch /stop
run_podman exec $cid touch /tmp/stop
run_podman wait $cid
}

Expand Down
4 changes: 2 additions & 2 deletions test/system/075-exec.bats
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,14 @@ load helpers
# #6829 : add username to /etc/passwd inside container if --userns=keep-id
@test "podman exec - with keep-id" {
run_podman run -d --userns=keep-id $IMAGE sh -c \
"echo READY;while [ ! -f /stop ]; do sleep 1; done"
"echo READY;while [ ! -f /tmp/stop ]; do sleep 1; done"
cid="$output"
wait_for_ready $cid

run_podman exec $cid id -un
is "$output" "$(id -un)" "container is running as current user"

run_podman exec --user=$(id -un) $cid touch /stop
run_podman exec --user=$(id -un) $cid touch /tmp/stop
run_podman wait $cid
run_podman rm $cid
}
Expand Down

7 comments on commit 703381b

@AndrewGMorgan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting inheritable capabilities like that looks like a bug to me. The kernel doesn't launch any process, PID=1 or anything, with any Inheritable capabilities. However, with this behavior, containerized distributions leak inheritable capabilities all over the place. I stumbled onto this strangeness, when I first tried to run my libcap regression tests in a container under Fedora.

GitHub's actions container rightly doesn't do this. ChromeOS's Linux in a box container doesn't do this. What motivates the choice here?

@giuseppe
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only reason I've added the inheritable capabilities to the default set is because Docker does it, so that users moving from Docker to Podman won't notice any difference.

I agree with you though and it is probably time to revisit that decision.

@rhatdan what do you think? Should we revert in part this change and do not add inheritable capabilities by default?

@vrothberg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 👍

@rhatdan
Copy link
Member

@rhatdan rhatdan commented on 703381b Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always like increasing security, but if it breaks anything we need a way for users to be able to turn it back on. Should there be an entry in containers.conf?

@AndrewGMorgan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the Linux application space uses user space code to raise Inheritable capabilities. They have no purpose beyond file capability handshakes and/or Ambient inheritance. Neither of which is part of the way a container launches it's first user space code. Raising Inheritable capabilities can be done by any process with the corresponding Permitted bit(s).

@giuseppe
Copy link
Member Author

@giuseppe giuseppe commented on 703381b Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always like increasing security, but if it breaks anything we need a way for users to be able to turn it back on. Should there be an entry in containers.conf?

do we need to tweak also the capabilities we use for exec? We are currently setting inheritable, permitted, ambient to the bounding set (if privileged), or to the effective set (if not privileged)

@AndrewGMorgan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wherever you are setting the Ambient vector, then you will have to set the Inheritable flag too for it to actually work. However, the code above explicitly sets the Ambient vector to empty.

Raising Inheritable flag capabilities like this just ruins the POSIX.1e model separation in meaning of Permitted and Inheritable file capabilities. It also shifts the expectations of in-container privilege Inheritance back in the direction of this old kernel issue from 2000.

Please sign in to comment.