| Message ID | 20240723125850.1228121-1-vmojzis@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | [v3] libsemanage: Preserve file context and ownership in policy store | expand |
On Tue, Jul 23, 2024 at 9:09 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> > --- > @@ -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)?
On 7/23/24 16:56, Stephen Smalley wrote: > On Tue, Jul 23, 2024 at 9:09 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> >> --- >> @@ -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
On Tue, Jul 23, 2024 at 12:54 PM Vit Mojzis <vmojzis@redhat.com> wrote: > > > > On 7/23/24 16:56, Stephen Smalley wrote: > > On Tue, Jul 23, 2024 at 9:09 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> > >> --- > >> @@ -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.
On Tue, Jul 23, 2024 at 9:09 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> > --- > @@ -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); I think you meant to wrap the body of the above if statement with { } so that the chown() only executes if geteuid() != 0 or getegid() != 0 too. Yes? This isn't Python ;) > +}
On 7/23/24 19:24, Stephen Smalley wrote: > On Tue, Jul 23, 2024 at 12:54 PM Vit Mojzis <vmojzis@redhat.com> wrote: >> >> >> On 7/23/24 16:56, Stephen Smalley wrote: >>> On Tue, Jul 23, 2024 at 9:09 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> >>>> --- >>>> @@ -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. >
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c index 27c5d349..a7dd1f6f 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> @@ -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,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); +} 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);
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 | 28 ++++++++++++++++++++++++++++ libsemanage/src/semanage_store.h | 1 + 2 files changed, 29 insertions(+)