Bug 1627114

Three things:

1. on Linux, it shall not be needed to scan /dev tree manually when
   sysfs' class/watchdog listing managed to collect anything
   (rationale: watchdog device drivers need to follow the API/protocol
    to be usefully exposed through kernel, and that's what will get
    the actual devices registered, and in turn, exposed through sysfs)

That would prevent unnecessary /dev lookups (at least singly redundant
since /dev/watchdog is merely a compatibility miscdev) and hence SELinux
complaining about /dev/[random device] accesses.

2. when on Linux and the sysfs scan is fruitless, as an extension
   of what's been said already, there's no point in trying further
   (unless very old Linux compatibility is needed, which likely it
   is not?)

3. to solve sysfs access guarded by SELinux alone, this looks like
   a viable solution:

--- sbd.te.orig	2018-09-10 16:20:26.346312881 +0200
+++ sbd.te	2018-09-10 16:20:31.664376819 +0200
@@ -44,6 +44,7 @@
 
 dev_read_rand(sbd_t)
 dev_write_watchdog(sbd_t)
+dev_read_sysfs(sbd_t)
 
 domain_read_all_domains_state(sbd_t)

Rechecked, and sorry for confusion, there are also "legacy" watchdog
devices, but there can only be one and only one (exactly /dev/watchdog).

So replace unconditional redundancy with conditional one at 1.,
and make only /dev/watchdog checked in addition at 2.

