On 03/06/2018 06:58 AM, Vit Mojzis wrote:
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
And access(,R_OK) by fopen(,"r")
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
Signed-off-by: Vit Mojzis <vmojzis@xxxxxxxxxx>
---
libsemanage/src/direct_api.c | 133 +++++++++++++++++++++++++--------------
libsemanage/src/semanage_store.c | 16 ++++-
2 files changed, 99 insertions(+), 50 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 49cac14f..4e2af810 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
int semanage_direct_connect(semanage_handle_t * sh)
{
const char *path;
+ struct stat sb;
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
@@ -302,10 +303,16 @@ int semanage_direct_connect(semanage_handle_t * sh)
/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
+ else if (errno == ENOENT) {
+ /* The file does not exist */
sepol_set_disable_dontaudit(sh->sepolh, 0);
+ } else {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto err;
+ }
return STATUS_SUCCESS;
@@ -1139,6 +1146,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;
assert(sh);
assert(modinfos);
@@ -1155,9 +1163,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+ goto cleanup; //an error in the "stat" call
+ }
status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1196,6 +1208,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
struct cil_db *cildb = NULL;
semanage_module_info_t *modinfos = NULL;
mode_t mask = umask(0077);
+ struct stat sb;
int do_rebuild, do_write_kernel, do_install;
int fcontexts_modified, ports_modified, seusers_modified,
@@ -1234,10 +1247,15 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
- else
+ else if (errno == ENOENT) {
+ /* The file does not exist */
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+ } else {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
We need to ensure that retval is set in all of these cases. Safest to always set it before goto cleanup,
either explicitly to -1 or to return value of stat() call above.
+ 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;
}
