libsemanage: Perform access check using euid instead of uid v2

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

[PATCH 1/3] libsemanage: remove access() check to make setuid programs work

[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 <vmojzis@redhat.com>
---
 libsemanage/src/direct_api.c     |  7 -------
 libsemanage/src/semanage_store.c | 17 ++++++++---------
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 88873c43..b7899d68 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -148,9 +148,6 @@ int semanage_direct_connect(semanage_handle_t * sh)
 		if (semanage_create_store(sh, 1))
 			goto err;
 
-	if (semanage_access_check(sh) < SEMANAGE_CAN_READ)
-		goto err;
-
 	sh->u.direct.translock_file_fd = -1;
 	sh->u.direct.activelock_file_fd = -1;
 
@@ -398,10 +395,6 @@ static int semanage_direct_disconnect(semanage_handle_t *sh)
 
 static int semanage_direct_begintrans(semanage_handle_t * sh)
 {
-
-	if (semanage_access_check(sh) != SEMANAGE_CAN_WRITE) {
-		return -1;
-	}
 	if (semanage_get_trans_lock(sh) < 0) {
 		return -1;
 	}
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 936e6495..4bd1d651 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -538,7 +538,6 @@ char *semanage_conf_path(void)
 int semanage_create_store(semanage_handle_t * sh, int create)
 {
 	struct stat sb;
-	int mode_mask = R_OK | W_OK | X_OK;
 	const char *path = semanage_files[SEMANAGE_ROOT];
 	int fd;
 
@@ -557,9 +556,9 @@ int semanage_create_store(semanage_handle_t * sh, int create)
 			return -1;
 		}
 	} else {
-		if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) == -1) {
+		if (!S_ISDIR(sb.st_mode)) {
 			ERR(sh,
-			    "Could not access module store at %s, or it is not a directory.",
+			    "Module store at %s is not a directory.",
 			    path);
 			return -1;
 		}
@@ -580,9 +579,9 @@ int semanage_create_store(semanage_handle_t * sh, int create)
 			return -1;
 		}
 	} else {
-		if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) == -1) {
+		if (!S_ISDIR(sb.st_mode)) {
 			ERR(sh,
-			    "Could not access module store active subdirectory at %s, or it is not a directory.",
+			    "Module store active subdirectory at %s is not a directory.",
 			    path);
 			return -1;
 		}
@@ -603,9 +602,9 @@ int semanage_create_store(semanage_handle_t * sh, int create)
 			return -1;
 		}
 	} else {
-		if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) == -1) {
+		if (!S_ISDIR(sb.st_mode)) {
 			ERR(sh,
-			    "Could not access module store active modules subdirectory at %s, or it is not a directory.",
+			    "Module store active modules subdirectory at %s is not a directory.",
 			    path);
 			return -1;
 		}
@@ -624,8 +623,8 @@ int semanage_create_store(semanage_handle_t * sh, int create)
 			return -1;
 		}
 	} else {
-		if (!S_ISREG(sb.st_mode) || access(path, R_OK | W_OK) == -1) {
-			ERR(sh, "Could not access lock file at %s.", path);
+		if (!S_ISREG(sb.st_mode)) {
+			ERR(sh, "Object at %s is not a lock file.", path);
 			return -1;
 		}
 	}
-- 
2.14.3

[PATCH 2/3] libsemanage: remove access() check to make setuid programs work

[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 <vmojzis@redhat.com>
---
 libsemanage/src/direct_api.c | 47 ++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index b7899d68..49cac14f 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -1171,6 +1171,14 @@ cleanup:
 	return status;
 }
 
+/* Copies a file from src to dst. If dst already exists then
+ * overwrite it. If source doesn't exist then return success.
+ * Returns 0 on success, -1 on error. */
+static int copy_file_if_exists(const char *src, const char *dst, mode_t mode){
+	int rc = semanage_copy_file(src, dst, mode);
+	return (rc < 0 && errno != ENOENT) ? rc : 0;
+}
+
 /********************* direct API functions ********************/
 
 /* Commits all changes in sandbox to the actual kernel policy.
@@ -1563,34 +1571,25 @@ rebuild:
 		goto cleanup;
 	}
 
-	path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL);
-	if (access(path, F_OK) == 0) {
-		retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
-							semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
-							sh->conf->file_mode);
-		if (retval < 0) {
-			goto cleanup;
-		}
+	retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
+						semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
+						sh->conf->file_mode);
+	if (retval < 0) {
+		goto cleanup;
 	}
 
-	path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
-	if (access(path, F_OK) == 0) {
-		retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
-							semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
-							sh->conf->file_mode);
-		if (retval < 0) {
-			goto cleanup;
-		}
+	retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
+						semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
+						sh->conf->file_mode);
+	if (retval < 0) {
+		goto cleanup;
 	}
 
-	path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
-	if (access(path, F_OK) == 0) {
-		retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
-							semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
-							sh->conf->file_mode);
-		if (retval < 0) {
-			goto cleanup;
-		}
+	retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
+						semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
+						sh->conf->file_mode);
+	if (retval < 0) {
+		goto cleanup;
 	}
 
 	/* run genhomedircon if its enabled, this should be the last operation
-- 
2.14.3

Re: [PATCH 2/3] libsemanage: remove access() check to make setuid programs work

[Stephen Smalley]

On 03/06/2018 06:58 AM, Vit Mojzis wrote:
> F_OK access checks only work properly as long as all directories along
> the path are accessible to real user running the program.
> Replace F_OK access checks by testing return value of open, write, etc.

Applied patches 1 and 2 (not 3 as per my comments).

> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
> 
> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
> ---
>  libsemanage/src/direct_api.c | 47 ++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index b7899d68..49cac14f 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -1171,6 +1171,14 @@ cleanup:
>  	return status;
>  }
>  
> +/* Copies a file from src to dst. If dst already exists then
> + * overwrite it. If source doesn't exist then return success.
> + * Returns 0 on success, -1 on error. */
> +static int copy_file_if_exists(const char *src, const char *dst, mode_t mode){
> +	int rc = semanage_copy_file(src, dst, mode);
> +	return (rc < 0 && errno != ENOENT) ? rc : 0;
> +}
> +
>  /********************* direct API functions ********************/
>  
>  /* Commits all changes in sandbox to the actual kernel policy.
> @@ -1563,34 +1571,25 @@ rebuild:
>  		goto cleanup;
>  	}
>  
> -	path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL);
> -	if (access(path, F_OK) == 0) {
> -		retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
> -							semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
> -							sh->conf->file_mode);
> -		if (retval < 0) {
> -			goto cleanup;
> -		}
> +	retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
> +						semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
> +						sh->conf->file_mode);
> +	if (retval < 0) {
> +		goto cleanup;
>  	}
>  
> -	path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
> -	if (access(path, F_OK) == 0) {
> -		retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
> -							semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
> -							sh->conf->file_mode);
> -		if (retval < 0) {
> -			goto cleanup;
> -		}
> +	retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
> +						semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
> +						sh->conf->file_mode);
> +	if (retval < 0) {
> +		goto cleanup;
>  	}
>  
> -	path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
> -	if (access(path, F_OK) == 0) {
> -		retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
> -							semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
> -							sh->conf->file_mode);
> -		if (retval < 0) {
> -			goto cleanup;
> -		}
> +	retval = copy_file_if_exists(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
> +						semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
> +						sh->conf->file_mode);
> +	if (retval < 0) {
> +		goto cleanup;
>  	}
>  
>  	/* run genhomedircon if its enabled, this should be the last operation
> 

[PATCH 3/3] libsemanage: replace access() checks to make setuid programs work

[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 <vmojzis@redhat.com>
---
 libsemanage/src/direct_api.c     | 133 +++++++++++++++++++++++++--------------
 libsemanage/src/semanage_store.c |  16 ++++-
 2 files changed, 99 insertions(+), 50 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 49cac14f..4e2af810 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
 int semanage_direct_connect(semanage_handle_t * sh)
 {
 	const char *path;
+	struct stat sb;
 
 	if (semanage_check_init(sh, sh->conf->store_root_path))
 		goto err;
@@ -302,10 +303,16 @@ int semanage_direct_connect(semanage_handle_t * sh)
 
 	/* set the disable dontaudit value */
 	path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
-	if (access(path, F_OK) == 0)
+
+	if (stat(path, &sb) == 0)
 		sepol_set_disable_dontaudit(sh->sepolh, 1);
-	else
+	else if (errno == ENOENT) {
+		/* The file does not exist */
 		sepol_set_disable_dontaudit(sh->sepolh, 0);
+	} else {
+		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+		goto err;
+	}
 
 	return STATUS_SUCCESS;
 
@@ -1139,6 +1146,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
 	int status = 0;
 	int i;
 	char cil_path[PATH_MAX];
+	struct stat sb;
 
 	assert(sh);
 	assert(modinfos);
@@ -1155,9 +1163,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
 		}
 
 		if (semanage_get_ignore_module_cache(sh) == 0 &&
-				access(cil_path, F_OK) == 0) {
+				(status = stat(cil_path, &sb)) == 0) {
 			continue;
 		}
