diff mbox series

[v2] libsemanage: Preserve file context and ownership in policy store

Message ID 20240719132918.931352-1-vmojzis@redhat.com (mailing list archive)
State Superseded
Delegated to: Petr Lautrbach
Headers show
Series [v2] libsemanage: Preserve file context and ownership in policy store | expand

Commit Message

Vit Mojzis July 19, 2024, 1:29 p.m. UTC
Make sure that file context (all parts) and ownership of
files/directories in policy store does not change no matter which user
and under which context executes policy rebuild.

Fixes:
  # semodule -B
  # ls -lZ  /etc/selinux/targeted/contexts/files

-rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0 421397 Jul 11 09:57 file_contexts
-rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0 593470 Jul 11 09:57 file_contexts.bin
-rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0  14704 Jul 11 09:57 file_contexts.homedirs
-rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0  20289 Jul 11 09:57 file_contexts.homedirs.bin

  SELinux user changed from system_u to the user used to execute semodule

  # capsh --user=testuser --caps="cap_dac_override,cap_chown+eip" --addamb=cap_dac_override,cap_chown -- -c "semodule -B"
  # ls -lZ  /etc/selinux/targeted/contexts/files

-rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0 421397 Jul 19 09:10 file_contexts
-rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0 593470 Jul 19 09:10 file_contexts.bin
-rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0  14704 Jul 19 09:10 file_contexts.homedirs
-rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0  20289 Jul 19 09:10 file_contexts.homedirs.bin

  Both file context and ownership changed -- causes remote login
  failures and other issues in some scenarios.

Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
---
 libsemanage/src/semanage_store.c | 23 ++++++++++++++++++++++-
 libsemanage/src/semanage_store.h |  1 +
 2 files changed, 23 insertions(+), 1 deletion(-)

Comments

Stephen Smalley July 22, 2024, 3:11 p.m. UTC | #1
On Fri, Jul 19, 2024 at 9:29 AM Vit Mojzis <vmojzis@redhat.com> wrote:
>
> Make sure that file context (all parts) and ownership of
> files/directories in policy store does not change no matter which user
> and under which context executes policy rebuild.
>
> Fixes:
>   # semodule -B
>   # ls -lZ  /etc/selinux/targeted/contexts/files
>
> -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0 421397 Jul 11 09:57 file_contexts
> -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0 593470 Jul 11 09:57 file_contexts.bin
> -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0  14704 Jul 11 09:57 file_contexts.homedirs
> -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0  20289 Jul 11 09:57 file_contexts.homedirs.bin
>
>   SELinux user changed from system_u to the user used to execute semodule
>
>   # capsh --user=testuser --caps="cap_dac_override,cap_chown+eip" --addamb=cap_dac_override,cap_chown -- -c "semodule -B"
>   # ls -lZ  /etc/selinux/targeted/contexts/files
>
> -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0 421397 Jul 19 09:10 file_contexts
> -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0 593470 Jul 19 09:10 file_contexts.bin
> -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0  14704 Jul 19 09:10 file_contexts.homedirs
> -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0  20289 Jul 19 09:10 file_contexts.homedirs.bin
>
>   Both file context and ownership changed -- causes remote login
>   failures and other issues in some scenarios.
>
> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
> ---
>  libsemanage/src/semanage_store.c | 23 ++++++++++++++++++++++-
>  libsemanage/src/semanage_store.h |  1 +
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 27c5d349..12c30ad2 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -36,6 +36,7 @@ typedef struct dbase_policydb dbase_t;
>  #include "database_policydb.h"
>  #include "handle.h"
>
> +#include <selinux/restorecon.h>
>  #include <selinux/selinux.h>
>  #include <sepol/policydb.h>
>  #include <sepol/module.h>
> @@ -731,7 +732,7 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode,
>
>         if (!mode)
>                 mode = S_IRUSR | S_IWUSR;
> -
> +

We generally don't make unrelated whitespace changes in a patch.

