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). 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.
Vit
