[3/3] libsemanage: replace access() checks to make setuid programs work

Commit Message

Vit Mojzis Feb. 28, 2018, 10:15 a.m. UTC

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().
And access(,R_OK) by fopen(,"r")

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431

Signed-off-by: Vit Mojzis <[e-mail]>
---
 libsemanage/src/direct_api.c     | 132 +++++++++++++++++++++++++--------------
 libsemanage/src/semanage_store.c |  14 ++++-
 2 files changed, 98 insertions(+), 48 deletions(-)

Comments

William Roberts Feb. 28, 2018, 5:50 p.m. UTC | #1

On Wed, Feb 28, 2018 at 2:15 AM, Vit Mojzis <[e-mail]> 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().
> And access(,R_OK) by fopen(,"r")
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
>
> Signed-off-by: Vit Mojzis <[e-mail]>
> ---
>  libsemanage/src/direct_api.c     | 132 +++++++++++++++++++++++++--------------
>  libsemanage/src/semanage_store.c |  14 ++++-
>  2 files changed, 98 insertions(+), 48 deletions(-)
>
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index f4d57cf8..d42a51cd 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>  int semanage_direct_connect(semanage_handle_t * sh)
>  {
>         const char *path;
> +       struct stat sb;
>
>         if (semanage_check_init(sh, sh->conf->store_root_path))
>                 goto err;
> @@ -302,10 +303,17 @@ 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)
> +
> +       if (stat(path, &sb) == 0)
>                 sepol_set_disable_dontaudit(sh->sepolh, 1);
> -       else
> +       else {
> +               if (errno != ENOENT) {
> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +                       goto err;
> +               }
> +
>                 sepol_set_disable_dontaudit(sh->sepolh, 0);
> +       }
>
>         return STATUS_SUCCESS;
>
> @@ -1139,6 +1147,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);
> @@ -1155,9 +1164,13 @@ 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 && errno != ENOENT) {
> +                       ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
> +                       goto cleanup; //an error in the "stat" call
> +               }
>
>                 status = semanage_compile_module(sh, &modinfos[i]);
>                 if (status < 0) {
> @@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>         struct cil_db *cildb = NULL;
>         semanage_module_info_t *modinfos = NULL;
>         mode_t mask = umask(0077);
> +       struct stat sb;
>
>         int do_rebuild, do_write_kernel, do_install;
>         int fcontexts_modified, ports_modified, seusers_modified,
> @@ -1226,10 +1240,16 @@ 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)
> +       if (stat(path, &sb) == 0)
>                 do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
> -       else
> +       else {
> +               if (errno != ENOENT) {
> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +                       goto cleanup;
> +               }
> +

I am not a huge fan of this if under else block of if (errno !=ENOENT).

maybe this is a bit cleaner:

if (stat() == 0)
  //exists
else if (errno == ENOENT)
  // doesn't exist
else
  //fail

>                 do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
> +       }
>         if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>                 FILE *touch;
>                 touch = fopen(path, "w");
> @@ -1251,10 +1271,17 @@ 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
> +       else {
> +               if (errno != ENOENT) {
> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +                       goto cleanup;
> +               }
> +
>                 do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
> +       }
> +
>         if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>                 FILE *touch;
>                 touch = fopen(path, "w");
> @@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>          * a rebuild.
>          */
>         if (!do_rebuild) {
> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
> -               if (access(path, F_OK) != 0) {
> -                       do_rebuild = 1;
> -                       goto rebuild;
> -               }
> -
> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
> -               if (access(path, F_OK) != 0) {
> -                       do_rebuild = 1;
> -                       goto rebuild;
> -               }
> -
> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
> -               if (access(path, F_OK) != 0) {
> -                       do_rebuild = 1;
> -                       goto rebuild;
> -               }
> -
> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
> -               if (access(path, F_OK) != 0) {
> -                       do_rebuild = 1;
> -                       goto rebuild;
> -               }
> -
> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> -               if (access(path, F_OK) != 0) {
> -                       do_rebuild = 1;
> -                       goto rebuild;
> -               }
> +               int files[] = {SEMANAGE_STORE_KERNEL,
> +                                          SEMANAGE_STORE_FC,
> +                                          SEMANAGE_STORE_SEUSERS,
> +                                          SEMANAGE_LINKED,
> +                                          SEMANAGE_SEUSERS_LINKED,
> +                                          SEMANAGE_USERS_EXTRA_LINKED};
> +
> +               for (i = 0; i < (int) sizeof(files); i++) {
> +                       path = semanage_path(SEMANAGE_TMP, files[i]);
> +                       if (stat(path, &sb) != 0) {
> +                               if (errno != ENOENT) {
> +                                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +                                       goto cleanup;
> +                               }
>
> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> -               if (access(path, F_OK) != 0) {
> -                       do_rebuild = 1;
> -                       goto rebuild;
> +                               do_rebuild = 1;
> +                               goto rebuild;
> +                       }
>                 }
>         }
>
> @@ -1457,7 +1468,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(SEMANAGE_TMP,
>                                                                   SEMANAGE_STORE_SEUSERS),
> @@ -1466,11 +1477,16 @@ rebuild:
>                                 goto cleanup;
>                         pseusers->dtable->drop_cache(pseusers->dbase);
>                 } else {
> +                       if (errno != ENOENT) {
> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +                               goto cleanup;
> +                       }
> +
>                         pseusers->dtable->clear(sh, pseusers->dbase);
>                 }
>
>                 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(SEMANAGE_TMP,
>                                                                   SEMANAGE_USERS_EXTRA),
> @@ -1479,6 +1495,11 @@ rebuild:
>                                 goto cleanup;
>                         pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>                 } else {
> +                       if (errno != ENOENT) {
> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +                               goto cleanup;
> +                       }
> +
>                         pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>                 }
>         }
> @@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>         ssize_t _data_len;
>         char *_data;
>         int compressed;
> +       struct stat sb;
>
>         /* get path of module */
>         rc = semanage_module_get_path(
> @@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>                 goto cleanup;
>         }
>
> -       if (access(module_path, F_OK) != 0) {
> -               ERR(sh, "Module does not exist: %s", module_path);
> +       if (stat(module_path, &sb) != 0) {
> +               ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>                 rc = -1;
>                 goto cleanup;
>         }
> @@ -1851,7 +1873,12 @@ 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) {
> +               if (errno != ENOENT) {
> +                       ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
> +                       goto cleanup;
> +               }
> +
>                 rc = semanage_compile_module(sh, _modinfo);
>                 if (rc < 0) {
>                         goto cleanup;
> @@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>         }
>
>         if (stat(path, &sb) < 0) {
> +               if (errno != ENOENT) {
> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +                       status = -1;
> +                       goto cleanup;
> +               }
> +
>                 *enabled = 1;
>         }
>         else {
> @@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>
>         /* set enabled/disabled status */
>         if (stat(fn, &sb) < 0) {
> +               if (errno != ENOENT) {
> +                       ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
> +                       status = -1;
> +                       goto cleanup;
> +               }
> +
>                 ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>                 if (ret != 0) {
>                         status = -1;
> @@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>         int status = 0;
>         int ret = 0;
>         int type;
> +       struct stat sb;
>
>         char path[PATH_MAX];
>         mode_t mask = umask(0077);
> @@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>                         goto cleanup;
>                 }
>
> -               if (access(path, F_OK) == 0) {
> +               if (stat(path, &sb) == 0) {
>                         ret = unlink(path);
>                         if (ret != 0) {
>                                 ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 4bd1d651..83c188dc 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>  {
>         char *semanage_conf = NULL;
>         int len;
> +       FILE *fp = NULL;
>
>         len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>         semanage_conf = calloc(len + 1, sizeof(char));
> @@ -522,7 +523,10 @@ char *semanage_conf_path(void)
>         snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>                  SEMANAGE_CONF_FILE);
>
> -       if (access(semanage_conf, R_OK) != 0) {
> +       //the following replaces access(semanage_conf, R_OK) check
> +       if ((fp = fopen(semanage_conf, "r")) != NULL) {
> +               fclose(fp);
> +       } else {
>                 snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>         }
>
> @@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>
>         int r;
> +       struct stat sb;
> +
> +       if (stat(path, &sb) < 0) {
> +               if (errno != ENOENT) {
> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +                       return -1;
> +               }
>
> -       if (access(path, F_OK) != 0) {
>                 return 0;
>         }
>
> --
> 2.14.3
>
>

William Roberts Feb. 28, 2018, 6:24 p.m. UTC | #2

Where is patch 2/2, I have yet to see it?

Did something get screwy and is it: [PATCH] libsemanage: Improve
warning for installing disabled module


On Wed, Feb 28, 2018 at 9:50 AM, William Roberts
<[e-mail]> wrote:
> On Wed, Feb 28, 2018 at 2:15 AM, Vit Mojzis <[e-mail]> 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().
>> And access(,R_OK) by fopen(,"r")
>>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
>>
>> Signed-off-by: Vit Mojzis <[e-mail]>
>> ---
>>  libsemanage/src/direct_api.c     | 132 +++++++++++++++++++++++++--------------
>>  libsemanage/src/semanage_store.c |  14 ++++-
>>  2 files changed, 98 insertions(+), 48 deletions(-)
>>
>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>> index f4d57cf8..d42a51cd 100644
>> --- a/libsemanage/src/direct_api.c
>> +++ b/libsemanage/src/direct_api.c
>> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>>  int semanage_direct_connect(semanage_handle_t * sh)
>>  {
>>         const char *path;
>> +       struct stat sb;
>>
>>         if (semanage_check_init(sh, sh->conf->store_root_path))
>>                 goto err;
>> @@ -302,10 +303,17 @@ 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)
>> +
>> +       if (stat(path, &sb) == 0)
>>                 sepol_set_disable_dontaudit(sh->sepolh, 1);
>> -       else
>> +       else {
>> +               if (errno != ENOENT) {
>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +                       goto err;
>> +               }
>> +
>>                 sepol_set_disable_dontaudit(sh->sepolh, 0);
>> +       }
>>
>>         return STATUS_SUCCESS;
>>
>> @@ -1139,6 +1147,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);
>> @@ -1155,9 +1164,13 @@ 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 && errno != ENOENT) {
>> +                       ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
>> +                       goto cleanup; //an error in the "stat" call
>> +               }
>>
>>                 status = semanage_compile_module(sh, &modinfos[i]);
>>                 if (status < 0) {
>> @@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>         struct cil_db *cildb = NULL;
>>         semanage_module_info_t *modinfos = NULL;
>>         mode_t mask = umask(0077);
>> +       struct stat sb;
>>
>>         int do_rebuild, do_write_kernel, do_install;
>>         int fcontexts_modified, ports_modified, seusers_modified,
>> @@ -1226,10 +1240,16 @@ 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)
>> +       if (stat(path, &sb) == 0)
>>                 do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
>> -       else
>> +       else {
>> +               if (errno != ENOENT) {
>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +                       goto cleanup;
>> +               }
>> +
>
> I am not a huge fan of this if under else block of if (errno !=ENOENT).
>
> maybe this is a bit cleaner:
>
> if (stat() == 0)
>   //exists
> else if (errno == ENOENT)
>   // doesn't exist
> else
>   //fail
>
>>                 do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
>> +       }
>>         if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>>                 FILE *touch;
>>                 touch = fopen(path, "w");
>> @@ -1251,10 +1271,17 @@ 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
>> +       else {
>> +               if (errno != ENOENT) {
>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +                       goto cleanup;
>> +               }
>> +
>>                 do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
>> +       }
>> +
>>         if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>>                 FILE *touch;
>>                 touch = fopen(path, "w");
>> @@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>          * a rebuild.
>>          */
>>         if (!do_rebuild) {
>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
>> -               if (access(path, F_OK) != 0) {
>> -                       do_rebuild = 1;
>> -                       goto rebuild;
>> -               }
>> -
>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
>> -               if (access(path, F_OK) != 0) {
>> -                       do_rebuild = 1;
>> -                       goto rebuild;
>> -               }
>> -
>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
>> -               if (access(path, F_OK) != 0) {
>> -                       do_rebuild = 1;
>> -                       goto rebuild;
>> -               }
>> -
>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
>> -               if (access(path, F_OK) != 0) {
>> -                       do_rebuild = 1;
>> -                       goto rebuild;
>> -               }
>> -
>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>> -               if (access(path, F_OK) != 0) {
>> -                       do_rebuild = 1;
>> -                       goto rebuild;
>> -               }
>> +               int files[] = {SEMANAGE_STORE_KERNEL,
>> +                                          SEMANAGE_STORE_FC,
>> +                                          SEMANAGE_STORE_SEUSERS,
>> +                                          SEMANAGE_LINKED,
>> +                                          SEMANAGE_SEUSERS_LINKED,
>> +                                          SEMANAGE_USERS_EXTRA_LINKED};
>> +
>> +               for (i = 0; i < (int) sizeof(files); i++) {
>> +                       path = semanage_path(SEMANAGE_TMP, files[i]);
>> +                       if (stat(path, &sb) != 0) {
>> +                               if (errno != ENOENT) {
>> +                                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +                                       goto cleanup;
>> +                               }
>>
>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>> -               if (access(path, F_OK) != 0) {
>> -                       do_rebuild = 1;
>> -                       goto rebuild;
>> +                               do_rebuild = 1;
>> +                               goto rebuild;
>> +                       }
>>                 }
>>         }
>>
>> @@ -1457,7 +1468,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(SEMANAGE_TMP,
>>                                                                   SEMANAGE_STORE_SEUSERS),
>> @@ -1466,11 +1477,16 @@ rebuild:
>>                                 goto cleanup;
>>                         pseusers->dtable->drop_cache(pseusers->dbase);
>>                 } else {
>> +                       if (errno != ENOENT) {
>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +                               goto cleanup;
>> +                       }
>> +
>>                         pseusers->dtable->clear(sh, pseusers->dbase);
>>                 }
>>
>>                 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(SEMANAGE_TMP,
>>                                                                   SEMANAGE_USERS_EXTRA),
>> @@ -1479,6 +1495,11 @@ rebuild:
>>                                 goto cleanup;
>>                         pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>>                 } else {
>> +                       if (errno != ENOENT) {
>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +                               goto cleanup;
>> +                       }
>> +
>>                         pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>>                 }
>>         }
>> @@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>         ssize_t _data_len;
>>         char *_data;
>>         int compressed;
>> +       struct stat sb;
>>
>>         /* get path of module */
>>         rc = semanage_module_get_path(
>> @@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>                 goto cleanup;
>>         }
>>
>> -       if (access(module_path, F_OK) != 0) {
>> -               ERR(sh, "Module does not exist: %s", module_path);
>> +       if (stat(module_path, &sb) != 0) {
>> +               ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>>                 rc = -1;
>>                 goto cleanup;
>>         }
>> @@ -1851,7 +1873,12 @@ 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) {
>> +               if (errno != ENOENT) {
>> +                       ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
>> +                       goto cleanup;
>> +               }
>> +
>>                 rc = semanage_compile_module(sh, _modinfo);
>>                 if (rc < 0) {
>>                         goto cleanup;
>> @@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>>         }
>>
>>         if (stat(path, &sb) < 0) {
>> +               if (errno != ENOENT) {
>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +                       status = -1;
>> +                       goto cleanup;
>> +               }
>> +
>>                 *enabled = 1;
>>         }
>>         else {
>> @@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>
>>         /* set enabled/disabled status */
>>         if (stat(fn, &sb) < 0) {
>> +               if (errno != ENOENT) {
>> +                       ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
>> +                       status = -1;
>> +                       goto cleanup;
>> +               }
>> +
>>                 ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>>                 if (ret != 0) {
>>                         status = -1;
>> @@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>         int status = 0;
>>         int ret = 0;
>>         int type;
>> +       struct stat sb;
>>
>>         char path[PATH_MAX];
>>         mode_t mask = umask(0077);
>> @@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>                         goto cleanup;
>>                 }
>>
>> -               if (access(path, F_OK) == 0) {
>> +               if (stat(path, &sb) == 0) {
>>                         ret = unlink(path);
>>                         if (ret != 0) {
>>                                 ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>> index 4bd1d651..83c188dc 100644
>> --- a/libsemanage/src/semanage_store.c
>> +++ b/libsemanage/src/semanage_store.c
>> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>>  {
>>         char *semanage_conf = NULL;
>>         int len;
>> +       FILE *fp = NULL;
>>
>>         len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>>         semanage_conf = calloc(len + 1, sizeof(char));
>> @@ -522,7 +523,10 @@ char *semanage_conf_path(void)
>>         snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>>                  SEMANAGE_CONF_FILE);
>>
>> -       if (access(semanage_conf, R_OK) != 0) {
>> +       //the following replaces access(semanage_conf, R_OK) check
>> +       if ((fp = fopen(semanage_conf, "r")) != NULL) {
>> +               fclose(fp);
>> +       } else {
>>                 snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>>         }
>>
>> @@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>>
>>         int r;
>> +       struct stat sb;
>> +
>> +       if (stat(path, &sb) < 0) {
>> +               if (errno != ENOENT) {
>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +                       return -1;
>> +               }
>>
>> -       if (access(path, F_OK) != 0) {
>>                 return 0;
>>         }
>>
>> --
>> 2.14.3
>>
>>
>
>
>
> --
> Respectfully,
>
> William C Roberts

Stephen Smalley Feb. 28, 2018, 6:41 p.m. UTC | #3

On 02/28/2018 05:15 AM, 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().
> And access(,R_OK) by fopen(,"r")
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
> 
> Signed-off-by: Vit Mojzis <[e-mail]>
> ---
>  libsemanage/src/direct_api.c     | 132 +++++++++++++++++++++++++--------------
>  libsemanage/src/semanage_store.c |  14 ++++-
>  2 files changed, 98 insertions(+), 48 deletions(-)
> 
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index f4d57cf8..d42a51cd 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>  int semanage_direct_connect(semanage_handle_t * sh)
>  {
>  	const char *path;
> +	struct stat sb;
>  
>  	if (semanage_check_init(sh, sh->conf->store_root_path))
>  		goto err;
> @@ -302,10 +303,17 @@ 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)
> +
> +	if (stat(path, &sb) == 0)
>  		sepol_set_disable_dontaudit(sh->sepolh, 1);
> -	else
> +	else {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			goto err;
> +		}
> +
>  		sepol_set_disable_dontaudit(sh->sepolh, 0);
> +	}
>  
>  	return STATUS_SUCCESS;
>  
> @@ -1139,6 +1147,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);
> @@ -1155,9 +1164,13 @@ 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 && errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
> +			goto cleanup; //an error in the "stat" call
> +		}
>  
>  		status = semanage_compile_module(sh, &modinfos[i]);
>  		if (status < 0) {
> @@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  	struct cil_db *cildb = NULL;
>  	semanage_module_info_t *modinfos = NULL;
>  	mode_t mask = umask(0077);
> +	struct stat sb;
>  
>  	int do_rebuild, do_write_kernel, do_install;
>  	int fcontexts_modified, ports_modified, seusers_modified,
> @@ -1226,10 +1240,16 @@ 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)
> +	if (stat(path, &sb) == 0)
>  		do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
> -	else
> +	else {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			goto cleanup;
> +		}
> +
>  		do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
> +	}
>  	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>  		FILE *touch;
>  		touch = fopen(path, "w");
> @@ -1251,10 +1271,17 @@ 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
> +	else {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			goto cleanup;
> +		}
> +
>  		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
> +	}
> +
>  	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>  		FILE *touch;
>  		touch = fopen(path, "w");
> @@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  	 * a rebuild.
>  	 */
>  	if (!do_rebuild) {
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> +		int files[] = {SEMANAGE_STORE_KERNEL,
> +					   SEMANAGE_STORE_FC,
> +					   SEMANAGE_STORE_SEUSERS,
> +					   SEMANAGE_LINKED,
> +					   SEMANAGE_SEUSERS_LINKED,
> +					   SEMANAGE_USERS_EXTRA_LINKED};
> +
> +		for (i = 0; i < (int) sizeof(files); i++) {
> +			path = semanage_path(SEMANAGE_TMP, files[i]);
> +			if (stat(path, &sb) != 0) {
> +				if (errno != ENOENT) {
> +					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +					goto cleanup;
> +				}
>  
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> +				do_rebuild = 1;
> +				goto rebuild;
> +			}
>  		}
>  	}
>  
> @@ -1457,7 +1468,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(SEMANAGE_TMP,
>  								  SEMANAGE_STORE_SEUSERS),
> @@ -1466,11 +1477,16 @@ rebuild:
>  				goto cleanup;
>  			pseusers->dtable->drop_cache(pseusers->dbase);
>  		} else {
> +			if (errno != ENOENT) {
> +				ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +				goto cleanup;
> +			}
> +
>  			pseusers->dtable->clear(sh, pseusers->dbase);
>  		}
>  
>  		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(SEMANAGE_TMP,
>  								  SEMANAGE_USERS_EXTRA),
> @@ -1479,6 +1495,11 @@ rebuild:
>  				goto cleanup;
>  			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>  		} else {
> +			if (errno != ENOENT) {
> +				ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +				goto cleanup;
> +			}
> +
>  			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>  		}
>  	}
> @@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  	ssize_t _data_len;
>  	char *_data;
>  	int compressed;
> +	struct stat sb;
>  
>  	/* get path of module */
>  	rc = semanage_module_get_path(
> @@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  		goto cleanup;
>  	}
>  
> -	if (access(module_path, F_OK) != 0) {
> -		ERR(sh, "Module does not exist: %s", module_path);
> +	if (stat(module_path, &sb) != 0) {
> +		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>  		rc = -1;
>  		goto cleanup;
>  	}
> @@ -1851,7 +1873,12 @@ 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) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
> +			goto cleanup;
> +		}
> +
>  		rc = semanage_compile_module(sh, _modinfo);
>  		if (rc < 0) {
>  			goto cleanup;
> @@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>  	}
>  
>  	if (stat(path, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			status = -1;
> +			goto cleanup;
> +		}
> +
>  		*enabled = 1;
>  	}
>  	else {
> @@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>  
>  	/* set enabled/disabled status */
>  	if (stat(fn, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
> +			status = -1;
> +			goto cleanup;
> +		}
> +
>  		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>  		if (ret != 0) {
>  			status = -1;
> @@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>  	int status = 0;
>  	int ret = 0;
>  	int type;
> +	struct stat sb;
>  
>  	char path[PATH_MAX];
>  	mode_t mask = umask(0077);
> @@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>  			goto cleanup;
>  		}
>  
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			ret = unlink(path);
>  			if (ret != 0) {
>  				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 4bd1d651..83c188dc 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>  {
>  	char *semanage_conf = NULL;
>  	int len;
> +	FILE *fp = NULL;
>  
>  	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>  	semanage_conf = calloc(len + 1, sizeof(char));
> @@ -522,7 +523,10 @@ char *semanage_conf_path(void)
>  	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>  		 SEMANAGE_CONF_FILE);
>  
> -	if (access(semanage_conf, R_OK) != 0) {
> +	//the following replaces access(semanage_conf, R_OK) check
> +	if ((fp = fopen(semanage_conf, "r")) != NULL) {

Arguably the test should have just been F_OK (i.e. does it exist) and
you can just make it a stat() instead.

> +		fclose(fp);
> +	} else {
>  		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>  	}
>  
> @@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>  
>  	int r;
> +	struct stat sb;
> +
> +	if (stat(path, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			return -1;
> +		}
>  
> -	if (access(path, F_OK) != 0) {
>  		return 0;
>  	}
>  
>

Stephen Smalley Feb. 28, 2018, 6:43 p.m. UTC | #4

On 02/28/2018 01:24 PM, William Roberts wrote:
> Where is patch 2/2, I have yet to see it?
> 
> Did something get screwy and is it: [PATCH] libsemanage: Improve
> warning for installing disabled module

No, 2/3 was a separate patch and had the 2/3 in the subject line.
I received all three from the list, both locally and on my gmail.

> 
> 
> On Wed, Feb 28, 2018 at 9:50 AM, William Roberts
> <[e-mail]> wrote:
>> On Wed, Feb 28, 2018 at 2:15 AM, Vit Mojzis <[e-mail]> 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().
>>> And access(,R_OK) by fopen(,"r")
>>>
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
>>>
>>> Signed-off-by: Vit Mojzis <[e-mail]>
>>> ---
>>>  libsemanage/src/direct_api.c     | 132 +++++++++++++++++++++++++--------------
>>>  libsemanage/src/semanage_store.c |  14 ++++-
>>>  2 files changed, 98 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>>> index f4d57cf8..d42a51cd 100644
>>> --- a/libsemanage/src/direct_api.c
>>> +++ b/libsemanage/src/direct_api.c
>>> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>>>  int semanage_direct_connect(semanage_handle_t * sh)
>>>  {
>>>         const char *path;
>>> +       struct stat sb;
>>>
>>>         if (semanage_check_init(sh, sh->conf->store_root_path))
>>>                 goto err;
>>> @@ -302,10 +303,17 @@ 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)
>>> +
>>> +       if (stat(path, &sb) == 0)
>>>                 sepol_set_disable_dontaudit(sh->sepolh, 1);
>>> -       else
>>> +       else {
>>> +               if (errno != ENOENT) {
>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +                       goto err;
>>> +               }
>>> +
>>>                 sepol_set_disable_dontaudit(sh->sepolh, 0);
>>> +       }
>>>
>>>         return STATUS_SUCCESS;
>>>
>>> @@ -1139,6 +1147,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);
>>> @@ -1155,9 +1164,13 @@ 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 && errno != ENOENT) {
>>> +                       ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
>>> +                       goto cleanup; //an error in the "stat" call
>>> +               }
>>>
>>>                 status = semanage_compile_module(sh, &modinfos[i]);
>>>                 if (status < 0) {
>>> @@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>         struct cil_db *cildb = NULL;
>>>         semanage_module_info_t *modinfos = NULL;
>>>         mode_t mask = umask(0077);
>>> +       struct stat sb;
>>>
>>>         int do_rebuild, do_write_kernel, do_install;
>>>         int fcontexts_modified, ports_modified, seusers_modified,
>>> @@ -1226,10 +1240,16 @@ 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)
>>> +       if (stat(path, &sb) == 0)
>>>                 do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>> -       else
>>> +       else {
>>> +               if (errno != ENOENT) {
>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +                       goto cleanup;
>>> +               }
>>> +
>>
>> I am not a huge fan of this if under else block of if (errno !=ENOENT).
>>
>> maybe this is a bit cleaner:
>>
>> if (stat() == 0)
>>   //exists
>> else if (errno == ENOENT)
>>   // doesn't exist
>> else
>>   //fail
>>
>>>                 do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>> +       }
>>>         if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>>>                 FILE *touch;
>>>                 touch = fopen(path, "w");
>>> @@ -1251,10 +1271,17 @@ 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
>>> +       else {
>>> +               if (errno != ENOENT) {
>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +                       goto cleanup;
>>> +               }
>>> +
>>>                 do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
>>> +       }
>>> +
>>>         if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>>>                 FILE *touch;
>>>                 touch = fopen(path, "w");
>>> @@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>          * a rebuild.
>>>          */
>>>         if (!do_rebuild) {
>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
>>> -               if (access(path, F_OK) != 0) {
>>> -                       do_rebuild = 1;
>>> -                       goto rebuild;
>>> -               }
>>> -
>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
>>> -               if (access(path, F_OK) != 0) {
>>> -                       do_rebuild = 1;
>>> -                       goto rebuild;
>>> -               }
>>> -
>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
>>> -               if (access(path, F_OK) != 0) {
>>> -                       do_rebuild = 1;
>>> -                       goto rebuild;
>>> -               }
>>> -
>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
>>> -               if (access(path, F_OK) != 0) {
>>> -                       do_rebuild = 1;
>>> -                       goto rebuild;
>>> -               }
>>> -
>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>>> -               if (access(path, F_OK) != 0) {
>>> -                       do_rebuild = 1;
>>> -                       goto rebuild;
>>> -               }
>>> +               int files[] = {SEMANAGE_STORE_KERNEL,
>>> +                                          SEMANAGE_STORE_FC,
>>> +                                          SEMANAGE_STORE_SEUSERS,
>>> +                                          SEMANAGE_LINKED,
>>> +                                          SEMANAGE_SEUSERS_LINKED,
>>> +                                          SEMANAGE_USERS_EXTRA_LINKED};
>>> +
>>> +               for (i = 0; i < (int) sizeof(files); i++) {
>>> +                       path = semanage_path(SEMANAGE_TMP, files[i]);
>>> +                       if (stat(path, &sb) != 0) {
>>> +                               if (errno != ENOENT) {
>>> +                                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +                                       goto cleanup;
>>> +                               }
>>>
>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>>> -               if (access(path, F_OK) != 0) {
>>> -                       do_rebuild = 1;
>>> -                       goto rebuild;
>>> +                               do_rebuild = 1;
>>> +                               goto rebuild;
>>> +                       }
>>>                 }
>>>         }
>>>
>>> @@ -1457,7 +1468,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(SEMANAGE_TMP,
>>>                                                                   SEMANAGE_STORE_SEUSERS),
>>> @@ -1466,11 +1477,16 @@ rebuild:
>>>                                 goto cleanup;
>>>                         pseusers->dtable->drop_cache(pseusers->dbase);
>>>                 } else {
>>> +                       if (errno != ENOENT) {
>>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +                               goto cleanup;
>>> +                       }
>>> +
>>>                         pseusers->dtable->clear(sh, pseusers->dbase);
>>>                 }
>>>
>>>                 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(SEMANAGE_TMP,
>>>                                                                   SEMANAGE_USERS_EXTRA),
>>> @@ -1479,6 +1495,11 @@ rebuild:
>>>                                 goto cleanup;
>>>                         pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>>>                 } else {
>>> +                       if (errno != ENOENT) {
>>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +                               goto cleanup;
>>> +                       }
>>> +
>>>                         pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>>>                 }
>>>         }
>>> @@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>         ssize_t _data_len;
>>>         char *_data;
>>>         int compressed;
>>> +       struct stat sb;
>>>
>>>         /* get path of module */
>>>         rc = semanage_module_get_path(
>>> @@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>                 goto cleanup;
>>>         }
>>>
>>> -       if (access(module_path, F_OK) != 0) {
>>> -               ERR(sh, "Module does not exist: %s", module_path);
>>> +       if (stat(module_path, &sb) != 0) {
>>> +               ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>>>                 rc = -1;
>>>                 goto cleanup;
>>>         }
>>> @@ -1851,7 +1873,12 @@ 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) {
>>> +               if (errno != ENOENT) {
>>> +                       ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
>>> +                       goto cleanup;
>>> +               }
>>> +
>>>                 rc = semanage_compile_module(sh, _modinfo);
>>>                 if (rc < 0) {
>>>                         goto cleanup;
>>> @@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>>>         }
>>>
>>>         if (stat(path, &sb) < 0) {
>>> +               if (errno != ENOENT) {
>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +                       status = -1;
>>> +                       goto cleanup;
>>> +               }
>>> +
>>>                 *enabled = 1;
>>>         }
>>>         else {
>>> @@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>>
>>>         /* set enabled/disabled status */
>>>         if (stat(fn, &sb) < 0) {
>>> +               if (errno != ENOENT) {
>>> +                       ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
>>> +                       status = -1;
>>> +                       goto cleanup;
>>> +               }
>>> +
>>>                 ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>>>                 if (ret != 0) {
>>>                         status = -1;
>>> @@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>         int status = 0;
>>>         int ret = 0;
>>>         int type;
>>> +       struct stat sb;
>>>
>>>         char path[PATH_MAX];
>>>         mode_t mask = umask(0077);
>>> @@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>                         goto cleanup;
>>>                 }
>>>
>>> -               if (access(path, F_OK) == 0) {
>>> +               if (stat(path, &sb) == 0) {
>>>                         ret = unlink(path);
>>>                         if (ret != 0) {
>>>                                 ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
>>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>>> index 4bd1d651..83c188dc 100644
>>> --- a/libsemanage/src/semanage_store.c
>>> +++ b/libsemanage/src/semanage_store.c
>>> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>>>  {
>>>         char *semanage_conf = NULL;
>>>         int len;
>>> +       FILE *fp = NULL;
>>>
>>>         len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>>>         semanage_conf = calloc(len + 1, sizeof(char));
>>> @@ -522,7 +523,10 @@ char *semanage_conf_path(void)
>>>         snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>>>                  SEMANAGE_CONF_FILE);
>>>
>>> -       if (access(semanage_conf, R_OK) != 0) {
>>> +       //the following replaces access(semanage_conf, R_OK) check
>>> +       if ((fp = fopen(semanage_conf, "r")) != NULL) {
>>> +               fclose(fp);
>>> +       } else {
>>>                 snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>>>         }
>>>
>>> @@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>>>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>>>
>>>         int r;
>>> +       struct stat sb;
>>> +
>>> +       if (stat(path, &sb) < 0) {
>>> +               if (errno != ENOENT) {
>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +                       return -1;
>>> +               }
>>>
>>> -       if (access(path, F_OK) != 0) {
>>>                 return 0;
>>>         }
>>>
>>> --
>>> 2.14.3
>>>
>>>
>>
>>
>>
>> --
>> Respectfully,
>>
>> William C Roberts
> 
> 
>

William Roberts Feb. 28, 2018, 7:07 p.m. UTC | #5

On Wed, Feb 28, 2018 at 10:43 AM, Stephen Smalley <[e-mail]> wrote:
> On 02/28/2018 01:24 PM, William Roberts wrote:
>> Where is patch 2/2, I have yet to see it?
>>
>> Did something get screwy and is it: [PATCH] libsemanage: Improve
>> warning for installing disabled module
>
> No, 2/3 was a separate patch and had the 2/3 in the subject line.
> I received all three from the list, both locally and on my gmail.
>

Thanks gmail for folding the patch 1/3 and 2/3 into the same "conversation" or
whatever it does.


>>
>>
>> On Wed, Feb 28, 2018 at 9:50 AM, William Roberts
>> <[e-mail]> wrote:
>>> On Wed, Feb 28, 2018 at 2:15 AM, Vit Mojzis <[e-mail]> 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().
>>>> And access(,R_OK) by fopen(,"r")
>>>>
>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
>>>>
>>>> Signed-off-by: Vit Mojzis <[e-mail]>
>>>> ---
>>>>  libsemanage/src/direct_api.c     | 132 +++++++++++++++++++++++++--------------
>>>>  libsemanage/src/semanage_store.c |  14 ++++-
>>>>  2 files changed, 98 insertions(+), 48 deletions(-)
>>>>
>>>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>>>> index f4d57cf8..d42a51cd 100644
>>>> --- a/libsemanage/src/direct_api.c
>>>> +++ b/libsemanage/src/direct_api.c
>>>> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>>>>  int semanage_direct_connect(semanage_handle_t * sh)
>>>>  {
>>>>         const char *path;
>>>> +       struct stat sb;
>>>>
>>>>         if (semanage_check_init(sh, sh->conf->store_root_path))
>>>>                 goto err;
>>>> @@ -302,10 +303,17 @@ 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)
>>>> +
>>>> +       if (stat(path, &sb) == 0)
>>>>                 sepol_set_disable_dontaudit(sh->sepolh, 1);
>>>> -       else
>>>> +       else {
>>>> +               if (errno != ENOENT) {
>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +                       goto err;
>>>> +               }
>>>> +
>>>>                 sepol_set_disable_dontaudit(sh->sepolh, 0);
>>>> +       }
>>>>
>>>>         return STATUS_SUCCESS;
>>>>
>>>> @@ -1139,6 +1147,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);
>>>> @@ -1155,9 +1164,13 @@ 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 && errno != ENOENT) {
>>>> +                       ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
>>>> +                       goto cleanup; //an error in the "stat" call
>>>> +               }
>>>>
>>>>                 status = semanage_compile_module(sh, &modinfos[i]);
>>>>                 if (status < 0) {
>>>> @@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>         struct cil_db *cildb = NULL;
>>>>         semanage_module_info_t *modinfos = NULL;
>>>>         mode_t mask = umask(0077);
>>>> +       struct stat sb;
>>>>
>>>>         int do_rebuild, do_write_kernel, do_install;
>>>>         int fcontexts_modified, ports_modified, seusers_modified,
>>>> @@ -1226,10 +1240,16 @@ 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)
>>>> +       if (stat(path, &sb) == 0)
>>>>                 do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>>> -       else
>>>> +       else {
>>>> +               if (errno != ENOENT) {
>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +                       goto cleanup;
>>>> +               }
>>>> +
>>>
>>> I am not a huge fan of this if under else block of if (errno !=ENOENT).
>>>
>>> maybe this is a bit cleaner:
>>>
>>> if (stat() == 0)
>>>   //exists
>>> else if (errno == ENOENT)
>>>   // doesn't exist
>>> else
>>>   //fail
>>>
>>>>                 do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>>> +       }
>>>>         if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>>>>                 FILE *touch;
>>>>                 touch = fopen(path, "w");
>>>> @@ -1251,10 +1271,17 @@ 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
>>>> +       else {
>>>> +               if (errno != ENOENT) {
>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +                       goto cleanup;
>>>> +               }
>>>> +
>>>>                 do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
>>>> +       }
>>>> +
>>>>         if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>>>>                 FILE *touch;
>>>>                 touch = fopen(path, "w");
>>>> @@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>          * a rebuild.
>>>>          */
>>>>         if (!do_rebuild) {
>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
>>>> -               if (access(path, F_OK) != 0) {
>>>> -                       do_rebuild = 1;
>>>> -                       goto rebuild;
>>>> -               }
>>>> -
>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
>>>> -               if (access(path, F_OK) != 0) {
>>>> -                       do_rebuild = 1;
>>>> -                       goto rebuild;
>>>> -               }
>>>> -
>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
>>>> -               if (access(path, F_OK) != 0) {
>>>> -                       do_rebuild = 1;
>>>> -                       goto rebuild;
>>>> -               }
>>>> -
>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
>>>> -               if (access(path, F_OK) != 0) {
>>>> -                       do_rebuild = 1;
>>>> -                       goto rebuild;
>>>> -               }
>>>> -
>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>>>> -               if (access(path, F_OK) != 0) {
>>>> -                       do_rebuild = 1;
>>>> -                       goto rebuild;
>>>> -               }
>>>> +               int files[] = {SEMANAGE_STORE_KERNEL,
>>>> +                                          SEMANAGE_STORE_FC,
>>>> +                                          SEMANAGE_STORE_SEUSERS,
>>>> +                                          SEMANAGE_LINKED,
>>>> +                                          SEMANAGE_SEUSERS_LINKED,
>>>> +                                          SEMANAGE_USERS_EXTRA_LINKED};
>>>> +
>>>> +               for (i = 0; i < (int) sizeof(files); i++) {
>>>> +                       path = semanage_path(SEMANAGE_TMP, files[i]);
>>>> +                       if (stat(path, &sb) != 0) {
>>>> +                               if (errno != ENOENT) {
>>>> +                                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +                                       goto cleanup;
>>>> +                               }
>>>>
>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>>>> -               if (access(path, F_OK) != 0) {
>>>> -                       do_rebuild = 1;
>>>> -                       goto rebuild;
>>>> +                               do_rebuild = 1;
>>>> +                               goto rebuild;
>>>> +                       }
>>>>                 }
>>>>         }
>>>>
>>>> @@ -1457,7 +1468,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(SEMANAGE_TMP,
>>>>                                                                   SEMANAGE_STORE_SEUSERS),
>>>> @@ -1466,11 +1477,16 @@ rebuild:
>>>>                                 goto cleanup;
>>>>                         pseusers->dtable->drop_cache(pseusers->dbase);
>>>>                 } else {
>>>> +                       if (errno != ENOENT) {
>>>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +                               goto cleanup;
>>>> +                       }
>>>> +
>>>>                         pseusers->dtable->clear(sh, pseusers->dbase);
>>>>                 }
>>>>
>>>>                 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(SEMANAGE_TMP,
>>>>                                                                   SEMANAGE_USERS_EXTRA),
>>>> @@ -1479,6 +1495,11 @@ rebuild:
>>>>                                 goto cleanup;
>>>>                         pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>>>>                 } else {
>>>> +                       if (errno != ENOENT) {
>>>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +                               goto cleanup;
>>>> +                       }
>>>> +
>>>>                         pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>>>>                 }
>>>>         }
>>>> @@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>         ssize_t _data_len;
>>>>         char *_data;
>>>>         int compressed;
>>>> +       struct stat sb;
>>>>
>>>>         /* get path of module */
>>>>         rc = semanage_module_get_path(
>>>> @@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>                 goto cleanup;
>>>>         }
>>>>
>>>> -       if (access(module_path, F_OK) != 0) {
>>>> -               ERR(sh, "Module does not exist: %s", module_path);
>>>> +       if (stat(module_path, &sb) != 0) {
>>>> +               ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>>>>                 rc = -1;
>>>>                 goto cleanup;
>>>>         }
>>>> @@ -1851,7 +1873,12 @@ 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) {
>>>> +               if (errno != ENOENT) {
>>>> +                       ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
>>>> +                       goto cleanup;
>>>> +               }
>>>> +
>>>>                 rc = semanage_compile_module(sh, _modinfo);
>>>>                 if (rc < 0) {
>>>>                         goto cleanup;
>>>> @@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>>>>         }
>>>>
>>>>         if (stat(path, &sb) < 0) {
>>>> +               if (errno != ENOENT) {
>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +                       status = -1;
>>>> +                       goto cleanup;
>>>> +               }
>>>> +
>>>>                 *enabled = 1;
>>>>         }
>>>>         else {
>>>> @@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>>>
>>>>         /* set enabled/disabled status */
>>>>         if (stat(fn, &sb) < 0) {
>>>> +               if (errno != ENOENT) {
>>>> +                       ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
>>>> +                       status = -1;
>>>> +                       goto cleanup;
>>>> +               }
>>>> +
>>>>                 ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>>>>                 if (ret != 0) {
>>>>                         status = -1;
>>>> @@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>>         int status = 0;
>>>>         int ret = 0;
>>>>         int type;
>>>> +       struct stat sb;
>>>>
>>>>         char path[PATH_MAX];
>>>>         mode_t mask = umask(0077);
>>>> @@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>>                         goto cleanup;
>>>>                 }
>>>>
>>>> -               if (access(path, F_OK) == 0) {
>>>> +               if (stat(path, &sb) == 0) {
>>>>                         ret = unlink(path);
>>>>                         if (ret != 0) {
>>>>                                 ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
>>>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>>>> index 4bd1d651..83c188dc 100644
>>>> --- a/libsemanage/src/semanage_store.c
>>>> +++ b/libsemanage/src/semanage_store.c
>>>> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>>>>  {
>>>>         char *semanage_conf = NULL;
>>>>         int len;
>>>> +       FILE *fp = NULL;
>>>>
>>>>         len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>>>>         semanage_conf = calloc(len + 1, sizeof(char));
>>>> @@ -522,7 +523,10 @@ char *semanage_conf_path(void)
>>>>         snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>>>>                  SEMANAGE_CONF_FILE);
>>>>
>>>> -       if (access(semanage_conf, R_OK) != 0) {
>>>> +       //the following replaces access(semanage_conf, R_OK) check
>>>> +       if ((fp = fopen(semanage_conf, "r")) != NULL) {
>>>> +               fclose(fp);
>>>> +       } else {
>>>>                 snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>>>>         }
>>>>
>>>> @@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>>>>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>>>>
>>>>         int r;
>>>> +       struct stat sb;
>>>> +
>>>> +       if (stat(path, &sb) < 0) {
>>>> +               if (errno != ENOENT) {
>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +                       return -1;
>>>> +               }
>>>>
>>>> -       if (access(path, F_OK) != 0) {
>>>>                 return 0;
>>>>         }
>>>>
>>>> --
>>>> 2.14.3
>>>>
>>>>

William Roberts Feb. 28, 2018, 7:26 p.m. UTC | #6

So peeking through the code base, I see:

int semanage_direct_is_managed(semanage_handle_t * sh)
{
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;

if (semanage_access_check(sh) < 0)
return 0;

return 1;

      err:
ERR(sh, "could not check whether policy is managed");
return STATUS_ERR;
}

Which semanage_access_check eventually gets down to a raw access check.

Which is only ever used in test_fcontext

semanage_access_check is also the only consumer of semanage_direct_access_check

which is the semanage_store_access_check is only consumed by the
former and test case and
the same could be said for semanage_store_access_check

I think this is a good time to roll in patch 4 and drop everything
relying on semanage_store_access_check.

Thoughts?

On Wed, Feb 28, 2018 at 11:07 AM, William Roberts
<[e-mail]> wrote:
> On Wed, Feb 28, 2018 at 10:43 AM, Stephen Smalley <[e-mail]> wrote:
>> On 02/28/2018 01:24 PM, William Roberts wrote:
>>> Where is patch 2/2, I have yet to see it?
>>>
>>> Did something get screwy and is it: [PATCH] libsemanage: Improve
>>> warning for installing disabled module
>>
>> No, 2/3 was a separate patch and had the 2/3 in the subject line.
>> I received all three from the list, both locally and on my gmail.
>>
>
> Thanks gmail for folding the patch 1/3 and 2/3 into the same "conversation" or
> whatever it does.
>
>
>>>
>>>
>>> On Wed, Feb 28, 2018 at 9:50 AM, William Roberts
>>> <[e-mail]> wrote:
>>>> On Wed, Feb 28, 2018 at 2:15 AM, Vit Mojzis <[e-mail]> 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().
>>>>> And access(,R_OK) by fopen(,"r")
>>>>>
>>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
>>>>>
>>>>> Signed-off-by: Vit Mojzis <[e-mail]>
>>>>> ---
>>>>>  libsemanage/src/direct_api.c     | 132 +++++++++++++++++++++++++--------------
>>>>>  libsemanage/src/semanage_store.c |  14 ++++-
>>>>>  2 files changed, 98 insertions(+), 48 deletions(-)
>>>>>
>>>>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>>>>> index f4d57cf8..d42a51cd 100644
>>>>> --- a/libsemanage/src/direct_api.c
>>>>> +++ b/libsemanage/src/direct_api.c
>>>>> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>>>>>  int semanage_direct_connect(semanage_handle_t * sh)
>>>>>  {
>>>>>         const char *path;
>>>>> +       struct stat sb;
>>>>>
>>>>>         if (semanage_check_init(sh, sh->conf->store_root_path))
>>>>>                 goto err;
>>>>> @@ -302,10 +303,17 @@ 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)
>>>>> +
>>>>> +       if (stat(path, &sb) == 0)
>>>>>                 sepol_set_disable_dontaudit(sh->sepolh, 1);
>>>>> -       else
>>>>> +       else {
>>>>> +               if (errno != ENOENT) {
>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>> +                       goto err;
>>>>> +               }
>>>>> +
>>>>>                 sepol_set_disable_dontaudit(sh->sepolh, 0);
>>>>> +       }
>>>>>
>>>>>         return STATUS_SUCCESS;
>>>>>
>>>>> @@ -1139,6 +1147,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);
>>>>> @@ -1155,9 +1164,13 @@ 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 && errno != ENOENT) {
>>>>> +                       ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
>>>>> +                       goto cleanup; //an error in the "stat" call
>>>>> +               }
>>>>>
>>>>>                 status = semanage_compile_module(sh, &modinfos[i]);
>>>>>                 if (status < 0) {
>>>>> @@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>>         struct cil_db *cildb = NULL;
>>>>>         semanage_module_info_t *modinfos = NULL;
>>>>>         mode_t mask = umask(0077);
>>>>> +       struct stat sb;
>>>>>
>>>>>         int do_rebuild, do_write_kernel, do_install;
>>>>>         int fcontexts_modified, ports_modified, seusers_modified,
>>>>> @@ -1226,10 +1240,16 @@ 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)
>>>>> +       if (stat(path, &sb) == 0)
>>>>>                 do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>>>> -       else
>>>>> +       else {
>>>>> +               if (errno != ENOENT) {
>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>> +                       goto cleanup;
>>>>> +               }
>>>>> +
>>>>
>>>> I am not a huge fan of this if under else block of if (errno !=ENOENT).
>>>>
>>>> maybe this is a bit cleaner:
>>>>
>>>> if (stat() == 0)
>>>>   //exists
>>>> else if (errno == ENOENT)
>>>>   // doesn't exist
>>>> else
>>>>   //fail
>>>>
>>>>>                 do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>>>> +       }
>>>>>         if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>>>>>                 FILE *touch;
>>>>>                 touch = fopen(path, "w");
>>>>> @@ -1251,10 +1271,17 @@ 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
>>>>> +       else {
>>>>> +               if (errno != ENOENT) {
>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>> +                       goto cleanup;
>>>>> +               }
>>>>> +
>>>>>                 do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
>>>>> +       }
>>>>> +
>>>>>         if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>>>>>                 FILE *touch;
>>>>>                 touch = fopen(path, "w");
>>>>> @@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>>          * a rebuild.
>>>>>          */
>>>>>         if (!do_rebuild) {
>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
>>>>> -               if (access(path, F_OK) != 0) {
>>>>> -                       do_rebuild = 1;
>>>>> -                       goto rebuild;
>>>>> -               }
>>>>> -
>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
>>>>> -               if (access(path, F_OK) != 0) {
>>>>> -                       do_rebuild = 1;
>>>>> -                       goto rebuild;
>>>>> -               }
>>>>> -
>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
>>>>> -               if (access(path, F_OK) != 0) {
>>>>> -                       do_rebuild = 1;
>>>>> -                       goto rebuild;
>>>>> -               }
>>>>> -
>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
>>>>> -               if (access(path, F_OK) != 0) {
>>>>> -                       do_rebuild = 1;
>>>>> -                       goto rebuild;
>>>>> -               }
>>>>> -
>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>>>>> -               if (access(path, F_OK) != 0) {
>>>>> -                       do_rebuild = 1;
>>>>> -                       goto rebuild;
>>>>> -               }
>>>>> +               int files[] = {SEMANAGE_STORE_KERNEL,
>>>>> +                                          SEMANAGE_STORE_FC,
>>>>> +                                          SEMANAGE_STORE_SEUSERS,
>>>>> +                                          SEMANAGE_LINKED,
>>>>> +                                          SEMANAGE_SEUSERS_LINKED,
>>>>> +                                          SEMANAGE_USERS_EXTRA_LINKED};
>>>>> +
>>>>> +               for (i = 0; i < (int) sizeof(files); i++) {
>>>>> +                       path = semanage_path(SEMANAGE_TMP, files[i]);
>>>>> +                       if (stat(path, &sb) != 0) {
>>>>> +                               if (errno != ENOENT) {
>>>>> +                                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>> +                                       goto cleanup;
>>>>> +                               }
>>>>>
>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>>>>> -               if (access(path, F_OK) != 0) {
>>>>> -                       do_rebuild = 1;
>>>>> -                       goto rebuild;
>>>>> +                               do_rebuild = 1;
>>>>> +                               goto rebuild;
>>>>> +                       }
>>>>>                 }
>>>>>         }
>>>>>
>>>>> @@ -1457,7 +1468,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(SEMANAGE_TMP,
>>>>>                                                                   SEMANAGE_STORE_SEUSERS),
>>>>> @@ -1466,11 +1477,16 @@ rebuild:
>>>>>                                 goto cleanup;
>>>>>                         pseusers->dtable->drop_cache(pseusers->dbase);
>>>>>                 } else {
>>>>> +                       if (errno != ENOENT) {
>>>>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>> +                               goto cleanup;
>>>>> +                       }
>>>>> +
>>>>>                         pseusers->dtable->clear(sh, pseusers->dbase);
>>>>>                 }
>>>>>
>>>>>                 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(SEMANAGE_TMP,
>>>>>                                                                   SEMANAGE_USERS_EXTRA),
>>>>> @@ -1479,6 +1495,11 @@ rebuild:
>>>>>                                 goto cleanup;
>>>>>                         pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>>>>>                 } else {
>>>>> +                       if (errno != ENOENT) {
>>>>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>> +                               goto cleanup;
>>>>> +                       }
>>>>> +
>>>>>                         pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>>>>>                 }
>>>>>         }
>>>>> @@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>>         ssize_t _data_len;
>>>>>         char *_data;
>>>>>         int compressed;
>>>>> +       struct stat sb;
>>>>>
>>>>>         /* get path of module */
>>>>>         rc = semanage_module_get_path(
>>>>> @@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>>                 goto cleanup;
>>>>>         }
>>>>>
>>>>> -       if (access(module_path, F_OK) != 0) {
>>>>> -               ERR(sh, "Module does not exist: %s", module_path);
>>>>> +       if (stat(module_path, &sb) != 0) {
>>>>> +               ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>>>>>                 rc = -1;
>>>>>                 goto cleanup;
>>>>>         }
>>>>> @@ -1851,7 +1873,12 @@ 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) {
>>>>> +               if (errno != ENOENT) {
>>>>> +                       ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
>>>>> +                       goto cleanup;
>>>>> +               }
>>>>> +
>>>>>                 rc = semanage_compile_module(sh, _modinfo);
>>>>>                 if (rc < 0) {
>>>>>                         goto cleanup;
>>>>> @@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>>>>>         }
>>>>>
>>>>>         if (stat(path, &sb) < 0) {
>>>>> +               if (errno != ENOENT) {
>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>> +                       status = -1;
>>>>> +                       goto cleanup;
>>>>> +               }
>>>>> +
>>>>>                 *enabled = 1;
>>>>>         }
>>>>>         else {
>>>>> @@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>>>>
>>>>>         /* set enabled/disabled status */
>>>>>         if (stat(fn, &sb) < 0) {
>>>>> +               if (errno != ENOENT) {
>>>>> +                       ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
>>>>> +                       status = -1;
>>>>> +                       goto cleanup;
>>>>> +               }
>>>>> +
>>>>>                 ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>>>>>                 if (ret != 0) {
>>>>>                         status = -1;
>>>>> @@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>>>         int status = 0;
>>>>>         int ret = 0;
>>>>>         int type;
>>>>> +       struct stat sb;
>>>>>
>>>>>         char path[PATH_MAX];
>>>>>         mode_t mask = umask(0077);
>>>>> @@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>>>                         goto cleanup;
>>>>>                 }
>>>>>
>>>>> -               if (access(path, F_OK) == 0) {
>>>>> +               if (stat(path, &sb) == 0) {
>>>>>                         ret = unlink(path);
>>>>>                         if (ret != 0) {
>>>>>                                 ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
>>>>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>>>>> index 4bd1d651..83c188dc 100644
>>>>> --- a/libsemanage/src/semanage_store.c
>>>>> +++ b/libsemanage/src/semanage_store.c
>>>>> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>>>>>  {
>>>>>         char *semanage_conf = NULL;
>>>>>         int len;
>>>>> +       FILE *fp = NULL;
>>>>>
>>>>>         len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>>>>>         semanage_conf = calloc(len + 1, sizeof(char));
>>>>> @@ -522,7 +523,10 @@ char *semanage_conf_path(void)
>>>>>         snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>>>>>                  SEMANAGE_CONF_FILE);
>>>>>
>>>>> -       if (access(semanage_conf, R_OK) != 0) {
>>>>> +       //the following replaces access(semanage_conf, R_OK) check
>>>>> +       if ((fp = fopen(semanage_conf, "r")) != NULL) {
>>>>> +               fclose(fp);
>>>>> +       } else {
>>>>>                 snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>>>>>         }
>>>>>
>>>>> @@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>>>>>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>>>>>
>>>>>         int r;
>>>>> +       struct stat sb;
>>>>> +
>>>>> +       if (stat(path, &sb) < 0) {
>>>>> +               if (errno != ENOENT) {
>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>> +                       return -1;
>>>>> +               }
>>>>>
>>>>> -       if (access(path, F_OK) != 0) {
>>>>>                 return 0;
>>>>>         }
>>>>>
>>>>> --
>>>>> 2.14.3
>>>>>
>>>>>

Stephen Smalley Feb. 28, 2018, 7:39 p.m. UTC | #7

On 02/28/2018 02:26 PM, William Roberts wrote:
> So peeking through the code base, I see:
> 
> int semanage_direct_is_managed(semanage_handle_t * sh)
> {
> if (semanage_check_init(sh, sh->conf->store_root_path))
> goto err;
> 
> if (semanage_access_check(sh) < 0)
> return 0;
> 
> return 1;
> 
>       err:
> ERR(sh, "could not check whether policy is managed");
> return STATUS_ERR;
> }
> 
> Which semanage_access_check eventually gets down to a raw access check.
> 
> Which is only ever used in test_fcontext
> 
> semanage_access_check is also the only consumer of semanage_direct_access_check
> 
> which is the semanage_store_access_check is only consumed by the
> former and test case and
> the same could be said for semanage_store_access_check
> 
> I think this is a good time to roll in patch 4 and drop everything
> relying on semanage_store_access_check.
> 
> Thoughts?

semanage_access_check() is part of the shared library ABI. Can't be
removed.  Used by seobject.py via the python bindings.  At most, we can
kill all internal users but the ABI has to remain.

> 
> On Wed, Feb 28, 2018 at 11:07 AM, William Roberts
> <[e-mail]> wrote:
>> On Wed, Feb 28, 2018 at 10:43 AM, Stephen Smalley <[e-mail]> wrote:
>>> On 02/28/2018 01:24 PM, William Roberts wrote:
>>>> Where is patch 2/2, I have yet to see it?
>>>>
>>>> Did something get screwy and is it: [PATCH] libsemanage: Improve
>>>> warning for installing disabled module
>>>
>>> No, 2/3 was a separate patch and had the 2/3 in the subject line.
>>> I received all three from the list, both locally and on my gmail.
>>>
>>
>> Thanks gmail for folding the patch 1/3 and 2/3 into the same "conversation" or
>> whatever it does.
>>
>>
>>>>
>>>>
>>>> On Wed, Feb 28, 2018 at 9:50 AM, William Roberts
>>>> <[e-mail]> wrote:
>>>>> On Wed, Feb 28, 2018 at 2:15 AM, Vit Mojzis <[e-mail]> 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().
>>>>>> And access(,R_OK) by fopen(,"r")
>>>>>>
>>>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
>>>>>>
>>>>>> Signed-off-by: Vit Mojzis <[e-mail]>
>>>>>> ---
>>>>>>  libsemanage/src/direct_api.c     | 132 +++++++++++++++++++++++++--------------
>>>>>>  libsemanage/src/semanage_store.c |  14 ++++-
>>>>>>  2 files changed, 98 insertions(+), 48 deletions(-)
>>>>>>
>>>>>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>>>>>> index f4d57cf8..d42a51cd 100644
>>>>>> --- a/libsemanage/src/direct_api.c
>>>>>> +++ b/libsemanage/src/direct_api.c
>>>>>> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>>>>>>  int semanage_direct_connect(semanage_handle_t * sh)
>>>>>>  {
>>>>>>         const char *path;
>>>>>> +       struct stat sb;
>>>>>>
>>>>>>         if (semanage_check_init(sh, sh->conf->store_root_path))
>>>>>>                 goto err;
>>>>>> @@ -302,10 +303,17 @@ 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)
>>>>>> +
>>>>>> +       if (stat(path, &sb) == 0)
>>>>>>                 sepol_set_disable_dontaudit(sh->sepolh, 1);
>>>>>> -       else
>>>>>> +       else {
>>>>>> +               if (errno != ENOENT) {
>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>> +                       goto err;
>>>>>> +               }
>>>>>> +
>>>>>>                 sepol_set_disable_dontaudit(sh->sepolh, 0);
>>>>>> +       }
>>>>>>
>>>>>>         return STATUS_SUCCESS;
>>>>>>
>>>>>> @@ -1139,6 +1147,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);
>>>>>> @@ -1155,9 +1164,13 @@ 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 && errno != ENOENT) {
>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
>>>>>> +                       goto cleanup; //an error in the "stat" call
>>>>>> +               }
>>>>>>
>>>>>>                 status = semanage_compile_module(sh, &modinfos[i]);
>>>>>>                 if (status < 0) {
>>>>>> @@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>>>         struct cil_db *cildb = NULL;
>>>>>>         semanage_module_info_t *modinfos = NULL;
>>>>>>         mode_t mask = umask(0077);
>>>>>> +       struct stat sb;
>>>>>>
>>>>>>         int do_rebuild, do_write_kernel, do_install;
>>>>>>         int fcontexts_modified, ports_modified, seusers_modified,
>>>>>> @@ -1226,10 +1240,16 @@ 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)
>>>>>> +       if (stat(path, &sb) == 0)
>>>>>>                 do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>>>>> -       else
>>>>>> +       else {
>>>>>> +               if (errno != ENOENT) {
>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>> +                       goto cleanup;
>>>>>> +               }
>>>>>> +
>>>>>
>>>>> I am not a huge fan of this if under else block of if (errno !=ENOENT).
>>>>>
>>>>> maybe this is a bit cleaner:
>>>>>
>>>>> if (stat() == 0)
>>>>>   //exists
>>>>> else if (errno == ENOENT)
>>>>>   // doesn't exist
>>>>> else
>>>>>   //fail
>>>>>
>>>>>>                 do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>>>>> +       }
>>>>>>         if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>>>>>>                 FILE *touch;
>>>>>>                 touch = fopen(path, "w");
>>>>>> @@ -1251,10 +1271,17 @@ 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
>>>>>> +       else {
>>>>>> +               if (errno != ENOENT) {
>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>> +                       goto cleanup;
>>>>>> +               }
>>>>>> +
>>>>>>                 do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
>>>>>> +       }
>>>>>> +
>>>>>>         if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>>>>>>                 FILE *touch;
>>>>>>                 touch = fopen(path, "w");
>>>>>> @@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>>>          * a rebuild.
>>>>>>          */
>>>>>>         if (!do_rebuild) {
>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>> -                       do_rebuild = 1;
>>>>>> -                       goto rebuild;
>>>>>> -               }
>>>>>> -
>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>> -                       do_rebuild = 1;
>>>>>> -                       goto rebuild;
>>>>>> -               }
>>>>>> -
>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>> -                       do_rebuild = 1;
>>>>>> -                       goto rebuild;
>>>>>> -               }
>>>>>> -
>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>> -                       do_rebuild = 1;
>>>>>> -                       goto rebuild;
>>>>>> -               }
>>>>>> -
>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>> -                       do_rebuild = 1;
>>>>>> -                       goto rebuild;
>>>>>> -               }
>>>>>> +               int files[] = {SEMANAGE_STORE_KERNEL,
>>>>>> +                                          SEMANAGE_STORE_FC,
>>>>>> +                                          SEMANAGE_STORE_SEUSERS,
>>>>>> +                                          SEMANAGE_LINKED,
>>>>>> +                                          SEMANAGE_SEUSERS_LINKED,
>>>>>> +                                          SEMANAGE_USERS_EXTRA_LINKED};
>>>>>> +
>>>>>> +               for (i = 0; i < (int) sizeof(files); i++) {
>>>>>> +                       path = semanage_path(SEMANAGE_TMP, files[i]);
>>>>>> +                       if (stat(path, &sb) != 0) {
>>>>>> +                               if (errno != ENOENT) {
>>>>>> +                                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>> +                                       goto cleanup;
>>>>>> +                               }
>>>>>>
>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>> -                       do_rebuild = 1;
>>>>>> -                       goto rebuild;
>>>>>> +                               do_rebuild = 1;
>>>>>> +                               goto rebuild;
>>>>>> +                       }
>>>>>>                 }
>>>>>>         }
>>>>>>
>>>>>> @@ -1457,7 +1468,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(SEMANAGE_TMP,
>>>>>>                                                                   SEMANAGE_STORE_SEUSERS),
>>>>>> @@ -1466,11 +1477,16 @@ rebuild:
>>>>>>                                 goto cleanup;
>>>>>>                         pseusers->dtable->drop_cache(pseusers->dbase);
>>>>>>                 } else {
>>>>>> +                       if (errno != ENOENT) {
>>>>>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>> +                               goto cleanup;
>>>>>> +                       }
>>>>>> +
>>>>>>                         pseusers->dtable->clear(sh, pseusers->dbase);
>>>>>>                 }
>>>>>>
>>>>>>                 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(SEMANAGE_TMP,
>>>>>>                                                                   SEMANAGE_USERS_EXTRA),
>>>>>> @@ -1479,6 +1495,11 @@ rebuild:
>>>>>>                                 goto cleanup;
>>>>>>                         pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>>>>>>                 } else {
>>>>>> +                       if (errno != ENOENT) {
>>>>>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>> +                               goto cleanup;
>>>>>> +                       }
>>>>>> +
>>>>>>                         pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>>>>>>                 }
>>>>>>         }
>>>>>> @@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>>>         ssize_t _data_len;
>>>>>>         char *_data;
>>>>>>         int compressed;
>>>>>> +       struct stat sb;
>>>>>>
>>>>>>         /* get path of module */
>>>>>>         rc = semanage_module_get_path(
>>>>>> @@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>>>                 goto cleanup;
>>>>>>         }
>>>>>>
>>>>>> -       if (access(module_path, F_OK) != 0) {
>>>>>> -               ERR(sh, "Module does not exist: %s", module_path);
>>>>>> +       if (stat(module_path, &sb) != 0) {
>>>>>> +               ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>>>>>>                 rc = -1;
>>>>>>                 goto cleanup;
>>>>>>         }
>>>>>> @@ -1851,7 +1873,12 @@ 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) {
>>>>>> +               if (errno != ENOENT) {
>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
>>>>>> +                       goto cleanup;
>>>>>> +               }
>>>>>> +
>>>>>>                 rc = semanage_compile_module(sh, _modinfo);
>>>>>>                 if (rc < 0) {
>>>>>>                         goto cleanup;
>>>>>> @@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>>>>>>         }
>>>>>>
>>>>>>         if (stat(path, &sb) < 0) {
>>>>>> +               if (errno != ENOENT) {
>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>> +                       status = -1;
>>>>>> +                       goto cleanup;
>>>>>> +               }
>>>>>> +
>>>>>>                 *enabled = 1;
>>>>>>         }
>>>>>>         else {
>>>>>> @@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>>>>>
>>>>>>         /* set enabled/disabled status */
>>>>>>         if (stat(fn, &sb) < 0) {
>>>>>> +               if (errno != ENOENT) {
>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
>>>>>> +                       status = -1;
>>>>>> +                       goto cleanup;
>>>>>> +               }
>>>>>> +
>>>>>>                 ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>>>>>>                 if (ret != 0) {
>>>>>>                         status = -1;
>>>>>> @@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>>>>         int status = 0;
>>>>>>         int ret = 0;
>>>>>>         int type;
>>>>>> +       struct stat sb;
>>>>>>
>>>>>>         char path[PATH_MAX];
>>>>>>         mode_t mask = umask(0077);
>>>>>> @@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>>>>                         goto cleanup;
>>>>>>                 }
>>>>>>
>>>>>> -               if (access(path, F_OK) == 0) {
>>>>>> +               if (stat(path, &sb) == 0) {
>>>>>>                         ret = unlink(path);
>>>>>>                         if (ret != 0) {
>>>>>>                                 ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
>>>>>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>>>>>> index 4bd1d651..83c188dc 100644
>>>>>> --- a/libsemanage/src/semanage_store.c
>>>>>> +++ b/libsemanage/src/semanage_store.c
>>>>>> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>>>>>>  {
>>>>>>         char *semanage_conf = NULL;
>>>>>>         int len;
>>>>>> +       FILE *fp = NULL;
>>>>>>
>>>>>>         len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>>>>>>         semanage_conf = calloc(len + 1, sizeof(char));
>>>>>> @@ -522,7 +523,10 @@ char *semanage_conf_path(void)
>>>>>>         snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>>>>>>                  SEMANAGE_CONF_FILE);
>>>>>>
>>>>>> -       if (access(semanage_conf, R_OK) != 0) {
>>>>>> +       //the following replaces access(semanage_conf, R_OK) check
>>>>>> +       if ((fp = fopen(semanage_conf, "r")) != NULL) {
>>>>>> +               fclose(fp);
>>>>>> +       } else {
>>>>>>                 snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>>>>>>         }
>>>>>>
>>>>>> @@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>>>>>>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>>>>>>
>>>>>>         int r;
>>>>>> +       struct stat sb;
>>>>>> +
>>>>>> +       if (stat(path, &sb) < 0) {
>>>>>> +               if (errno != ENOENT) {
>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>> +                       return -1;
>>>>>> +               }
>>>>>>
>>>>>> -       if (access(path, F_OK) != 0) {
>>>>>>                 return 0;
>>>>>>         }
>>>>>>
>>>>>> --
>>>>>> 2.14.3
>>>>>>
>>>>>>
> 
> 
>

William Roberts Feb. 28, 2018, 7:44 p.m. UTC | #8

On Wed, Feb 28, 2018 at 11:39 AM, Stephen Smalley <[e-mail]> wrote:
> On 02/28/2018 02:26 PM, William Roberts wrote:
>> So peeking through the code base, I see:
>>
>> int semanage_direct_is_managed(semanage_handle_t * sh)
>> {
>> if (semanage_check_init(sh, sh->conf->store_root_path))
>> goto err;
>>
>> if (semanage_access_check(sh) < 0)
>> return 0;
>>
>> return 1;
>>
>>       err:
>> ERR(sh, "could not check whether policy is managed");
>> return STATUS_ERR;
>> }
>>
>> Which semanage_access_check eventually gets down to a raw access check.
>>
>> Which is only ever used in test_fcontext
>>
>> semanage_access_check is also the only consumer of semanage_direct_access_check
>>
>> which is the semanage_store_access_check is only consumed by the
>> former and test case and
>> the same could be said for semanage_store_access_check
>>
>> I think this is a good time to roll in patch 4 and drop everything
>> relying on semanage_store_access_check.
>>
>> Thoughts?
>
> semanage_access_check() is part of the shared library ABI. Can't be
> removed.  Used by seobject.py via the python bindings.  At most, we can
> kill all internal users but the ABI has to remain.

Ahh yes, duh.

Outside of just killing off internal users of it, should we modify it
to not use access?
So it at least works under setuid?

>
>>
>> On Wed, Feb 28, 2018 at 11:07 AM, William Roberts
>> <[e-mail]> wrote:
>>> On Wed, Feb 28, 2018 at 10:43 AM, Stephen Smalley <[e-mail]> wrote:
>>>> On 02/28/2018 01:24 PM, William Roberts wrote:
>>>>> Where is patch 2/2, I have yet to see it?
>>>>>
>>>>> Did something get screwy and is it: [PATCH] libsemanage: Improve
>>>>> warning for installing disabled module
>>>>
>>>> No, 2/3 was a separate patch and had the 2/3 in the subject line.
>>>> I received all three from the list, both locally and on my gmail.
>>>>
>>>
>>> Thanks gmail for folding the patch 1/3 and 2/3 into the same "conversation" or
>>> whatever it does.
>>>
>>>
>>>>>
>>>>>
>>>>> On Wed, Feb 28, 2018 at 9:50 AM, William Roberts
>>>>> <[e-mail]> wrote:
>>>>>> On Wed, Feb 28, 2018 at 2:15 AM, Vit Mojzis <[e-mail]> 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().
>>>>>>> And access(,R_OK) by fopen(,"r")
>>>>>>>
>>>>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
>>>>>>>
>>>>>>> Signed-off-by: Vit Mojzis <[e-mail]>
>>>>>>> ---
>>>>>>>  libsemanage/src/direct_api.c     | 132 +++++++++++++++++++++++++--------------
>>>>>>>  libsemanage/src/semanage_store.c |  14 ++++-
>>>>>>>  2 files changed, 98 insertions(+), 48 deletions(-)
>>>>>>>
>>>>>>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>>>>>>> index f4d57cf8..d42a51cd 100644
>>>>>>> --- a/libsemanage/src/direct_api.c
>>>>>>> +++ b/libsemanage/src/direct_api.c
>>>>>>> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>>>>>>>  int semanage_direct_connect(semanage_handle_t * sh)
>>>>>>>  {
>>>>>>>         const char *path;
>>>>>>> +       struct stat sb;
>>>>>>>
>>>>>>>         if (semanage_check_init(sh, sh->conf->store_root_path))
>>>>>>>                 goto err;
>>>>>>> @@ -302,10 +303,17 @@ 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)
>>>>>>> +
>>>>>>> +       if (stat(path, &sb) == 0)
>>>>>>>                 sepol_set_disable_dontaudit(sh->sepolh, 1);
>>>>>>> -       else
>>>>>>> +       else {
>>>>>>> +               if (errno != ENOENT) {
>>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>>> +                       goto err;
>>>>>>> +               }
>>>>>>> +
>>>>>>>                 sepol_set_disable_dontaudit(sh->sepolh, 0);
>>>>>>> +       }
>>>>>>>
>>>>>>>         return STATUS_SUCCESS;
>>>>>>>
>>>>>>> @@ -1139,6 +1147,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);
>>>>>>> @@ -1155,9 +1164,13 @@ 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 && errno != ENOENT) {
>>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
>>>>>>> +                       goto cleanup; //an error in the "stat" call
>>>>>>> +               }
>>>>>>>
>>>>>>>                 status = semanage_compile_module(sh, &modinfos[i]);
>>>>>>>                 if (status < 0) {
>>>>>>> @@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>>>>         struct cil_db *cildb = NULL;
>>>>>>>         semanage_module_info_t *modinfos = NULL;
>>>>>>>         mode_t mask = umask(0077);
>>>>>>> +       struct stat sb;
>>>>>>>
>>>>>>>         int do_rebuild, do_write_kernel, do_install;
>>>>>>>         int fcontexts_modified, ports_modified, seusers_modified,
>>>>>>> @@ -1226,10 +1240,16 @@ 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)
>>>>>>> +       if (stat(path, &sb) == 0)
>>>>>>>                 do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>>>>>> -       else
>>>>>>> +       else {
>>>>>>> +               if (errno != ENOENT) {
>>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>>> +                       goto cleanup;
>>>>>>> +               }
>>>>>>> +
>>>>>>
>>>>>> I am not a huge fan of this if under else block of if (errno !=ENOENT).
>>>>>>
>>>>>> maybe this is a bit cleaner:
>>>>>>
>>>>>> if (stat() == 0)
>>>>>>   //exists
>>>>>> else if (errno == ENOENT)
>>>>>>   // doesn't exist
>>>>>> else
>>>>>>   //fail
>>>>>>
>>>>>>>                 do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>>>>>> +       }
>>>>>>>         if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>>>>>>>                 FILE *touch;
>>>>>>>                 touch = fopen(path, "w");
>>>>>>> @@ -1251,10 +1271,17 @@ 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
>>>>>>> +       else {
>>>>>>> +               if (errno != ENOENT) {
>>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>>> +                       goto cleanup;
>>>>>>> +               }
>>>>>>> +
>>>>>>>                 do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
>>>>>>> +       }
>>>>>>> +
>>>>>>>         if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>>>>>>>                 FILE *touch;
>>>>>>>                 touch = fopen(path, "w");
>>>>>>> @@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>>>>          * a rebuild.
>>>>>>>          */
>>>>>>>         if (!do_rebuild) {
>>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
>>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>>> -                       do_rebuild = 1;
>>>>>>> -                       goto rebuild;
>>>>>>> -               }
>>>>>>> -
>>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
>>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>>> -                       do_rebuild = 1;
>>>>>>> -                       goto rebuild;
>>>>>>> -               }
>>>>>>> -
>>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
>>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>>> -                       do_rebuild = 1;
>>>>>>> -                       goto rebuild;
>>>>>>> -               }
>>>>>>> -
>>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
>>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>>> -                       do_rebuild = 1;
>>>>>>> -                       goto rebuild;
>>>>>>> -               }
>>>>>>> -
>>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>>> -                       do_rebuild = 1;
>>>>>>> -                       goto rebuild;
>>>>>>> -               }
>>>>>>> +               int files[] = {SEMANAGE_STORE_KERNEL,
>>>>>>> +                                          SEMANAGE_STORE_FC,
>>>>>>> +                                          SEMANAGE_STORE_SEUSERS,
>>>>>>> +                                          SEMANAGE_LINKED,
>>>>>>> +                                          SEMANAGE_SEUSERS_LINKED,
>>>>>>> +                                          SEMANAGE_USERS_EXTRA_LINKED};
>>>>>>> +
>>>>>>> +               for (i = 0; i < (int) sizeof(files); i++) {
>>>>>>> +                       path = semanage_path(SEMANAGE_TMP, files[i]);
>>>>>>> +                       if (stat(path, &sb) != 0) {
>>>>>>> +                               if (errno != ENOENT) {
>>>>>>> +                                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>>> +                                       goto cleanup;
>>>>>>> +                               }
>>>>>>>
>>>>>>> -               path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>>>>>>> -               if (access(path, F_OK) != 0) {
>>>>>>> -                       do_rebuild = 1;
>>>>>>> -                       goto rebuild;
>>>>>>> +                               do_rebuild = 1;
>>>>>>> +                               goto rebuild;
>>>>>>> +                       }
>>>>>>>                 }
>>>>>>>         }
>>>>>>>
>>>>>>> @@ -1457,7 +1468,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(SEMANAGE_TMP,
>>>>>>>                                                                   SEMANAGE_STORE_SEUSERS),
>>>>>>> @@ -1466,11 +1477,16 @@ rebuild:
>>>>>>>                                 goto cleanup;
>>>>>>>                         pseusers->dtable->drop_cache(pseusers->dbase);
>>>>>>>                 } else {
>>>>>>> +                       if (errno != ENOENT) {
>>>>>>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>>> +                               goto cleanup;
>>>>>>> +                       }
>>>>>>> +
>>>>>>>                         pseusers->dtable->clear(sh, pseusers->dbase);
>>>>>>>                 }
>>>>>>>
>>>>>>>                 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(SEMANAGE_TMP,
>>>>>>>                                                                   SEMANAGE_USERS_EXTRA),
>>>>>>> @@ -1479,6 +1495,11 @@ rebuild:
>>>>>>>                                 goto cleanup;
>>>>>>>                         pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>>>>>>>                 } else {
>>>>>>> +                       if (errno != ENOENT) {
>>>>>>> +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>>> +                               goto cleanup;
>>>>>>> +                       }
>>>>>>> +
>>>>>>>                         pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>>>>>>>                 }
>>>>>>>         }
>>>>>>> @@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>>>>         ssize_t _data_len;
>>>>>>>         char *_data;
>>>>>>>         int compressed;
>>>>>>> +       struct stat sb;
>>>>>>>
>>>>>>>         /* get path of module */
>>>>>>>         rc = semanage_module_get_path(
>>>>>>> @@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>>>>                 goto cleanup;
>>>>>>>         }
>>>>>>>
>>>>>>> -       if (access(module_path, F_OK) != 0) {
>>>>>>> -               ERR(sh, "Module does not exist: %s", module_path);
>>>>>>> +       if (stat(module_path, &sb) != 0) {
>>>>>>> +               ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>>>>>>>                 rc = -1;
>>>>>>>                 goto cleanup;
>>>>>>>         }
>>>>>>> @@ -1851,7 +1873,12 @@ 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) {
>>>>>>> +               if (errno != ENOENT) {
>>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
>>>>>>> +                       goto cleanup;
>>>>>>> +               }
>>>>>>> +
>>>>>>>                 rc = semanage_compile_module(sh, _modinfo);
>>>>>>>                 if (rc < 0) {
>>>>>>>                         goto cleanup;
>>>>>>> @@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>>>>>>>         }
>>>>>>>
>>>>>>>         if (stat(path, &sb) < 0) {
>>>>>>> +               if (errno != ENOENT) {
>>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>>> +                       status = -1;
>>>>>>> +                       goto cleanup;
>>>>>>> +               }
>>>>>>> +
>>>>>>>                 *enabled = 1;
>>>>>>>         }
>>>>>>>         else {
>>>>>>> @@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>>>>>>
>>>>>>>         /* set enabled/disabled status */
>>>>>>>         if (stat(fn, &sb) < 0) {
>>>>>>> +               if (errno != ENOENT) {
>>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
>>>>>>> +                       status = -1;
>>>>>>> +                       goto cleanup;
>>>>>>> +               }
>>>>>>> +
>>>>>>>                 ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>>>>>>>                 if (ret != 0) {
>>>>>>>                         status = -1;
>>>>>>> @@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>>>>>         int status = 0;
>>>>>>>         int ret = 0;
>>>>>>>         int type;
>>>>>>> +       struct stat sb;
>>>>>>>
>>>>>>>         char path[PATH_MAX];
>>>>>>>         mode_t mask = umask(0077);
>>>>>>> @@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>>>>>                         goto cleanup;
>>>>>>>                 }
>>>>>>>
>>>>>>> -               if (access(path, F_OK) == 0) {
>>>>>>> +               if (stat(path, &sb) == 0) {
>>>>>>>                         ret = unlink(path);
>>>>>>>                         if (ret != 0) {
>>>>>>>                                 ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
>>>>>>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>>>>>>> index 4bd1d651..83c188dc 100644
>>>>>>> --- a/libsemanage/src/semanage_store.c
>>>>>>> +++ b/libsemanage/src/semanage_store.c
>>>>>>> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>>>>>>>  {
>>>>>>>         char *semanage_conf = NULL;
>>>>>>>         int len;
>>>>>>> +       FILE *fp = NULL;
>>>>>>>
>>>>>>>         len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>>>>>>>         semanage_conf = calloc(len + 1, sizeof(char));
>>>>>>> @@ -522,7 +523,10 @@ char *semanage_conf_path(void)
>>>>>>>         snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>>>>>>>                  SEMANAGE_CONF_FILE);
>>>>>>>
>>>>>>> -       if (access(semanage_conf, R_OK) != 0) {
>>>>>>> +       //the following replaces access(semanage_conf, R_OK) check
>>>>>>> +       if ((fp = fopen(semanage_conf, "r")) != NULL) {
>>>>>>> +               fclose(fp);
>>>>>>> +       } else {
>>>>>>>                 snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>>>>>>>         }
>>>>>>>
>>>>>>> @@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>>>>>>>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>>>>>>>
>>>>>>>         int r;
>>>>>>> +       struct stat sb;
>>>>>>> +
>>>>>>> +       if (stat(path, &sb) < 0) {
>>>>>>> +               if (errno != ENOENT) {
>>>>>>> +                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>>>>> +                       return -1;
>>>>>>> +               }
>>>>>>>
>>>>>>> -       if (access(path, F_OK) != 0) {
>>>>>>>                 return 0;
>>>>>>>         }
>>>>>>>
>>>>>>> --
>>>>>>> 2.14.3
>>>>>>>
>>>>>>>
>>
>>
>>
>

Stephen Smalley Feb. 28, 2018, 8:53 p.m. UTC | #9

On 02/28/2018 02:44 PM, William Roberts wrote:
> On Wed, Feb 28, 2018 at 11:39 AM, Stephen Smalley <[e-mail]> wrote:
>> On 02/28/2018 02:26 PM, William Roberts wrote:
>>> So peeking through the code base, I see:
>>>
>>> int semanage_direct_is_managed(semanage_handle_t * sh)
>>> {
>>> if (semanage_check_init(sh, sh->conf->store_root_path))
>>> goto err;
>>>
>>> if (semanage_access_check(sh) < 0)
>>> return 0;
>>>
>>> return 1;
>>>
>>>       err:
>>> ERR(sh, "could not check whether policy is managed");
>>> return STATUS_ERR;
>>> }
>>>
>>> Which semanage_access_check eventually gets down to a raw access check.
>>>
>>> Which is only ever used in test_fcontext
>>>
>>> semanage_access_check is also the only consumer of semanage_direct_access_check
>>>
>>> which is the semanage_store_access_check is only consumed by the
>>> former and test case and
>>> the same could be said for semanage_store_access_check
>>>
>>> I think this is a good time to roll in patch 4 and drop everything
>>> relying on semanage_store_access_check.
>>>
>>> Thoughts?
>>
>> semanage_access_check() is part of the shared library ABI. Can't be
>> removed.  Used by seobject.py via the python bindings.  At most, we can
>> kill all internal users but the ABI has to remain.
> 
> Ahh yes, duh.
> 
> Outside of just killing off internal users of it, should we modify it
> to not use access?
> So it at least works under setuid?

I don't think it is worthwhile, and it isn't clear how one would test
the directory writability case.  Plus semanage_access_check() is only
advisory and the caller is free to ignore the result or not call it in
the first place.  It is really only intended to allow a program to save
itself some work if it isn't going to be able to access the policy store
at all, not as a security feature.  Also, as I've said before, I
wouldn't warrant libsemanage to be safe if called from a setuid program;
no one has ever audited it for such to my knowledge.

Stephen Smalley March 1, 2018, 2:40 p.m. UTC | #10

On 02/28/2018 03:53 PM, Stephen Smalley wrote:
> On 02/28/2018 02:44 PM, William Roberts wrote:
>> On Wed, Feb 28, 2018 at 11:39 AM, Stephen Smalley <[e-mail]> wrote:
>>> On 02/28/2018 02:26 PM, William Roberts wrote:
>>>> So peeking through the code base, I see:
>>>>
>>>> int semanage_direct_is_managed(semanage_handle_t * sh)
>>>> {
>>>> if (semanage_check_init(sh, sh->conf->store_root_path))
>>>> goto err;
>>>>
>>>> if (semanage_access_check(sh) < 0)
>>>> return 0;
>>>>
>>>> return 1;
>>>>
>>>>       err:
>>>> ERR(sh, "could not check whether policy is managed");
>>>> return STATUS_ERR;
>>>> }
>>>>
>>>> Which semanage_access_check eventually gets down to a raw access check.
>>>>
>>>> Which is only ever used in test_fcontext
>>>>
>>>> semanage_access_check is also the only consumer of semanage_direct_access_check
>>>>
>>>> which is the semanage_store_access_check is only consumed by the
>>>> former and test case and
>>>> the same could be said for semanage_store_access_check
>>>>
>>>> I think this is a good time to roll in patch 4 and drop everything
>>>> relying on semanage_store_access_check.
>>>>
>>>> Thoughts?
>>>
>>> semanage_access_check() is part of the shared library ABI. Can't be
>>> removed.  Used by seobject.py via the python bindings.  At most, we can
>>> kill all internal users but the ABI has to remain.
>>
>> Ahh yes, duh.
>>
>> Outside of just killing off internal users of it, should we modify it
>> to not use access?
>> So it at least works under setuid?
> 
> I don't think it is worthwhile, and it isn't clear how one would test
> the directory writability case.  Plus semanage_access_check() is only
> advisory and the caller is free to ignore the result or not call it in
> the first place.  It is really only intended to allow a program to save
> itself some work if it isn't going to be able to access the policy store
> at all, not as a security feature.  Also, as I've said before, I
> wouldn't warrant libsemanage to be safe if called from a setuid program;
> no one has ever audited it for such to my knowledge.

By the way, please test that these patches do not end up yielding silent
failures or confusing error messages when users run semanage or semodule
commands with insufficient permissions, e.g. semanage login -l or
semodule -l as non-root.  That's what semanage_access_check() was for,
to check up front and provide a useful error message before doing any
other work.

Stephen Smalley March 1, 2018, 2:46 p.m. UTC | #11

On 03/01/2018 09:40 AM, Stephen Smalley wrote:
> On 02/28/2018 03:53 PM, Stephen Smalley wrote:
>> On 02/28/2018 02:44 PM, William Roberts wrote:
>>> On Wed, Feb 28, 2018 at 11:39 AM, Stephen Smalley <[e-mail]> wrote:
>>>> On 02/28/2018 02:26 PM, William Roberts wrote:
>>>>> So peeking through the code base, I see:
>>>>>
>>>>> int semanage_direct_is_managed(semanage_handle_t * sh)
>>>>> {
>>>>> if (semanage_check_init(sh, sh->conf->store_root_path))
>>>>> goto err;
>>>>>
>>>>> if (semanage_access_check(sh) < 0)
>>>>> return 0;
>>>>>
>>>>> return 1;
>>>>>
>>>>>       err:
>>>>> ERR(sh, "could not check whether policy is managed");
>>>>> return STATUS_ERR;
>>>>> }
>>>>>
>>>>> Which semanage_access_check eventually gets down to a raw access check.
>>>>>
>>>>> Which is only ever used in test_fcontext
>>>>>
>>>>> semanage_access_check is also the only consumer of semanage_direct_access_check
>>>>>
>>>>> which is the semanage_store_access_check is only consumed by the
>>>>> former and test case and
>>>>> the same could be said for semanage_store_access_check
>>>>>
>>>>> I think this is a good time to roll in patch 4 and drop everything
>>>>> relying on semanage_store_access_check.
>>>>>
>>>>> Thoughts?
>>>>
>>>> semanage_access_check() is part of the shared library ABI. Can't be
>>>> removed.  Used by seobject.py via the python bindings.  At most, we can
>>>> kill all internal users but the ABI has to remain.
>>>
>>> Ahh yes, duh.
>>>
>>> Outside of just killing off internal users of it, should we modify it
>>> to not use access?
>>> So it at least works under setuid?
>>
>> I don't think it is worthwhile, and it isn't clear how one would test
>> the directory writability case.  Plus semanage_access_check() is only
>> advisory and the caller is free to ignore the result or not call it in
>> the first place.  It is really only intended to allow a program to save
>> itself some work if it isn't going to be able to access the policy store
>> at all, not as a security feature.  Also, as I've said before, I
>> wouldn't warrant libsemanage to be safe if called from a setuid program;
>> no one has ever audited it for such to my knowledge.
> 
> By the way, please test that these patches do not end up yielding silent
> failures or confusing error messages when users run semanage or semodule
> commands with insufficient permissions, e.g. semanage login -l or
> semodule -l as non-root.  That's what semanage_access_check() was for,
> to check up front and provide a useful error message before doing any
> other work.

And for comparison, this is what we get today before these patches:
$ semanage login -l
ValueError: SELinux policy is not managed or store cannot be accessed.

$ semodule -l
libsemanage.semanage_create_store: Could not access module store at
/var/lib/selinux/targeted, or it is not a directory. (Permission denied).
libsemanage.semanage_direct_connect: could not establish direct
connection (Permission denied).
semodule:  Could not connect to policy handler

The messages don't have to be the same, but we need to make sure we have
some useful diagnostic that points to the underlying problem.

Patch

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4d57cf8..d42a51cd 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@  int semanage_direct_is_managed(semanage_handle_t * sh)
 int semanage_direct_connect(semanage_handle_t * sh)
 {
 	const char *path;
+	struct stat sb;
 
 	if (semanage_check_init(sh, sh->conf->store_root_path))
 		goto err;
@@ -302,10 +303,17 @@  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)
+
+	if (stat(path, &sb) == 0)
 		sepol_set_disable_dontaudit(sh->sepolh, 1);
-	else
+	else {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			goto err;
+		}
+
 		sepol_set_disable_dontaudit(sh->sepolh, 0);
+	}
 
 	return STATUS_SUCCESS;
 
@@ -1139,6 +1147,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);
@@ -1155,9 +1164,13 @@  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 && errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+			goto cleanup; //an error in the "stat" call
+		}
 
 		status = semanage_compile_module(sh, &modinfos[i]);
 		if (status < 0) {
@@ -1188,6 +1201,7 @@  static int semanage_direct_commit(semanage_handle_t * sh)
 	struct cil_db *cildb = NULL;
 	semanage_module_info_t *modinfos = NULL;
 	mode_t mask = umask(0077);
+	struct stat sb;
 
 	int do_rebuild, do_write_kernel, do_install;
 	int fcontexts_modified, ports_modified, seusers_modified,
@@ -1226,10 +1240,16 @@  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)
+	if (stat(path, &sb) == 0)
 		do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
-	else
+	else {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			goto cleanup;
+		}
+
 		do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+	}
 	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
 		FILE *touch;
 		touch = fopen(path, "w");
@@ -1251,10 +1271,17 @@  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
+	else {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			goto cleanup;
+		}
+
 		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+	}
+
 	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
 		FILE *touch;
 		touch = fopen(path, "w");
@@ -1291,40 +1318,24 @@  static int semanage_direct_commit(semanage_handle_t * sh)
 	 * a rebuild.
 	 */
 	if (!do_rebuild) {
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
+		int files[] = {SEMANAGE_STORE_KERNEL,
+					   SEMANAGE_STORE_FC,
+					   SEMANAGE_STORE_SEUSERS,
+					   SEMANAGE_LINKED,
+					   SEMANAGE_SEUSERS_LINKED,
+					   SEMANAGE_USERS_EXTRA_LINKED};
+
+		for (i = 0; i < (int) sizeof(files); i++) {
+			path = semanage_path(SEMANAGE_TMP, files[i]);
+			if (stat(path, &sb) != 0) {
+				if (errno != ENOENT) {
+					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+					goto cleanup;
+				}
 
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
+				do_rebuild = 1;
+				goto rebuild;
+			}
 		}
 	}
 
@@ -1457,7 +1468,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(SEMANAGE_TMP,
 								  SEMANAGE_STORE_SEUSERS),
@@ -1466,11 +1477,16 @@  rebuild:
 				goto cleanup;
 			pseusers->dtable->drop_cache(pseusers->dbase);
 		} else {
+			if (errno != ENOENT) {
+				ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+				goto cleanup;
+			}
+
 			pseusers->dtable->clear(sh, pseusers->dbase);
 		}
 
 		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(SEMANAGE_TMP,
 								  SEMANAGE_USERS_EXTRA),
@@ -1479,6 +1495,11 @@  rebuild:
 				goto cleanup;
 			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
 		} else {
+			if (errno != ENOENT) {
+				ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+				goto cleanup;
+			}
+
 			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
 		}
 	}
@@ -1809,6 +1830,7 @@  static int semanage_direct_extract(semanage_handle_t * sh,
 	ssize_t _data_len;
 	char *_data;
 	int compressed;
+	struct stat sb;
 
 	/* get path of module */
 	rc = semanage_module_get_path(
@@ -1821,8 +1843,8 @@  static int semanage_direct_extract(semanage_handle_t * sh,
 		goto cleanup;
 	}
 
-	if (access(module_path, F_OK) != 0) {
-		ERR(sh, "Module does not exist: %s", module_path);
+	if (stat(module_path, &sb) != 0) {
+		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
 		rc = -1;
 		goto cleanup;
 	}
@@ -1851,7 +1873,12 @@  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) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+			goto cleanup;
+		}
+
 		rc = semanage_compile_module(sh, _modinfo);
 		if (rc < 0) {
 			goto cleanup;
@@ -1996,6 +2023,12 @@  static int semanage_direct_get_enabled(semanage_handle_t *sh,
 	}
 
 	if (stat(path, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			status = -1;
+			goto cleanup;
+		}
+
 		*enabled = 1;
 	}
 	else {
@@ -2322,6 +2355,12 @@  static int semanage_direct_get_module_info(semanage_handle_t *sh,
 
 	/* set enabled/disabled status */
 	if (stat(fn, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+			status = -1;
+			goto cleanup;
+		}
+
 		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
 		if (ret != 0) {
 			status = -1;
@@ -2730,6 +2769,7 @@  static int semanage_direct_install_info(semanage_handle_t *sh,
 	int status = 0;
 	int ret = 0;
 	int type;
+	struct stat sb;
 
 	char path[PATH_MAX];
 	mode_t mask = umask(0077);
@@ -2830,7 +2870,7 @@  static int semanage_direct_install_info(semanage_handle_t *sh,
 			goto cleanup;
 		}
 
-		if (access(path, F_OK) == 0) {
+		if (stat(path, &sb) == 0) {
 			ret = unlink(path);
 			if (ret != 0) {
 				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..83c188dc 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@  char *semanage_conf_path(void)
 {
 	char *semanage_conf = NULL;
 	int len;
+	FILE *fp = NULL;
 
 	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
 	semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,10 @@  char *semanage_conf_path(void)
 	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
 		 SEMANAGE_CONF_FILE);
 
-	if (access(semanage_conf, R_OK) != 0) {
+	//the following replaces access(semanage_conf, R_OK) check
+	if ((fp = fopen(semanage_conf, "r")) != NULL) {
+		fclose(fp);
+	} else {
 		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
 	}
 
@@ -1508,8 +1512,14 @@  int semanage_split_fc(semanage_handle_t * sh)
 static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
 
 	int r;
+	struct stat sb;
+
+	if (stat(path, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			return -1;
+		}
 
-	if (access(path, F_OK) != 0) {
 		return 0;
 	}