>         mask = umask(0);
>         if ((out = open(tmp, O_WRONLY | O_CREAT | O_TRUNC, mode)) == -1) {
>                 umask(mask);
> @@ -767,6 +768,8 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode,
>         if (!retval && rename(tmp, dst) == -1)
>                 return -1;
>
> +       semanage_setfiles(dst);
> +
>  out:
>         errno = errsv;
>         return retval;
> @@ -819,6 +822,8 @@ static int semanage_copy_dir_flags(const char *src, const char *dst, int flag)
>                         goto cleanup;
>                 }
>                 umask(mask);
> +
> +               semanage_setfiles(dst);
>         }
>
>         for (i = 0; i < len; i++) {
> @@ -837,6 +842,7 @@ static int semanage_copy_dir_flags(const char *src, const char *dst, int flag)
>                                 goto cleanup;
>                         }
>                         umask(mask);
> +                       semanage_setfiles(path2);
>                 } else if (S_ISREG(sb.st_mode) && flag == 1) {
>                         mask = umask(0077);
>                         if (semanage_copy_file(path, path2, sb.st_mode,
> @@ -938,6 +944,7 @@ int semanage_mkdir(semanage_handle_t *sh, const char *path)
>
>                 }
>                 umask(mask);
> +               semanage_setfiles(path);
>         }
>         else {
>                 /* check that it really is a directory */
> @@ -1614,16 +1621,19 @@ static int semanage_validate_and_compile_fcontexts(semanage_handle_t * sh)
>                     semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC)) != 0) {
>                 goto cleanup;
>         }
> +       semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_BIN));
>
>         if (sefcontext_compile(sh,
>                     semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL)) != 0) {
>                 goto cleanup;
>         }
> +       semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL_BIN));
>
>         if (sefcontext_compile(sh,
>                     semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_HOMEDIRS)) != 0) {
>                 goto cleanup;
>         }
> +       semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_HOMEDIRS_BIN));
>
>         status = 0;
>  cleanup:
> @@ -3018,3 +3028,14 @@ int semanage_nc_sort(semanage_handle_t * sh, const char *buf, size_t buf_len,
>
>         return 0;
>  }
> +
> +/* Make sure the file context and ownership of files in the policy
> + * store does not change */
> +void semanage_setfiles(const char *path){
> +       /* Fix the user and role portions of the context, ignore errors
> +        * since this is not a critical operation */
> +       selinux_restorecon(path, SELINUX_RESTORECON_SET_SPECFILE_CTX | SELINUX_RESTORECON_IGNORE_NOENTRY);
> +       /* Make sure "path" is owned by root */
> +       if(geteuid() != 0 || getegid() != 0)
> +               chown(path, 0, 0);
> +}

Arguably should check stat.st_uid/gid from stat(2) of path although
perhaps it doesn't matter.
Need to make sure that these paths only exist in root-owned
directories and can't be used to trigger a chown of some other
arbitrary file to root ownership, e.g. some suid binary.
Maybe refuse to chown() if stat.st_mode & (S_IFREG|S_ISUID) ||
stat.st_mode & (S_IFREG|S_ISGID)?

