Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Label /dev/dma_heap with dma_device_dir_t #763

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

zpytela
Copy link
Contributor

@zpytela zpytela commented May 31, 2021

With commit a091bcd (Label /dev/dma_heap/* char devices with dma_device_t)
a new dma_device_t type was assigned to the /dev/dma_heap directory
and all files in it. The basic dev_node() interface just assigns the
type to the device_node attribute, which prevents many domains from
searching the directory.

This commits adds the dma_device_t type to file_type, non_auth_file_type,
and non_security_file_type attributes which should allow the access for
domains requiring this access.

An example AVC denial:

type=PROCTITLE msg=audit(05/31/2021 09:03:08.452:397) :
proctitle=/usr/bin/python3 -Es /usr/sbin/setroubleshootd -f
type=PATH msg=audit(05/31/2021 09:03:08.452:397) : item=0 name=/dev/dma_heap/*
nametype=UNKNOWN cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
type=SYSCALL msg=audit(05/31/2021 09:03:08.452:397) : arch=x86_64 syscall=newfstatat
success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x7fd607775ec0
a2=0x7fd60774bf60 a3=0x0 items=1 ppid=1 pid=2498 auid=unset uid=setroubleshoot
gid=setroubleshoot euid=setroubleshoot suid=setroubleshoot fsuid=setroubleshoot
egid=setroubleshoot sgid=setroubleshoot fsgid=setroubleshoot tty=(none) ses=unset
comm=setroubleshootd exe=/usr/bin/python3.9 subj=system_u:system_r:setroubleshootd_t:s0
type=AVC msg=audit(05/31/2021 09:03:08.452:397) : avc: denied { search }
for pid=2498 comm=setroubleshootd name=dma_heap dev="devtmpfs" ino=102
scontext=system_u:system_r:setroubleshootd_t:s0
tcontext=system_u:object_r:dma_device_t:s0 tclass=dir permissive=0

Resolves: rhbz#1965743

@ghost
Copy link

ghost commented May 31, 2021 via email

@WOnder93
Copy link
Member

It should be OK as there are almost no "bad" rules for chr/blk files labeled file_type:

$ sesearch -A -dt -t file_type -c chr_file
allow amanda_t file_type:chr_file { getattr ioctl lock open read };
allow domain file_type:chr_file map; [ domain_can_mmap_files ]:True
allow files_unconfined_type file_type:chr_file { append audit_access create execute execute_no_trans getattr ioctl link lock map mounton open quotaon read relabelfrom relabelto rename setattr swapon unlink watch watch_mount watch_reads watch_sb watch_with_perm write };
allow init_t file_type:chr_file { getattr relabelfrom relabelto };
allow locate_t file_type:chr_file getattr;
allow pegasus_openlmi_logicalfile_t file_type:chr_file { getattr ioctl lock open read };
allow secadm_t file_type:chr_file { getattr relabelfrom relabelto };
allow setfiles_mac_t file_type:chr_file { getattr relabelfrom relabelto };
allow setfiles_t file_type:chr_file { getattr relabelfrom relabelto };
allow sysadm_t file_type:chr_file { getattr relabelfrom relabelto };
$ sesearch -A -dt -t file_type -c blk_file
allow amanda_t file_type:blk_file { getattr ioctl lock open read };
allow domain file_type:blk_file map; [ domain_can_mmap_files ]:True
allow files_unconfined_type file_type:blk_file { append audit_access create execmod execute getattr ioctl link lock map mounton open quotaon read relabelfrom relabelto rename setattr swapon unlink watch watch_mount watch_reads watch_sb watch_with_perm write };
allow init_t file_type:blk_file { getattr relabelfrom relabelto };
allow locate_t file_type:blk_file getattr;
allow pegasus_openlmi_logicalfile_t file_type:blk_file { getattr ioctl lock open read };
allow secadm_t file_type:blk_file { getattr relabelfrom relabelto };
allow setfiles_mac_t file_type:blk_file { getattr relabelfrom relabelto };
allow setfiles_t file_type:blk_file { getattr relabelfrom relabelto };
allow sysadm_t file_type:blk_file { getattr relabelfrom relabelto };

Only amanda_t and pegasus_openlmi_logicalfile_t have some weird excessive rules, but that is a separate issue...

The disadvantage of labeling the dir just device_t is that it is then harder to ensure proper transition for the child nodes. But an alternative would be to add a separate label for the /dev/dma_heap dir (which would belong to file_type) and unconditionally transition chr/blk files in that to dma_device_t (which would remain unchanged).

@ghost
Copy link

ghost commented May 31, 2021 via email

@zpytela
Copy link
Contributor Author

zpytela commented May 31, 2021

I pondered 2 options:

  • let /dev/dma_heap/ get the default device_t type and define a transition for all known filenames
  • label /dev/dma_heap/ with private type so that any file can get the proper type

I think that both of them have their pros and con. Given that I don't know how many different device names can appear in the future, cannot ensure the names will be unique in the entire /dev, and that the first approach means just one transition, I decided for the latter. In existing policy, we have examples for both, in Fedora as well as in refpolicy. Now there also is the idea of the intermediate type for the directory.

you might be surprised how few type
transitions are actually needed in /dev as udev generally takes pretty
good care of manually labeling them

This is a valid point. Removing existing transition would require testing in different scenarios though, so I don't want to pursue development this way now.

Instead I would just not associate a device node type with anything
other than device nodes (ie only char files in /dev, and i suppose block
if you consider storage to be a device node, but files, dirs and
but files, dirs and symlinks arent device node types be any stretch of the imagination and
should just be labeled device_t instead

I agree with this, whenever possible. But also note 1. device_t is in device_node attribute, too; 2. existing types like devpts_t or hugetlbfs_t are also file_type.

regardless fwiw i just wanted to mention my view. i just dont think its
helpful, sensible or efficient. but if its a conscience decision to do
this then that is better than it being a mistake.

I may not always completely agree with you (e. g. when RHEL needs to be taken into consideration), but I appreciate all your comments. In this case it was a deliberate decision for one of the option while I thought the other can be used as well. One of the principles I try to follow is consistency; surely it does not exclude a major change when there is a reason for it.

@ghost
Copy link

ghost commented May 31, 2021 via email

@WOnder93
Copy link
Member

I don't think we can simply remove the transitions, unfortunately. There would be a window of time between the device node creation and udev relabeling it and in that window things can go wrong. I don't know if I could find them now, but I think I remember at least one or two bugzillas where this was a problem. You may think the window will be always too small, but the scheduler is unpredictable and udev is just bound to lose the race sooner or later...

@zpytela
Copy link
Contributor Author

zpytela commented May 31, 2021

I pondered 2 options: * let /dev/dma_heap/ get the default device_t type and define a transition for all known filenames * label /dev/dma_heap/ with private type so that any file can get the proper type I think that both of them have their pros and con. Given that I don't know how many different device names can appear in the future, cannot ensure the names will be unique in the entire /dev, and that the first approach means just one transition, I decided for the latter. In existing policy, we have examples for both, in Fedora as well as in refpolicy. Now there also is the idea of the intermediate type for the directory.
Yes, It was easy for me to state how things should ideally work but indeed unfortunately things already escalated in both fedora as well as refpolicy. It would be quite hard to rectify all of this.

We can start with small bits any time. Just to clarify: of the two remaining out of the three mentioned options, which one is "ideal" according to you? Or even something else? The third one being

  • label /dev/dma_heap/ with dma_device_directory_t type and define a transition for all known filenames

Subsequently, do the same for the other non-device nodes? That would be an intrusive change, and at first glance it also seems to be difficult for lnk_files.

...

Instead I would just not associate a device node type with anything other than device nodes (ie only char files in /dev, and i suppose block if you consider storage to be a device node, but files, dirs and but files, dirs and symlinks arent device node types be any stretch of the imagination and should just be labeled device_t instead I agree with this, whenever possible. But also note 1. device_t is in device_node attribute, too; 2. existing types like devpts_t or hugetlbfs_t are also file_type.
Yes as said above, things already escalated, but now it will escalate further. Its hard to write solid policy when things are inconsistent. be mindful that files_unconfined() grants more than just unconfined access to files.

I see classes service and filesystem, did not consider it being that important as the new type is not assigned to anything but /dev/dma_heap.

regardless fwiw i just wanted to mention my view. i just dont think its helpful, sensible or efficient. but if its a conscience decision to do this then that is better than it being a mistake. I may not always completely agree with you (e. g. when RHEL needs to be taken into consideration), but I appreciate all your comments. In this case it was a deliberate decision for one of the option while I thought the other can be used as well. One of the principles I try to follow is consistency; surely it does not exclude a major change when there is a reason for it.
That's heathly, especially when it comes to SELinux policy where things are subjective from time to time. I also don't mind that you go the other way. I am just glad to youre aware of the consequences.

The biggest benefit of public pull requests is that they are open to other people with different knowledge or perspective. Sometimes finding the better solution is clear, sometimes it happens to be clear only for other person than the author, and sometimes there are tradeoffs to be made, short-term or long-term. It is easy to miss some particular aspect of the problem or solution.

@WOnder93
Copy link
Member

The third one being

  • label /dev/dma_heap/ with dma_device_directory_t type and define a transition for all known filenames

Not for all known filenames. Once you have a special type for the directory, you can just do a regular non-filename transition for any file under it. The whole point of this option is to have only one filename transition for the directory (which has a nice fixed name).

@ghost
Copy link

ghost commented Jun 1, 2021 via email

@ghost
Copy link

ghost commented Jun 1, 2021 via email

@ghost
Copy link

ghost commented Jun 1, 2021

Ive seen cases where even transitions will not do the job: nodes being created in initramfs before selinux is enabled and being accessed before either systemd or udev have chance to label them. In that scenario you have a problem either way.

Since this happened only recently it is still fresh in my memory. This is what I think happened there.

Basically kmod was blocked access to a mislabeled /dev/full very early in the boot processes (it had event_device_t instead of null_device_t). And so I suggested what I guess anyone would suggest: add a name-based type transition to override any existing type transition rule to event_device_t. (in refpolicy there is a anon type transition: (type_transition kernel_t device_t chr_file "*" event_device_t). To our surprise that did not work simply because /dev/full was already there before selinux. How did it end up with event_device_t then? I suspect that is some "magic" related to how "fsuse trans" works. That rule probably causes the event_device_t transition rule to apply to as soon as selinux gets enabled. This was about the loading the autofs4 kernel module (in Debian). Debian does not have that module built-in but systemd needs it. I think systemd used kmod to try to load the autofs4 module after it loaded the selinux policy but before it relabels /dev.

In Fedora that probably wouldnt happen as, i guess the autofs4 module is built-in but it is an example where type transition rules will not help you.

@ghost
Copy link

ghost commented Jun 1, 2021

result there was that kmod failed to load the autofs module because it needed access to /dev/full

options to solve:

  • make autofs a built-in
  • allow kmod_t to write event_device_t char files
  • replace the anon (kernel_t device_t chr_file "" event_device_t type transition with kernel_t device_t chr_file "" null_device_t to trick fsuse trans to label it all null_device_t so that kmod can access it because event_device_t is a non-issue

With commit a091bcd (Label /dev/dma_heap/* char devices with dma_device_t)
a new dma_device_t type was assigned to the /dev/dma_heap directory
and all files in it. The basic dev_node() interface called for dma_device_t
just assigns the type to the device_node attribute, which prevents many
domains from searching the directory with the same label.

This commits labels the /dev/dma_heap directory with the new
dma_device_dir_t type and adds it to the file_type, non_auth_file_type,
and non_security_file_type attributes, allowing the access for domains
requiring this access, and adds unnamed file transition to dma_device_t
for block files created in this directory.

An example AVC denial after the directory was labeled dma_device_t:

type=PROCTITLE msg=audit(05/31/2021 09:03:08.452:397) :
proctitle=/usr/bin/python3 -Es /usr/sbin/setroubleshootd -f
type=PATH msg=audit(05/31/2021 09:03:08.452:397) : item=0 name=/dev/dma_heap/*
nametype=UNKNOWN cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
type=SYSCALL msg=audit(05/31/2021 09:03:08.452:397) : arch=x86_64 syscall=newfstatat
success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x7fd607775ec0
a2=0x7fd60774bf60 a3=0x0 items=1 ppid=1 pid=2498 auid=unset uid=setroubleshoot
gid=setroubleshoot euid=setroubleshoot suid=setroubleshoot fsuid=setroubleshoot
egid=setroubleshoot sgid=setroubleshoot fsgid=setroubleshoot tty=(none) ses=unset
comm=setroubleshootd exe=/usr/bin/python3.9 subj=system_u:system_r:setroubleshootd_t:s0
type=AVC msg=audit(05/31/2021 09:03:08.452:397) : avc:  denied  { search }
for  pid=2498 comm=setroubleshootd name=dma_heap dev="devtmpfs" ino=102
scontext=system_u:system_r:setroubleshootd_t:s0
tcontext=system_u:object_r:dma_device_t:s0 tclass=dir permissive=0

Resolves: rhbz#1965743
@zpytela zpytela force-pushed the fb-dmadevice-dir branch from d3c1888 to 1522c0b Compare June 2, 2021 10:47
@zpytela
Copy link
Contributor Author

zpytela commented Jun 2, 2021

I've just changed this commit to label /dev/dma_heap with dma_device_dir_t.

@tao12345666333
Copy link

Personally, I think dma_device_dir_t is good to me.

@zpytela zpytela changed the title Assign dma_device_t to files_type Label /dev/dma_heap with dma_device_dir_t Jun 3, 2021
@ghost
Copy link

ghost commented Jun 3, 2021

Yes its fine albeit a bit expensive.

@zpytela
Copy link
Contributor Author

zpytela commented Jun 3, 2021

I will merge the PR now as it seems to me be aligned with our previous discussion.

Yes its fine albeit a bit expensive.

Frankly I don't understand what is expensive on this solution: there is one new type and onw transition which is unnamed. So until we get to you proposal get rid of all or almost all of them, this looks like a small change.

@zpytela zpytela merged commit 77d425c into fedora-selinux:rawhide Jun 3, 2021
@zpytela zpytela deleted the fb-dmadevice-dir branch June 3, 2021 14:59
@ghost
Copy link

ghost commented Jun 3, 2021 via email

@zpytela
Copy link
Contributor Author

zpytela commented Jun 3, 2021

Zdeněk Pytela @.***> writes:
I will merge the PR now as it seems to me be aligned with our previous discussion. Yes its fine albeit a bit expensive. Frankly I don't understand what is expensive on this solution: there is one new type and onw transition which is unnamed. So until we get to you proposal get rid of all or almost all of them, this looks like a small change.
Yes this particular change may only add one type and one type transition but it sets a precedent as this is not the only directory in /dev Expensive is relative to the option of keeping all directories type device_t as the parent where you do not have to add anything.

I agree to label all directories device_t as long as we can ensure filenames in the given directory will be unique in /dev.
Thank you for all you comments, I think the result is now better.

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.

3 participants