Bug 896243 - Blindly dropping capabilities is a bad idea
Summary: Blindly dropping capabilities is a bad idea
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: systemd
Version: 18
Hardware: Unspecified
OS: Linux
low
low
Target Milestone: ---
Assignee: systemd-maint
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-01-16 21:52 UTC by Lukáš Tinkl
Modified: 2015-07-13 17:36 UTC (History)
16 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-01-28 17:46:33 UTC


Attachments (Terms of Use)

Description Lukáš Tinkl 2013-01-16 21:52:55 UTC
Description of problem:
login1 falsely reports hibernation as unavailable, whereas upower and pm-utils support it correctly

Version-Release number of selected component (if applicable):
systemd-197-1.fc18.1.x86_64
upower-0.9.19-1.fc18.x86_64

How reproducible:
[root@goblin power]# cat /sys/power/disk
platform [shutdown] reboot suspend

[ltinkl@goblin ~]$ cat /sys/power/disk 
[disabled]

Actual results:
CanHibernate() reports "na"

Expected results:
CanHibernate() reports "yes"

Additional info:

Comment 1 Lennart Poettering 2013-01-16 22:26:37 UTC
This is caused by an incompatible Fedora specific kernel change.

Fedora added a kernel patch that makes it impossible to read /sys/power/disk for userspace processes lacking CAP_COMPROMISE_KERNEL. When logind tries to figure out whether hibernation is available it needs to read that file. logind runs on minimal capabilities, and lacked CAP_COMPROMISE_KERNEL hence. As a result loging always reports CanHibernate() as "na".

This is a recent issue only, as the patch was very recently added only, apparently only for testing?

userspace libraries such as libcap don't know the capability. We rely on libcap to translate the capability bits to strings and back for us. Given that libcap doesn't know this cap we hence can't even work-around this kernel breakage.

adding a capability check where none was required before broke compatibility with userspace that run at minimal caps.

also, protecting reading a file like that (in contrast to writing) doesn't make much sense anyway. it's fine if unprivileged code learns that the machine can hybrid suspend.

Not sure what to do about this. Easiest would be to revert the kernel patch. Otherwise we can work around this by adding CAP_COMPROMISE_KERNEL, but for this is nasty, since the patch isn't upstream, libcap doesn't support it, and adding it to our upstream systemd code would break things for folks who run a kernel without that bit.

Anyway, reassigning to kernel.

Comment 2 Josh Boyer 2013-01-17 00:14:46 UTC
CAP_COMPROMISE_KERNEL is the capability added for UEFI Secure Boot.  It should only make a difference if your machine is actually booted in Secure Boot mode.  Hibernate is disabled for everyone in that case.

On my machine with Secure Boot enabled, this is working as expected:

[jwboyer@localhost ~]$ cat /sys/power/disk 
[disabled]
[jwboyer@localhost ~]$ sudo su -
[root@localhost ~]# cat /sys/power/disk 
[disabled]
[root@localhost ~]# 

I'll look at why we get differing results on non-Secure Boot machines.  It's a simple bug.

Comment 3 Kay Sievers 2013-01-17 00:34:25 UTC
I see the same behaviour as an ordinary user:
  [kay@lon ~]$ cat /sys/power/disk 
  [disabled]

  [kay@lon ~]$ sudo cat /sys/power/disk
  [platform] shutdown reboot suspend 

This is a laptop booted in EFI mode, but without secure boot.

