On Fri, Jun 5, 2020 at 2:21 AM Chirantan Ekbote <chirantan@xxxxxxxxxxxx> wrote: > > On Mon, Jun 1, 2020 at 9:42 PM Stephen Smalley > <stephen.smalley.work@xxxxxxxxx> wrote: > > > > On Mon, Jun 1, 2020 at 3:29 AM Chirantan Ekbote <chirantan@xxxxxxxxxxxx> wrote: > > > > > > Normally a process needs CAP_SYS_ADMIN in the namespace that mounted a > > > particular filesystem in order to set a security xattr. However, this > > > restriction is relaxed for the security.selinux xattr: the file owner > > > or a process with CAP_FOWNER in its namespace may set this attribute. > > > > > > Apply this relaxed restriction to the security.sehash xattr as well. > > > Since this xattr is mainly a performance optimization when labeling > > > files recursively it shouldn't have stricter requirements than setting > > > the selinux xattr in the first place. > > > > First, setting either security.<non-selinux> or security.selinux has > > an additional MAC check beyond the DAC/capability check; in the former > > case there is the FILE__SETATTR check and in the latter there are the > > FILE__RELABELFROM/TO checks. We need to preserve some kind of SELinux > > permission check here. > > > > So I understand correctly, what you're asking for is to change this section: > > if (is_sehash) > return 0; > > to this instead: > > if (is_sehash) > return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); > > Is that correct? I would suggest using FILE__RELABELFROM instead, under the raionale that a process that is allowed to set security.sehash would also be allowed to set security.selinux. In contrast, a process might be alowed to set another attribute (FILE__SETATTR) but not security.selinux or security.sehash. > > Second, security.sehash logic in userspace was introduced by Android > > in its libselinux fork and then copied in upstream logic. I'm not > > sure Android wants to relax the current requirement for CAP_SYS_ADMIN > > - I have copied them above. A possible concern is that an > > unprivileged process could disable the relabeling of a part of the > > tree that it owns upon an upgrade, which could have unexpected > > consequences. > > That's a good point. Is this not an issue for the selinux xattr > because the selinux check could prevent a process from changing the > label of a file it owns? Correct. > The background for this patch is that I have a fuse server that runs > in a user namespace. It runs as root in that namespace and keeps all > the file system caps so that it can set selinux xattrs. However, it > cannot set the sehash xattr as that needs CAP_SYS_ADMIN in the parent > namespace. Looking at the code I thought that might have just been an > oversight but if it's intentional then do you have any suggestions for > how to make this work? I'd rather not weaken the sandbox for this > process just so that it can set this one xattr. I'd be willing to move from requiring CAP_SYS_ADMIN to performing a SELinux permission check (either FILE__RELABELFROM or a new one), but I'd like the Android folks to chime in here. Maybe you can ping them through other channels since they haven't responded yet.
