libsemanage: Perform access check using euid instead of uid v2

[Vit Mojzis]

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

[Vit Mojzis]

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 <***@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;
}
}

[Vit Mojzis]

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 <***@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

[Stephen Smalley]

Post by Vit Mojzis
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).

Post by Vit Mojzis
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
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
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.
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

[Vit Mojzis]

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 <***@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;
}

[Stephen Smalley]

Post by Vit Mojzis
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
---
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.

Post by Vit Mojzis
+ 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

Post by Vit Mojzis
+
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

Post by Vit Mojzis
+ }
- 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;
+ }
}
}
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),
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

Post by Vit Mojzis
}
+
+
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),
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

Post by Vit Mojzis
}
}
@@ -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

Post by Vit Mojzis
+ 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.

Post by Vit Mojzis
+ }
+
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;
}

[Stephen Smalley]

Post by Stephen Smalley

Post by Vit Mojzis
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.

Post by Stephen Smalley

Post by Vit Mojzis
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
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.

Post by Vit Mojzis
+ 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

Post by Vit Mojzis
+
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

Post by Vit Mojzis
+ }
- 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;
+ }
}
}
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),
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

Post by Vit Mojzis
}
+
+
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),
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

Post by Vit Mojzis
}
}
@@ -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

Post by Vit Mojzis
+ 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.

Post by Vit Mojzis
+ }
+
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;
}

[Vit Mojzis]

Post by Stephen Smalley

Post by Vit Mojzis
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
---
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)?

Post by Stephen Smalley

Post by Vit Mojzis
+ 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

Post by Vit Mojzis
+
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

Post by Vit Mojzis
+ }
- 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;
+ }
}
}
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),
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

Post by Vit Mojzis
}
+
+
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),
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

Post by Vit Mojzis
}
}
@@ -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

Post by Vit Mojzis
+ 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.

Post by Vit Mojzis
+ }
+
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;
}

[Stephen Smalley]

Post by Vit Mojzis

Post by Vit Mojzis
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
---
  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;
      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.

Post by Vit Mojzis

Post by Vit Mojzis
+        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

Post by Vit Mojzis
+
      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

Post by Vit Mojzis
+                }
  -        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;
+            }
          }
      }
              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),
              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

Post by Vit Mojzis
          }
  +
+
          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),
              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

Post by Vit Mojzis
          }
      }
      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

Post by Vit Mojzis
+            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.

Post by Vit Mojzis
+        }
+
          snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
      }
  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;
      }
 

[Stephen Smalley]

Post by Stephen Smalley

Post by Vit Mojzis

Post by Vit Mojzis
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
---
  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;
      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)?

retval = foo(args);
if (retval < 0) {
ERR(...)
goto cleanup;
}
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.

Post by Stephen Smalley

Post by Vit Mojzis

Post by Vit Mojzis
+        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

Post by Vit Mojzis
+
      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

Post by Vit Mojzis
+                }
  -        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;
+            }
          }
      }
              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),
              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

Post by Vit Mojzis
          }
  +
+
          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),
              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

Post by Vit Mojzis
          }
      }
      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

Post by Vit Mojzis
+            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.

Post by Vit Mojzis
+        }
+
          snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
      }
  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;
      }
 

[Vit Mojzis]

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 <***@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;
}

[Stephen Smalley]

Post by Vit Mojzis
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
---
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;
+ }
}
}
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),
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),
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.

Post by Vit Mojzis
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;
}

[Vit Mojzis]

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 <***@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;
}

[Stephen Smalley]

Post by Vit Mojzis
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.

Post by Vit Mojzis
---
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;
+ }
}
}
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),
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),
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;
}

[Petr Lautrbach]

Post by Stephen Smalley

Post by Vit Mojzis
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

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!

Post by Stephen Smalley

Post by Vit Mojzis
---
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;
+ }
}
}
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),
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),
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;
}

[Petr Lautrbach]

Post by Vit Mojzis
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
---
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=***@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++) {
...

Post by Vit Mojzis
+ 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;
+ }
}
}
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),
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),
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

[Vit Mojzis]

Fix sizeof calculation in array iteration introduced by commit
6bb8282c4cf66e93daa9684dbe9c75bb6b1e09a7
"libsemanage: replace access() checks to make setuid programs work"

Signed-off-by: Vit Mojzis <***@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) {

[William Roberts]

Post by Vit Mojzis
Fix sizeof calculation in array iteration introduced by commit
6bb8282c4cf66e93daa9684dbe9c75bb6b1e09a7
"libsemanage: replace access() checks to make setuid programs work"
---
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.

[William Roberts]

On Mon, Mar 19, 2018 at 8:19 AM, William Roberts

Post by William Roberts

Post by Vit Mojzis
Fix sizeof calculation in array iteration introduced by commit
6bb8282c4cf66e93daa9684dbe9c75bb6b1e09a7
"libsemanage: replace access() checks to make setuid programs work"
---
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

Post by William Roberts
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.