> diff --git a/libsemanage/src/semanage_store.h b/libsemanage/src/semanage_store.h
> index 1fc77da8..e21dadeb 100644
> --- a/libsemanage/src/semanage_store.h
> +++ b/libsemanage/src/semanage_store.h
> @@ -124,6 +124,7 @@ int semanage_get_cil_paths(semanage_handle_t * sh, semanage_module_info_t *modin
>  int semanage_get_active_modules(semanage_handle_t *sh,
>                                semanage_module_info_t **modinfo, int *num_modules);
>
> +void semanage_setfiles(const char *path);
>
>  /* lock file routines */
>  int semanage_get_trans_lock(semanage_handle_t * sh);
> --
> 2.43.0
>
>
Stephen Smalley July 22, 2024, 7:19 p.m. UTC | #2
On Mon, Jul 22, 2024 at 11:11 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jul 19, 2024 at 9:29 AM Vit Mojzis <vmojzis@redhat.com> wrote:
> >
> > Make sure that file context (all parts) and ownership of
> > files/directories in policy store does not change no matter which user
> > and under which context executes policy rebuild.
> >
> > Fixes:
> >   # semodule -B
> >   # ls -lZ  /etc/selinux/targeted/contexts/files
> >
> > -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0 421397 Jul 11 09:57 file_contexts
> > -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0 593470 Jul 11 09:57 file_contexts.bin
> > -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0  14704 Jul 11 09:57 file_contexts.homedirs
> > -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0  20289 Jul 11 09:57 file_contexts.homedirs.bin
> >
> >   SELinux user changed from system_u to the user used to execute semodule
> >
> >   # capsh --user=testuser --caps="cap_dac_override,cap_chown+eip" --addamb=cap_dac_override,cap_chown -- -c "semodule -B"
> >   # ls -lZ  /etc/selinux/targeted/contexts/files
> >
> > -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0 421397 Jul 19 09:10 file_contexts
> > -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0 593470 Jul 19 09:10 file_contexts.bin
> > -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0  14704 Jul 19 09:10 file_contexts.homedirs
> > -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0  20289 Jul 19 09:10 file_contexts.homedirs.bin
> >
> >   Both file context and ownership changed -- causes remote login
> >   failures and other issues in some scenarios.
> >
> > Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
> > ---
> >  libsemanage/src/semanage_store.c | 23 ++++++++++++++++++++++-
> >  libsemanage/src/semanage_store.h |  1 +
> >  2 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> > index 27c5d349..12c30ad2 100644
> > --- a/libsemanage/src/semanage_store.c
> > +++ b/libsemanage/src/semanage_store.c
> > @@ -36,6 +36,7 @@ typedef struct dbase_policydb dbase_t;
> >  #include "database_policydb.h"
> >  #include "handle.h"
> >
> > +#include <selinux/restorecon.h>
> >  #include <selinux/selinux.h>
> >  #include <sepol/policydb.h>
> >  #include <sepol/module.h>
> > @@ -731,7 +732,7 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode,
> >
> >         if (!mode)
> >                 mode = S_IRUSR | S_IWUSR;
> > -
> > +
>
> We generally don't make unrelated whitespace changes in a patch.
>
> >         mask = umask(0);
> >         if ((out = open(tmp, O_WRONLY | O_CREAT | O_TRUNC, mode)) == -1) {
> >                 umask(mask);
> > @@ -767,6 +768,8 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode,
> >         if (!retval && rename(tmp, dst) == -1)
> >                 return -1;
> >
> > +       semanage_setfiles(dst);
> > +
> >  out:
> >         errno = errsv;
> >         return retval;
> > @@ -819,6 +822,8 @@ static int semanage_copy_dir_flags(const char *src, const char *dst, int flag)
> >                         goto cleanup;
> >                 }
> >                 umask(mask);
> > +
> > +               semanage_setfiles(dst);
> >         }
> >
> >         for (i = 0; i < len; i++) {
> > @@ -837,6 +842,7 @@ static int semanage_copy_dir_flags(const char *src, const char *dst, int flag)
> >                                 goto cleanup;
> >                         }
> >                         umask(mask);
> > +                       semanage_setfiles(path2);
> >                 } else if (S_ISREG(sb.st_mode) && flag == 1) {
> >                         mask = umask(0077);
> >                         if (semanage_copy_file(path, path2, sb.st_mode,
> > @@ -938,6 +944,7 @@ int semanage_mkdir(semanage_handle_t *sh, const char *path)
> >
> >                 }
> >                 umask(mask);
> > +               semanage_setfiles(path);
> >         }
> >         else {
> >                 /* check that it really is a directory */
> > @@ -1614,16 +1621,19 @@ static int semanage_validate_and_compile_fcontexts(semanage_handle_t * sh)
> >                     semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC)) != 0) {
> >                 goto cleanup;
> >         }
> > +       semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_BIN));
> >
> >         if (sefcontext_compile(sh,
> >                     semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL)) != 0) {
> >                 goto cleanup;
> >         }
> > +       semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL_BIN));
> >
> >         if (sefcontext_compile(sh,
> >                     semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_HOMEDIRS)) != 0) {
> >                 goto cleanup;
> >         }
> > +       semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_HOMEDIRS_BIN));
> >
> >         status = 0;
> >  cleanup:
> > @@ -3018,3 +3028,14 @@ int semanage_nc_sort(semanage_handle_t * sh, const char *buf, size_t buf_len,
> >
> >         return 0;
> >  }
> > +
> > +/* Make sure the file context and ownership of files in the policy
> > + * store does not change */
> > +void semanage_setfiles(const char *path){
> > +       /* Fix the user and role portions of the context, ignore errors
> > +        * since this is not a critical operation */
> > +       selinux_restorecon(path, SELINUX_RESTORECON_SET_SPECFILE_CTX | SELINUX_RESTORECON_IGNORE_NOENTRY);
> > +       /* Make sure "path" is owned by root */
> > +       if(geteuid() != 0 || getegid() != 0)
> > +               chown(path, 0, 0);
> > +}
>
> Arguably should check stat.st_uid/gid from stat(2) of path although
> perhaps it doesn't matter.
> Need to make sure that these paths only exist in root-owned
> directories and can't be used to trigger a chown of some other
> arbitrary file to root ownership, e.g. some suid binary.
> Maybe refuse to chown() if stat.st_mode & (S_IFREG|S_ISUID) ||
> stat.st_mode & (S_IFREG|S_ISGID)?

