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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 7/23/24 19:24, Stephen Smalley wrote:
On Tue, Jul 23, 2024 at 12:54 PM Vit Mojzis <vmojzis@xxxxxxxxxx> wrote:


On 7/23/24 16:56, Stephen Smalley wrote:
On Tue, Jul 23, 2024 at 9:09 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>
---
@@ -3018,3 +3028,21 @@ 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){
+       struct stat sb;
+
+       /* 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)
+               /* Skip files with the SUID or SGID bit set -- abuse protection */
+               if ((stat(path, &sb) == -1) ||
+                   (S_ISREG(sb.st_mode) && (sb.st_mode & (S_ISUID | S_ISGID))))
+                               return;
+               chown(path, 0, 0);
+}
Did you consider the fd = open(path, O_PATH); fstat(fd, &sb); ...
fchown(fd, 0, 0); pattern to avoid a race between the check and chown
(e.g. link changed from one file to another in between)?

Briefly, the code would be a bit less readable (interweaving writing
file content and ownership/labeling) and we'd still need the current
approach for any file created by another binary (e.g. sefcontext_compile).
Not sure I understand that last part - why can't you do the same
open(path, O_PATH); fstat(fd, &sb); ... fchown(fd, 0, 0); for files
created by a helper program - just do it in the parent after the child
exits?

Sorry, I misunderstood. I thought you wanted to add the fchown into existing code that opens the file. I actually tried that in semanage_copy_file, but chown to root caused the rename to fail (and selinux_restorecon had to be changed to setfscreatecon because of permissions as well) so I had to go
back to fixing everything after the rename.
Also, I had to switch O_PATH for O_RDONLY since O_PATH does not permit fchown.

Good catch with the curly braces after "if". What a rookie mistake.


I'll rewrite it if you prefer that approach, but do you expect such
races to be common? The whole ownership issue already seems like a
corner-case.
Shrug. That sort of race has been exploited in the past to relabel or
chown a file of your choosing by switching out one symlink for another
at the last minute but admittedly we are doing the restorecon by path
so your code is consistent.
No big deal to me either way.






[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux