On Fri, Jun 5, 2020 at 8:23 AM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > > 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. One thing to note however is that setting of security.sehash is just an optimization to avoid unnecessarily walking the tree again. Not sure if that matters for your use case.
