Skip to content

Retain all caps when invoked by uid 0#205

Closed
cgwalters wants to merge 1 commit intocontainers:masterfrom
cgwalters:privileged-caps
Closed

Retain all caps when invoked by uid 0#205
cgwalters wants to merge 1 commit intocontainers:masterfrom
cgwalters:privileged-caps

Conversation

@cgwalters
Copy link
Collaborator

In #101, specifically
commit cde7fab we started dropping
all capabilities, even if the caller was privileged.

This broke rpm-ostree, which runs RPM scripts using bwrap, and some
of those scripts depend on capabilities (mostly CAP_DAC_OVERRIDE).

Fix this by retaining capabilities by default if the caller's uid is zero.

I considered having the logic be to simply retain any capabilities the invoking
process has (imagine filecaps binaries like ping or
/usr/bin/gnome-keyring-daemon using bwrap) but we currently explicitly abort
in that scenario to catch broken packages which used file capabilites for bwrap
itself (we switched to suid). For now this works, and if down the line there's a
real-world use case for capability-bearing non-zero-uid processes to invoke
bwrap and retain those privileges, we can revisit.

Closes: #197

@giuseppe
Copy link
Member

giuseppe commented Aug 8, 2017

The change makes sense to me.

@jlebon
Copy link
Contributor

jlebon commented Aug 9, 2017

But this will still cause https://pagure.io/releng/issue/6550 to regress though, right? Since we're still unconditionally calling capset().

@cgwalters
Copy link
Collaborator Author

That's true, though actually cde7fab already triggered this since we started dropping caps for the init process. It's pretty tempting to patch systemd to stop doing that seccomp filter for capset().

…lter

In <containers#101>, specifically
commit cde7fab we started dropping
all capabilities, even if the caller was privileged.

This broke rpm-ostree, which runs RPM scripts using bwrap, and some
of those scripts depend on capabilities (mostly `CAP_DAC_OVERRIDE`).

Fix this by retaining capabilities by default if the caller's uid is zero.

I considered having the logic be to simply retain any capabilities the invoking
process has (imagine filecaps binaries like `ping` or
`/usr/bin/gnome-keyring-daemon` using bwrap) but we currently explicitly abort
in that scenario to catch broken packages which used file capabilites for bwrap
itself (we switched to suid). For now this works, and if down the line there's a
real-world use case for capability-bearing non-zero-uid processes to invoke
bwrap *and* retain those privileges, we can revisit.

Another twist here is that we need to do some gymnastics to first avoid calling
`capset()` if we don't need to, as that can fail due to systemd installing a
seccomp filter that denies it (for dubious reasons).  Then we also need to ignore
`EPERM` when dropping caps in the init process.  (I considered unilaterally
handling `EPERM`, but it seems nicer to avoid calling `capset()` unless we need to)

Closes: containers#197
@cgwalters
Copy link
Collaborator Author

Updated to work around this. Good catch!

Tested with: strace -f -e inject=capset:error=EPERM bwrap --ro-bind /usr /usr --symlink usr/lib64 /lib64 --proc /proc --dev /dev /usr/sbin/capsh --print

@alexlarsson
Copy link
Collaborator

Makes sense to me to, and the patch looks good. Some issue with the CI though

@jlebon
Copy link
Contributor

jlebon commented Aug 14, 2017

bot, retest this please

@jlebon
Copy link
Contributor

jlebon commented Aug 14, 2017

We had some issues with the Fedora Registry last week, though that should be resolved now.
Looks good to me as well!

@cgwalters
Copy link
Collaborator Author

@rh-atomic-bot r=alexlarsson 98667f6

@rh-atomic-bot
Copy link

⌛ Testing commit 98667f6 with merge abc5664...

@rh-atomic-bot
Copy link

☀️ Test successful - status-redhatci
Approved by: alexlarsson
Pushing abc5664 to master...

miabbott added a commit to miabbott/sig-atomic-buildscripts that referenced this pull request Aug 14, 2017
In containers/bubblewrap#205, the issue that we were seeing with
`bubblewrap` (containers/bubblewrap#101) was fixed.  As such, we
can thaw out `bubblewrap` and continue to track git master.
cgwalters pushed a commit to CentOS/sig-atomic-buildscripts that referenced this pull request Aug 14, 2017
In containers/bubblewrap#205, the issue that we were seeing with
`bubblewrap` (containers/bubblewrap#101) was fixed.  As such, we
can thaw out `bubblewrap` and continue to track git master.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants