On Mon, 2017-06-26 at 14:38 +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 <[email protected]>
Also, even with all three patches applied, there are still a number of
residual access() calls within semanage_store.c. Is that intentional?
> ---
> libsemanage/src/direct_api.c | 50 +++++++++++++++++++++++++++++-----
> ----------
> 1 file changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/libsemanage/src/direct_api.c
> b/libsemanage/src/direct_api.c
> index ed11a7c..3d1c354 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -296,10 +296,19 @@ 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)
> sepol_set_disable_dontaudit(sh->sepolh, 1);
> - else
> - sepol_set_disable_dontaudit(sh->sepolh, 0);
> + else{
> + if (errno == ENOENT){
> + sepol_set_disable_dontaudit(sh->sepolh, 0);
> + }
> + else{
> + ERR(sh, "Unable to access disable_dontaudit
> file at %s: %s\n",path , strerror(errno));
> + goto err;
> + }
> + }
>
> return STATUS_SUCCESS;
>
> @@ -1114,6 +1123,7 @@ static int
> semanage_compile_hll_modules(semanage_handle_t *sh,
> int status = 0;
> int i;
> char cil_path[PATH_MAX];
> + struct stat sb;
>
> assert(sh);
> assert(modinfos);
> @@ -1130,9 +1140,12 @@ static int
> semanage_compile_hll_modules(semanage_handle_t *sh,
> }
>
> if (semanage_get_ignore_module_cache(sh) == 0 &&
> - access(cil_path, F_OK) == 0) {
> + (status = stat(cil_path, &sb)) == 0)
> {
> continue;
> }
> + if (status != 0 && status != ENOENT) {
> + goto cleanup; //an error in the "stat" call
> + }
>
> status = semanage_compile_module(sh, &modinfos[i]);
> if (status < 0) {
> @@ -1200,7 +1213,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);
> @@ -1225,7 +1239,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);
> @@ -1266,37 +1280,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;
> }
> @@ -1431,7 +1445,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_path(SE
> MANAGE_TMP,
> SE
> MANAGE_STORE_SEUSERS),
> @@ -1444,7 +1458,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_path(SE
> MANAGE_TMP,
> SE
> MANAGE_USERS_EXTRA),
> @@ -1785,7 +1799,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;
> @@ -1815,7 +1830,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;
> @@ -2790,7 +2805,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));
