* libsemanage: Perform access check using euid instead of uid v2 @ 2018-03-06 11:58 Vit Mojzis 2018-03-06 11:58 ` [PATCH 1/3] libsemanage: remove access() check to make setuid programs work Vit Mojzis ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Vit Mojzis @ 2018-03-06 11:58 UTC (permalink / raw) To: selinux Changes: - replace semanage_copy_file by copy_file_if_exists to make sure "retval" is 0 if the file does not exist - restructure if statements to be more clear ("fail" is last part of the statement) - replace read test (attempt to open the file) by stat() call ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] libsemanage: remove access() check to make setuid programs work 2018-03-06 11:58 libsemanage: Perform access check using euid instead of uid v2 Vit Mojzis @ 2018-03-06 11:58 ` Vit Mojzis 2018-03-06 11:58 ` [PATCH 2/3] " Vit Mojzis 2018-03-06 11:58 ` [PATCH 3/3] libsemanage: replace access() checks " Vit Mojzis 2 siblings, 0 replies; 19+ messages in thread From: Vit Mojzis @ 2018-03-06 11:58 UTC (permalink / raw) To: selinux access() uses real UID instead of effective UID which causes false negative checks in setuid programs. Remove redundant access() checks Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 Signed-off-by: Vit Mojzis <vmojzis@redhat.com> --- libsemanage/src/direct_api.c | 7 ------- libsemanage/src/semanage_store.c | 17 ++++++++--------- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c index 88873c43..b7899d68 100644 --- a/libsemanage/src/direct_api.c +++ b/libsemanage/src/direct_api.c @@ -148,9 +148,6 @@ int semanage_direct_connect(semanage_handle_t * sh) if (semanage_create_store(sh, 1)) goto err; - if (semanage_access_check(sh) < SEMANAGE_CAN_READ) - goto err; - sh->u.direct.translock_file_fd = -1; sh->u.direct.activelock_file_fd = -1; @@ -398,10 +395,6 @@ static int semanage_direct_disconnect(semanage_handle_t *sh) static int semanage_direct_begintrans(semanage_handle_t * sh) { - - if (semanage_access_check(sh) != SEMANAGE_CAN_WRITE) { - return -1; - } if (semanage_get_trans_lock(sh) < 0) { return -1; } diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c index 936e6495..4bd1d651 100644 --- a/libsemanage/src/semanage_store.c +++ b/libsemanage/src/semanage_store.c @@ -538,7 +538,6 @@ char *semanage_conf_path(void) int semanage_create_store(semanage_handle_t * sh, int create) { struct stat sb; - int mode_mask = R_OK | W_OK | X_OK; const char *path = semanage_files[SEMANAGE_ROOT]; int fd; @@ -557,9 +556,9 @@ int semanage_create_store(semanage_handle_t * sh, int create) return -1; } } else { - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) == -1) { + if (!S_ISDIR(sb.st_mode)) { ERR(sh, - "Could not access module store at %s, or it is not a directory.", + "Module store at %s is not a directory.", path); return -1; } @@ -580,9 +579,9 @@ int semanage_create_store(semanage_handle_t * sh, int create) return -1; } } else { - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) == -1) { + if (!S_ISDIR(sb.st_mode)) { ERR(sh, - "Could not access module store active subdirectory at %s, or it is not a directory.", + "Module store active subdirectory at %s is not a directory.", path); return -1; } @@ -603,9 +602,9 @@ int semanage_create_store(semanage_handle_t * sh, int create) return -1; } } else { - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) == -1) { + if (!S_ISDIR(sb.st_mode)) { ERR(sh, - "Could not access module store active modules subdirectory at %s, or it is not a directory.", + "Module store active modules subdirectory at %s is not a directory.", path); return -1; } @@ -624,8 +623,8 @@ int semanage_create_store(semanage_handle_t * sh, int create) return -1; } } else { - if (!S_ISREG(sb.st_mode) || access(path, R_OK | W_OK) == -1) { - ERR(sh, "Could not access lock file at %s.", path); + if (!S_ISREG(sb.st_mode)) { + ERR(sh, "Object at %s is not a lock file.", path); return -1; } } -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] libsemanage: remove access() check to make setuid programs work 2018-03-06 11:58 libsemanage: Perform access check using euid instead of uid v2 Vit Mojzis 2018-03-06 11:58 ` [PATCH 1/3] libsemanage: remove access() check to make setuid programs work Vit Mojzis @ 2018-03-06 11:58 ` Vit Mojzis 2018-03-08 19:54 ` Stephen Smalley 2018-03-06 11:58 ` [PATCH 3/3] libsemanage: replace access() checks " Vit Mojzis 2 siblings, 1 reply; 19+ messages in thread From: Vit Mojzis @ 2018-03-06 11:58 UTC (permalink / raw) To: selinux F_OK access checks only work properly as long as all directories along the path are accessible to real user running the program. Replace F_OK access checks by testing return value of open, write, etc. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 Signed-off-by: Vit Mojzis <vmojzis@redhat.com> --- libsemanage/src/direct_api.c | 47 ++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c index b7899d68..49cac14f 100644 --- a/libsemanage/src/direct_api.c +++ b/libsemanage/src/direct_api.c @@ -1171,6 +1171,14 @@ cleanup: return status; } +/* Copies a file from src to dst. If dst already exists then + * overwrite it. If source doesn't exist then return success. + * Returns 0 on success, -1 on error. */ +static int copy_file_if_exists(const char *src, const char *dst, mode_t mode){ + int rc = semanage_copy_file(src, dst, mode); + return (rc < 0 && errno != ENOENT) ? rc : 0; +} + /********************* direct API functions ********************/ /* Commits all changes in sandbox to the actual kernel policy. @@ -1563,34 +1571,25 @@ rebuild: goto cleanup; } - path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL); - if (access(path, F_OK) == 0) { - retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL), - semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL), - sh->conf->file_mode); - if (retval < 0) { - goto cleanup; - } + retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL), + semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL), + sh->conf->file_mode); + if (retval < 0) { + goto cleanup; } - path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC); - if (access(path, F_OK) == 0) { - retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC), - semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC), - sh->conf->file_mode); - if (retval < 0) { - goto cleanup; - } + retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC), + semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC), + sh->conf->file_mode); + if (retval < 0) { + goto cleanup; } - path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS); - if (access(path, F_OK) == 0) { - retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS), - semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS), - sh->conf->file_mode); - if (retval < 0) { - goto cleanup; - } + retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS), + semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS), + sh->conf->file_mode); + if (retval < 0) { + goto cleanup; } /* run genhomedircon if its enabled, this should be the last operation -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] libsemanage: remove access() check to make setuid programs work 2018-03-06 11:58 ` [PATCH 2/3] " Vit Mojzis @ 2018-03-08 19:54 ` Stephen Smalley 0 siblings, 0 replies; 19+ messages in thread From: Stephen Smalley @ 2018-03-08 19:54 UTC (permalink / raw) To: Vit Mojzis, selinux On 03/06/2018 06:58 AM, Vit Mojzis wrote: > F_OK access checks only work properly as long as all directories along > the path are accessible to real user running the program. > Replace F_OK access checks by testing return value of open, write, etc. Applied patches 1 and 2 (not 3 as per my comments). > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > --- > libsemanage/src/direct_api.c | 47 ++++++++++++++++++++++---------------------- > 1 file changed, 23 insertions(+), 24 deletions(-) > > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c > index b7899d68..49cac14f 100644 > --- a/libsemanage/src/direct_api.c > +++ b/libsemanage/src/direct_api.c > @@ -1171,6 +1171,14 @@ cleanup: > return status; > } > > +/* Copies a file from src to dst. If dst already exists then > + * overwrite it. If source doesn't exist then return success. > + * Returns 0 on success, -1 on error. */ > +static int copy_file_if_exists(const char *src, const char *dst, mode_t mode){ > + int rc = semanage_copy_file(src, dst, mode); > + return (rc < 0 && errno != ENOENT) ? rc : 0; > +} > + > /********************* direct API functions ********************/ > > /* Commits all changes in sandbox to the actual kernel policy. > @@ -1563,34 +1571,25 @@ rebuild: > goto cleanup; > } > > - path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL); > - if (access(path, F_OK) == 0) { > - retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL), > - semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL), > - sh->conf->file_mode); > - if (retval < 0) { > - goto cleanup; > - } > + retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL), > + semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL), > + sh->conf->file_mode); > + if (retval < 0) { > + goto cleanup; > } > > - path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC); > - if (access(path, F_OK) == 0) { > - retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC), > - semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC), > - sh->conf->file_mode); > - if (retval < 0) { > - goto cleanup; > - } > + retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC), > + semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC), > + sh->conf->file_mode); > + if (retval < 0) { > + goto cleanup; > } > > - path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS); > - if (access(path, F_OK) == 0) { > - retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS), > - semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS), > - sh->conf->file_mode); > - if (retval < 0) { > - goto cleanup; > - } > + retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS), > + semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS), > + sh->conf->file_mode); > + if (retval < 0) { > + goto cleanup; > } > > /* run genhomedircon if its enabled, this should be the last operation > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work 2018-03-06 11:58 libsemanage: Perform access check using euid instead of uid v2 Vit Mojzis 2018-03-06 11:58 ` [PATCH 1/3] libsemanage: remove access() check to make setuid programs work Vit Mojzis 2018-03-06 11:58 ` [PATCH 2/3] " Vit Mojzis @ 2018-03-06 11:58 ` Vit Mojzis 2018-03-07 14:59 ` Stephen Smalley 2 siblings, 1 reply; 19+ messages in thread From: Vit Mojzis @ 2018-03-06 11:58 UTC (permalink / raw) To: selinux 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@redhat.com> --- 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)); + 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; + } + 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; + } - 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; } + + 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; } } @@ -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)); + 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; + } + 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; } -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work 2018-03-06 11:58 ` [PATCH 3/3] libsemanage: replace access() checks " Vit Mojzis @ 2018-03-07 14:59 ` Stephen Smalley 2018-03-08 20:24 ` Stephen Smalley 2018-03-09 13:14 ` Vit Mojzis 0 siblings, 2 replies; 19+ messages in thread From: Stephen Smalley @ 2018-03-07 14:59 UTC (permalink / raw) To: Vit Mojzis, selinux 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@redhat.com> > --- > 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. > + 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; > } > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work 2018-03-07 14:59 ` Stephen Smalley @ 2018-03-08 20:24 ` Stephen Smalley 2018-03-09 13:14 ` Vit Mojzis 1 sibling, 0 replies; 19+ messages in thread From: Stephen Smalley @ 2018-03-08 20:24 UTC (permalink / raw) To: Vit Mojzis, selinux On 03/07/2018 09:59 AM, 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") Also, in addition to fixing the code, please update the patch description since the last line is no longer accurate. >> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 >> >> Signed-off-by: Vit Mojzis <vmojzis@redhat.com> >> --- >> 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. > >> + 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; >> } >> >> > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work 2018-03-07 14:59 ` Stephen Smalley 2018-03-08 20:24 ` Stephen Smalley @ 2018-03-09 13:14 ` Vit Mojzis 2018-03-09 13:28 ` Stephen Smalley 1 sibling, 1 reply; 19+ messages in thread From: Vit Mojzis @ 2018-03-09 13:14 UTC (permalink / raw) To: Stephen Smalley, selinux 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@redhat.com> >> --- >> 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)? >> + 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; >> } >> >> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work 2018-03-09 13:14 ` Vit Mojzis @ 2018-03-09 13:28 ` Stephen Smalley 2018-03-09 13:36 ` Stephen Smalley 0 siblings, 1 reply; 19+ messages in thread From: Stephen Smalley @ 2018-03-09 13:28 UTC (permalink / raw) To: Vit Mojzis, selinux 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@redhat.com> >>> --- >>> 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; >>> } >>> > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work 2018-03-09 13:28 ` Stephen Smalley @ 2018-03-09 13:36 ` Stephen Smalley 2018-03-09 15:21 ` [PATCH] " Vit Mojzis 0 siblings, 1 reply; 19+ messages in thread From: Stephen Smalley @ 2018-03-09 13:36 UTC (permalink / raw) To: Vit Mojzis, selinux On 03/09/2018 08:28 AM, Stephen Smalley wrote: > 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@redhat.com> >>>> --- >>>> 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. That said, you don't have to fix cases that you didn't introduce in your patch; that's up to you, and you can fix pre-existing cases via a separate patch if desired. > >> >>>> + 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; >>>> } >>>> >> >> > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] libsemanage: replace access() checks to make setuid programs work 2018-03-09 13:36 ` Stephen Smalley @ 2018-03-09 15:21 ` Vit Mojzis 2018-03-09 15:31 ` Stephen Smalley 0 siblings, 1 reply; 19+ messages in thread From: Vit Mojzis @ 2018-03-09 15:21 UTC (permalink / raw) To: selinux access() uses real UID instead of effective UID which causes false negative checks in setuid programs. Replace access() calls (mostly tests for file existence) by stat(). Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 Signed-off-by: Vit Mojzis <vmojzis@redhat.com> --- libsemanage/src/direct_api.c | 137 +++++++++++++++++++++++++-------------- libsemanage/src/semanage_store.c | 11 +++- 2 files changed, 98 insertions(+), 50 deletions(-) diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c index 92d7517d..439122df 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,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) { + /* 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)); + retval = -1; + goto cleanup; + } if (sepol_get_disable_dontaudit(sh->sepolh) == 1) { FILE *touch; touch = fopen(path, "w"); @@ -1259,10 +1278,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) { + /* 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)); + retval = -1; + goto cleanup; + } + if (sepol_get_preserve_tunables(sh->sepolh) == 1) { FILE *touch; touch = fopen(path, "w"); @@ -1299,40 +1325,25 @@ 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)); + retval = -1; + 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; + } } } @@ -1465,7 +1476,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 +1484,17 @@ 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)); + retval = -1; + goto cleanup; } 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 +1502,13 @@ 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)); + retval = -1; + goto cleanup; } } @@ -1817,6 +1838,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 +1851,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 +1881,13 @@ 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)); + rc = -1; + goto cleanup; + } + rc = semanage_compile_module(sh, _modinfo); if (rc < 0) { goto cleanup; @@ -2004,6 +2032,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 +2364,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 +2778,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 +2879,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..1e71ab67 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,7 @@ 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 && errno == ENONET) { snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE); } @@ -1508,8 +1509,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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: replace access() checks to make setuid programs work 2018-03-09 15:21 ` [PATCH] " Vit Mojzis @ 2018-03-09 15:31 ` Stephen Smalley 2018-03-09 15:39 ` Vit Mojzis 0 siblings, 1 reply; 19+ messages in thread From: Stephen Smalley @ 2018-03-09 15:31 UTC (permalink / raw) To: Vit Mojzis, selinux On 03/09/2018 10:21 AM, Vit Mojzis wrote: > access() uses real UID instead of effective UID which causes false > negative checks in setuid programs. > Replace access() calls (mostly tests for file existence) by stat(). > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > --- > libsemanage/src/direct_api.c | 137 +++++++++++++++++++++++++-------------- > libsemanage/src/semanage_store.c | 11 +++- > 2 files changed, 98 insertions(+), 50 deletions(-) > > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c > index 92d7517d..439122df 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,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) { > + /* 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)); > + retval = -1; > + goto cleanup; > + } > if (sepol_get_disable_dontaudit(sh->sepolh) == 1) { > FILE *touch; > touch = fopen(path, "w"); > @@ -1259,10 +1278,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) { > + /* 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)); > + retval = -1; > + goto cleanup; > + } > + > if (sepol_get_preserve_tunables(sh->sepolh) == 1) { > FILE *touch; > touch = fopen(path, "w"); > @@ -1299,40 +1325,25 @@ 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)); > + retval = -1; > + 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; > + } > } > } > > @@ -1465,7 +1476,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 +1484,17 @@ 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)); > + retval = -1; > + goto cleanup; > } > > 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 +1502,13 @@ 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)); > + retval = -1; > + goto cleanup; > } > } > > @@ -1817,6 +1838,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 +1851,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 +1881,13 @@ 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)); > + rc = -1; > + goto cleanup; > + } > + > rc = semanage_compile_module(sh, _modinfo); > if (rc < 0) { > goto cleanup; > @@ -2004,6 +2032,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 +2364,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 +2778,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 +2879,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..1e71ab67 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,7 @@ 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 && errno == ENONET) { Typo: ENONET vs ENOENT. > snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE); > } > > @@ -1508,8 +1509,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; > } > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] libsemanage: replace access() checks to make setuid programs work 2018-03-09 15:31 ` Stephen Smalley @ 2018-03-09 15:39 ` Vit Mojzis 2018-03-09 15:51 ` Stephen Smalley 2018-03-17 7:26 ` Petr Lautrbach 0 siblings, 2 replies; 19+ messages in thread From: Vit Mojzis @ 2018-03-09 15:39 UTC (permalink / raw) To: selinux access() uses real UID instead of effective UID which causes false negative checks in setuid programs. Replace access() calls (mostly tests for file existence) by stat(). Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 Signed-off-by: Vit Mojzis <vmojzis@redhat.com> --- libsemanage/src/direct_api.c | 137 +++++++++++++++++++++++++-------------- libsemanage/src/semanage_store.c | 11 +++- 2 files changed, 98 insertions(+), 50 deletions(-) diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c index 92d7517d..439122df 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,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) { + /* 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)); + retval = -1; + goto cleanup; + } if (sepol_get_disable_dontaudit(sh->sepolh) == 1) { FILE *touch; touch = fopen(path, "w"); @@ -1259,10 +1278,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) { + /* 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)); + retval = -1; + goto cleanup; + } + if (sepol_get_preserve_tunables(sh->sepolh) == 1) { FILE *touch; touch = fopen(path, "w"); @@ -1299,40 +1325,25 @@ 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)); + retval = -1; + 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; + } } } @@ -1465,7 +1476,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 +1484,17 @@ 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)); + retval = -1; + goto cleanup; } 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 +1502,13 @@ 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)); + retval = -1; + goto cleanup; } } @@ -1817,6 +1838,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 +1851,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 +1881,13 @@ 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)); + rc = -1; + goto cleanup; + } + rc = semanage_compile_module(sh, _modinfo); if (rc < 0) { goto cleanup; @@ -2004,6 +2032,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 +2364,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 +2778,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 +2879,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..14ad99c1 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,7 @@ 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 && errno == ENOENT) { snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE); } @@ -1508,8 +1509,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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: replace access() checks to make setuid programs work 2018-03-09 15:39 ` Vit Mojzis @ 2018-03-09 15:51 ` Stephen Smalley 2018-03-13 10:58 ` Petr Lautrbach 2018-03-17 7:26 ` Petr Lautrbach 1 sibling, 1 reply; 19+ messages in thread From: Stephen Smalley @ 2018-03-09 15:51 UTC (permalink / raw) To: Vit Mojzis, selinux On 03/09/2018 10:39 AM, Vit Mojzis wrote: > access() uses real UID instead of effective UID which causes false > negative checks in setuid programs. > Replace access() calls (mostly tests for file existence) by stat(). > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 Thanks, I've put this up as a PR for testing here: https://github.com/SELinuxProject/selinux/pull/84 I won't be around next week so someone else can merge it or I will get to it when I return. > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > --- > libsemanage/src/direct_api.c | 137 +++++++++++++++++++++++++-------------- > libsemanage/src/semanage_store.c | 11 +++- > 2 files changed, 98 insertions(+), 50 deletions(-) > > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c > index 92d7517d..439122df 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,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) { > + /* 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)); > + retval = -1; > + goto cleanup; > + } > if (sepol_get_disable_dontaudit(sh->sepolh) == 1) { > FILE *touch; > touch = fopen(path, "w"); > @@ -1259,10 +1278,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) { > + /* 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)); > + retval = -1; > + goto cleanup; > + } > + > if (sepol_get_preserve_tunables(sh->sepolh) == 1) { > FILE *touch; > touch = fopen(path, "w"); > @@ -1299,40 +1325,25 @@ 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)); > + retval = -1; > + 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; > + } > } > } > > @@ -1465,7 +1476,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 +1484,17 @@ 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)); > + retval = -1; > + goto cleanup; > } > > 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 +1502,13 @@ 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)); > + retval = -1; > + goto cleanup; > } > } > > @@ -1817,6 +1838,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 +1851,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 +1881,13 @@ 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)); > + rc = -1; > + goto cleanup; > + } > + > rc = semanage_compile_module(sh, _modinfo); > if (rc < 0) { > goto cleanup; > @@ -2004,6 +2032,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 +2364,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 +2778,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 +2879,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..14ad99c1 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,7 @@ 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 && errno == ENOENT) { > snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE); > } > > @@ -1508,8 +1509,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; > } > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: replace access() checks to make setuid programs work 2018-03-09 15:51 ` Stephen Smalley @ 2018-03-13 10:58 ` Petr Lautrbach 0 siblings, 0 replies; 19+ messages in thread From: Petr Lautrbach @ 2018-03-13 10:58 UTC (permalink / raw) To: Stephen Smalley; +Cc: Vit Mojzis, selinux [-- Attachment #1: Type: text/plain, Size: 11950 bytes --] On Fri, Mar 09, 2018 at 10:51:20AM -0500, Stephen Smalley wrote: > On 03/09/2018 10:39 AM, Vit Mojzis wrote: > > access() uses real UID instead of effective UID which causes false > > negative checks in setuid programs. > > Replace access() calls (mostly tests for file existence) by stat(). > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 > > Thanks, I've put this up as a PR for testing here: > https://github.com/SELinuxProject/selinux/pull/84 > > I won't be around next week so someone else can merge it or I will get to it when I return. This is merged now. Thanks! > > > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > > --- > > libsemanage/src/direct_api.c | 137 +++++++++++++++++++++++++-------------- > > libsemanage/src/semanage_store.c | 11 +++- > > 2 files changed, 98 insertions(+), 50 deletions(-) > > > > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c > > index 92d7517d..439122df 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,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) { > > + /* 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)); > > + retval = -1; > > + goto cleanup; > > + } > > if (sepol_get_disable_dontaudit(sh->sepolh) == 1) { > > FILE *touch; > > touch = fopen(path, "w"); > > @@ -1259,10 +1278,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) { > > + /* 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)); > > + retval = -1; > > + goto cleanup; > > + } > > + > > if (sepol_get_preserve_tunables(sh->sepolh) == 1) { > > FILE *touch; > > touch = fopen(path, "w"); > > @@ -1299,40 +1325,25 @@ 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)); > > + retval = -1; > > + 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; > > + } > > } > > } > > > > @@ -1465,7 +1476,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 +1484,17 @@ 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)); > > + retval = -1; > > + goto cleanup; > > } > > > > 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 +1502,13 @@ 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)); > > + retval = -1; > > + goto cleanup; > > } > > } > > > > @@ -1817,6 +1838,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 +1851,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 +1881,13 @@ 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)); > > + rc = -1; > > + goto cleanup; > > + } > > + > > rc = semanage_compile_module(sh, _modinfo); > > if (rc < 0) { > > goto cleanup; > > @@ -2004,6 +2032,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 +2364,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 +2778,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 +2879,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..14ad99c1 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,7 @@ 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 && errno == ENOENT) { > > snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE); > > } > > > > @@ -1508,8 +1509,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; > > } > > > > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: replace access() checks to make setuid programs work 2018-03-09 15:39 ` Vit Mojzis 2018-03-09 15:51 ` Stephen Smalley @ 2018-03-17 7:26 ` Petr Lautrbach 2018-03-19 14:46 ` [PATCH] libsemanage/direct_api.c: Fix iterating over array Vit Mojzis 1 sibling, 1 reply; 19+ messages in thread From: Petr Lautrbach @ 2018-03-17 7:26 UTC (permalink / raw) To: Vit Mojzis; +Cc: selinux [-- Attachment #1: Type: text/plain, Size: 11775 bytes --] On Fri, Mar 09, 2018 at 04:39:44PM +0100, Vit Mojzis wrote: > access() uses real UID instead of effective UID which causes false > negative checks in setuid programs. > Replace access() calls (mostly tests for file existence) by stat(). > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > --- > libsemanage/src/direct_api.c | 137 +++++++++++++++++++++++++-------------- > libsemanage/src/semanage_store.c | 11 +++- > 2 files changed, 98 insertions(+), 50 deletions(-) > > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c > index 92d7517d..439122df 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,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) { > + /* 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)); > + retval = -1; > + goto cleanup; > + } > if (sepol_get_disable_dontaudit(sh->sepolh) == 1) { > FILE *touch; > touch = fopen(path, "w"); > @@ -1259,10 +1278,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) { > + /* 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)); > + retval = -1; > + goto cleanup; > + } > + > if (sepol_get_preserve_tunables(sh->sepolh) == 1) { > FILE *touch; > touch = fopen(path, "w"); > @@ -1299,40 +1325,25 @@ 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++) { The size of the files needs to be divided by size of int, otherwise it causes segfault: # semanage fcontext -m -t httpd_log_t "/var/opt/quest/vas/vasd(/ [1] 3063 segmentation fault (core dumped) semanage fcontext -m -t httpd_log_t "/var/opt/quest/vas/vasd(/.*)?" backtrace: #0 0x00007fffe7901f9b in semanage_path (store=store@entry=SEMANAGE_TMP, path_name=3918632424) at semanage_store.c:487 #1 0x00007fffe78f4be1 in semanage_direct_commit (sh=0x55555840da00) at direct_api.c:1336 #2 0x00007fffe78f9f71 in semanage_commit (sh=0x55555840da00) at handle.c:428 Something like this should fix it: #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) for (i = 0; i < ARRAY_SIZE(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)); > + retval = -1; > + 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; > + } > } > } > > @@ -1465,7 +1476,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 +1484,17 @@ 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)); > + retval = -1; > + goto cleanup; > } > > 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 +1502,13 @@ 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)); > + retval = -1; > + goto cleanup; > } > } > > @@ -1817,6 +1838,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 +1851,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 +1881,13 @@ 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)); > + rc = -1; > + goto cleanup; > + } > + > rc = semanage_compile_module(sh, _modinfo); > if (rc < 0) { > goto cleanup; > @@ -2004,6 +2032,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 +2364,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 +2778,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 +2879,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..14ad99c1 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,7 @@ 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 && errno == ENOENT) { > snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE); > } > > @@ -1508,8 +1509,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 > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] libsemanage/direct_api.c: Fix iterating over array 2018-03-17 7:26 ` Petr Lautrbach @ 2018-03-19 14:46 ` Vit Mojzis 2018-03-19 15:19 ` William Roberts 0 siblings, 1 reply; 19+ messages in thread From: Vit Mojzis @ 2018-03-19 14:46 UTC (permalink / raw) To: selinux Fix sizeof calculation in array iteration introduced by commit 6bb8282c4cf66e93daa9684dbe9c75bb6b1e09a7 "libsemanage: replace access() checks to make setuid programs work" Signed-off-by: Vit Mojzis <vmojzis@redhat.com> --- libsemanage/src/direct_api.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c index 439122df..e7ec952f 100644 --- a/libsemanage/src/direct_api.c +++ b/libsemanage/src/direct_api.c @@ -60,6 +60,7 @@ #define PIPE_READ 0 #define PIPE_WRITE 1 +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) static void semanage_direct_destroy(semanage_handle_t * sh); static int semanage_direct_disconnect(semanage_handle_t * sh); @@ -1332,7 +1333,7 @@ static int semanage_direct_commit(semanage_handle_t * sh) SEMANAGE_SEUSERS_LINKED, SEMANAGE_USERS_EXTRA_LINKED}; - for (i = 0; i < (int) sizeof(files); i++) { + for (i = 0; i < (int) ARRAY_SIZE(files); i++) { path = semanage_path(SEMANAGE_TMP, files[i]); if (stat(path, &sb) != 0) { if (errno != ENOENT) { -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage/direct_api.c: Fix iterating over array 2018-03-19 14:46 ` [PATCH] libsemanage/direct_api.c: Fix iterating over array Vit Mojzis @ 2018-03-19 15:19 ` William Roberts 2018-03-19 16:18 ` William Roberts 0 siblings, 1 reply; 19+ messages in thread From: William Roberts @ 2018-03-19 15:19 UTC (permalink / raw) To: Vit Mojzis; +Cc: selinux On Mon, Mar 19, 2018 at 7:46 AM, Vit Mojzis <vmojzis@redhat.com> wrote: > Fix sizeof calculation in array iteration introduced by commit > 6bb8282c4cf66e93daa9684dbe9c75bb6b1e09a7 > "libsemanage: replace access() checks to make setuid programs work" > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > --- > libsemanage/src/direct_api.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c > index 439122df..e7ec952f 100644 > --- a/libsemanage/src/direct_api.c > +++ b/libsemanage/src/direct_api.c > @@ -60,6 +60,7 @@ > > #define PIPE_READ 0 > #define PIPE_WRITE 1 > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > > static void semanage_direct_destroy(semanage_handle_t * sh); > static int semanage_direct_disconnect(semanage_handle_t * sh); > @@ -1332,7 +1333,7 @@ static int semanage_direct_commit(semanage_handle_t * sh) > SEMANAGE_SEUSERS_LINKED, > SEMANAGE_USERS_EXTRA_LINKED}; > > - for (i = 0; i < (int) sizeof(files); i++) { > + for (i = 0; i < (int) ARRAY_SIZE(files); i++) { > path = semanage_path(SEMANAGE_TMP, files[i]); > if (stat(path, &sb) != 0) { > if (errno != ENOENT) { > -- > 2.14.3 > > ack Just noticing: that code has i as an int, which it probably should be size_t and the files array as an array of int's, it should probably use the enum type. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage/direct_api.c: Fix iterating over array 2018-03-19 15:19 ` William Roberts @ 2018-03-19 16:18 ` William Roberts 0 siblings, 0 replies; 19+ messages in thread From: William Roberts @ 2018-03-19 16:18 UTC (permalink / raw) To: Vit Mojzis; +Cc: selinux On Mon, Mar 19, 2018 at 8:19 AM, William Roberts <bill.c.roberts@gmail.com> wrote: > On Mon, Mar 19, 2018 at 7:46 AM, Vit Mojzis <vmojzis@redhat.com> wrote: >> Fix sizeof calculation in array iteration introduced by commit >> 6bb8282c4cf66e93daa9684dbe9c75bb6b1e09a7 >> "libsemanage: replace access() checks to make setuid programs work" >> >> Signed-off-by: Vit Mojzis <vmojzis@redhat.com> >> --- >> libsemanage/src/direct_api.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c >> index 439122df..e7ec952f 100644 >> --- a/libsemanage/src/direct_api.c >> +++ b/libsemanage/src/direct_api.c >> @@ -60,6 +60,7 @@ >> >> #define PIPE_READ 0 >> #define PIPE_WRITE 1 >> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) >> >> static void semanage_direct_destroy(semanage_handle_t * sh); >> static int semanage_direct_disconnect(semanage_handle_t * sh); >> @@ -1332,7 +1333,7 @@ static int semanage_direct_commit(semanage_handle_t * sh) >> SEMANAGE_SEUSERS_LINKED, >> SEMANAGE_USERS_EXTRA_LINKED}; >> >> - for (i = 0; i < (int) sizeof(files); i++) { >> + for (i = 0; i < (int) ARRAY_SIZE(files); i++) { >> path = semanage_path(SEMANAGE_TMP, files[i]); >> if (stat(path, &sb) != 0) { >> if (errno != ENOENT) { >> -- >> 2.14.3 >> >> > ack Thanks, merged: https://github.com/SELinuxProject/selinux/pull/87 > > Just noticing: that code has i as an int, which it probably should be size_t and > the files array as an array of int's, it should probably use the > enum type. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-03-19 16:18 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-06 11:58 libsemanage: Perform access check using euid instead of uid v2 Vit Mojzis 2018-03-06 11:58 ` [PATCH 1/3] libsemanage: remove access() check to make setuid programs work Vit Mojzis 2018-03-06 11:58 ` [PATCH 2/3] " Vit Mojzis 2018-03-08 19:54 ` Stephen Smalley 2018-03-06 11:58 ` [PATCH 3/3] libsemanage: replace access() checks " Vit Mojzis 2018-03-07 14:59 ` Stephen Smalley 2018-03-08 20:24 ` Stephen Smalley 2018-03-09 13:14 ` Vit Mojzis 2018-03-09 13:28 ` Stephen Smalley 2018-03-09 13:36 ` Stephen Smalley 2018-03-09 15:21 ` [PATCH] " Vit Mojzis 2018-03-09 15:31 ` Stephen Smalley 2018-03-09 15:39 ` Vit Mojzis 2018-03-09 15:51 ` Stephen Smalley 2018-03-13 10:58 ` Petr Lautrbach 2018-03-17 7:26 ` Petr Lautrbach 2018-03-19 14:46 ` [PATCH] libsemanage/direct_api.c: Fix iterating over array Vit Mojzis 2018-03-19 15:19 ` William Roberts 2018-03-19 16:18 ` William Roberts
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.