[Rootless] --privileged does not grant SYS_ADMIN to non-UID-0 #13449
Comments
|
Looks like the cap is set to me. I have a feeling masking or some other security feature like SELinux is blocking the access? |
|
Can you try your test command again using |
|
@rhatdan You don't have a I recall this being related to Docker compat - Docker does not grant certain capabilities to containers when a non-root user is set, even if the container is privileged. I'm on vacation so I can't chase down specific bugs related to this, but I'm 90% sure this was changed to make our behavior closer to Docker's (and because defaulting to giving less caps is generally more secure, which is itself a strong argument). |
|
You are right, we don't give all caps to the default user. Just add them to the bounding set. |
|
|
Docker is slightly different but the user definitely does not get CAP_SYS_ADMIN. If running with cap-add, Docker and Podman also differ. |
|
From what I can see Docker doesn't have any special handling for uid != 0 and It seems that a lot of code in our capabilities handling is just there to emulate the issue in Docker. We should do the right thing and set all the caps without any special handling for I suggest we do something like: diff --git a/pkg/specgen/generate/security.go b/pkg/specgen/generate/security.go
index 988c29832..c643fde92 100644
--- a/pkg/specgen/generate/security.go
+++ b/pkg/specgen/generate/security.go
@@ -124,7 +124,7 @@ func securityConfigureGenerator(s *specgen.SpecGenerator, g *generate.Generator,
capsRequiredRequested = strings.Split(val, ",")
}
}
- if !s.Privileged && len(capsRequiredRequested) > 0 {
+ if len(capsRequiredRequested) > 0 {
// Pass capRequiredRequested in CapAdd field to normalize capabilities names
capsRequired, err := capabilities.MergeCapabilities(nil, capsRequiredRequested, nil)
if err != nil {
@@ -158,9 +158,14 @@ func securityConfigureGenerator(s *specgen.SpecGenerator, g *generate.Generator,
configSpec.Process.Capabilities.Effective = caplist
configSpec.Process.Capabilities.Permitted = caplist
} else {
- mergedCaps, err := capabilities.MergeCapabilities(nil, s.CapAdd, nil)
+ var startingCaps []string
+ if s.Privileged {
+ startingCaps = caplist
+ }
+
+ mergedCaps, err := capabilities.MergeCapabilities(startingCaps, s.CapAdd, s.CapDrop)
if err != nil {
- return errors.Wrapf(err, "capabilities requested by user are not valid: %q", strings.Join(s.CapAdd, ","))
+ return err
}
boundingSet, err := capabilities.BoundingSet()
if err != nil { |
|
I believe the first time this came up it was considered a CVE because we were granting "excess capabilities" - so I'm not opposed, but we should be cautious given potential security implications here. Will try and find the CVE once I'm done reviewing issues and PRs |
|
that would be done only with |
|
Evidently, it is not. I found the CVE: https://access.redhat.com/security/cve/CVE-2021-20188 It's not specifically SYS_ADMIN (I believe it's DAC_OVERRIDE) but it's definitely a too-many-caps issue. |
|
Maybe it is too late to fix it, but I disagree with that CVE and the analysis. The current behavior is quite confusing as it looks like there is a separation between the "container capabilities" and the "PID 1 capabilities". To me, they are the same thing. The command line I specify, IMO, should apply to the process that is launched. Instead, it seems that I think we just depend on a buggy behavior from Docker, since |
|
I would prefer we figure a way for users to specify it perhaps in containers.conf. I like the separation between root user having the caps and rootless requiring a setuid app to get it. |
|
so is |
|
The bounding set should also be usable by setuid binaries, right? So with |
|
Thanks for the feedback. Since it is working as expected, I am closing the issue. |
|
Sorry, I'm not sure I see how this is expected behavior? How am I supposed to give an arbitrary process, and any process it calls/forks, any permissions they need using only |
--cap-add is useful when running a privileged container with UID != 0, so that individual capabilities can be added to the container process. Closes: containers#13449 Signed-off-by: Giuseppe Scrivano <[email protected]>
|
you can add them individually with |
|
Perfect! Thanks for making that PR. That will solve the main problem that led me to make this issue in the first place. |
github-actions
bot
added
the
locked - please file new issue/PR
Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)
/kind bug
Description
In rootless mode,
--privilegeddoes not grantSYS_ADMINto the container unless the container is running as UID 0.Steps to reproduce the issue:
Containerfileandpodman build -t issue-demo .--privilegedand try to mounttest.imgas a non-root user with FUSE (which requiresSYS_ADMIN). Observe that this fails:--cap-add SYS_ADMINand try to mounttest.imgas a non-root user with FUSE. Observe that this is successful:--privilegedonce more, but specify-u 0to run the container as "root". Try to mount the image; observe that this is successful:Describe the results you received:
Using the
--privilegedflag does not grantSYS_ADMINto non-root container users. It only grantsSYS_ADMINto UID 0.Using
--cap-add SYS_ADMINproperly grantsSYS_ADMINto any container user, regardless of UID.Describe the results you expected:
I expected the
--privilegedflag to grantSYS_ADMINto all container users, regardless of UID.Additional information you deem important (e.g. issue happens only occasionally):
I am running podman in rootless mode. Unfortunately I am not equipped to test this in root mode. This behavior I described also happens with
--userns=keep-id.Output of
podman version:Output of
podman info --debug:Package info (e.g. output of
rpm -q podmanorapt list podman):Have you tested with the latest version of Podman and have you checked the Podman Troubleshooting Guide? (https://github.com/containers/podman/blob/main/troubleshooting.md)
Yes - this is the latest version available to my distribution. I have checked the troubleshooting guide, and a maintainer commented in another issue suggesting I file this issue.
Additional environment details (AWS, VirtualBox, physical, etc.):
Physical headless server
The text was updated successfully, but these errors were encountered: