On Fri, 2017-05-19 at 14:22 +0200, Vit Mojzis wrote: > On 5.5.2017 22:32, Stephen Smalley wrote: > > On Fri, 2017-05-05 at 14:49 +0200, Vit Mojzis wrote: > > > access() uses real UID instead of effective UID which causes > > > false > > > negative checks in setuid programs. > > > Replace access(,F_OK) (i.e. tests for file existence) by stat(). > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 > > > > > > Signed-off-by: Vit Mojzis <vmojzis@xxxxxxxxxx> > > > --- > > > libsemanage/src/direct_api.c | 36 +++++++++++++++++++++------ > > > ------- > > > -- > > > 1 file changed, 21 insertions(+), 15 deletions(-) > > > > > > diff --git a/libsemanage/src/direct_api.c > > > b/libsemanage/src/direct_api.c > > > index 508277d..35ca1b0 100644 > > > --- a/libsemanage/src/direct_api.c > > > +++ b/libsemanage/src/direct_api.c > > > @@ -272,7 +272,9 @@ int semanage_direct_connect(semanage_handle_t > > > * > > > sh) > > > > > > /* set the disable dontaudit value */ > > > path = semanage_path(SEMANAGE_ACTIVE, > > > SEMANAGE_DISABLE_DONTAUDIT); > > > - if (access(path, F_OK) == 0) > > > + > > > + struct stat sb; > > > + if (stat(path, &sb) == 0) > > > > Need to check errno as well to ensure that it is just ENOENT when > > stat() returns non-zero; anything else is an error. > > Thank you for the feedback. I was trying to keep behavioral changes > to a > minimum. In case "access" call failed (it can encounter all the > errors > that "stat" can except for EOVERFLOW) the code assumed that the file > doesn't exist and this patch preserves said behavior. > But if you think think that the calling function should fail instead, > I > can rework the patch. Except that use of stat() does introduce one additional potential failure case, i.e. SELinux getattr permission denial on the file. Unlikely, of course, but possible. And since you removed the semanage_access_check() in the earlier patch, we are no longer checking that we can even read/search the active directory prior to making this call. So our first permission denial (on either the active directory or the file itself) might occur here, and we would wrongly interpret it as the file not existing after your change. It is also more correct to explicitly test errno for ENOENT regardless, so we might as well fix that while we are making these changes. > > stat() can produce a SELinux getattr denial, whereas access(F_OK) > > doesn't perform any SELinux check beyond directory search access. > > That is a fair point and I don't have a solution for it. However in > my > understanding the calling process should have the "getattr" access > permission (and it should even be able to create the file in most > cases). Yes, I'm not concerned about needing getattr permission itself; I just want to ensure that we correctly handle it when it is denied. > > > > > sepol_set_disable_dontaudit(sh->sepolh, 1); > > > else > > > sepol_set_disable_dontaudit(sh->sepolh, 0); > > > @@ -1101,8 +1103,9 @@ static int > > > semanage_compile_hll_modules(semanage_handle_t *sh, > > > goto cleanup; > > > } > > > > > > + struct stat sb; > > > if (semanage_get_ignore_module_cache(sh) == 0 > > > && > > > - access(cil_path, F_OK) == 0) { > > > + stat(cil_path, &sb) == 0) { > > > continue; > > > } > > > > > > @@ -1165,7 +1168,8 @@ static int > > > semanage_direct_commit(semanage_handle_t * sh) > > > > > > /* Create or remove the disable_dontaudit flag file. */ > > > path = semanage_path(SEMANAGE_TMP, > > > SEMANAGE_DISABLE_DONTAUDIT); > > > - if (access(path, F_OK) == 0) > > > + struct stat sb; > > > + if (stat(path, &sb) == 0) > > > do_rebuild |= !(sepol_get_disable_dontaudit(sh- > > > > sepolh) == 1); > > > > > > else > > > do_rebuild |= (sepol_get_disable_dontaudit(sh- > > > > sepolh) == 1); > > > > > > @@ -1190,7 +1194,7 @@ static int > > > semanage_direct_commit(semanage_handle_t * sh) > > > > > > /* Create or remove the preserve_tunables flag file. */ > > > path = semanage_path(SEMANAGE_TMP, > > > SEMANAGE_PRESERVE_TUNABLES); > > > - if (access(path, F_OK) == 0) > > > + if (stat(path, &sb) == 0) > > > do_rebuild |= !(sepol_get_preserve_tunables(sh- > > > > sepolh) == 1); > > > > > > else > > > do_rebuild |= (sepol_get_preserve_tunables(sh- > > > > sepolh) == 1); > > > > > > @@ -1231,37 +1235,37 @@ static int > > > semanage_direct_commit(semanage_handle_t * sh) > > > */ > > > if (!do_rebuild) { > > > path = semanage_path(SEMANAGE_TMP, > > > SEMANAGE_STORE_KERNEL); > > > - if (access(path, F_OK) != 0) { > > > + if (stat(path, &sb) != 0) { > > > do_rebuild = 1; > > > goto rebuild; > > > } > > > > > > path = semanage_path(SEMANAGE_TMP, > > > SEMANAGE_STORE_FC); > > > - if (access(path, F_OK) != 0) { > > > + if (stat(path, &sb) != 0) { > > > do_rebuild = 1; > > > goto rebuild; > > > } > > > > > > path = semanage_path(SEMANAGE_TMP, > > > SEMANAGE_STORE_SEUSERS); > > > - if (access(path, F_OK) != 0) { > > > + if (stat(path, &sb) != 0) { > > > do_rebuild = 1; > > > goto rebuild; > > > } > > > > > > path = semanage_path(SEMANAGE_TMP, > > > SEMANAGE_LINKED); > > > - if (access(path, F_OK) != 0) { > > > + if (stat(path, &sb) != 0) { > > > do_rebuild = 1; > > > goto rebuild; > > > } > > > > > > path = semanage_path(SEMANAGE_TMP, > > > SEMANAGE_SEUSERS_LINKED); > > > - if (access(path, F_OK) != 0) { > > > + if (stat(path, &sb) != 0) { > > > do_rebuild = 1; > > > goto rebuild; > > > } > > > > > > path = semanage_path(SEMANAGE_TMP, > > > SEMANAGE_USERS_EXTRA_LINKED); > > > - if (access(path, F_OK) != 0) { > > > + if (stat(path, &sb) != 0) { > > > do_rebuild = 1; > > > goto rebuild; > > > } > > > @@ -1395,7 +1399,7 @@ rebuild: > > > goto cleanup; > > > > > > path = semanage_path(SEMANAGE_TMP, > > > SEMANAGE_SEUSERS_LINKED); > > > - if (access(path, F_OK) == 0) { > > > + if (stat(path, &sb) == 0) { > > > retval = semanage_copy_file(path, > > > semanage_pa > > > th(SE > > > MANAGE_TMP, > > > > > > SE > > > MANAGE_STORE_SEUSERS), > > > @@ -1408,7 +1412,7 @@ rebuild: > > > } > > > > > > path = semanage_path(SEMANAGE_TMP, > > > SEMANAGE_USERS_EXTRA_LINKED); > > > - if (access(path, F_OK) == 0) { > > > + if (stat(path, &sb) == 0) { > > > retval = semanage_copy_file(path, > > > semanage_pa > > > th(SE > > > MANAGE_TMP, > > > > > > SE > > > MANAGE_USERS_EXTRA), > > > @@ -1732,7 +1736,8 @@ static int > > > semanage_direct_extract(semanage_handle_t * sh, > > > goto cleanup; > > > } > > > > > > - if (access(module_path, F_OK) != 0) { > > > + struct stat sb; > > > + if (stat(module_path, &sb) != 0) { > > > ERR(sh, "Module does not exist: %s", > > > module_path); > > > rc = -1; > > > goto cleanup; > > > @@ -1762,7 +1767,7 @@ static int > > > semanage_direct_extract(semanage_handle_t * sh, > > > goto cleanup; > > > } > > > > > > - if (extract_cil == 1 && strcmp(_modinfo->lang_ext, > > > "cil") && > > > access(input_file, F_OK) != 0) { > > > + if (extract_cil == 1 && strcmp(_modinfo->lang_ext, > > > "cil") && > > > stat(input_file, &sb) != 0) { > > > rc = semanage_compile_module(sh, _modinfo); > > > if (rc < 0) { > > > goto cleanup; > > > @@ -2737,7 +2742,8 @@ static int > > > semanage_direct_install_info(semanage_handle_t *sh, > > > goto cleanup; > > > } > > > > > > - if (access(path, F_OK) == 0) { > > > + struct stat sb; > > > + if (stat(path, &sb) == 0) { > > > ret = unlink(path); > > > if (ret != 0) { > > > ERR(sh, "Error while removing > > > cached > > > CIL file %s: %s", path, strerror(errno));
