On Wed, Feb 28, 2018 at 2:15 AM, Vit Mojzis <[email protected]> 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 <[email protected]>
> ---
> 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