Comment 4 Josh Boyer 2013-01-17 00:43:07 UTC
(In reply to comment #3)
> I see the same behaviour as an ordinary user:
>   [kay@lon ~]$ cat /sys/power/disk 
>   [disabled]
> 
>   [kay@lon ~]$ sudo cat /sys/power/disk
>   [platform] shutdown reboot suspend 
> 
> This is a laptop booted in EFI mode, but without secure boot.

Right.  As I said, that's a bug.  I see the same thing on my non-SB machines.  The patch should do absolutely nothing if Secure Boot isn't enabled.  I'll fix it shortly.

Comment 5 Josh Boyer 2013-01-17 03:14:12 UTC
OK, should be fixed.  Will be in f18 3.7.2-205 and rawhide whenever it builds again.

Secure Boot machine:

[jwboyer@localhost ~]$ cat /sys/power/state 
mem 
[jwboyer@localhost ~]$ cat /sys/power/disk 
[disabled]
[jwboyer@localhost ~]$ sudo su -
[root@localhost ~]# cat /sys/power/state 
mem 
[root@localhost ~]# cat /sys/power/disk 
[disabled]
[root@localhost ~]# uname -a
Linux localhost.localdomain 3.7.2-204.1.fc18.x86_64 #1 SMP Wed Jan 16 20:57:02 EST 2013 x86_64 x86_64 x86_64 GNU/Linux
[root@localhost ~]# 

Non-Secure Boot machine:

[jwboyer@vader ~]$ cat /sys/power/state
standby mem disk
[jwboyer@vader ~]$ cat /sys/power/disk
[platform] shutdown reboot suspend 
[jwboyer@vader ~]$ sudo su -
[root@vader ~]# cat /sys/power/state 
standby mem disk
[root@vader ~]# cat /sys/power/disk 
[platform] shutdown reboot suspend 
[root@vader ~]# uname -a
Linux vader 3.7.2-204.1.fc18.x86_64 #1 SMP Wed Jan 16 20:57:02 EST 2013 x86_64 x86_64 x86_64 GNU/Linux
[root@vader ~]#

Comment 6 Fedora Update System 2013-01-23 20:07:48 UTC
kernel-3.7.4-204.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/kernel-3.7.4-204.fc18

Comment 7 Lukáš Tinkl 2013-01-24 13:59:35 UTC
kernel-3.7.2-204.fc18.x86_64 fixes it only partially, CanSuspend() returns "yes" while CanHibernate() still returns "na", unlike upower which correctly says the hibernation is supported.

Comment 8 Josh Boyer 2013-01-24 14:03:51 UTC
Please provide the output of the commands I ran in comment #5.  Also the output of "dmesg | grep -i secure"

Comment 9 Lukáš Tinkl 2013-01-24 14:44:47 UTC
[ltinkl@goblin ~]$ cat /sys/power/state
mem

[ltinkl@goblin ~]$ cat /sys/power/disk 
[disabled]

[root@goblin ~]# cat /sys/power/state
mem disk

[root@goblin ~]# cat /sys/power/disk
[platform] shutdown reboot suspend

[root@goblin ~]# dmesg | grep -i secure
[    2.154022] sdhci: Secure Digital Host Controller Interface driver

[root@goblin ~]# uname -a
Linux goblin 3.7.2-204.fc18.x86_64 #1 SMP Wed Jan 16 16:22:52 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux

Comment 10 Josh Boyer 2013-01-24 14:55:27 UTC
Fascinating...

Can you attach the full dmesg output?  Also, as a normal user, the output of 'cat /proc/self/status' ?

Comment 11 Josh Boyer 2013-01-24 14:59:06 UTC
Oh, nevermind.  3.7.2-204.fc18 doesn't have the fixes in it.  You need 3.7.3-201 or newer.

Note that comment #6 is 3.7.4-204, not 3.7.2-204.

Comment 12 Fedora Update System 2013-01-24 22:39:53 UTC
Package kernel-3.7.4-204.fc18:
* should fix your issue,
* was pushed to the Fedora 18 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing kernel-3.7.4-204.fc18'
as soon as you are able to, then reboot.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-1443/kernel-3.7.4-204.fc18
then log in and leave karma (feedback).

Comment 13 Fedora Update System 2013-01-26 16:01:39 UTC
kernel-3.7.4-204.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 14 Lukáš Tinkl 2013-01-28 11:26:49 UTC
Verified, kernel-3.7.4-204.fc18 fixes the problem for me

Comment 15 Matthew Garrett 2013-01-28 16:37:11 UTC
Obviously breaking logind is a bad thing here, but logind really shouldn't be blindly dropping capabilities. The entire point of capabilities is to prevent processes from being able to do things that you would otherwise expect the process to be able to do, so you can't safely make the assumption that dropping capabilities you don't know about will leave you with the functionality you want.

Comment 16 Lennart Poettering 2013-01-28 17:46:33 UTC
Well, that's a bit contradicting the "least privileges" idea. 

It's a fundamental issue with the capabilities model: since programs list exactly the privileges the need you can never make the privileges logic anymore fine-grained, since existing apps will then miss those new capabilities.

You say "The entire point of capabilities is to prevent processes from being able to do things that you would otherwise expect the process to be able to do."

However, I am pretty sure the more common understanding goes the other way: "The entire point of capabilities is to allow processes to do exactly the things they should be allowed to do, and not more." 

Most capabilities examples suggest the latter definition. For example, the example right in front of the libcap-ng example:

http://people.redhat.com/sgrubb/libcap-ng/

And this is implemented that way quite comprehensively currently. For example, all file system capabilities on Fedora are actually set to limit the set to a minimum, rather than just dropping a few. (Hint, just type "filecap" into a shell).

Feel free to bring this up with the capabilities folks, but systemd is currently perfectly in line with how people do implement capabilities all across the board.

Comment 17 Matthew Garrett 2013-01-28 17:48:25 UTC
You agree that this means that adding any new capabilities will break existing userspace, though?

Comment 18 Lennart Poettering 2013-01-29 03:48:57 UTC
(In reply to comment #17)
> You agree that this means that adding any new capabilities will break
> existing userspace, though?

If done without care, yes.

However, the recent CAP_SYSLOG addition actually did get it right AFAIR, because they actually check for either the new (CAP_SYSLOG) or the old capability (CAP_SYS_ADMIN), and grant access for either. That way clients can use the new, fine grained CAP_SYSLOG and that's all they need, or they can use the old, coarse CAP_SYS_ADMIN and still work.

of course, the same scheme doesn't really apply to the CAP_COMPROMISE_KERNEL issue, since it defeats the point leaving CAP_SYS_ADMIN as an alternative cap needed for hibernation...

Comment 19 Matthew Garrett 2013-01-29 03:54:37 UTC
It doesn't seem to be possible to handle the case where capability checks are added where none previously existed, though - there's no way to check for an alternative there. The risk of doing it the other way (only drop capabilities you know about) is that if a new capability permits something that was previously forbidden, you'll have more privileges than you expected. So both approaches do seem to be broken and I have no idea how to add new capabilities without breaking existing userspace.


Note You need to log in before you can comment on or make changes to this bug.