Sorry, I munged that - should be something like S_ISREG(sb.st_mode) &&
(sb.st_mode & (S_ISUID|S_ISGID)).
Obviously still subject to races unless you do something like fd =
open(path, O_PATH); fstat(fd, &sb); <test sb.st_mode>; fchown(fd, 0,
0);
Vit Mojzis July 23, 2024, 12:55 p.m. UTC | #3
On 7/22/24 21:19, Stephen Smalley wrote:
> On Mon, Jul 22, 2024 at 11:11 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Fri, Jul 19, 2024 at 9:29 AM Vit Mojzis <vmojzis@redhat.com> wrote:
>>> Make sure that file context (all parts) and ownership of
>>> files/directories in policy store does not change no matter which user
>>> and under which context executes policy rebuild.
>>>
>>> Fixes:
>>>    # semodule -B
>>>    # ls -lZ  /etc/selinux/targeted/contexts/files
>>>
>>> -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0 421397 Jul 11 09:57 file_contexts
>>> -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0 593470 Jul 11 09:57 file_contexts.bin
>>> -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0  14704 Jul 11 09:57 file_contexts.homedirs
>>> -rw-r--r--. 1 root root unconfined_u:object_r:file_context_t:s0  20289 Jul 11 09:57 file_contexts.homedirs.bin
>>>
>>>    SELinux user changed from system_u to the user used to execute semodule
>>>
>>>    # capsh --user=testuser --caps="cap_dac_override,cap_chown+eip" --addamb=cap_dac_override,cap_chown -- -c "semodule -B"
>>>    # ls -lZ  /etc/selinux/targeted/contexts/files
>>>
>>> -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0 421397 Jul 19 09:10 file_contexts
>>> -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0 593470 Jul 19 09:10 file_contexts.bin
>>> -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0  14704 Jul 19 09:10 file_contexts.homedirs
>>> -rw-r--r--. 1 testuser testuser unconfined_u:object_r:file_context_t:s0  20289 Jul 19 09:10 file_contexts.homedirs.bin
>>>
>>>    Both file context and ownership changed -- causes remote login
>>>    failures and other issues in some scenarios.
>>>
>>> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
>>> ---
>>>   libsemanage/src/semanage_store.c | 23 ++++++++++++++++++++++-
>>>   libsemanage/src/semanage_store.h |  1 +
>>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>>> index 27c5d349..12c30ad2 100644
>>> --- a/libsemanage/src/semanage_store.c
>>> +++ b/libsemanage/src/semanage_store.c
>>> @@ -36,6 +36,7 @@ typedef struct dbase_policydb dbase_t;
>>>   #include "database_policydb.h"
>>>   #include "handle.h"
>>>
>>> +#include <selinux/restorecon.h>
>>>   #include <selinux/selinux.h>
>>>   #include <sepol/policydb.h>
>>>   #include <sepol/module.h>
>>> @@ -731,7 +732,7 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode,
>>>
>>>          if (!mode)
>>>                  mode = S_IRUSR | S_IWUSR;
>>> -
>>> +
>> We generally don't make unrelated whitespace changes in a patch.
>>
>>>          mask = umask(0);
>>>          if ((out = open(tmp, O_WRONLY | O_CREAT | O_TRUNC, mode)) == -1) {
>>>                  umask(mask);
>>> @@ -767,6 +768,8 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode,
>>>          if (!retval && rename(tmp, dst) == -1)
>>>                  return -1;
>>>
>>> +       semanage_setfiles(dst);
>>> +
>>>   out:
>>>          errno = errsv;
>>>          return retval;
>>> @@ -819,6 +822,8 @@ static int semanage_copy_dir_flags(const char *src, const char *dst, int flag)
>>>                          goto cleanup;
>>>                  }
>>>                  umask(mask);
>>> +
>>> +               semanage_setfiles(dst);
>>>          }
>>>
>>>          for (i = 0; i < len; i++) {
>>> @@ -837,6 +842,7 @@ static int semanage_copy_dir_flags(const char *src, const char *dst, int flag)
>>>                                  goto cleanup;
>>>                          }
>>>                          umask(mask);
>>> +                       semanage_setfiles(path2);
>>>                  } else if (S_ISREG(sb.st_mode) && flag == 1) {
>>>                          mask = umask(0077);
>>>                          if (semanage_copy_file(path, path2, sb.st_mode,
>>> @@ -938,6 +944,7 @@ int semanage_mkdir(semanage_handle_t *sh, const char *path)
>>>
>>>                  }
>>>                  umask(mask);
>>> +               semanage_setfiles(path);
>>>          }
>>>          else {
>>>                  /* check that it really is a directory */
>>> @@ -1614,16 +1621,19 @@ static int semanage_validate_and_compile_fcontexts(semanage_handle_t * sh)
>>>                      semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC)) != 0) {
>>>                  goto cleanup;
>>>          }
>>> +       semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_BIN));
>>>
>>>          if (sefcontext_compile(sh,
>>>                      semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL)) != 0) {
>>>                  goto cleanup;
>>>          }
>>> +       semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL_BIN));
>>>
>>>          if (sefcontext_compile(sh,
>>>                      semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_HOMEDIRS)) != 0) {
>>>                  goto cleanup;
>>>          }
>>> +       semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_HOMEDIRS_BIN));
>>>
>>>          status = 0;
>>>   cleanup:
>>> @@ -3018,3 +3028,14 @@ int semanage_nc_sort(semanage_handle_t * sh, const char *buf, size_t buf_len,
>>>
>>>          return 0;
>>>   }
>>> +
>>> +/* Make sure the file context and ownership of files in the policy
>>> + * store does not change */
>>> +void semanage_setfiles(const char *path){
>>> +       /* Fix the user and role portions of the context, ignore errors
>>> +        * since this is not a critical operation */
>>> +       selinux_restorecon(path, SELINUX_RESTORECON_SET_SPECFILE_CTX | SELINUX_RESTORECON_IGNORE_NOENTRY);
>>> +       /* Make sure "path" is owned by root */
>>> +       if(geteuid() != 0 || getegid() != 0)
>>> +               chown(path, 0, 0);
>>> +}
>> Arguably should check stat.st_uid/gid from stat(2) of path although
>> perhaps it doesn't matter.
Did you mean so that chown is not used needlessly, or to protect from 
changing ownership of other users' files?

