On 24.8.2021 15.47, Stephen Smalley wrote:
On Tue, Aug 24, 2021 at 6:55 AM Topi Miettinen <toiwoton@xxxxxxxxx> wrote:Hello, Doing some post-processing of the SELinux policy (more about this some other time), I noticed that permissions for class "dir" could be tightened, for example "execmod" doesn't make a lot of sense for directories. Perhaps there are even more useless permissions? Let's grep for all use cases of permissions with DIR__ prefix: $ git grep DIR__ -- :^drivers/ :^arch/ security/selinux/hooks.c: DIR__ADD_NAME | DIR__SEARCH, security/selinux/hooks.c: av = DIR__SEARCH; security/selinux/hooks.c: av |= (kind ? DIR__REMOVE_NAME : DIR__ADD_NAME); security/selinux/hooks.c: av = DIR__RMDIR; security/selinux/hooks.c: DIR__REMOVE_NAME | DIR__SEARCH, &ad); security/selinux/hooks.c: old_isec->sclass, DIR__REPARENT, &ad); security/selinux/hooks.c: av = DIR__ADD_NAME | DIR__SEARCH; security/selinux/hooks.c: av |= DIR__REMOVE_NAME; security/selinux/hooks.c: (new_is_dir ? DIR__RMDIR : FILE__UNLINK), &ad); security/selinux/hooks.c: av |= DIR__SEARCH; security/selinux/hooks.c: av |= DIR__WRITE; security/selinux/hooks.c: av |= DIR__READ; So, no instances of for example DIR__IOCTL or DIR__OPEN can be seen. But blindly removing all unreferenced DIR__ perms from policy is a big mistake, which I learned the hard way as the system didn't work normally anymore after installing such a filtered policy. The reason for this is that in security/selinux/hooks.c, FILE__ perms are used sometimes as DIR__ perms and "git grep" won't find them. While semantically incorrect, the #defines for DIR__xyz are identical bitwise to corresponding FILE__xyz and so either works. Perhaps the situation could be improved: 1. Add a comment to security/selinux/hooks.c to alert readers that FILE__ perms are sometimes used in place of DIR__ perms and why this is in fact OK. 2. Add static asserts to verify the hard way that each DIR__abc #define matches the corresponding FILE__abc #define. If one day this would no longer be the case, the compiler would warn. 3. Add a new unified set of #defines, for example COMMON_INODE__xyz, to document that the same perms are used for both files and directories. 4. Replace the semantically incorrect uses of FILE__ with something more complex (but technically useless) like if (S_ISDIR(xyz) av = DIR__abc else av = FILE__abc and since the values are identical bitwise, compilers could be expected to eliminate the useless checks and branches. Would patches be acceptable along some of these ideas? Maybe the unused permissions could be even removed entirely with lots of work and perhaps no real benefit.Some history and a few observations. These are the common file permissions as declared in the common access vector blocks in the policy inherited by all the file classes and as defined as COMMON_FILE_PERMS in security/selinux/include/classmap.h. We used to have COMMON_FILE__READ, COMMON_FILE__WRITE, ... permission definitions as well but those went away with the migration to dynamic class/perm mapping and weren't being used in the code anyway; we have always just used FILE__x in the code when it was a common file permission. execmod was moved from being file-specific to being duplicated for chr_file to being taken into the common file perms (b424485abe2b16580a178b469917a7b6ee0c152a). open was moved to common file perms and the explicit per-class mapping dropped from the code in 49b7b8de46d293113a0a0bb026ff7bd833c73367. Before removing a permission from a class you need to ensure that the check can never fire. It isn't enough that the operation might not be implemented for the object; if the permission check can be reached then we either need the permission to remain or replicate the check against the object type before checking permission and return the same error as the underlying code ala ENOTSUP or whatever.
Could there really be a case where a permission check for "execmod" could be reachable for class "dir"? Executing on some strange file system which does not distinguish files from directories?
Only options #2 and #4 ensure that the code won't compile if someone removes a permission that is still being checked, so if we make a change, I'd be inclined to do one of those two. If we were to go with option 4, then the conditional should be based on the security class stored in the inode security blob not based on the mode bits.
OK. I tested the check with some compilers at godbolt.org and it seems that with optimization enabled the check disappears, so there should be no change in performance.
-Topi
