On 03/09/2018 08:14 AM, Vit Mojzis wrote: > > > On 7.3.2018 15:59, Stephen Smalley wrote: >> On 03/06/2018 06:58 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 <vmojzis@xxxxxxxxxx> >>> --- >>> libsemanage/src/direct_api.c | 133 +++++++++++++++++++++++++-------------- >>> libsemanage/src/semanage_store.c | 16 ++++- >>> 2 files changed, 99 insertions(+), 50 deletions(-) >>> >>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c >>> index 49cac14f..4e2af810 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,16 @@ 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) { >>> + /* The file does not exist */ >>> sepol_set_disable_dontaudit(sh->sepolh, 0); >>> + } else { >>> + ERR(sh, "Unable to access %s: %s\n", path, strerror(errno)); >>> + goto err; >>> + } >>> return STATUS_SUCCESS; >>> @@ -1139,6 +1146,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 +1163,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) { >>> @@ -1196,6 +1208,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, >>> @@ -1234,10 +1247,15 @@ 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) { >>> + /* The file does not exist */ >>> do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1); >>> + } else { >>> + ERR(sh, "Unable to access %s: %s\n", path, strerror(errno)); >> We need to ensure that retval is set in all of these cases. Safest to always set it before goto cleanup, >> either explicitly to -1 or to return value of stat() call above. > Thank you. > The retval is actually initialized to -1 (and stat() returns -1 for any error) so it's not necessary to set it here. It will be necessary a little bit down the road after the first assignment to retval, you're right. > Should I set it to -1 here anyway to make it less prone to breaking due to future changes in the code (what about the other ERR calls in this section of the code)? Yes, I would do that for safety here and in any other case where we didn't just set retval and test it. IOW, any case where we have: retval = foo(args); if (retval < 0) { ERR(...) goto cleanup; } is fine as is, but any case where we have: if (some-condition-other-than-retval-less-than-0) { ERR(...) goto cleanup; } likely ought to explicitly set retval. If we have too many of those, then maybe we ought to have an err: label that sets retval to -1 explicitly and goto it instead. > >>> + goto cleanup; >>> + } >>> if (sepol_get_disable_dontaudit(sh->sepolh) == 1) { >>> FILE *touch; >>> touch = fopen(path, "w"); >>> @@ -1259,10 +1277,16 @@ 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) { >>> + /* The file does not exist */ >>> do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1); >>> + } else { >>> + ERR(sh, "Unable to access %s: %s\n", path, strerror(errno)); >>> + goto cleanup; >>> + } >> Ditto >> >>> + >>> if (sepol_get_preserve_tunables(sh->sepolh) == 1) { >>> FILE *touch; >>> touch = fopen(path, "w"); >>> @@ -1299,40 +1323,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; >> Ditto >> >>> + } >>> - 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; >>> + } >>> } >>> } >>> @@ -1465,7 +1473,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), >>> @@ -1473,12 +1481,18 @@ rebuild: >>> if (retval < 0) >>> goto cleanup; >>> pseusers->dtable->drop_cache(pseusers->dbase); >>> - } else { >>> + } else if (errno == ENOENT) { >>> + /* The file does not exist */ >>> pseusers->dtable->clear(sh, pseusers->dbase); >>> + } else { >>> + ERR(sh, "Unable to access %s: %s\n", path, strerror(errno)); >>> + goto cleanup; >> Ditto >> >>> } >>> + >>> + >>> 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), >>> @@ -1486,8 +1500,12 @@ rebuild: >>> if (retval < 0) >>> goto cleanup; >>> pusers_extra->dtable->drop_cache(pusers_extra->dbase); >>> - } else { >>> + } else if (errno == ENOENT) { >>> + /* The file does not exist */ >>> pusers_extra->dtable->clear(sh, pusers_extra->dbase); >>> + } else { >>> + ERR(sh, "Unable to access %s: %s\n", path, strerror(errno)); >>> + goto cleanup; >> Ditto >> >>> } >>> } >>> @@ -1817,6 +1835,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( >>> @@ -1829,8 +1848,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; >>> } >>> @@ -1859,7 +1878,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)); >> Ditto >> >>> + goto cleanup; >>> + >>> + >>> rc = semanage_compile_module(sh, _modinfo); >>> if (rc < 0) { >>> goto cleanup; >>> @@ -2004,6 +2028,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 { >>> @@ -2330,6 +2360,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; >>> @@ -2738,6 +2774,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); >>> @@ -2838,7 +2875,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..10f8b306 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; >>> + struct stat sb; >>> len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE); >>> semanage_conf = calloc(len + 1, sizeof(char)); >>> @@ -522,7 +523,12 @@ 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) { >>> + if (stat(semanage_conf, &sb) != 0) { >>> + if (errno != ENOENT) { >>> + /* semanage_conf exists but can't be accessed */ >>> + return NULL; >> Not sure this is what we want. I think we'd just return semanage_conf here, and then let the caller fail when it tries to access it, so that we get a useful error message with the pathname. >> >>> + } >>> + >>> snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE); >>> } >>> @@ -1508,8 +1514,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; >>> } >>> > >