>> Need to make sure that these paths only exist in root-owned
>> directories and can't be used to trigger a chown of some other
>> arbitrary file to root ownership, e.g. some suid binary.
>> Maybe refuse to chown() if stat.st_mode & (S_IFREG|S_ISUID) ||
>> stat.st_mode & (S_IFREG|S_ISGID)?
Right,  that seems like a good idea. Thank you.

> Sorry, I munged that - should be something like S_ISREG(sb.st_mode) &&
> (sb.st_mode & (S_ISUID|S_ISGID)).
> Obviously still subject to races unless you do something like fd =
> open(path, O_PATH); fstat(fd, &sb); <test sb.st_mode>; fchown(fd, 0,
> 0);
>
diff mbox series

Patch

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 27c5d349..12c30ad2 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -36,6 +36,7 @@  typedef struct dbase_policydb dbase_t;
 #include "database_policydb.h"
 #include "handle.h"
 
+#include <selinux/restorecon.h>
 #include <selinux/selinux.h>
 #include <sepol/policydb.h>
 #include <sepol/module.h>
@@ -731,7 +732,7 @@  int semanage_copy_file(const char *src, const char *dst, mode_t mode,
 
 	if (!mode)
 		mode = S_IRUSR | S_IWUSR;
-	
+
 	mask = umask(0);
 	if ((out = open(tmp, O_WRONLY | O_CREAT | O_TRUNC, mode)) == -1) {
 		umask(mask);
@@ -767,6 +768,8 @@  int semanage_copy_file(const char *src, const char *dst, mode_t mode,
 	if (!retval && rename(tmp, dst) == -1)
 		return -1;
 
+	semanage_setfiles(dst);
+
 out:
 	errno = errsv;
 	return retval;
@@ -819,6 +822,8 @@  static int semanage_copy_dir_flags(const char *src, const char *dst, int flag)
 			goto cleanup;
 		}
 		umask(mask);
+
+		semanage_setfiles(dst);
 	}
 
 	for (i = 0; i < len; i++) {
@@ -837,6 +842,7 @@  static int semanage_copy_dir_flags(const char *src, const char *dst, int flag)
 				goto cleanup;
 			}
 			umask(mask);
+			semanage_setfiles(path2);
 		} else if (S_ISREG(sb.st_mode) && flag == 1) {
 			mask = umask(0077);
 			if (semanage_copy_file(path, path2, sb.st_mode,
@@ -938,6 +944,7 @@  int semanage_mkdir(semanage_handle_t *sh, const char *path)
 
 		}
 		umask(mask);
+		semanage_setfiles(path);
 	}
 	else {
 		/* check that it really is a directory */
@@ -1614,16 +1621,19 @@  static int semanage_validate_and_compile_fcontexts(semanage_handle_t * sh)
 		    semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC)) != 0) {
 		goto cleanup;
 	}
