security/selinux/hooks.c: FILE__ perms used as DIR__ perms

security/selinux/hooks.c: FILE__ perms used as DIR__ perms

[Topi Miettinen]

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.

-Topi

Re: security/selinux/hooks.c: FILE__ perms used as DIR__ perms

[Stephen Smalley]

On Tue, Aug 24, 2021 at 6:55 AM Topi Miettinen <toiwoton@gmail.com> 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.

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.

Re: security/selinux/hooks.c: FILE__ perms used as DIR__ perms

[Topi Miettinen]

On 24.8.2021 15.47, Stephen Smalley wrote:
> On Tue, Aug 24, 2021 at 6:55 AM Topi Miettinen <toiwoton@gmail.com> 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

Re: security/selinux/hooks.c: FILE__ perms used as DIR__ perms

[Stephen Smalley]

On Tue, Aug 24, 2021 at 12:00 PM Topi Miettinen <toiwoton@gmail.com> wrote:
>
> On 24.8.2021 15.47, Stephen Smalley wrote:
> > 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?

Probably not. I think it is only applicable to file, chr_file, and
blk_file. That said I have been surprised before, e.g.
https://lore.kernel.org/selinux/20170512164124.16981-1-sds@tycho.nsa.gov/