On Wed, Feb 28, 2018 at 11:39 AM, Stephen Smalley <[email protected]> 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 >> <[email protected]> wrote: >>> On Wed, Feb 28, 2018 at 10:43 AM, Stephen Smalley <[email protected]> >>> 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 >>>>> <[email protected]> wrote: >>>>>> 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