(In reply to Jan Pokorný from comment #2)
> Rechecked, and sorry for confusion, there are also "legacy" watchdog
> devices, but there can only be one and only one (exactly /dev/watchdog).
> 
> So replace unconditional redundancy with conditional one at 1.,
> and make only /dev/watchdog checked in addition at 2.

The idea of the approach is to require as little as possible regarding how the device-nodes do get into the /dev-directory.
Main thing in mind are fancy udev-rules (like the ones for persistent netdevice naming e.g.) or anything created there manually.
Scanning of sysfs and storage into user-space is done so that what is found under /dev can be matched efficiently.

(In reply to Klaus Wenninger from comment #3)
> (In reply to Jan Pokorný from comment #2)
> > Rechecked, and sorry for confusion, there are also "legacy" watchdog
> > devices, but there can only be one and only one (exactly /dev/watchdog).
> > 
> > So replace unconditional redundancy with conditional one at 1.,
> > and make only /dev/watchdog checked in addition at 2.
> 
> The idea of the approach is to require as little as possible regarding how
> the device-nodes do get into the /dev-directory.
> Main thing in mind are fancy udev-rules (like the ones for persistent
> netdevice naming e.g.) or anything created there manually.
> Scanning of sysfs and storage into user-space is done so that what is found
> under /dev can be matched efficiently.

Poor performance should be acceptable as the discovery is just done when the cluster is setup and not on each and every start or even more often.

For RHEL-7.5 patch as proposed by poki seems to do the trick:

diff --git a/policy-rhel-7.5-contrib.patch b/policy-rhel-7.5-contrib.patch
index 44d3736..f343fc1 100644
--- a/policy-rhel-7.5-contrib.patch
+++ b/policy-rhel-7.5-contrib.patch
@@ -99355,7 +99355,7 @@ new file mode 100644
 index 0000000000..7d54ef1d63
 --- /dev/null
 +++ b/sbd.te
-@@ -0,0 +1,62 @@
+@@ -0,0 +1,63 @@
 +policy_module(sbd, 1.0.0)
 +
 +########################################
@@ -99402,6 +99402,7 @@ index 0000000000..7d54ef1d63
 +
 +dev_read_rand(sbd_t)
 +dev_write_watchdog(sbd_t)
++dev_read_sysfs(sbd_t)
 +
 +domain_read_all_domains_state(sbd_t)
 +

Please collect SELinux denials which are triggered by the reproducer:

# ausearch -m avc -m user_avc -m selinux_err -m user_selinux_err -i -ts today

Thank you.

Created attachment 1482348 [details]
dump of avcs created during sbd traversing /dev

Created attachment 1482349 [details]
avcs with sys added to policy

took the freedom to add the avc-lists from my setup.
First without and afterwards with dev_read_sysfs(sbd_t) added.
Traversing /dev still adds a lot of avcs but the watchdog-ones are missing after the modification.

Tried the following:

[root@node2 ~]# ls -l /dev/watch*
crw-------. 1 root root  10, 130 Sep 10 19:29 /dev/watchdog
crw-------. 1 root root 252,   1 Sep 10 19:29 /dev/watchdog1

[root@node2 ~]# mknod /dev/testwatch c 10 130
[root@node2 ~]# mknod /dev/testwatch1 c 252 1
[root@node2 ~]# sbd query-watchdog

Discovered 4 watchdog devices:

[1] /dev/watchdog
Identity: i6300ESB timer
Driver: <unknown>

[2] /dev/watchdog1
Identity: Software Watchdog
Driver: softdog
CAUTION: Not recommended for use with sbd.

[3] /dev/testwatch
Identity: i6300ESB timer
Driver: <unknown>

[4] /dev/testwatch1
Identity: Software Watchdog
Driver: softdog
CAUTION: Not recommended for use with sbd.

fine that far ...

both

pcs stonith sbd enable --watchdog=/dev/testwatch
pcs stonith sbd enable --watchdog=/dev/testwatch1

are failing with selinux enabled showing that adding 
dev_read_sysfs(sbd_t) to the policy is not enough :-(

Milos,

Comment #12 should have the output of that command, let us know if you need anything else from there.

Thanks,
Chris

Please run following command:

# semanage permissive -a sbd_t

The command will switch the sbd_t domain (which causes denials) to permissive mode while the rest of system stays in enforcing mode. Re-run the reproducer and collect SELinux denials via ausearch:

# ausearch -m avc -m user_avc -m selinux_err -m user_selinux_err -i -ts today

Following command switches the sbd_t back to enforcing mode:

# semanage permissive -d sbd_t

Thank you.

re [comment 3]:

> (In reply to Jan Pokorný from comment #2)
>> Rechecked, and sorry for confusion, there are also "legacy" watchdog
>> devices, but there can only be one and only one (exactly
>> /dev/watchdog).
>> 
>> So replace unconditional redundancy with conditional one at 1.,
>> and make only /dev/watchdog checked in addition at 2.

> The idea of the approach is to require as little as possible regarding
> how the device-nodes do get into the /dev-directory.
> Main thing in mind are fancy udev-rules (like the ones for persistent
> netdevice naming e.g.) or anything created there manually.
> Scanning of sysfs and storage into user-space is done so that what is
> found under /dev can be matched efficiently.

Is there any issue in reading respective `uevent` file from sysfs
hierarchy right away and parsing `DEVNAME` variable there?  It appears
as if later `/dev` scanning could be skipped altogether, just perhaps
reassuring the device ID matches.  That would be true minimalistic
(regarding overhead) solution as opposed to visiting every and each
`/dev` file, triggering all these security alarms (SELinux, audit rules,
...) for the special ones of them.


re [comment 4]:

> Poor performance should be acceptable as the discovery is just done
> when the cluster is setup and not on each and every start or even
> more often.

Every extra step poses a risk of doing something wrong (see the
security alarms above), not necessarily just in terms of performance.

re [comment 15]:

> failing with selinux enabled showing that adding 
> dev_read_sysfs(sbd_t) to the policy is not enough :-(

Entirely expected, since the likely avoidable (see previous discussion)
extensive `/dev` scan...

re [comment 0]:

> There is also a bunch of similar lines mentioning the following paths:
> /dev/autofs /dev/btrfs-control /dev/cpu_dma_latency /dev/crash /dev/fb0
> /dev/fuse /dev/hpet /dev/kmsg /dev/loop-control /dev/mcelog /dev/mem
> /dev/network_latency /dev/network_throughput /dev/nvram /dev/oldmem
> /dev/port /dev/ppp /dev/ptmx /dev/rtc0 /dev/rtc0 /dev/sg0 /dev/snapshot
> /dev/sr0 /dev/uhid /dev/uinput /dev/usbmon0 /dev/usbmon1 /dev/usbmon2
> /dev/usbmon3 /dev/usbmon4 /dev/vga_arbiter /dev/vhci /dev/vhost-net
> /proc/kcore /run/systemd/initctl/fifo

^ I assume these are also of dedicated contexts different from watchdog
  reifications and hence triggering AVCs

(In reply to Jan Pokorný from comment #19)
> re [comment 15]:
> 
> > failing with selinux enabled showing that adding 
> > dev_read_sysfs(sbd_t) to the policy is not enough :-(
> 
> Entirely expected, since the likely avoidable (see previous discussion)
> extensive `/dev` scan...
> 
> re [comment 0]:
> 
> > There is also a bunch of similar lines mentioning the following paths:
> > /dev/autofs /dev/btrfs-control /dev/cpu_dma_latency /dev/crash /dev/fb0
> > /dev/fuse /dev/hpet /dev/kmsg /dev/loop-control /dev/mcelog /dev/mem
> > /dev/network_latency /dev/network_throughput /dev/nvram /dev/oldmem
> > /dev/port /dev/ppp /dev/ptmx /dev/rtc0 /dev/rtc0 /dev/sg0 /dev/snapshot
> > /dev/sr0 /dev/uhid /dev/uinput /dev/usbmon0 /dev/usbmon1 /dev/usbmon2
> > /dev/usbmon3 /dev/usbmon4 /dev/vga_arbiter /dev/vhci /dev/vhost-net
> > /proc/kcore /run/systemd/initctl/fifo
> 
> ^ I assume these are also of dedicated contexts different from watchdog
>   reifications and hence triggering AVCs

Of course other approaches are possible.
But everything coming to my mind is making more assumptions.
See e.g. my example as of above.
Performance wise it is done once when the cluster is setup ...

(In reply to Milos Malik from comment #17)
> Please run following command:
> 
> # semanage permissive -a sbd_t
> 
> The command will switch the sbd_t domain (which causes denials) to
> permissive mode while the rest of system stays in enforcing mode. Re-run the
> reproducer and collect SELinux denials via ausearch:
> 
> # ausearch -m avc -m user_avc -m selinux_err -m user_selinux_err -i -ts today
> 
> Following command switches the sbd_t back to enforcing mode:
> 
> # semanage permissive -d sbd_t
> 
> Thank you.

setting the sbd_t domain to permissive doesn't show any denials when I repeat the reproduce afterwards.

It seems that we need to switch whole machine to permissive mode to discover all SELinux denials. To be sure that nothing is hidden from us, let's get rid of dontaudit rules too:

1. # setenforce 0
2. # semodule -DB
3. re-run the reproducer
4. collect SELinux denials
5. # semodule -B
6. # setenforce 1

Thanks again. Unfortunately, SELinux troubleshooting sometimes needs more steps.

Created attachment 1482390 [details]
avcs in permissive mode as a reply to comment #22

Created attachment 1482391 [details]
ausearch output requested in comment 17

Data requested in comment 17. This is from a different machine than what Klaus provided, with selinux-policy-3.13.1-192.el7.noarch, no custom policies in place.

Here is the output of audit2allow when collected SELinux denials are piped into it:

#============= sbd_t ==============
allow sbd_t apm_bios_t:chr_file getattr;
allow sbd_t autofs_device_t:chr_file getattr;
allow sbd_t clock_device_t:chr_file getattr;
allow sbd_t crash_device_t:chr_file getattr;
allow sbd_t event_device_t:chr_file getattr;
allow sbd_t framebuf_device_t:chr_file getattr;
allow sbd_t fuse_device_t:chr_file getattr;
allow sbd_t initctl_t:fifo_file getattr;

#!!!! This avc can be allowed using the boolean 'domain_can_write_kmsg'
allow sbd_t kmsg_device_t:chr_file getattr;
allow sbd_t kvm_device_t:chr_file getattr;
allow sbd_t loop_control_device_t:chr_file getattr;
allow sbd_t lvm_control_t:chr_file getattr;
allow sbd_t memory_device_t:chr_file getattr;
allow sbd_t netcontrol_device_t:chr_file getattr;
allow sbd_t nvram_device_t:chr_file getattr;
allow sbd_t ppp_device_t:chr_file getattr;
allow sbd_t proc_kcore_t:file getattr;
allow sbd_t ptmx_t:chr_file getattr;
allow sbd_t sysfs_t:dir read;
allow sbd_t sysfs_t:file { getattr open read };
allow sbd_t sysfs_t:lnk_file read;

#!!!! This avc can be allowed using the boolean 'daemons_use_tty'
allow sbd_t tty_device_t:chr_file getattr;
allow sbd_t uhid_device_t:chr_file getattr;
allow sbd_t usb_device_t:chr_file getattr;
allow sbd_t usbmon_device_t:chr_file getattr;
allow sbd_t vhost_device_t:chr_file getattr;
allow sbd_t xserver_misc_device_t:chr_file getattr;

SELinux denials were collected in permissive mode and after removing dontaudit rules, therefore I believe the list of policy rules to fix this bug is complete.

Thank you.

The are slight differences in the list of policy rules:

#============= sbd_t ==============
allow sbd_t apm_bios_t:chr_file getattr;
allow sbd_t autofs_device_t:chr_file getattr;
allow sbd_t clock_device_t:chr_file getattr;
allow sbd_t crash_device_t:chr_file getattr;
allow sbd_t event_device_t:chr_file getattr;
allow sbd_t framebuf_device_t:chr_file getattr;
allow sbd_t fuse_device_t:chr_file getattr;
allow sbd_t initctl_t:fifo_file getattr;

#!!!! This avc can be allowed using the boolean 'domain_can_write_kmsg'
allow sbd_t kmsg_device_t:chr_file getattr;
allow sbd_t loop_control_device_t:chr_file getattr;
allow sbd_t lvm_control_t:chr_file getattr;
allow sbd_t memory_device_t:chr_file getattr;
allow sbd_t netcontrol_device_t:chr_file getattr;
allow sbd_t nvram_device_t:chr_file getattr;
allow sbd_t ppp_device_t:chr_file getattr;
allow sbd_t proc_kcore_t:file getattr;
allow sbd_t ptmx_t:chr_file getattr;
>> allow sbd_t removable_device_t:blk_file getattr;
>> allow sbd_t scsi_generic_device_t:chr_file getattr;
allow sbd_t sysfs_t:dir read;
allow sbd_t sysfs_t:file { getattr open read };
allow sbd_t sysfs_t:lnk_file read;
allow sbd_t uhid_device_t:chr_file getattr;
allow sbd_t usbmon_device_t:chr_file getattr;
allow sbd_t vhost_device_t:chr_file getattr;
allow sbd_t xserver_misc_device_t:chr_file getattr;

(In reply to Milos Malik from comment #27)
> The are slight differences in the list of policy rules:
> 
> #============= sbd_t ==============
> allow sbd_t apm_bios_t:chr_file getattr;
> allow sbd_t autofs_device_t:chr_file getattr;
> allow sbd_t clock_device_t:chr_file getattr;
> allow sbd_t crash_device_t:chr_file getattr;
> allow sbd_t event_device_t:chr_file getattr;
> allow sbd_t framebuf_device_t:chr_file getattr;
> allow sbd_t fuse_device_t:chr_file getattr;
> allow sbd_t initctl_t:fifo_file getattr;
> 
> #!!!! This avc can be allowed using the boolean 'domain_can_write_kmsg'
> allow sbd_t kmsg_device_t:chr_file getattr;
> allow sbd_t loop_control_device_t:chr_file getattr;
> allow sbd_t lvm_control_t:chr_file getattr;
> allow sbd_t memory_device_t:chr_file getattr;
> allow sbd_t netcontrol_device_t:chr_file getattr;
> allow sbd_t nvram_device_t:chr_file getattr;
> allow sbd_t ppp_device_t:chr_file getattr;
> allow sbd_t proc_kcore_t:file getattr;
> allow sbd_t ptmx_t:chr_file getattr;
> >> allow sbd_t removable_device_t:blk_file getattr;
> >> allow sbd_t scsi_generic_device_t:chr_file getattr;
> allow sbd_t sysfs_t:dir read;
> allow sbd_t sysfs_t:file { getattr open read };
> allow sbd_t sysfs_t:lnk_file read;
> allow sbd_t uhid_device_t:chr_file getattr;
> allow sbd_t usbmon_device_t:chr_file getattr;
> allow sbd_t vhost_device_t:chr_file getattr;
> allow sbd_t xserver_misc_device_t:chr_file getattr;

Well when querying for watchdog devices it is traversing what is found under /dev. So dependent on the setup and which devices are physically present e.g. you will get differences.
This is as well why I'm not so sure if it is wise to add a large list like that.
Would it be possible to allow just any char-device? (Which you would be doing with that list anyway somehow and even more ...)

Lukas, can you answer the question in comment#28?

(In reply to Klaus Wenninger from comment #28)

> Would it be possible to allow just any char-device? (Which you would be
> doing with that list anyway somehow and even more ...)

Just to explain that a little further ...

If it is no strict requirement somewhere to keep the avc-log clean I wouldn't have any problems if a watchdog-query creates some entries when accessing /dev-files that are anyway no valid watchdog-devices.
The current policy for accessing these device-files might even be enough for now (remember adding sys-access-stuff to the policy makes the pcs-testcase succeed).
Of course it would have been nice to be able to use /dev-files with arbitrary names that reference watchdog-devices (see my test with /dev/testwatch above) but as I got it for now this is no strict requirement. Though being able to find  these files easier was part of the intention why the query-mechanism in sbd is implemented the way it is atm.

Hi All, 

I added all needed rules and issue should be fixed. I'll try to get all needed flags and create build today. 

Thanks,
Lukas.

(In reply to Lukas Vrabec from comment #31)
> Hi All, 
> 
> I added all needed rules and issue should be fixed. I'll try to get all
> needed flags and create build today. 
> 
> Thanks,
> Lukas.

Thanks!
Do you have a link to a scratch-build or something?

Regards,
Klaus

(In reply to Klaus Wenninger from comment #32)
> (In reply to Lukas Vrabec from comment #31)
> > Hi All, 
> > 
> > I added all needed rules and issue should be fixed. I'll try to get all
> > needed flags and create build today. 
> > 
> > Thanks,
> > Lukas.
> 
> Thanks!
> Do you have a link to a scratch-build or something?
> 
> Regards,
> Klaus

Just for completeness ... it now covers what I've tested in comment #15 as well.

Please update the selinux-policy to the latest version (now 3.13.1-225.el7) and re-run your scenario. If the scenario still triggers any SELinux denials, please switch the bug to ASSIGNED.

(In reply to Milos Malik from comment #37)
> Please update the selinux-policy to the latest version (now 3.13.1-225.el7)
> and re-run your scenario. If the scenario still triggers any SELinux
> denials, please switch the bug to ASSIGNED.

Interesting ... I would have expected more SELinux denials to files other than chr but this is the only stuff left:

[root@node2 ~]# ausearch -m avc -m user_avc -m selinux_err -m user_selinux_err -i -ts 12:54:22
----
type=PROCTITLE msg=audit(09/14/2018 12:54:32.419:1029) : proctitle=/usr/sbin/sbd query-watchdog 
type=SYSCALL msg=audit(09/14/2018 12:54:32.419:1029) : arch=x86_64 syscall=stat success=no exit=EACCES(Permission denied) a0=0x7fff5b2c03f0 a1=0x7fff5b2c0050 a2=0x7fff5b2c0050 a3=0x2 items=0 ppid=28507 pid=28510 auid=unset uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=(none) ses=unset comm=sbd exe=/usr/sbin/sbd subj=system_u:system_r:sbd_t:s0 key=(null) 
type=AVC msg=audit(09/14/2018 12:54:32.419:1029) : avc:  denied  { getattr } for  pid=28510 comm=sbd path=/run/systemd/initctl/fifo dev="tmpfs" ino=11536 scontext=system_u:system_r:sbd_t:s0 tcontext=system_u:object_r:initctl_t:s0 tclass=fifo_file 
----
type=PROCTITLE msg=audit(09/14/2018 12:54:32.419:1030) : proctitle=/usr/sbin/sbd query-watchdog 
type=SYSCALL msg=audit(09/14/2018 12:54:32.419:1030) : arch=x86_64 syscall=stat success=no exit=EACCES(Permission denied) a0=0x7fff5b2c03f0 a1=0x7fff5b2c0050 a2=0x7fff5b2c0050 a3=0x2 items=0 ppid=28507 pid=28510 auid=unset uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=(none) ses=unset comm=sbd exe=/usr/sbin/sbd subj=system_u:system_r:sbd_t:s0 key=(null) 
type=AVC msg=audit(09/14/2018 12:54:32.419:1030) : avc:  denied  { getattr } for  pid=28510 comm=sbd path=/proc/kcore dev="proc" ino=4026532033 scontext=system_u:system_r:sbd_t:s0 tcontext=system_u:object_r:proc_kcore_t:s0 tclass=file 

As already said above: If we don't have a strategy to fully prevent SELinux denial entries this is perfectly fine.
The two files are referenced via symlinks from /dev.
Not sure if it exactly makes sense to give more rights to sbd because of that.
I would explicitly like to support symlinks (as they might be used to have persistent ordering of watchdog-devices) but I could of course leave aside everything that points outside of /dev in sbd.

(In reply to Klaus Wenninger from comment #38)

> 
> Interesting ... I would have expected more SELinux denials to files other
> than chr but this is the only stuff left:
> 

Ok, do understand why we don't have more - just getting the denial on stat and that is skipped for anything else but DT_CHR or DT_LNK.

(In reply to Klaus Wenninger from comment #38)

> I would explicitly like to support symlinks (as they might be used to have
> persistent ordering of watchdog-devices) but I could of course leave aside
> everything that points outside of /dev in sbd.

Tried with sbd skipping symlinks not pointing into /dev and that leads to empty avs-log as expected.

But actually there is a little bit more to it:

In my example I'm using a symlink of arbitrary name that points to a kernel-created watchdog-file. That is working as access will first follow the symlink and then selinux checks the context of the target (watchdog_device_t in that case).
If somebody on the other hand creates a node of arbitrary name using the major:minor of a watchdog device that is gonna land in the unconfined-context and thus sbd won't be allowed to access the file.
Unless of course it is added to the watchdog_device_t-context.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2018:3111