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

Vit Mojzis <vmojzis@xxxxxxxxxx> · Tue, 23 Jul 2024 14:55:18 +0200

On 7/22/24 21:19, Stephen Smalley wrote:
On Mon, Jul 22, 2024 at 11:11 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
On Fri, Jul 19, 2024 at 9:29 AM Vit Mojzis <vmojzis@xxxxxxxxxx> 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@xxxxxxxxxx>
---
  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);