+	semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_BIN));
 
 	if (sefcontext_compile(sh,
 		    semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL)) != 0) {
 		goto cleanup;
 	}
+	semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL_BIN));
 
 	if (sefcontext_compile(sh,
 		    semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_HOMEDIRS)) != 0) {
 		goto cleanup;
 	}
+	semanage_setfiles(semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_HOMEDIRS_BIN));
 
 	status = 0;
 cleanup:
@@ -3018,3 +3028,14 @@  int semanage_nc_sort(semanage_handle_t * sh, const char *buf, size_t buf_len,
 
 	return 0;
 }
+
+/* Make sure the file context and ownership of files in the policy
+ * store does not change */
+void semanage_setfiles(const char *path){
+	/* Fix the user and role portions of the context, ignore errors
+	 * since this is not a critical operation */
+	selinux_restorecon(path, SELINUX_RESTORECON_SET_SPECFILE_CTX | SELINUX_RESTORECON_IGNORE_NOENTRY);
+	/* Make sure "path" is owned by root */
+	if(geteuid() != 0 || getegid() != 0)
+		chown(path, 0, 0);
+}
diff --git a/libsemanage/src/semanage_store.h b/libsemanage/src/semanage_store.h
index 1fc77da8..e21dadeb 100644
--- a/libsemanage/src/semanage_store.h
+++ b/libsemanage/src/semanage_store.h
@@ -124,6 +124,7 @@  int semanage_get_cil_paths(semanage_handle_t * sh, semanage_module_info_t *modin
 int semanage_get_active_modules(semanage_handle_t *sh,
 			       semanage_module_info_t **modinfo, int *num_modules);
 
+void semanage_setfiles(const char *path);
 
 /* lock file routines */
 int semanage_get_trans_lock(semanage_handle_t * sh);