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?
> 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.