+		if (status != 0 && errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+			goto cleanup; //an error in the "stat" call
+		}
 
 		status = semanage_compile_module(sh, &modinfos[i]);
 		if (status < 0) {
@@ -1196,6 +1208,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 	struct cil_db *cildb = NULL;
 	semanage_module_info_t *modinfos = NULL;
 	mode_t mask = umask(0077);
+	struct stat sb;
 
 	int do_rebuild, do_write_kernel, do_install;
 	int fcontexts_modified, ports_modified, seusers_modified,
@@ -1234,10 +1247,15 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 
 	/* Create or remove the disable_dontaudit flag file. */
 	path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
-	if (access(path, F_OK) == 0)
+	if (stat(path, &sb) == 0)
 		do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
-	else
+	else if (errno == ENOENT) {
+		/* The file does not exist */
 		do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+	} else {
+		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+		goto cleanup;
+	}
 	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
 		FILE *touch;
 		touch = fopen(path, "w");
@@ -1259,10 +1277,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 
 	/* Create or remove the preserve_tunables flag file. */
 	path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
-	if (access(path, F_OK) == 0)
+	if (stat(path, &sb) == 0)
 		do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
-	else
+	else if (errno == ENOENT) {
+		/* The file does not exist */
 		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+	} else {
+		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+		goto cleanup;
+	}
+
 	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
 		FILE *touch;
 		touch = fopen(path, "w");
@@ -1299,40 +1323,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 	 * a rebuild.
 	 */
 	if (!do_rebuild) {
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
+		int files[] = {SEMANAGE_STORE_KERNEL,
+					   SEMANAGE_STORE_FC,
+					   SEMANAGE_STORE_SEUSERS,
+					   SEMANAGE_LINKED,
+					   SEMANAGE_SEUSERS_LINKED,
+					   SEMANAGE_USERS_EXTRA_LINKED};
+
+		for (i = 0; i < (int) sizeof(files); i++) {
+			path = semanage_path(SEMANAGE_TMP, files[i]);
+			if (stat(path, &sb) != 0) {
+				if (errno != ENOENT) {
+					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+					goto cleanup;
+				}
 
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
+				do_rebuild = 1;
+				goto rebuild;
+			}
 		}
 	}
 
@@ -1465,7 +1473,7 @@ rebuild:
 			goto cleanup;
 
 		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
-		if (access(path, F_OK) == 0) {
+		if (stat(path, &sb) == 0) {
 			retval = semanage_copy_file(path,
 						    semanage_path(SEMANAGE_TMP,
 								  SEMANAGE_STORE_SEUSERS),
@@ -1473,12 +1481,18 @@ rebuild:
 			if (retval < 0)
 				goto cleanup;
 			pseusers->dtable->drop_cache(pseusers->dbase);
-		} else {
+		} else if (errno == ENOENT) {
+			/* The file does not exist */
 			pseusers->dtable->clear(sh, pseusers->dbase);
+		} else {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			goto cleanup;
 		}
 
+
+
 		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
-		if (access(path, F_OK) == 0) {
+		if (stat(path, &sb) == 0) {
 			retval = semanage_copy_file(path,
 						    semanage_path(SEMANAGE_TMP,
 								  SEMANAGE_USERS_EXTRA),
@@ -1486,8 +1500,12 @@ rebuild:
 			if (retval < 0)
 				goto cleanup;
 			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
-		} else {
+		} else if (errno == ENOENT) {
+			/* The file does not exist */
 			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
+		} else {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			goto cleanup;
 		}
 	}
 
@@ -1817,6 +1835,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
 	ssize_t _data_len;
 	char *_data;
 	int compressed;
+	struct stat sb;
 
 	/* get path of module */
 	rc = semanage_module_get_path(
@@ -1829,8 +1848,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
 		goto cleanup;
 	}
 
-	if (access(module_path, F_OK) != 0) {
-		ERR(sh, "Module does not exist: %s", module_path);
+	if (stat(module_path, &sb) != 0) {
+		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
 		rc = -1;
 		goto cleanup;
 	}
@@ -1859,7 +1878,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
 		goto cleanup;
 	}
 
-	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+			goto cleanup;
+		}
+
 		rc = semanage_compile_module(sh, _modinfo);
 		if (rc < 0) {
 			goto cleanup;
@@ -2004,6 +2028,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
 	}
 
 	if (stat(path, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			status = -1;
+			goto cleanup;
+		}
+
 		*enabled = 1;
 	}
 	else {
@@ -2330,6 +2360,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
 
 	/* set enabled/disabled status */
 	if (stat(fn, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+			status = -1;
+			goto cleanup;
+		}
+
 		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
 		if (ret != 0) {
 			status = -1;
@@ -2738,6 +2774,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
 	int status = 0;
 	int ret = 0;
 	int type;
+	struct stat sb;
 
 	char path[PATH_MAX];
 	mode_t mask = umask(0077);
@@ -2838,7 +2875,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
 			goto cleanup;
 		}
 
-		if (access(path, F_OK) == 0) {
+		if (stat(path, &sb) == 0) {
 			ret = unlink(path);
 			if (ret != 0) {
 				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..10f8b306 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
 {
 	char *semanage_conf = NULL;
 	int len;
+	struct stat sb;
 
 	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
 	semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,12 @@ char *semanage_conf_path(void)
 	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
 		 SEMANAGE_CONF_FILE);
 
-	if (access(semanage_conf, R_OK) != 0) {
+	if (stat(semanage_conf, &sb) != 0) {
+		if (errno != ENOENT) {
+			/* semanage_conf exists but can't be accessed */
+			return NULL;
+		}
+
 		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
 	}
 
@@ -1508,8 +1514,14 @@ int semanage_split_fc(semanage_handle_t * sh)
 static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
 
 	int r;
+	struct stat sb;
+
+	if (stat(path, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			return -1;
+		}
 
-	if (access(path, F_OK) != 0) {
 		return 0;
 	}
 
-- 
2.14.3

Re: [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work

[Stephen Smalley]

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

We need to ensure that retval is set in all of these cases.  Safest to always set it before goto cleanup,
either explicitly to -1 or to return value of stat() call above.

> +		goto cleanup;
> +	}
>  	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>  		FILE *touch;
>  		touch = fopen(path, "w");
> @@ -1259,10 +1277,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  
>  	/* Create or remove the preserve_tunables flag file. */
>  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
> -	if (access(path, F_OK) == 0)
> +	if (stat(path, &sb) == 0)
>  		do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
> -	else
> +	else if (errno == ENOENT) {
> +		/* The file does not exist */
>  		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
> +	} else {
> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +		goto cleanup;
> +	}

Ditto

> +
>  	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>  		FILE *touch;
>  		touch = fopen(path, "w");
> @@ -1299,40 +1323,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  	 * a rebuild.
>  	 */
>  	if (!do_rebuild) {
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> +		int files[] = {SEMANAGE_STORE_KERNEL,
> +					   SEMANAGE_STORE_FC,
> +					   SEMANAGE_STORE_SEUSERS,
> +					   SEMANAGE_LINKED,
> +					   SEMANAGE_SEUSERS_LINKED,
> +					   SEMANAGE_USERS_EXTRA_LINKED};
> +
> +		for (i = 0; i < (int) sizeof(files); i++) {
> +			path = semanage_path(SEMANAGE_TMP, files[i]);
> +			if (stat(path, &sb) != 0) {
> +				if (errno != ENOENT) {
> +					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +					goto cleanup;

Ditto

> +				}
>  
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> +				do_rebuild = 1;
> +				goto rebuild;
> +			}
>  		}
>  	}
>  
> @@ -1465,7 +1473,7 @@ rebuild:
>  			goto cleanup;
>  
>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			retval = semanage_copy_file(path,
>  						    semanage_path(SEMANAGE_TMP,
>  								  SEMANAGE_STORE_SEUSERS),
> @@ -1473,12 +1481,18 @@ rebuild:
>  			if (retval < 0)
>  				goto cleanup;
>  			pseusers->dtable->drop_cache(pseusers->dbase);
> -		} else {
> +		} else if (errno == ENOENT) {
> +			/* The file does not exist */
>  			pseusers->dtable->clear(sh, pseusers->dbase);
> +		} else {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			goto cleanup;

Ditto

>  		}
>  
> +
> +
>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			retval = semanage_copy_file(path,
>  						    semanage_path(SEMANAGE_TMP,
>  								  SEMANAGE_USERS_EXTRA),
> @@ -1486,8 +1500,12 @@ rebuild:
>  			if (retval < 0)
>  				goto cleanup;
>  			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
> -		} else {
> +		} else if (errno == ENOENT) {
> +			/* The file does not exist */
>  			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
> +		} else {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			goto cleanup;

Ditto

>  		}
>  	}
>  
> @@ -1817,6 +1835,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  	ssize_t _data_len;
>  	char *_data;
>  	int compressed;
> +	struct stat sb;
>  
>  	/* get path of module */
>  	rc = semanage_module_get_path(
> @@ -1829,8 +1848,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  		goto cleanup;
>  	}
>  
> -	if (access(module_path, F_OK) != 0) {
> -		ERR(sh, "Module does not exist: %s", module_path);
> +	if (stat(module_path, &sb) != 0) {
> +		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>  		rc = -1;
>  		goto cleanup;
>  	}
> @@ -1859,7 +1878,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  		goto cleanup;
>  	}
>  
> -	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
> +	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));

Ditto

> +			goto cleanup;
> +	
> +
>  		rc = semanage_compile_module(sh, _modinfo);
>  		if (rc < 0) {
>  			goto cleanup;
> @@ -2004,6 +2028,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>  	}
>  
>  	if (stat(path, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			status = -1;
> +			goto cleanup;
> +		}
> +
>  		*enabled = 1;
>  	}
>  	else {
> @@ -2330,6 +2360,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>  
>  	/* set enabled/disabled status */
>  	if (stat(fn, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
> +			status = -1;
> +			goto cleanup;
> +		}
> +
>  		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>  		if (ret != 0) {
>  			status = -1;
> @@ -2738,6 +2774,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>  	int status = 0;
>  	int ret = 0;
>  	int type;
> +	struct stat sb;
>  
>  	char path[PATH_MAX];
>  	mode_t mask = umask(0077);
> @@ -2838,7 +2875,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>  			goto cleanup;
>  		}
>  
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			ret = unlink(path);
>  			if (ret != 0) {
>  				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 4bd1d651..10f8b306 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>  {
>  	char *semanage_conf = NULL;
>  	int len;
> +	struct stat sb;
>  
>  	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>  	semanage_conf = calloc(len + 1, sizeof(char));
> @@ -522,7 +523,12 @@ char *semanage_conf_path(void)
>  	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>  		 SEMANAGE_CONF_FILE);
>  
> -	if (access(semanage_conf, R_OK) != 0) {
> +	if (stat(semanage_conf, &sb) != 0) {
> +		if (errno != ENOENT) {
> +			/* semanage_conf exists but can't be accessed */
> +			return NULL;

Not sure this is what we want. I think we'd just return semanage_conf here, and then let the caller fail when it tries to access it, so that we get a useful error message with the pathname.

> +		}
> +
>  		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>  	}
>  
> @@ -1508,8 +1514,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>  
>  	int r;
> +	struct stat sb;
> +
> +	if (stat(path, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			return -1;
> +		}
>  
> -	if (access(path, F_OK) != 0) {
>  		return 0;
>  	}
>  
> 

Re: [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work

[Stephen Smalley]

On 03/07/2018 09:59 AM, Stephen Smalley wrote:
> On 03/06/2018 06:58 AM, Vit Mojzis wrote:
>> access() uses real UID instead of effective UID which causes false
>> negative checks in setuid programs.
>> Replace access(,F_OK) (i.e. tests for file existence) by stat().
>> And access(,R_OK) by fopen(,"r")

Also, in addition to fixing the code, please update the patch description since the last line is no longer accurate.

>>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
>>
>> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
>> ---
>>  libsemanage/src/direct_api.c     | 133 +++++++++++++++++++++++++--------------
>>  libsemanage/src/semanage_store.c |  16 ++++-
>>  2 files changed, 99 insertions(+), 50 deletions(-)
>>
>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>> index 49cac14f..4e2af810 100644
>> --- a/libsemanage/src/direct_api.c
>> +++ b/libsemanage/src/direct_api.c
>> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>>  int semanage_direct_connect(semanage_handle_t * sh)
>>  {
>>  	const char *path;
>> +	struct stat sb;
>>  
>>  	if (semanage_check_init(sh, sh->conf->store_root_path))
>>  		goto err;
>> @@ -302,10 +303,16 @@ int semanage_direct_connect(semanage_handle_t * sh)
>>  
>>  	/* set the disable dontaudit value */
>>  	path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
>> -	if (access(path, F_OK) == 0)
>> +
>> +	if (stat(path, &sb) == 0)
>>  		sepol_set_disable_dontaudit(sh->sepolh, 1);
>> -	else
>> +	else if (errno == ENOENT) {
>> +		/* The file does not exist */
>>  		sepol_set_disable_dontaudit(sh->sepolh, 0);
>> +	} else {
>> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +		goto err;
>> +	}
>>  
>>  	return STATUS_SUCCESS;
>>  
>> @@ -1139,6 +1146,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
>>  	int status = 0;
>>  	int i;
>>  	char cil_path[PATH_MAX];
>> +	struct stat sb;
>>  
>>  	assert(sh);
>>  	assert(modinfos);
>> @@ -1155,9 +1163,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
>>  		}
>>  
>>  		if (semanage_get_ignore_module_cache(sh) == 0 &&
>> -				access(cil_path, F_OK) == 0) {
>> +				(status = stat(cil_path, &sb)) == 0) {
>>  			continue;
>>  		}
>> +		if (status != 0 && errno != ENOENT) {
>> +			ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
>> +			goto cleanup; //an error in the "stat" call
>> +		}
>>  
>>  		status = semanage_compile_module(sh, &modinfos[i]);
>>  		if (status < 0) {
>> @@ -1196,6 +1208,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>  	struct cil_db *cildb = NULL;
>>  	semanage_module_info_t *modinfos = NULL;
>>  	mode_t mask = umask(0077);
>> +	struct stat sb;
>>  
>>  	int do_rebuild, do_write_kernel, do_install;
>>  	int fcontexts_modified, ports_modified, seusers_modified,
>> @@ -1234,10 +1247,15 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>  
>>  	/* Create or remove the disable_dontaudit flag file. */
>>  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
>> -	if (access(path, F_OK) == 0)
>> +	if (stat(path, &sb) == 0)
>>  		do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
>> -	else
>> +	else if (errno == ENOENT) {
>> +		/* The file does not exist */
>>  		do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
>> +	} else {
>> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> 
> We need to ensure that retval is set in all of these cases.  Safest to always set it before goto cleanup,
> either explicitly to -1 or to return value of stat() call above.
> 
>> +		goto cleanup;
>> +	}
>>  	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>>  		FILE *touch;
>>  		touch = fopen(path, "w");
>> @@ -1259,10 +1277,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>  
>>  	/* Create or remove the preserve_tunables flag file. */
>>  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
>> -	if (access(path, F_OK) == 0)
>> +	if (stat(path, &sb) == 0)
>>  		do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
>> -	else
>> +	else if (errno == ENOENT) {
>> +		/* The file does not exist */
>>  		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
>> +	} else {
>> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +		goto cleanup;
>> +	}
> 
> Ditto
> 
>> +
>>  	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>>  		FILE *touch;
>>  		touch = fopen(path, "w");
>> @@ -1299,40 +1323,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>  	 * a rebuild.
>>  	 */
>>  	if (!do_rebuild) {
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> -		}
>> -
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> -		}
>> -
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> -		}
>> -
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> -		}
>> -
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> -		}
>> +		int files[] = {SEMANAGE_STORE_KERNEL,
>> +					   SEMANAGE_STORE_FC,
>> +					   SEMANAGE_STORE_SEUSERS,
>> +					   SEMANAGE_LINKED,
>> +					   SEMANAGE_SEUSERS_LINKED,
>> +					   SEMANAGE_USERS_EXTRA_LINKED};
>> +
>> +		for (i = 0; i < (int) sizeof(files); i++) {
>> +			path = semanage_path(SEMANAGE_TMP, files[i]);
>> +			if (stat(path, &sb) != 0) {
>> +				if (errno != ENOENT) {
>> +					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +					goto cleanup;
> 
> Ditto
> 
>> +				}
>>  
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> +				do_rebuild = 1;
>> +				goto rebuild;
>> +			}
>>  		}
>>  	}
>>  
>> @@ -1465,7 +1473,7 @@ rebuild:
>>  			goto cleanup;
>>  
>>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>> -		if (access(path, F_OK) == 0) {
>> +		if (stat(path, &sb) == 0) {
>>  			retval = semanage_copy_file(path,
>>  						    semanage_path(SEMANAGE_TMP,
>>  								  SEMANAGE_STORE_SEUSERS),
>> @@ -1473,12 +1481,18 @@ rebuild:
>>  			if (retval < 0)
>>  				goto cleanup;
>>  			pseusers->dtable->drop_cache(pseusers->dbase);
>> -		} else {
>> +		} else if (errno == ENOENT) {
>> +			/* The file does not exist */
>>  			pseusers->dtable->clear(sh, pseusers->dbase);
>> +		} else {
>> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +			goto cleanup;
> 
> Ditto
> 
>>  		}
>>  
>> +
>> +
>>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>> -		if (access(path, F_OK) == 0) {
>> +		if (stat(path, &sb) == 0) {
>>  			retval = semanage_copy_file(path,
>>  						    semanage_path(SEMANAGE_TMP,
>>  								  SEMANAGE_USERS_EXTRA),
>> @@ -1486,8 +1500,12 @@ rebuild:
>>  			if (retval < 0)
>>  				goto cleanup;
>>  			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>> -		} else {
>> +		} else if (errno == ENOENT) {
>> +			/* The file does not exist */
>>  			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>> +		} else {
>> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +			goto cleanup;
> 
> Ditto
> 
>>  		}
>>  	}
>>  
>> @@ -1817,6 +1835,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>  	ssize_t _data_len;
>>  	char *_data;
>>  	int compressed;
>> +	struct stat sb;
>>  
>>  	/* get path of module */
>>  	rc = semanage_module_get_path(
>> @@ -1829,8 +1848,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>  		goto cleanup;
>>  	}
>>  
>> -	if (access(module_path, F_OK) != 0) {
>> -		ERR(sh, "Module does not exist: %s", module_path);
>> +	if (stat(module_path, &sb) != 0) {
>> +		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>>  		rc = -1;
>>  		goto cleanup;
>>  	}
>> @@ -1859,7 +1878,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>  		goto cleanup;
>>  	}
>>  
>> -	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
>> +	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
>> +		if (errno != ENOENT) {
>> +			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
> 
> Ditto
> 
>> +			goto cleanup;
>> +	
>> +
>>  		rc = semanage_compile_module(sh, _modinfo);
>>  		if (rc < 0) {
>>  			goto cleanup;
>> @@ -2004,6 +2028,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>>  	}
>>  
>>  	if (stat(path, &sb) < 0) {
>> +		if (errno != ENOENT) {
>> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +			status = -1;
>> +			goto cleanup;
>> +		}
>> +
>>  		*enabled = 1;
>>  	}
>>  	else {
>> @@ -2330,6 +2360,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>  
>>  	/* set enabled/disabled status */
>>  	if (stat(fn, &sb) < 0) {
>> +		if (errno != ENOENT) {
>> +			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
>> +			status = -1;
>> +			goto cleanup;
>> +		}
>> +
>>  		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>>  		if (ret != 0) {
>>  			status = -1;
>> @@ -2738,6 +2774,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>  	int status = 0;
>>  	int ret = 0;
>>  	int type;
>> +	struct stat sb;
>>  
>>  	char path[PATH_MAX];
>>  	mode_t mask = umask(0077);
>> @@ -2838,7 +2875,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>  			goto cleanup;
>>  		}
>>  
>> -		if (access(path, F_OK) == 0) {
>> +		if (stat(path, &sb) == 0) {
>>  			ret = unlink(path);
>>  			if (ret != 0) {
>>  				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>> index 4bd1d651..10f8b306 100644
>> --- a/libsemanage/src/semanage_store.c
>> +++ b/libsemanage/src/semanage_store.c
>> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>>  {
>>  	char *semanage_conf = NULL;
>>  	int len;
>> +	struct stat sb;
>>  
>>  	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>>  	semanage_conf = calloc(len + 1, sizeof(char));
>> @@ -522,7 +523,12 @@ char *semanage_conf_path(void)
>>  	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>>  		 SEMANAGE_CONF_FILE);
>>  
>> -	if (access(semanage_conf, R_OK) != 0) {
>> +	if (stat(semanage_conf, &sb) != 0) {
>> +		if (errno != ENOENT) {
>> +			/* semanage_conf exists but can't be accessed */
>> +			return NULL;
> 
> Not sure this is what we want. I think we'd just return semanage_conf here, and then let the caller fail when it tries to access it, so that we get a useful error message with the pathname.
> 
>> +		}
>> +
>>  		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>>  	}
>>  
>> @@ -1508,8 +1514,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>>  
>>  	int r;
>> +	struct stat sb;
>> +
>> +	if (stat(path, &sb) < 0) {
>> +		if (errno != ENOENT) {
>> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +			return -1;
>> +		}
>>  
>> -	if (access(path, F_OK) != 0) {
>>  		return 0;
>>  	}
>>  
>>
> 
> 

Re: [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work

[Vit Mojzis]

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

>> +		goto cleanup;
>> +	}
>>   	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>>   		FILE *touch;
>>   		touch = fopen(path, "w");
>> @@ -1259,10 +1277,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>   
>>   	/* Create or remove the preserve_tunables flag file. */
>>   	path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
>> -	if (access(path, F_OK) == 0)
>> +	if (stat(path, &sb) == 0)
>>   		do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
>> -	else
>> +	else if (errno == ENOENT) {
>> +		/* The file does not exist */
>>   		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
>> +	} else {
>> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +		goto cleanup;
>> +	}
> Ditto
>
>> +
>>   	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>>   		FILE *touch;
>>   		touch = fopen(path, "w");
>> @@ -1299,40 +1323,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>   	 * a rebuild.
>>   	 */
>>   	if (!do_rebuild) {
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> -		}
>> -
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> -		}
>> -
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> -		}
>> -
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> -		}
>> -
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> -		}
>> +		int files[] = {SEMANAGE_STORE_KERNEL,
>> +					   SEMANAGE_STORE_FC,
>> +					   SEMANAGE_STORE_SEUSERS,
>> +					   SEMANAGE_LINKED,
>> +					   SEMANAGE_SEUSERS_LINKED,
>> +					   SEMANAGE_USERS_EXTRA_LINKED};
>> +
>> +		for (i = 0; i < (int) sizeof(files); i++) {
>> +			path = semanage_path(SEMANAGE_TMP, files[i]);
>> +			if (stat(path, &sb) != 0) {
>> +				if (errno != ENOENT) {
>> +					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +					goto cleanup;
> Ditto
>
>> +				}
>>   
>> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>> -		if (access(path, F_OK) != 0) {
>> -			do_rebuild = 1;
>> -			goto rebuild;
>> +				do_rebuild = 1;
>> +				goto rebuild;
>> +			}
>>   		}
>>   	}
>>   
>> @@ -1465,7 +1473,7 @@ rebuild:
>>   			goto cleanup;
>>   
>>   		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>> -		if (access(path, F_OK) == 0) {
>> +		if (stat(path, &sb) == 0) {
>>   			retval = semanage_copy_file(path,
>>   						    semanage_path(SEMANAGE_TMP,
>>   								  SEMANAGE_STORE_SEUSERS),
>> @@ -1473,12 +1481,18 @@ rebuild:
>>   			if (retval < 0)
>>   				goto cleanup;
>>   			pseusers->dtable->drop_cache(pseusers->dbase);
>> -		} else {
>> +		} else if (errno == ENOENT) {
>> +			/* The file does not exist */
>>   			pseusers->dtable->clear(sh, pseusers->dbase);
>> +		} else {
>> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +			goto cleanup;
> Ditto
>
>>   		}
>>   
>> +
>> +
>>   		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>> -		if (access(path, F_OK) == 0) {
>> +		if (stat(path, &sb) == 0) {
>>   			retval = semanage_copy_file(path,
>>   						    semanage_path(SEMANAGE_TMP,
>>   								  SEMANAGE_USERS_EXTRA),
>> @@ -1486,8 +1500,12 @@ rebuild:
>>   			if (retval < 0)
>>   				goto cleanup;
>>   			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>> -		} else {
>> +		} else if (errno == ENOENT) {
>> +			/* The file does not exist */
>>   			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>> +		} else {
>> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +			goto cleanup;
> Ditto
>
>>   		}
>>   	}
>>   
>> @@ -1817,6 +1835,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>   	ssize_t _data_len;
>>   	char *_data;
>>   	int compressed;
>> +	struct stat sb;
>>   
>>   	/* get path of module */
>>   	rc = semanage_module_get_path(
>> @@ -1829,8 +1848,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>   		goto cleanup;
>>   	}
>>   
>> -	if (access(module_path, F_OK) != 0) {
>> -		ERR(sh, "Module does not exist: %s", module_path);
>> +	if (stat(module_path, &sb) != 0) {
>> +		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>>   		rc = -1;
>>   		goto cleanup;
>>   	}
>> @@ -1859,7 +1878,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>   		goto cleanup;
>>   	}
>>   
>> -	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
>> +	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
>> +		if (errno != ENOENT) {
>> +			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
> Ditto
>
>> +			goto cleanup;
>> +	
>> +
>>   		rc = semanage_compile_module(sh, _modinfo);
>>   		if (rc < 0) {
>>   			goto cleanup;
>> @@ -2004,6 +2028,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>>   	}
>>   
>>   	if (stat(path, &sb) < 0) {
>> +		if (errno != ENOENT) {
>> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +			status = -1;
>> +			goto cleanup;
>> +		}
>> +
>>   		*enabled = 1;
>>   	}
>>   	else {
>> @@ -2330,6 +2360,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>   
>>   	/* set enabled/disabled status */
>>   	if (stat(fn, &sb) < 0) {
>> +		if (errno != ENOENT) {
>> +			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
>> +			status = -1;
>> +			goto cleanup;
>> +		}
>> +
>>   		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>>   		if (ret != 0) {
>>   			status = -1;
>> @@ -2738,6 +2774,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>   	int status = 0;
>>   	int ret = 0;
>>   	int type;
>> +	struct stat sb;
>>   
>>   	char path[PATH_MAX];
>>   	mode_t mask = umask(0077);
>> @@ -2838,7 +2875,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>   			goto cleanup;
>>   		}
>>   
>> -		if (access(path, F_OK) == 0) {
>> +		if (stat(path, &sb) == 0) {
>>   			ret = unlink(path);
>>   			if (ret != 0) {
>>   				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>> index 4bd1d651..10f8b306 100644
>> --- a/libsemanage/src/semanage_store.c
>> +++ b/libsemanage/src/semanage_store.c
>> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>>   {
>>   	char *semanage_conf = NULL;
>>   	int len;
>> +	struct stat sb;
>>   
>>   	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>>   	semanage_conf = calloc(len + 1, sizeof(char));
>> @@ -522,7 +523,12 @@ char *semanage_conf_path(void)
>>   	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>>   		 SEMANAGE_CONF_FILE);
>>   
>> -	if (access(semanage_conf, R_OK) != 0) {
>> +	if (stat(semanage_conf, &sb) != 0) {
>> +		if (errno != ENOENT) {
>> +			/* semanage_conf exists but can't be accessed */
>> +			return NULL;
> Not sure this is what we want. I think we'd just return semanage_conf here, and then let the caller fail when it tries to access it, so that we get a useful error message with the pathname.
>
>> +		}
>> +
>>   		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>>   	}
>>   
>> @@ -1508,8 +1514,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>>   static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>>   
>>   	int r;
>> +	struct stat sb;
>> +
>> +	if (stat(path, &sb) < 0) {
>> +		if (errno != ENOENT) {
>> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>> +			return -1;
>> +		}
>>   
>> -	if (access(path, F_OK) != 0) {
>>   		return 0;
>>   	}
>>   
>>

Re: [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work

[Stephen Smalley]

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

Yes, I would do that for safety here and in any other case where we didn't just set retval and test it.  IOW, any case where we have:
	retval = foo(args);
	if (retval < 0) {
		ERR(...)
		goto cleanup;
	}
is fine as is, but any case where we have:
	if (some-condition-other-than-retval-less-than-0) {
		ERR(...)
		goto cleanup;
	}
likely ought to explicitly set retval.

If we have too many of those, then maybe we ought to have an err: label that sets retval to -1 explicitly and goto it instead.

> 
>>> +        goto cleanup;
>>> +    }
>>>       if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>>>           FILE *touch;
>>>           touch = fopen(path, "w");
>>> @@ -1259,10 +1277,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>         /* Create or remove the preserve_tunables flag file. */
>>>       path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
>>> -    if (access(path, F_OK) == 0)
>>> +    if (stat(path, &sb) == 0)
>>>           do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
>>> -    else
>>> +    else if (errno == ENOENT) {
>>> +        /* The file does not exist */
>>>           do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
>>> +    } else {
>>> +        ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +        goto cleanup;
>>> +    }
>> Ditto
>>
>>> +
>>>       if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>>>           FILE *touch;
>>>           touch = fopen(path, "w");
>>> @@ -1299,40 +1323,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>        * a rebuild.
>>>        */
>>>       if (!do_rebuild) {
>>> -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
>>> -        if (access(path, F_OK) != 0) {
>>> -            do_rebuild = 1;
>>> -            goto rebuild;
>>> -        }
>>> -
>>> -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
>>> -        if (access(path, F_OK) != 0) {
>>> -            do_rebuild = 1;
>>> -            goto rebuild;
>>> -        }
>>> -
>>> -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
>>> -        if (access(path, F_OK) != 0) {
>>> -            do_rebuild = 1;
>>> -            goto rebuild;
>>> -        }
>>> -
>>> -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
>>> -        if (access(path, F_OK) != 0) {
>>> -            do_rebuild = 1;
>>> -            goto rebuild;
>>> -        }
>>> -
>>> -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>>> -        if (access(path, F_OK) != 0) {
>>> -            do_rebuild = 1;
>>> -            goto rebuild;
>>> -        }
>>> +        int files[] = {SEMANAGE_STORE_KERNEL,
>>> +                       SEMANAGE_STORE_FC,
>>> +                       SEMANAGE_STORE_SEUSERS,
>>> +                       SEMANAGE_LINKED,
>>> +                       SEMANAGE_SEUSERS_LINKED,
>>> +                       SEMANAGE_USERS_EXTRA_LINKED};
>>> +
>>> +        for (i = 0; i < (int) sizeof(files); i++) {
>>> +            path = semanage_path(SEMANAGE_TMP, files[i]);
>>> +            if (stat(path, &sb) != 0) {
>>> +                if (errno != ENOENT) {
>>> +                    ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +                    goto cleanup;
>> Ditto
>>
>>> +                }
>>>   -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>>> -        if (access(path, F_OK) != 0) {
>>> -            do_rebuild = 1;
>>> -            goto rebuild;
>>> +                do_rebuild = 1;
>>> +                goto rebuild;
>>> +            }
>>>           }
>>>       }
>>>   @@ -1465,7 +1473,7 @@ rebuild:
>>>               goto cleanup;
>>>             path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>>> -        if (access(path, F_OK) == 0) {
>>> +        if (stat(path, &sb) == 0) {
>>>               retval = semanage_copy_file(path,
>>>                               semanage_path(SEMANAGE_TMP,
>>>                                     SEMANAGE_STORE_SEUSERS),
>>> @@ -1473,12 +1481,18 @@ rebuild:
>>>               if (retval < 0)
>>>                   goto cleanup;
>>>               pseusers->dtable->drop_cache(pseusers->dbase);
>>> -        } else {
>>> +        } else if (errno == ENOENT) {
>>> +            /* The file does not exist */
>>>               pseusers->dtable->clear(sh, pseusers->dbase);
>>> +        } else {
>>> +            ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +            goto cleanup;
>> Ditto
>>
>>>           }
>>>   +
>>> +
>>>           path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>>> -        if (access(path, F_OK) == 0) {
>>> +        if (stat(path, &sb) == 0) {
>>>               retval = semanage_copy_file(path,
>>>                               semanage_path(SEMANAGE_TMP,
>>>                                     SEMANAGE_USERS_EXTRA),
>>> @@ -1486,8 +1500,12 @@ rebuild:
>>>               if (retval < 0)
>>>                   goto cleanup;
>>>               pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>>> -        } else {
>>> +        } else if (errno == ENOENT) {
>>> +            /* The file does not exist */
>>>               pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>>> +        } else {
>>> +            ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +            goto cleanup;
>> Ditto
>>
>>>           }
>>>       }
>>>   @@ -1817,6 +1835,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>       ssize_t _data_len;
>>>       char *_data;
>>>       int compressed;
>>> +    struct stat sb;
>>>         /* get path of module */
>>>       rc = semanage_module_get_path(
>>> @@ -1829,8 +1848,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>           goto cleanup;
>>>       }
>>>   -    if (access(module_path, F_OK) != 0) {
>>> -        ERR(sh, "Module does not exist: %s", module_path);
>>> +    if (stat(module_path, &sb) != 0) {
>>> +        ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>>>           rc = -1;
>>>           goto cleanup;
>>>       }
>>> @@ -1859,7 +1878,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>           goto cleanup;
>>>       }
>>>   -    if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
>>> +    if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
>>> +        if (errno != ENOENT) {
>>> +            ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
>> Ditto
>>
>>> +            goto cleanup;
>>> +   
>>> +
>>>           rc = semanage_compile_module(sh, _modinfo);
>>>           if (rc < 0) {
>>>               goto cleanup;
>>> @@ -2004,6 +2028,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>>>       }
>>>         if (stat(path, &sb) < 0) {
>>> +        if (errno != ENOENT) {
>>> +            ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +            status = -1;
>>> +            goto cleanup;
>>> +        }
>>> +
>>>           *enabled = 1;
>>>       }
>>>       else {
>>> @@ -2330,6 +2360,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>>         /* set enabled/disabled status */
>>>       if (stat(fn, &sb) < 0) {
>>> +        if (errno != ENOENT) {
>>> +            ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
>>> +            status = -1;
>>> +            goto cleanup;
>>> +        }
>>> +
>>>           ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>>>           if (ret != 0) {
>>>               status = -1;
>>> @@ -2738,6 +2774,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>       int status = 0;
>>>       int ret = 0;
>>>       int type;
>>> +    struct stat sb;
>>>         char path[PATH_MAX];
>>>       mode_t mask = umask(0077);
>>> @@ -2838,7 +2875,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>               goto cleanup;
>>>           }
>>>   -        if (access(path, F_OK) == 0) {
>>> +        if (stat(path, &sb) == 0) {
>>>               ret = unlink(path);
>>>               if (ret != 0) {
>>>                   ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
>>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>>> index 4bd1d651..10f8b306 100644
>>> --- a/libsemanage/src/semanage_store.c
>>> +++ b/libsemanage/src/semanage_store.c
>>> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>>>   {
>>>       char *semanage_conf = NULL;
>>>       int len;
>>> +    struct stat sb;
>>>         len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>>>       semanage_conf = calloc(len + 1, sizeof(char));
>>> @@ -522,7 +523,12 @@ char *semanage_conf_path(void)
>>>       snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>>>            SEMANAGE_CONF_FILE);
>>>   -    if (access(semanage_conf, R_OK) != 0) {
>>> +    if (stat(semanage_conf, &sb) != 0) {
>>> +        if (errno != ENOENT) {
>>> +            /* semanage_conf exists but can't be accessed */
>>> +            return NULL;
>> Not sure this is what we want. I think we'd just return semanage_conf here, and then let the caller fail when it tries to access it, so that we get a useful error message with the pathname.
>>
>>> +        }
>>> +
>>>           snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>>>       }
>>>   @@ -1508,8 +1514,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>>>   static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>>>         int r;
>>> +    struct stat sb;
>>> +
>>> +    if (stat(path, &sb) < 0) {
>>> +        if (errno != ENOENT) {
>>> +            ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> +            return -1;
>>> +        }
>>>   -    if (access(path, F_OK) != 0) {
>>>           return 0;
>>>       }
>>>  
> 
> 

Re: [PATCH 3/3] libsemanage: replace access() checks to make setuid programs work

[Stephen Smalley]

On 03/09/2018 08:28 AM, Stephen Smalley wrote:
> On 03/09/2018 08:14 AM, Vit Mojzis wrote:
>>
>>
>> On 7.3.2018 15:59, Stephen Smalley wrote:
>>> On 03/06/2018 06:58 AM, Vit Mojzis wrote:
>>>> access() uses real UID instead of effective UID which causes false
>>>> negative checks in setuid programs.
>>>> Replace access(,F_OK) (i.e. tests for file existence) by stat().
>>>> And access(,R_OK) by fopen(,"r")
>>>>
>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
>>>>
>>>> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
>>>> ---
>>>>   libsemanage/src/direct_api.c     | 133 +++++++++++++++++++++++++--------------
>>>>   libsemanage/src/semanage_store.c |  16 ++++-
>>>>   2 files changed, 99 insertions(+), 50 deletions(-)
>>>>
>>>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>>>> index 49cac14f..4e2af810 100644
>>>> --- a/libsemanage/src/direct_api.c
>>>> +++ b/libsemanage/src/direct_api.c
>>>> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>>>>   int semanage_direct_connect(semanage_handle_t * sh)
>>>>   {
>>>>       const char *path;
>>>> +    struct stat sb;
>>>>         if (semanage_check_init(sh, sh->conf->store_root_path))
>>>>           goto err;
>>>> @@ -302,10 +303,16 @@ int semanage_direct_connect(semanage_handle_t * sh)
>>>>         /* set the disable dontaudit value */
>>>>       path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
>>>> -    if (access(path, F_OK) == 0)
>>>> +
>>>> +    if (stat(path, &sb) == 0)
>>>>           sepol_set_disable_dontaudit(sh->sepolh, 1);
>>>> -    else
>>>> +    else if (errno == ENOENT) {
>>>> +        /* The file does not exist */
>>>>           sepol_set_disable_dontaudit(sh->sepolh, 0);
>>>> +    } else {
>>>> +        ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +        goto err;
>>>> +    }
>>>>         return STATUS_SUCCESS;
>>>>   @@ -1139,6 +1146,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
>>>>       int status = 0;
>>>>       int i;
>>>>       char cil_path[PATH_MAX];
>>>> +    struct stat sb;
>>>>         assert(sh);
>>>>       assert(modinfos);
>>>> @@ -1155,9 +1163,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
>>>>           }
>>>>             if (semanage_get_ignore_module_cache(sh) == 0 &&
>>>> -                access(cil_path, F_OK) == 0) {
>>>> +                (status = stat(cil_path, &sb)) == 0) {
>>>>               continue;
>>>>           }
>>>> +        if (status != 0 && errno != ENOENT) {
>>>> +            ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
>>>> +            goto cleanup; //an error in the "stat" call
>>>> +        }
>>>>             status = semanage_compile_module(sh, &modinfos[i]);
>>>>           if (status < 0) {
>>>> @@ -1196,6 +1208,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>       struct cil_db *cildb = NULL;
>>>>       semanage_module_info_t *modinfos = NULL;
>>>>       mode_t mask = umask(0077);
>>>> +    struct stat sb;
>>>>         int do_rebuild, do_write_kernel, do_install;
>>>>       int fcontexts_modified, ports_modified, seusers_modified,
>>>> @@ -1234,10 +1247,15 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>         /* Create or remove the disable_dontaudit flag file. */
>>>>       path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
>>>> -    if (access(path, F_OK) == 0)
>>>> +    if (stat(path, &sb) == 0)
>>>>           do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>>> -    else
>>>> +    else if (errno == ENOENT) {
>>>> +        /* The file does not exist */
>>>>           do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
>>>> +    } else {
>>>> +        ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>> We need to ensure that retval is set in all of these cases.  Safest to always set it before goto cleanup,
>>> either explicitly to -1 or to return value of stat() call above.
>> Thank you.
>> The retval is actually initialized to -1 (and stat() returns -1 for any error) so it's not necessary to set it here. It will be necessary a little bit down the road after the first assignment to retval, you're right.
>> Should I set it to -1 here anyway to make it less prone to breaking due to future changes in the code (what about the other ERR calls in this section of the code)?
> 
> Yes, I would do that for safety here and in any other case where we didn't just set retval and test it.  IOW, any case where we have:
> 	retval = foo(args);
> 	if (retval < 0) {
> 		ERR(...)
> 		goto cleanup;
> 	}
> is fine as is, but any case where we have:
> 	if (some-condition-other-than-retval-less-than-0) {
> 		ERR(...)
> 		goto cleanup;
> 	}
> likely ought to explicitly set retval.
> 
> If we have too many of those, then maybe we ought to have an err: label that sets retval to -1 explicitly and goto it instead.

That said, you don't have to fix cases that you didn't introduce in your patch; that's up to you, and you can fix pre-existing cases via a separate patch if desired.
> 
>>
>>>> +        goto cleanup;
>>>> +    }
>>>>       if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>>>>           FILE *touch;
>>>>           touch = fopen(path, "w");
>>>> @@ -1259,10 +1277,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>         /* Create or remove the preserve_tunables flag file. */
>>>>       path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
>>>> -    if (access(path, F_OK) == 0)
>>>> +    if (stat(path, &sb) == 0)
>>>>           do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
>>>> -    else
>>>> +    else if (errno == ENOENT) {
>>>> +        /* The file does not exist */
>>>>           do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
>>>> +    } else {
>>>> +        ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +        goto cleanup;
>>>> +    }
>>> Ditto
>>>
>>>> +
>>>>       if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>>>>           FILE *touch;
>>>>           touch = fopen(path, "w");
>>>> @@ -1299,40 +1323,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>>>        * a rebuild.
>>>>        */
>>>>       if (!do_rebuild) {
>>>> -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
>>>> -        if (access(path, F_OK) != 0) {
>>>> -            do_rebuild = 1;
>>>> -            goto rebuild;
>>>> -        }
>>>> -
>>>> -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
>>>> -        if (access(path, F_OK) != 0) {
>>>> -            do_rebuild = 1;
>>>> -            goto rebuild;
>>>> -        }
>>>> -
>>>> -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
>>>> -        if (access(path, F_OK) != 0) {
>>>> -            do_rebuild = 1;
>>>> -            goto rebuild;
>>>> -        }
>>>> -
>>>> -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
>>>> -        if (access(path, F_OK) != 0) {
>>>> -            do_rebuild = 1;
>>>> -            goto rebuild;
>>>> -        }
>>>> -
>>>> -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>>>> -        if (access(path, F_OK) != 0) {
>>>> -            do_rebuild = 1;
>>>> -            goto rebuild;
>>>> -        }
>>>> +        int files[] = {SEMANAGE_STORE_KERNEL,
>>>> +                       SEMANAGE_STORE_FC,
>>>> +                       SEMANAGE_STORE_SEUSERS,
>>>> +                       SEMANAGE_LINKED,
>>>> +                       SEMANAGE_SEUSERS_LINKED,
>>>> +                       SEMANAGE_USERS_EXTRA_LINKED};
>>>> +
>>>> +        for (i = 0; i < (int) sizeof(files); i++) {
>>>> +            path = semanage_path(SEMANAGE_TMP, files[i]);
>>>> +            if (stat(path, &sb) != 0) {
>>>> +                if (errno != ENOENT) {
>>>> +                    ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +                    goto cleanup;
>>> Ditto
>>>
>>>> +                }
>>>>   -        path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>>>> -        if (access(path, F_OK) != 0) {
>>>> -            do_rebuild = 1;
>>>> -            goto rebuild;
>>>> +                do_rebuild = 1;
>>>> +                goto rebuild;
>>>> +            }
>>>>           }
>>>>       }
>>>>   @@ -1465,7 +1473,7 @@ rebuild:
>>>>               goto cleanup;
>>>>             path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
>>>> -        if (access(path, F_OK) == 0) {
>>>> +        if (stat(path, &sb) == 0) {
>>>>               retval = semanage_copy_file(path,
>>>>                               semanage_path(SEMANAGE_TMP,
>>>>                                     SEMANAGE_STORE_SEUSERS),
>>>> @@ -1473,12 +1481,18 @@ rebuild:
>>>>               if (retval < 0)
>>>>                   goto cleanup;
>>>>               pseusers->dtable->drop_cache(pseusers->dbase);
>>>> -        } else {
>>>> +        } else if (errno == ENOENT) {
>>>> +            /* The file does not exist */
>>>>               pseusers->dtable->clear(sh, pseusers->dbase);
>>>> +        } else {
>>>> +            ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +            goto cleanup;
>>> Ditto
>>>
>>>>           }
>>>>   +
>>>> +
>>>>           path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
>>>> -        if (access(path, F_OK) == 0) {
>>>> +        if (stat(path, &sb) == 0) {
>>>>               retval = semanage_copy_file(path,
>>>>                               semanage_path(SEMANAGE_TMP,
>>>>                                     SEMANAGE_USERS_EXTRA),
>>>> @@ -1486,8 +1500,12 @@ rebuild:
>>>>               if (retval < 0)
>>>>                   goto cleanup;
>>>>               pusers_extra->dtable->drop_cache(pusers_extra->dbase);
>>>> -        } else {
>>>> +        } else if (errno == ENOENT) {
>>>> +            /* The file does not exist */
>>>>               pusers_extra->dtable->clear(sh, pusers_extra->dbase);
>>>> +        } else {
>>>> +            ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +            goto cleanup;
>>> Ditto
>>>
>>>>           }
>>>>       }
>>>>   @@ -1817,6 +1835,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>       ssize_t _data_len;
>>>>       char *_data;
>>>>       int compressed;
>>>> +    struct stat sb;
>>>>         /* get path of module */
>>>>       rc = semanage_module_get_path(
>>>> @@ -1829,8 +1848,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>           goto cleanup;
>>>>       }
>>>>   -    if (access(module_path, F_OK) != 0) {
>>>> -        ERR(sh, "Module does not exist: %s", module_path);
>>>> +    if (stat(module_path, &sb) != 0) {
>>>> +        ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>>>>           rc = -1;
>>>>           goto cleanup;
>>>>       }
>>>> @@ -1859,7 +1878,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>>>>           goto cleanup;
>>>>       }
>>>>   -    if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
>>>> +    if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
>>>> +        if (errno != ENOENT) {
>>>> +            ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
>>> Ditto
>>>
>>>> +            goto cleanup;
>>>> +   
>>>> +
>>>>           rc = semanage_compile_module(sh, _modinfo);
>>>>           if (rc < 0) {
>>>>               goto cleanup;
>>>> @@ -2004,6 +2028,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>>>>       }
>>>>         if (stat(path, &sb) < 0) {
>>>> +        if (errno != ENOENT) {
>>>> +            ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +            status = -1;
>>>> +            goto cleanup;
>>>> +        }
>>>> +
>>>>           *enabled = 1;
>>>>       }
>>>>       else {
>>>> @@ -2330,6 +2360,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>>>>         /* set enabled/disabled status */
>>>>       if (stat(fn, &sb) < 0) {
>>>> +        if (errno != ENOENT) {
>>>> +            ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
>>>> +            status = -1;
>>>> +            goto cleanup;
>>>> +        }
>>>> +
>>>>           ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>>>>           if (ret != 0) {
>>>>               status = -1;
>>>> @@ -2738,6 +2774,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>>       int status = 0;
>>>>       int ret = 0;
>>>>       int type;
>>>> +    struct stat sb;
>>>>         char path[PATH_MAX];
>>>>       mode_t mask = umask(0077);
>>>> @@ -2838,7 +2875,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>>>>               goto cleanup;
>>>>           }
>>>>   -        if (access(path, F_OK) == 0) {
>>>> +        if (stat(path, &sb) == 0) {
>>>>               ret = unlink(path);
>>>>               if (ret != 0) {
>>>>                   ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
>>>> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
>>>> index 4bd1d651..10f8b306 100644
>>>> --- a/libsemanage/src/semanage_store.c
>>>> +++ b/libsemanage/src/semanage_store.c
>>>> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>>>>   {
>>>>       char *semanage_conf = NULL;
>>>>       int len;
>>>> +    struct stat sb;
>>>>         len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>>>>       semanage_conf = calloc(len + 1, sizeof(char));
>>>> @@ -522,7 +523,12 @@ char *semanage_conf_path(void)
>>>>       snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>>>>            SEMANAGE_CONF_FILE);
>>>>   -    if (access(semanage_conf, R_OK) != 0) {
>>>> +    if (stat(semanage_conf, &sb) != 0) {
>>>> +        if (errno != ENOENT) {
>>>> +            /* semanage_conf exists but can't be accessed */
>>>> +            return NULL;
>>> Not sure this is what we want. I think we'd just return semanage_conf here, and then let the caller fail when it tries to access it, so that we get a useful error message with the pathname.
>>>
>>>> +        }
>>>> +
>>>>           snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>>>>       }
>>>>   @@ -1508,8 +1514,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>>>>   static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>>>>         int r;
>>>> +    struct stat sb;
>>>> +
>>>> +    if (stat(path, &sb) < 0) {
>>>> +        if (errno != ENOENT) {
>>>> +            ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
>>>> +            return -1;
>>>> +        }
>>>>   -    if (access(path, F_OK) != 0) {
>>>>           return 0;
>>>>       }
>>>>  
>>
>>
> 

[PATCH] libsemanage: replace access() checks to make setuid programs work

[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 <vmojzis@redhat.com>
---
 libsemanage/src/direct_api.c     | 137 +++++++++++++++++++++++++--------------
 libsemanage/src/semanage_store.c |  11 +++-
 2 files changed, 98 insertions(+), 50 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 92d7517d..439122df 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
 int semanage_direct_connect(semanage_handle_t * sh)
 {
 	const char *path;
+	struct stat sb;
 
 	if (semanage_check_init(sh, sh->conf->store_root_path))
 		goto err;
@@ -302,10 +303,16 @@ int semanage_direct_connect(semanage_handle_t * sh)
 
 	/* set the disable dontaudit value */
 	path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
-	if (access(path, F_OK) == 0)
+
+	if (stat(path, &sb) == 0)
 		sepol_set_disable_dontaudit(sh->sepolh, 1);
-	else
+	else if (errno == ENOENT) {
+		/* The file does not exist */
 		sepol_set_disable_dontaudit(sh->sepolh, 0);
+	} else {
+		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+		goto err;
+	}
 
 	return STATUS_SUCCESS;
 
@@ -1139,6 +1146,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
 	int status = 0;
 	int i;
 	char cil_path[PATH_MAX];
+	struct stat sb;
 
 	assert(sh);
 	assert(modinfos);
@@ -1155,9 +1163,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
 		}
 
 		if (semanage_get_ignore_module_cache(sh) == 0 &&
-				access(cil_path, F_OK) == 0) {
+				(status = stat(cil_path, &sb)) == 0) {
 			continue;
 		}
+		if (status != 0 && errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+			goto cleanup; //an error in the "stat" call
+		}
 
 		status = semanage_compile_module(sh, &modinfos[i]);
 		if (status < 0) {
@@ -1196,6 +1208,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 	struct cil_db *cildb = NULL;
 	semanage_module_info_t *modinfos = NULL;
 	mode_t mask = umask(0077);
+	struct stat sb;
 
 	int do_rebuild, do_write_kernel, do_install;
 	int fcontexts_modified, ports_modified, seusers_modified,
@@ -1234,10 +1247,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 
 	/* Create or remove the disable_dontaudit flag file. */
 	path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
-	if (access(path, F_OK) == 0)
+	if (stat(path, &sb) == 0)
 		do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
-	else
+	else if (errno == ENOENT) {
+		/* The file does not exist */
 		do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+	} else {
+		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+		retval = -1;
+		goto cleanup;
+	}
 	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
 		FILE *touch;
 		touch = fopen(path, "w");
@@ -1259,10 +1278,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 
 	/* Create or remove the preserve_tunables flag file. */
 	path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
-	if (access(path, F_OK) == 0)
+	if (stat(path, &sb) == 0)
 		do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
-	else
+	else if (errno == ENOENT) {
+		/* The file does not exist */
 		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+	} else {
+		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+		retval = -1;
+		goto cleanup;
+	}
+
 	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
 		FILE *touch;
 		touch = fopen(path, "w");
@@ -1299,40 +1325,25 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 	 * a rebuild.
 	 */
 	if (!do_rebuild) {
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
+		int files[] = {SEMANAGE_STORE_KERNEL,
+					   SEMANAGE_STORE_FC,
+					   SEMANAGE_STORE_SEUSERS,
+					   SEMANAGE_LINKED,
+					   SEMANAGE_SEUSERS_LINKED,
+					   SEMANAGE_USERS_EXTRA_LINKED};
+
+		for (i = 0; i < (int) sizeof(files); i++) {
+			path = semanage_path(SEMANAGE_TMP, files[i]);
+			if (stat(path, &sb) != 0) {
+				if (errno != ENOENT) {
+					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+					retval = -1;
+					goto cleanup;
+				}
 
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
+				do_rebuild = 1;
+				goto rebuild;
+			}
 		}
 	}
 
@@ -1465,7 +1476,7 @@ rebuild:
 			goto cleanup;
 
 		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
-		if (access(path, F_OK) == 0) {
+		if (stat(path, &sb) == 0) {
 			retval = semanage_copy_file(path,
 						    semanage_path(SEMANAGE_TMP,
 								  SEMANAGE_STORE_SEUSERS),
@@ -1473,12 +1484,17 @@ rebuild:
 			if (retval < 0)
 				goto cleanup;
 			pseusers->dtable->drop_cache(pseusers->dbase);
-		} else {
+		} else if (errno == ENOENT) {
+			/* The file does not exist */
 			pseusers->dtable->clear(sh, pseusers->dbase);
+		} else {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			retval = -1;
+			goto cleanup;
 		}
 
 		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
-		if (access(path, F_OK) == 0) {
+		if (stat(path, &sb) == 0) {
 			retval = semanage_copy_file(path,
 						    semanage_path(SEMANAGE_TMP,
 								  SEMANAGE_USERS_EXTRA),
@@ -1486,8 +1502,13 @@ rebuild:
 			if (retval < 0)
 				goto cleanup;
 			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
-		} else {
+		} else if (errno == ENOENT) {
+			/* The file does not exist */
 			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
+		} else {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			retval = -1;
+			goto cleanup;
 		}
 	}
 
@@ -1817,6 +1838,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
 	ssize_t _data_len;
 	char *_data;
 	int compressed;
+	struct stat sb;
 
 	/* get path of module */
 	rc = semanage_module_get_path(
@@ -1829,8 +1851,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
 		goto cleanup;
 	}
 
-	if (access(module_path, F_OK) != 0) {
-		ERR(sh, "Module does not exist: %s", module_path);
+	if (stat(module_path, &sb) != 0) {
+		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
 		rc = -1;
 		goto cleanup;
 	}
@@ -1859,7 +1881,13 @@ static int semanage_direct_extract(semanage_handle_t * sh,
 		goto cleanup;
 	}
 
-	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+			rc = -1;
+			goto cleanup;
+		}
+
 		rc = semanage_compile_module(sh, _modinfo);
 		if (rc < 0) {
 			goto cleanup;
@@ -2004,6 +2032,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
 	}
 
 	if (stat(path, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			status = -1;
+			goto cleanup;
+		}
+
 		*enabled = 1;
 	}
 	else {
@@ -2330,6 +2364,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
 
 	/* set enabled/disabled status */
 	if (stat(fn, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+			status = -1;
+			goto cleanup;
+		}
+
 		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
 		if (ret != 0) {
 			status = -1;
@@ -2738,6 +2778,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
 	int status = 0;
 	int ret = 0;
 	int type;
+	struct stat sb;
 
 	char path[PATH_MAX];
 	mode_t mask = umask(0077);
@@ -2838,7 +2879,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
 			goto cleanup;
 		}
 
-		if (access(path, F_OK) == 0) {
+		if (stat(path, &sb) == 0) {
 			ret = unlink(path);
 			if (ret != 0) {
 				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..1e71ab67 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
 {
 	char *semanage_conf = NULL;
 	int len;
+	struct stat sb;
 
 	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
 	semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,7 @@ char *semanage_conf_path(void)
 	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
 		 SEMANAGE_CONF_FILE);
 
-	if (access(semanage_conf, R_OK) != 0) {
+	if (stat(semanage_conf, &sb) != 0 && errno == ENONET) {
 		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
 	}
 
@@ -1508,8 +1509,14 @@ int semanage_split_fc(semanage_handle_t * sh)
 static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
 
 	int r;
+	struct stat sb;
+
+	if (stat(path, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			return -1;
+		}
 
-	if (access(path, F_OK) != 0) {
 		return 0;
 	}
 
-- 
2.14.3

Re: [PATCH] libsemanage: replace access() checks to make setuid programs work

[Stephen Smalley]

On 03/09/2018 10:21 AM, Vit Mojzis wrote:
> access() uses real UID instead of effective UID which causes false
> negative checks in setuid programs.
> Replace access() calls (mostly tests for file existence) by stat().
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
> 
> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
> ---
>  libsemanage/src/direct_api.c     | 137 +++++++++++++++++++++++++--------------
>  libsemanage/src/semanage_store.c |  11 +++-
>  2 files changed, 98 insertions(+), 50 deletions(-)
> 
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index 92d7517d..439122df 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>  int semanage_direct_connect(semanage_handle_t * sh)
>  {
>  	const char *path;
> +	struct stat sb;
>  
>  	if (semanage_check_init(sh, sh->conf->store_root_path))
>  		goto err;
> @@ -302,10 +303,16 @@ int semanage_direct_connect(semanage_handle_t * sh)
>  
>  	/* set the disable dontaudit value */
>  	path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
> -	if (access(path, F_OK) == 0)
> +
> +	if (stat(path, &sb) == 0)
>  		sepol_set_disable_dontaudit(sh->sepolh, 1);
> -	else
> +	else if (errno == ENOENT) {
> +		/* The file does not exist */
>  		sepol_set_disable_dontaudit(sh->sepolh, 0);
> +	} else {
> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +		goto err;
> +	}
>  
>  	return STATUS_SUCCESS;
>  
> @@ -1139,6 +1146,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
>  	int status = 0;
>  	int i;
>  	char cil_path[PATH_MAX];
> +	struct stat sb;
>  
>  	assert(sh);
>  	assert(modinfos);
> @@ -1155,9 +1163,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
>  		}
>  
>  		if (semanage_get_ignore_module_cache(sh) == 0 &&
> -				access(cil_path, F_OK) == 0) {
> +				(status = stat(cil_path, &sb)) == 0) {
>  			continue;
>  		}
> +		if (status != 0 && errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
> +			goto cleanup; //an error in the "stat" call
> +		}
>  
>  		status = semanage_compile_module(sh, &modinfos[i]);
>  		if (status < 0) {
> @@ -1196,6 +1208,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  	struct cil_db *cildb = NULL;
>  	semanage_module_info_t *modinfos = NULL;
>  	mode_t mask = umask(0077);
> +	struct stat sb;
>  
>  	int do_rebuild, do_write_kernel, do_install;
>  	int fcontexts_modified, ports_modified, seusers_modified,
> @@ -1234,10 +1247,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  
>  	/* Create or remove the disable_dontaudit flag file. */
>  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
> -	if (access(path, F_OK) == 0)
> +	if (stat(path, &sb) == 0)
>  		do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
> -	else
> +	else if (errno == ENOENT) {
> +		/* The file does not exist */
>  		do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
> +	} else {
> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +		retval = -1;
> +		goto cleanup;
> +	}
>  	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>  		FILE *touch;
>  		touch = fopen(path, "w");
> @@ -1259,10 +1278,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  
>  	/* Create or remove the preserve_tunables flag file. */
>  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
> -	if (access(path, F_OK) == 0)
> +	if (stat(path, &sb) == 0)
>  		do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
> -	else
> +	else if (errno == ENOENT) {
> +		/* The file does not exist */
>  		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
> +	} else {
> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +		retval = -1;
> +		goto cleanup;
> +	}
> +
>  	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>  		FILE *touch;
>  		touch = fopen(path, "w");
> @@ -1299,40 +1325,25 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  	 * a rebuild.
>  	 */
>  	if (!do_rebuild) {
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> +		int files[] = {SEMANAGE_STORE_KERNEL,
> +					   SEMANAGE_STORE_FC,
> +					   SEMANAGE_STORE_SEUSERS,
> +					   SEMANAGE_LINKED,
> +					   SEMANAGE_SEUSERS_LINKED,
> +					   SEMANAGE_USERS_EXTRA_LINKED};
> +
> +		for (i = 0; i < (int) sizeof(files); i++) {
> +			path = semanage_path(SEMANAGE_TMP, files[i]);
> +			if (stat(path, &sb) != 0) {
> +				if (errno != ENOENT) {
> +					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +					retval = -1;
> +					goto cleanup;
> +				}
>  
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> +				do_rebuild = 1;
> +				goto rebuild;
> +			}
>  		}
>  	}
>  
> @@ -1465,7 +1476,7 @@ rebuild:
>  			goto cleanup;
>  
>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			retval = semanage_copy_file(path,
>  						    semanage_path(SEMANAGE_TMP,
>  								  SEMANAGE_STORE_SEUSERS),
> @@ -1473,12 +1484,17 @@ rebuild:
>  			if (retval < 0)
>  				goto cleanup;
>  			pseusers->dtable->drop_cache(pseusers->dbase);
> -		} else {
> +		} else if (errno == ENOENT) {
> +			/* The file does not exist */
>  			pseusers->dtable->clear(sh, pseusers->dbase);
> +		} else {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			retval = -1;
> +			goto cleanup;
>  		}
>  
>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			retval = semanage_copy_file(path,
>  						    semanage_path(SEMANAGE_TMP,
>  								  SEMANAGE_USERS_EXTRA),
> @@ -1486,8 +1502,13 @@ rebuild:
>  			if (retval < 0)
>  				goto cleanup;
>  			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
> -		} else {
> +		} else if (errno == ENOENT) {
> +			/* The file does not exist */
>  			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
> +		} else {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			retval = -1;
> +			goto cleanup;
>  		}
>  	}
>  
> @@ -1817,6 +1838,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  	ssize_t _data_len;
>  	char *_data;
>  	int compressed;
> +	struct stat sb;
>  
>  	/* get path of module */
>  	rc = semanage_module_get_path(
> @@ -1829,8 +1851,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  		goto cleanup;
>  	}
>  
> -	if (access(module_path, F_OK) != 0) {
> -		ERR(sh, "Module does not exist: %s", module_path);
> +	if (stat(module_path, &sb) != 0) {
> +		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>  		rc = -1;
>  		goto cleanup;
>  	}
> @@ -1859,7 +1881,13 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  		goto cleanup;
>  	}
>  
> -	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
> +	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
> +			rc = -1;
> +			goto cleanup;
> +		}
> +
>  		rc = semanage_compile_module(sh, _modinfo);
>  		if (rc < 0) {
>  			goto cleanup;
> @@ -2004,6 +2032,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>  	}
>  
>  	if (stat(path, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			status = -1;
> +			goto cleanup;
> +		}
> +
>  		*enabled = 1;
>  	}
>  	else {
> @@ -2330,6 +2364,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>  
>  	/* set enabled/disabled status */
>  	if (stat(fn, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
> +			status = -1;
> +			goto cleanup;
> +		}
> +
>  		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>  		if (ret != 0) {
>  			status = -1;
> @@ -2738,6 +2778,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>  	int status = 0;
>  	int ret = 0;
>  	int type;
> +	struct stat sb;
>  
>  	char path[PATH_MAX];
>  	mode_t mask = umask(0077);
> @@ -2838,7 +2879,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>  			goto cleanup;
>  		}
>  
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			ret = unlink(path);
>  			if (ret != 0) {
>  				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 4bd1d651..1e71ab67 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>  {
>  	char *semanage_conf = NULL;
>  	int len;
> +	struct stat sb;
>  
>  	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>  	semanage_conf = calloc(len + 1, sizeof(char));
> @@ -522,7 +523,7 @@ char *semanage_conf_path(void)
>  	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>  		 SEMANAGE_CONF_FILE);
>  
> -	if (access(semanage_conf, R_OK) != 0) {
> +	if (stat(semanage_conf, &sb) != 0 && errno == ENONET) {

Typo: ENONET vs ENOENT.

>  		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>  	}
>  
> @@ -1508,8 +1509,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>  
>  	int r;
> +	struct stat sb;
> +
> +	if (stat(path, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			return -1;
> +		}
>  
> -	if (access(path, F_OK) != 0) {
>  		return 0;
>  	}
>  
> 

[PATCH] libsemanage: replace access() checks to make setuid programs work

[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 <vmojzis@redhat.com>
---
 libsemanage/src/direct_api.c     | 137 +++++++++++++++++++++++++--------------
 libsemanage/src/semanage_store.c |  11 +++-
 2 files changed, 98 insertions(+), 50 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 92d7517d..439122df 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
 int semanage_direct_connect(semanage_handle_t * sh)
 {
 	const char *path;
+	struct stat sb;
 
 	if (semanage_check_init(sh, sh->conf->store_root_path))
 		goto err;
@@ -302,10 +303,16 @@ int semanage_direct_connect(semanage_handle_t * sh)
 
 	/* set the disable dontaudit value */
 	path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
-	if (access(path, F_OK) == 0)
+
+	if (stat(path, &sb) == 0)
 		sepol_set_disable_dontaudit(sh->sepolh, 1);
-	else
+	else if (errno == ENOENT) {
+		/* The file does not exist */
 		sepol_set_disable_dontaudit(sh->sepolh, 0);
+	} else {
+		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+		goto err;
+	}
 
 	return STATUS_SUCCESS;
 
@@ -1139,6 +1146,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
 	int status = 0;
 	int i;
 	char cil_path[PATH_MAX];
+	struct stat sb;
 
 	assert(sh);
 	assert(modinfos);
@@ -1155,9 +1163,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
 		}
 
 		if (semanage_get_ignore_module_cache(sh) == 0 &&
-				access(cil_path, F_OK) == 0) {
+				(status = stat(cil_path, &sb)) == 0) {
 			continue;
 		}
+		if (status != 0 && errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+			goto cleanup; //an error in the "stat" call
+		}
 
 		status = semanage_compile_module(sh, &modinfos[i]);
 		if (status < 0) {
@@ -1196,6 +1208,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 	struct cil_db *cildb = NULL;
 	semanage_module_info_t *modinfos = NULL;
 	mode_t mask = umask(0077);
+	struct stat sb;
 
 	int do_rebuild, do_write_kernel, do_install;
 	int fcontexts_modified, ports_modified, seusers_modified,
@@ -1234,10 +1247,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 
 	/* Create or remove the disable_dontaudit flag file. */
 	path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
-	if (access(path, F_OK) == 0)
+	if (stat(path, &sb) == 0)
 		do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
-	else
+	else if (errno == ENOENT) {
+		/* The file does not exist */
 		do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+	} else {
+		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+		retval = -1;
+		goto cleanup;
+	}
 	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
 		FILE *touch;
 		touch = fopen(path, "w");
@@ -1259,10 +1278,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 
 	/* Create or remove the preserve_tunables flag file. */
 	path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
-	if (access(path, F_OK) == 0)
+	if (stat(path, &sb) == 0)
 		do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
-	else
+	else if (errno == ENOENT) {
+		/* The file does not exist */
 		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+	} else {
+		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+		retval = -1;
+		goto cleanup;
+	}
+
 	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
 		FILE *touch;
 		touch = fopen(path, "w");
@@ -1299,40 +1325,25 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 	 * a rebuild.
 	 */
 	if (!do_rebuild) {
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
-
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
-		}
+		int files[] = {SEMANAGE_STORE_KERNEL,
+					   SEMANAGE_STORE_FC,
+					   SEMANAGE_STORE_SEUSERS,
+					   SEMANAGE_LINKED,
+					   SEMANAGE_SEUSERS_LINKED,
+					   SEMANAGE_USERS_EXTRA_LINKED};
+
+		for (i = 0; i < (int) sizeof(files); i++) {
+			path = semanage_path(SEMANAGE_TMP, files[i]);
+			if (stat(path, &sb) != 0) {
+				if (errno != ENOENT) {
+					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+					retval = -1;
+					goto cleanup;
+				}
 
-		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
-		if (access(path, F_OK) != 0) {
-			do_rebuild = 1;
-			goto rebuild;
+				do_rebuild = 1;
+				goto rebuild;
+			}
 		}
 	}
 
@@ -1465,7 +1476,7 @@ rebuild:
 			goto cleanup;
 
 		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
-		if (access(path, F_OK) == 0) {
+		if (stat(path, &sb) == 0) {
 			retval = semanage_copy_file(path,
 						    semanage_path(SEMANAGE_TMP,
 								  SEMANAGE_STORE_SEUSERS),
@@ -1473,12 +1484,17 @@ rebuild:
 			if (retval < 0)
 				goto cleanup;
 			pseusers->dtable->drop_cache(pseusers->dbase);
-		} else {
+		} else if (errno == ENOENT) {
+			/* The file does not exist */
 			pseusers->dtable->clear(sh, pseusers->dbase);
+		} else {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			retval = -1;
+			goto cleanup;
 		}
 
 		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
-		if (access(path, F_OK) == 0) {
+		if (stat(path, &sb) == 0) {
 			retval = semanage_copy_file(path,
 						    semanage_path(SEMANAGE_TMP,
 								  SEMANAGE_USERS_EXTRA),
@@ -1486,8 +1502,13 @@ rebuild:
 			if (retval < 0)
 				goto cleanup;
 			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
-		} else {
+		} else if (errno == ENOENT) {
+			/* The file does not exist */
 			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
+		} else {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			retval = -1;
+			goto cleanup;
 		}
 	}
 
@@ -1817,6 +1838,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
 	ssize_t _data_len;
 	char *_data;
 	int compressed;
+	struct stat sb;
 
 	/* get path of module */
 	rc = semanage_module_get_path(
@@ -1829,8 +1851,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
 		goto cleanup;
 	}
 
-	if (access(module_path, F_OK) != 0) {
-		ERR(sh, "Module does not exist: %s", module_path);
+	if (stat(module_path, &sb) != 0) {
+		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
 		rc = -1;
 		goto cleanup;
 	}
@@ -1859,7 +1881,13 @@ static int semanage_direct_extract(semanage_handle_t * sh,
 		goto cleanup;
 	}
 
-	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+			rc = -1;
+			goto cleanup;
+		}
+
 		rc = semanage_compile_module(sh, _modinfo);
 		if (rc < 0) {
 			goto cleanup;
@@ -2004,6 +2032,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
 	}
 
 	if (stat(path, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			status = -1;
+			goto cleanup;
+		}
+
 		*enabled = 1;
 	}
 	else {
@@ -2330,6 +2364,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
 
 	/* set enabled/disabled status */
 	if (stat(fn, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+			status = -1;
+			goto cleanup;
+		}
+
 		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
 		if (ret != 0) {
 			status = -1;
@@ -2738,6 +2778,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
 	int status = 0;
 	int ret = 0;
 	int type;
+	struct stat sb;
 
 	char path[PATH_MAX];
 	mode_t mask = umask(0077);
@@ -2838,7 +2879,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
 			goto cleanup;
 		}
 
-		if (access(path, F_OK) == 0) {
+		if (stat(path, &sb) == 0) {
 			ret = unlink(path);
 			if (ret != 0) {
 				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..14ad99c1 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
 {
 	char *semanage_conf = NULL;
 	int len;
+	struct stat sb;
 
 	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
 	semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,7 @@ char *semanage_conf_path(void)
 	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
 		 SEMANAGE_CONF_FILE);
 
-	if (access(semanage_conf, R_OK) != 0) {
+	if (stat(semanage_conf, &sb) != 0 && errno == ENOENT) {
 		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
 	}
 
@@ -1508,8 +1509,14 @@ int semanage_split_fc(semanage_handle_t * sh)
 static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
 
 	int r;
+	struct stat sb;
+
+	if (stat(path, &sb) < 0) {
+		if (errno != ENOENT) {
+			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+			return -1;
+		}
 
-	if (access(path, F_OK) != 0) {
 		return 0;
 	}
 
-- 
2.14.3

Re: [PATCH] libsemanage: replace access() checks to make setuid programs work

[Stephen Smalley]

On 03/09/2018 10:39 AM, Vit Mojzis wrote:
> access() uses real UID instead of effective UID which causes false
> negative checks in setuid programs.
> Replace access() calls (mostly tests for file existence) by stat().
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431

Thanks, I've put this up as a PR for testing here:
https://github.com/SELinuxProject/selinux/pull/84

I won't be around next week so someone else can merge it or I will get to it when I return.

> 
> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
> ---
>  libsemanage/src/direct_api.c     | 137 +++++++++++++++++++++++++--------------
>  libsemanage/src/semanage_store.c |  11 +++-
>  2 files changed, 98 insertions(+), 50 deletions(-)
> 
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index 92d7517d..439122df 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>  int semanage_direct_connect(semanage_handle_t * sh)
>  {
>  	const char *path;
> +	struct stat sb;
>  
>  	if (semanage_check_init(sh, sh->conf->store_root_path))
>  		goto err;
> @@ -302,10 +303,16 @@ int semanage_direct_connect(semanage_handle_t * sh)
>  
>  	/* set the disable dontaudit value */
>  	path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
> -	if (access(path, F_OK) == 0)
> +
> +	if (stat(path, &sb) == 0)
>  		sepol_set_disable_dontaudit(sh->sepolh, 1);
> -	else
> +	else if (errno == ENOENT) {
> +		/* The file does not exist */
>  		sepol_set_disable_dontaudit(sh->sepolh, 0);
> +	} else {
> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +		goto err;
> +	}
>  
>  	return STATUS_SUCCESS;
>  
> @@ -1139,6 +1146,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
>  	int status = 0;
>  	int i;
>  	char cil_path[PATH_MAX];
> +	struct stat sb;
>  
>  	assert(sh);
>  	assert(modinfos);
> @@ -1155,9 +1163,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
>  		}
>  
>  		if (semanage_get_ignore_module_cache(sh) == 0 &&
> -				access(cil_path, F_OK) == 0) {
> +				(status = stat(cil_path, &sb)) == 0) {
>  			continue;
>  		}
> +		if (status != 0 && errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
> +			goto cleanup; //an error in the "stat" call
> +		}
>  
>  		status = semanage_compile_module(sh, &modinfos[i]);
>  		if (status < 0) {
> @@ -1196,6 +1208,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  	struct cil_db *cildb = NULL;
>  	semanage_module_info_t *modinfos = NULL;
>  	mode_t mask = umask(0077);
> +	struct stat sb;
>  
>  	int do_rebuild, do_write_kernel, do_install;
>  	int fcontexts_modified, ports_modified, seusers_modified,
> @@ -1234,10 +1247,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  
>  	/* Create or remove the disable_dontaudit flag file. */
>  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
> -	if (access(path, F_OK) == 0)
> +	if (stat(path, &sb) == 0)
>  		do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
> -	else
> +	else if (errno == ENOENT) {
> +		/* The file does not exist */
>  		do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
> +	} else {
> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +		retval = -1;
> +		goto cleanup;
> +	}
>  	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>  		FILE *touch;
>  		touch = fopen(path, "w");
> @@ -1259,10 +1278,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  
>  	/* Create or remove the preserve_tunables flag file. */
>  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
> -	if (access(path, F_OK) == 0)
> +	if (stat(path, &sb) == 0)
>  		do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
> -	else
> +	else if (errno == ENOENT) {
> +		/* The file does not exist */
>  		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
> +	} else {
> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +		retval = -1;
> +		goto cleanup;
> +	}
> +
>  	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>  		FILE *touch;
>  		touch = fopen(path, "w");
> @@ -1299,40 +1325,25 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  	 * a rebuild.
>  	 */
>  	if (!do_rebuild) {
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> +		int files[] = {SEMANAGE_STORE_KERNEL,
> +					   SEMANAGE_STORE_FC,
> +					   SEMANAGE_STORE_SEUSERS,
> +					   SEMANAGE_LINKED,
> +					   SEMANAGE_SEUSERS_LINKED,
> +					   SEMANAGE_USERS_EXTRA_LINKED};
> +
> +		for (i = 0; i < (int) sizeof(files); i++) {
> +			path = semanage_path(SEMANAGE_TMP, files[i]);
> +			if (stat(path, &sb) != 0) {
> +				if (errno != ENOENT) {
> +					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +					retval = -1;
> +					goto cleanup;
> +				}
>  
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> +				do_rebuild = 1;
> +				goto rebuild;
> +			}
>  		}
>  	}
>  
> @@ -1465,7 +1476,7 @@ rebuild:
>  			goto cleanup;
>  
>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			retval = semanage_copy_file(path,
>  						    semanage_path(SEMANAGE_TMP,
>  								  SEMANAGE_STORE_SEUSERS),
> @@ -1473,12 +1484,17 @@ rebuild:
>  			if (retval < 0)
>  				goto cleanup;
>  			pseusers->dtable->drop_cache(pseusers->dbase);
> -		} else {
> +		} else if (errno == ENOENT) {
> +			/* The file does not exist */
>  			pseusers->dtable->clear(sh, pseusers->dbase);
> +		} else {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			retval = -1;
> +			goto cleanup;
>  		}
>  
>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			retval = semanage_copy_file(path,
>  						    semanage_path(SEMANAGE_TMP,
>  								  SEMANAGE_USERS_EXTRA),
> @@ -1486,8 +1502,13 @@ rebuild:
>  			if (retval < 0)
>  				goto cleanup;
>  			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
> -		} else {
> +		} else if (errno == ENOENT) {
> +			/* The file does not exist */
>  			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
> +		} else {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			retval = -1;
> +			goto cleanup;
>  		}
>  	}
>  
> @@ -1817,6 +1838,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  	ssize_t _data_len;
>  	char *_data;
>  	int compressed;
> +	struct stat sb;
>  
>  	/* get path of module */
>  	rc = semanage_module_get_path(
> @@ -1829,8 +1851,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  		goto cleanup;
>  	}
>  
> -	if (access(module_path, F_OK) != 0) {
> -		ERR(sh, "Module does not exist: %s", module_path);
> +	if (stat(module_path, &sb) != 0) {
> +		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>  		rc = -1;
>  		goto cleanup;
>  	}
> @@ -1859,7 +1881,13 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  		goto cleanup;
>  	}
>  
> -	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
> +	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
> +			rc = -1;
> +			goto cleanup;
> +		}
> +
>  		rc = semanage_compile_module(sh, _modinfo);
>  		if (rc < 0) {
>  			goto cleanup;
> @@ -2004,6 +2032,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>  	}
>  
>  	if (stat(path, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			status = -1;
> +			goto cleanup;
> +		}
> +
>  		*enabled = 1;
>  	}
>  	else {
> @@ -2330,6 +2364,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>  
>  	/* set enabled/disabled status */
>  	if (stat(fn, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
> +			status = -1;
> +			goto cleanup;
> +		}
> +
>  		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>  		if (ret != 0) {
>  			status = -1;
> @@ -2738,6 +2778,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>  	int status = 0;
>  	int ret = 0;
>  	int type;
> +	struct stat sb;
>  
>  	char path[PATH_MAX];
>  	mode_t mask = umask(0077);
> @@ -2838,7 +2879,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>  			goto cleanup;
>  		}
>  
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			ret = unlink(path);
>  			if (ret != 0) {
>  				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 4bd1d651..14ad99c1 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>  {
>  	char *semanage_conf = NULL;
>  	int len;
> +	struct stat sb;
>  
>  	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>  	semanage_conf = calloc(len + 1, sizeof(char));
> @@ -522,7 +523,7 @@ char *semanage_conf_path(void)
>  	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>  		 SEMANAGE_CONF_FILE);
>  
> -	if (access(semanage_conf, R_OK) != 0) {
> +	if (stat(semanage_conf, &sb) != 0 && errno == ENOENT) {
>  		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>  	}
>  
> @@ -1508,8 +1509,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>  
>  	int r;
> +	struct stat sb;
> +
> +	if (stat(path, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			return -1;
> +		}
>  
> -	if (access(path, F_OK) != 0) {
>  		return 0;
>  	}
>  
> 

Re: [PATCH] libsemanage: replace access() checks to make setuid programs work

[Petr Lautrbach]

[-- Attachment #1: Type: text/plain, Size: 11950 bytes --]

On Fri, Mar 09, 2018 at 10:51:20AM -0500, Stephen Smalley wrote:
> On 03/09/2018 10:39 AM, Vit Mojzis wrote:
> > access() uses real UID instead of effective UID which causes false
> > negative checks in setuid programs.
> > Replace access() calls (mostly tests for file existence) by stat().
> > 
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
> 
> Thanks, I've put this up as a PR for testing here:
> https://github.com/SELinuxProject/selinux/pull/84
> 
> I won't be around next week so someone else can merge it or I will get to it when I return.

This is merged now. Thanks!

> > 
> > Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
> > ---
> >  libsemanage/src/direct_api.c     | 137 +++++++++++++++++++++++++--------------
> >  libsemanage/src/semanage_store.c |  11 +++-
> >  2 files changed, 98 insertions(+), 50 deletions(-)
> > 
> > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> > index 92d7517d..439122df 100644
> > --- a/libsemanage/src/direct_api.c
> > +++ b/libsemanage/src/direct_api.c
> > @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
> >  int semanage_direct_connect(semanage_handle_t * sh)
> >  {
> >  	const char *path;
> > +	struct stat sb;
> >  
> >  	if (semanage_check_init(sh, sh->conf->store_root_path))
> >  		goto err;
> > @@ -302,10 +303,16 @@ int semanage_direct_connect(semanage_handle_t * sh)
> >  
> >  	/* set the disable dontaudit value */
> >  	path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
> > -	if (access(path, F_OK) == 0)
> > +
> > +	if (stat(path, &sb) == 0)
> >  		sepol_set_disable_dontaudit(sh->sepolh, 1);
> > -	else
> > +	else if (errno == ENOENT) {
> > +		/* The file does not exist */
> >  		sepol_set_disable_dontaudit(sh->sepolh, 0);
> > +	} else {
> > +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> > +		goto err;
> > +	}
> >  
> >  	return STATUS_SUCCESS;
> >  
> > @@ -1139,6 +1146,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
> >  	int status = 0;
> >  	int i;
> >  	char cil_path[PATH_MAX];
> > +	struct stat sb;
> >  
> >  	assert(sh);
> >  	assert(modinfos);
> > @@ -1155,9 +1163,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
> >  		}
> >  
> >  		if (semanage_get_ignore_module_cache(sh) == 0 &&
> > -				access(cil_path, F_OK) == 0) {
> > +				(status = stat(cil_path, &sb)) == 0) {
> >  			continue;
> >  		}
> > +		if (status != 0 && errno != ENOENT) {
> > +			ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
> > +			goto cleanup; //an error in the "stat" call
> > +		}
> >  
> >  		status = semanage_compile_module(sh, &modinfos[i]);
> >  		if (status < 0) {
> > @@ -1196,6 +1208,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
> >  	struct cil_db *cildb = NULL;
> >  	semanage_module_info_t *modinfos = NULL;
> >  	mode_t mask = umask(0077);
> > +	struct stat sb;
> >  
> >  	int do_rebuild, do_write_kernel, do_install;
> >  	int fcontexts_modified, ports_modified, seusers_modified,
> > @@ -1234,10 +1247,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
> >  
> >  	/* Create or remove the disable_dontaudit flag file. */
> >  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
> > -	if (access(path, F_OK) == 0)
> > +	if (stat(path, &sb) == 0)
> >  		do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
> > -	else
> > +	else if (errno == ENOENT) {
> > +		/* The file does not exist */
> >  		do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
> > +	} else {
> > +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> > +		retval = -1;
> > +		goto cleanup;
> > +	}
> >  	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
> >  		FILE *touch;
> >  		touch = fopen(path, "w");
> > @@ -1259,10 +1278,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
> >  
> >  	/* Create or remove the preserve_tunables flag file. */
> >  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
> > -	if (access(path, F_OK) == 0)
> > +	if (stat(path, &sb) == 0)
> >  		do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
> > -	else
> > +	else if (errno == ENOENT) {
> > +		/* The file does not exist */
> >  		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
> > +	} else {
> > +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> > +		retval = -1;
> > +		goto cleanup;
> > +	}
> > +
> >  	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
> >  		FILE *touch;
> >  		touch = fopen(path, "w");
> > @@ -1299,40 +1325,25 @@ static int semanage_direct_commit(semanage_handle_t * sh)
> >  	 * a rebuild.
> >  	 */
> >  	if (!do_rebuild) {
> > -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
> > -		if (access(path, F_OK) != 0) {
> > -			do_rebuild = 1;
> > -			goto rebuild;
> > -		}
> > -
> > -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
> > -		if (access(path, F_OK) != 0) {
> > -			do_rebuild = 1;
> > -			goto rebuild;
> > -		}
> > -
> > -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
> > -		if (access(path, F_OK) != 0) {
> > -			do_rebuild = 1;
> > -			goto rebuild;
> > -		}
> > -
> > -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
> > -		if (access(path, F_OK) != 0) {
> > -			do_rebuild = 1;
> > -			goto rebuild;
> > -		}
> > -
> > -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> > -		if (access(path, F_OK) != 0) {
> > -			do_rebuild = 1;
> > -			goto rebuild;
> > -		}
> > +		int files[] = {SEMANAGE_STORE_KERNEL,
> > +					   SEMANAGE_STORE_FC,
> > +					   SEMANAGE_STORE_SEUSERS,
> > +					   SEMANAGE_LINKED,
> > +					   SEMANAGE_SEUSERS_LINKED,
> > +					   SEMANAGE_USERS_EXTRA_LINKED};
> > +
> > +		for (i = 0; i < (int) sizeof(files); i++) {
> > +			path = semanage_path(SEMANAGE_TMP, files[i]);
> > +			if (stat(path, &sb) != 0) {
> > +				if (errno != ENOENT) {
> > +					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> > +					retval = -1;
> > +					goto cleanup;
> > +				}
> >  
> > -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> > -		if (access(path, F_OK) != 0) {
> > -			do_rebuild = 1;
> > -			goto rebuild;
> > +				do_rebuild = 1;
> > +				goto rebuild;
> > +			}
> >  		}
> >  	}
> >  
> > @@ -1465,7 +1476,7 @@ rebuild:
> >  			goto cleanup;
> >  
> >  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> > -		if (access(path, F_OK) == 0) {
> > +		if (stat(path, &sb) == 0) {
> >  			retval = semanage_copy_file(path,
> >  						    semanage_path(SEMANAGE_TMP,
> >  								  SEMANAGE_STORE_SEUSERS),
> > @@ -1473,12 +1484,17 @@ rebuild:
> >  			if (retval < 0)
> >  				goto cleanup;
> >  			pseusers->dtable->drop_cache(pseusers->dbase);
> > -		} else {
> > +		} else if (errno == ENOENT) {
> > +			/* The file does not exist */
> >  			pseusers->dtable->clear(sh, pseusers->dbase);
> > +		} else {
> > +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> > +			retval = -1;
> > +			goto cleanup;
> >  		}
> >  
> >  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> > -		if (access(path, F_OK) == 0) {
> > +		if (stat(path, &sb) == 0) {
> >  			retval = semanage_copy_file(path,
> >  						    semanage_path(SEMANAGE_TMP,
> >  								  SEMANAGE_USERS_EXTRA),
> > @@ -1486,8 +1502,13 @@ rebuild:
> >  			if (retval < 0)
> >  				goto cleanup;
> >  			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
> > -		} else {
> > +		} else if (errno == ENOENT) {
> > +			/* The file does not exist */
> >  			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
> > +		} else {
> > +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> > +			retval = -1;
> > +			goto cleanup;
> >  		}
> >  	}
> >  
> > @@ -1817,6 +1838,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
> >  	ssize_t _data_len;
> >  	char *_data;
> >  	int compressed;
> > +	struct stat sb;
> >  
> >  	/* get path of module */
> >  	rc = semanage_module_get_path(
> > @@ -1829,8 +1851,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
> >  		goto cleanup;
> >  	}
> >  
> > -	if (access(module_path, F_OK) != 0) {
> > -		ERR(sh, "Module does not exist: %s", module_path);
> > +	if (stat(module_path, &sb) != 0) {
> > +		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
> >  		rc = -1;
> >  		goto cleanup;
> >  	}
> > @@ -1859,7 +1881,13 @@ static int semanage_direct_extract(semanage_handle_t * sh,
> >  		goto cleanup;
> >  	}
> >  
> > -	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
> > +	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
> > +		if (errno != ENOENT) {
> > +			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
> > +			rc = -1;
> > +			goto cleanup;
> > +		}
> > +
> >  		rc = semanage_compile_module(sh, _modinfo);
> >  		if (rc < 0) {
> >  			goto cleanup;
> > @@ -2004,6 +2032,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
> >  	}
> >  
> >  	if (stat(path, &sb) < 0) {
> > +		if (errno != ENOENT) {
> > +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> > +			status = -1;
> > +			goto cleanup;
> > +		}
> > +
> >  		*enabled = 1;
> >  	}
> >  	else {
> > @@ -2330,6 +2364,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
> >  
> >  	/* set enabled/disabled status */
> >  	if (stat(fn, &sb) < 0) {
> > +		if (errno != ENOENT) {
> > +			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
> > +			status = -1;
> > +			goto cleanup;
> > +		}
> > +
> >  		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
> >  		if (ret != 0) {
> >  			status = -1;
> > @@ -2738,6 +2778,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
> >  	int status = 0;
> >  	int ret = 0;
> >  	int type;
> > +	struct stat sb;
> >  
> >  	char path[PATH_MAX];
> >  	mode_t mask = umask(0077);
> > @@ -2838,7 +2879,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
> >  			goto cleanup;
> >  		}
> >  
> > -		if (access(path, F_OK) == 0) {
> > +		if (stat(path, &sb) == 0) {
> >  			ret = unlink(path);
> >  			if (ret != 0) {
> >  				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
> > diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> > index 4bd1d651..14ad99c1 100644
> > --- a/libsemanage/src/semanage_store.c
> > +++ b/libsemanage/src/semanage_store.c
> > @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
> >  {
> >  	char *semanage_conf = NULL;
> >  	int len;
> > +	struct stat sb;
> >  
> >  	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
> >  	semanage_conf = calloc(len + 1, sizeof(char));
> > @@ -522,7 +523,7 @@ char *semanage_conf_path(void)
> >  	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
> >  		 SEMANAGE_CONF_FILE);
> >  
> > -	if (access(semanage_conf, R_OK) != 0) {
> > +	if (stat(semanage_conf, &sb) != 0 && errno == ENOENT) {
> >  		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
> >  	}
> >  
> > @@ -1508,8 +1509,14 @@ int semanage_split_fc(semanage_handle_t * sh)
> >  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
> >  
> >  	int r;
> > +	struct stat sb;
> > +
> > +	if (stat(path, &sb) < 0) {
> > +		if (errno != ENOENT) {
> > +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> > +			return -1;
> > +		}
> >  
> > -	if (access(path, F_OK) != 0) {
> >  		return 0;
> >  	}
> >  
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] libsemanage: replace access() checks to make setuid programs work

[Petr Lautrbach]

[-- Attachment #1: Type: text/plain, Size: 11775 bytes --]

On Fri, Mar 09, 2018 at 04:39:44PM +0100, Vit Mojzis wrote:
> access() uses real UID instead of effective UID which causes false
> negative checks in setuid programs.
> Replace access() calls (mostly tests for file existence) by stat().
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
> 
> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
> ---
>  libsemanage/src/direct_api.c     | 137 +++++++++++++++++++++++++--------------
>  libsemanage/src/semanage_store.c |  11 +++-
>  2 files changed, 98 insertions(+), 50 deletions(-)
> 
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index 92d7517d..439122df 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
>  int semanage_direct_connect(semanage_handle_t * sh)
>  {
>  	const char *path;
> +	struct stat sb;
>  
>  	if (semanage_check_init(sh, sh->conf->store_root_path))
>  		goto err;
> @@ -302,10 +303,16 @@ int semanage_direct_connect(semanage_handle_t * sh)
>  
>  	/* set the disable dontaudit value */
>  	path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
> -	if (access(path, F_OK) == 0)
> +
> +	if (stat(path, &sb) == 0)
>  		sepol_set_disable_dontaudit(sh->sepolh, 1);
> -	else
> +	else if (errno == ENOENT) {
> +		/* The file does not exist */
>  		sepol_set_disable_dontaudit(sh->sepolh, 0);
> +	} else {
> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +		goto err;
> +	}
>  
>  	return STATUS_SUCCESS;
>  
> @@ -1139,6 +1146,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
>  	int status = 0;
>  	int i;
>  	char cil_path[PATH_MAX];
> +	struct stat sb;
>  
>  	assert(sh);
>  	assert(modinfos);
> @@ -1155,9 +1163,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
>  		}
>  
>  		if (semanage_get_ignore_module_cache(sh) == 0 &&
> -				access(cil_path, F_OK) == 0) {
> +				(status = stat(cil_path, &sb)) == 0) {
>  			continue;
>  		}
> +		if (status != 0 && errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
> +			goto cleanup; //an error in the "stat" call
> +		}
>  
>  		status = semanage_compile_module(sh, &modinfos[i]);
>  		if (status < 0) {
> @@ -1196,6 +1208,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  	struct cil_db *cildb = NULL;
>  	semanage_module_info_t *modinfos = NULL;
>  	mode_t mask = umask(0077);
> +	struct stat sb;
>  
>  	int do_rebuild, do_write_kernel, do_install;
>  	int fcontexts_modified, ports_modified, seusers_modified,
> @@ -1234,10 +1247,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  
>  	/* Create or remove the disable_dontaudit flag file. */
>  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
> -	if (access(path, F_OK) == 0)
> +	if (stat(path, &sb) == 0)
>  		do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
> -	else
> +	else if (errno == ENOENT) {
> +		/* The file does not exist */
>  		do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
> +	} else {
> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +		retval = -1;
> +		goto cleanup;
> +	}
>  	if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
>  		FILE *touch;
>  		touch = fopen(path, "w");
> @@ -1259,10 +1278,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  
>  	/* Create or remove the preserve_tunables flag file. */
>  	path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
> -	if (access(path, F_OK) == 0)
> +	if (stat(path, &sb) == 0)
>  		do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
> -	else
> +	else if (errno == ENOENT) {
> +		/* The file does not exist */
>  		do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
> +	} else {
> +		ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +		retval = -1;
> +		goto cleanup;
> +	}
> +
>  	if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
>  		FILE *touch;
>  		touch = fopen(path, "w");
> @@ -1299,40 +1325,25 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>  	 * a rebuild.
>  	 */
>  	if (!do_rebuild) {
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> -
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> -		}
> +		int files[] = {SEMANAGE_STORE_KERNEL,
> +					   SEMANAGE_STORE_FC,
> +					   SEMANAGE_STORE_SEUSERS,
> +					   SEMANAGE_LINKED,
> +					   SEMANAGE_SEUSERS_LINKED,
> +					   SEMANAGE_USERS_EXTRA_LINKED};
> +
> +		for (i = 0; i < (int) sizeof(files); i++) {

The size of the files needs to be divided by size of int, otherwise it causes segfault:

     # semanage fcontext -m -t httpd_log_t "/var/opt/quest/vas/vasd(/
     [1]    3063 segmentation fault (core dumped)  semanage fcontext -m -t httpd_log_t "/var/opt/quest/vas/vasd(/.*)?"
    
    backtrace:
     #0  0x00007fffe7901f9b in semanage_path (store=store@entry=SEMANAGE_TMP, path_name=3918632424) at semanage_store.c:487
     #1  0x00007fffe78f4be1 in semanage_direct_commit (sh=0x55555840da00) at direct_api.c:1336
     #2  0x00007fffe78f9f71 in semanage_commit (sh=0x55555840da00) at handle.c:428


Something like this should fix it:

#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))

for (i = 0; i < ARRAY_SIZE(files); i++) {
...




> +			path = semanage_path(SEMANAGE_TMP, files[i]);
> +			if (stat(path, &sb) != 0) {
> +				if (errno != ENOENT) {
> +					ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +					retval = -1;
> +					goto cleanup;
> +				}
>  
> -		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> -		if (access(path, F_OK) != 0) {
> -			do_rebuild = 1;
> -			goto rebuild;
> +				do_rebuild = 1;
> +				goto rebuild;
> +			}
>  		}
>  	}
>  
> @@ -1465,7 +1476,7 @@ rebuild:
>  			goto cleanup;
>  
>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			retval = semanage_copy_file(path,
>  						    semanage_path(SEMANAGE_TMP,
>  								  SEMANAGE_STORE_SEUSERS),
> @@ -1473,12 +1484,17 @@ rebuild:
>  			if (retval < 0)
>  				goto cleanup;
>  			pseusers->dtable->drop_cache(pseusers->dbase);
> -		} else {
> +		} else if (errno == ENOENT) {
> +			/* The file does not exist */
>  			pseusers->dtable->clear(sh, pseusers->dbase);
> +		} else {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			retval = -1;
> +			goto cleanup;
>  		}
>  
>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			retval = semanage_copy_file(path,
>  						    semanage_path(SEMANAGE_TMP,
>  								  SEMANAGE_USERS_EXTRA),
> @@ -1486,8 +1502,13 @@ rebuild:
>  			if (retval < 0)
>  				goto cleanup;
>  			pusers_extra->dtable->drop_cache(pusers_extra->dbase);
> -		} else {
> +		} else if (errno == ENOENT) {
> +			/* The file does not exist */
>  			pusers_extra->dtable->clear(sh, pusers_extra->dbase);
> +		} else {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			retval = -1;
> +			goto cleanup;
>  		}
>  	}
>  
> @@ -1817,6 +1838,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  	ssize_t _data_len;
>  	char *_data;
>  	int compressed;
> +	struct stat sb;
>  
>  	/* get path of module */
>  	rc = semanage_module_get_path(
> @@ -1829,8 +1851,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  		goto cleanup;
>  	}
>  
> -	if (access(module_path, F_OK) != 0) {
> -		ERR(sh, "Module does not exist: %s", module_path);
> +	if (stat(module_path, &sb) != 0) {
> +		ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
>  		rc = -1;
>  		goto cleanup;
>  	}
> @@ -1859,7 +1881,13 @@ static int semanage_direct_extract(semanage_handle_t * sh,
>  		goto cleanup;
>  	}
>  
> -	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
> +	if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
> +			rc = -1;
> +			goto cleanup;
> +		}
> +
>  		rc = semanage_compile_module(sh, _modinfo);
>  		if (rc < 0) {
>  			goto cleanup;
> @@ -2004,6 +2032,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
>  	}
>  
>  	if (stat(path, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			status = -1;
> +			goto cleanup;
> +		}
> +
>  		*enabled = 1;
>  	}
>  	else {
> @@ -2330,6 +2364,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
>  
>  	/* set enabled/disabled status */
>  	if (stat(fn, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
> +			status = -1;
> +			goto cleanup;
> +		}
> +
>  		ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
>  		if (ret != 0) {
>  			status = -1;
> @@ -2738,6 +2778,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>  	int status = 0;
>  	int ret = 0;
>  	int type;
> +	struct stat sb;
>  
>  	char path[PATH_MAX];
>  	mode_t mask = umask(0077);
> @@ -2838,7 +2879,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
>  			goto cleanup;
>  		}
>  
> -		if (access(path, F_OK) == 0) {
> +		if (stat(path, &sb) == 0) {
>  			ret = unlink(path);
>  			if (ret != 0) {
>  				ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 4bd1d651..14ad99c1 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -514,6 +514,7 @@ char *semanage_conf_path(void)
>  {
>  	char *semanage_conf = NULL;
>  	int len;
> +	struct stat sb;
>  
>  	len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
>  	semanage_conf = calloc(len + 1, sizeof(char));
> @@ -522,7 +523,7 @@ char *semanage_conf_path(void)
>  	snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(),
>  		 SEMANAGE_CONF_FILE);
>  
> -	if (access(semanage_conf, R_OK) != 0) {
> +	if (stat(semanage_conf, &sb) != 0 && errno == ENOENT) {
>  		snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
>  	}
>  
> @@ -1508,8 +1509,14 @@ int semanage_split_fc(semanage_handle_t * sh)
>  static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
>  
>  	int r;
> +	struct stat sb;
> +
> +	if (stat(path, &sb) < 0) {
> +		if (errno != ENOENT) {
> +			ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> +			return -1;
> +		}
>  
> -	if (access(path, F_OK) != 0) {
>  		return 0;
>  	}
>  
> -- 
> 2.14.3
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] libsemanage/direct_api.c: Fix iterating over array

[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 <vmojzis@redhat.com>
---
 libsemanage/src/direct_api.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 439122df..e7ec952f 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -60,6 +60,7 @@
 
 #define PIPE_READ 0
 #define PIPE_WRITE 1
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 
 static void semanage_direct_destroy(semanage_handle_t * sh);
 static int semanage_direct_disconnect(semanage_handle_t * sh);
@@ -1332,7 +1333,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
 					   SEMANAGE_SEUSERS_LINKED,
 					   SEMANAGE_USERS_EXTRA_LINKED};
 
-		for (i = 0; i < (int) sizeof(files); i++) {
+		for (i = 0; i < (int) ARRAY_SIZE(files); i++) {
 			path = semanage_path(SEMANAGE_TMP, files[i]);
 			if (stat(path, &sb) != 0) {
 				if (errno != ENOENT) {
-- 
2.14.3

Re: [PATCH] libsemanage/direct_api.c: Fix iterating over array

[William Roberts]

On Mon, Mar 19, 2018 at 7:46 AM, Vit Mojzis <vmojzis@redhat.com> wrote:
> Fix sizeof calculation in array iteration introduced by commit
> 6bb8282c4cf66e93daa9684dbe9c75bb6b1e09a7
> "libsemanage: replace access() checks to make setuid programs work"
>
> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
> ---
>  libsemanage/src/direct_api.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index 439122df..e7ec952f 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -60,6 +60,7 @@
>
>  #define PIPE_READ 0
>  #define PIPE_WRITE 1
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>
>  static void semanage_direct_destroy(semanage_handle_t * sh);
>  static int semanage_direct_disconnect(semanage_handle_t * sh);
> @@ -1332,7 +1333,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>                                            SEMANAGE_SEUSERS_LINKED,
>                                            SEMANAGE_USERS_EXTRA_LINKED};
>
> -               for (i = 0; i < (int) sizeof(files); i++) {
> +               for (i = 0; i < (int) ARRAY_SIZE(files); i++) {
>                         path = semanage_path(SEMANAGE_TMP, files[i]);
>                         if (stat(path, &sb) != 0) {
>                                 if (errno != ENOENT) {
> --
> 2.14.3
>
>
ack

Just noticing: that code has i as an int, which it probably should be size_t and
the files array as an array of int's, it should probably use the
enum type.

Re: [PATCH] libsemanage/direct_api.c: Fix iterating over array

[William Roberts]

On Mon, Mar 19, 2018 at 8:19 AM, William Roberts
<bill.c.roberts@gmail.com> wrote:
> On Mon, Mar 19, 2018 at 7:46 AM, Vit Mojzis <vmojzis@redhat.com> wrote:
>> Fix sizeof calculation in array iteration introduced by commit
>> 6bb8282c4cf66e93daa9684dbe9c75bb6b1e09a7
>> "libsemanage: replace access() checks to make setuid programs work"
>>
>> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
>> ---
>>  libsemanage/src/direct_api.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>> index 439122df..e7ec952f 100644
>> --- a/libsemanage/src/direct_api.c
>> +++ b/libsemanage/src/direct_api.c
>> @@ -60,6 +60,7 @@
>>
>>  #define PIPE_READ 0
>>  #define PIPE_WRITE 1
>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>>
>>  static void semanage_direct_destroy(semanage_handle_t * sh);
>>  static int semanage_direct_disconnect(semanage_handle_t * sh);
>> @@ -1332,7 +1333,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
>>                                            SEMANAGE_SEUSERS_LINKED,
>>                                            SEMANAGE_USERS_EXTRA_LINKED};
>>
>> -               for (i = 0; i < (int) sizeof(files); i++) {
>> +               for (i = 0; i < (int) ARRAY_SIZE(files); i++) {
>>                         path = semanage_path(SEMANAGE_TMP, files[i]);
>>                         if (stat(path, &sb) != 0) {
>>                                 if (errno != ENOENT) {
>> --
>> 2.14.3
>>
>>
> ack

Thanks, merged:
https://github.com/SELinuxProject/selinux/pull/87

>
> Just noticing: that code has i as an int, which it probably should be size_t and
> the files array as an array of int's, it should probably use the
> enum type.