All of lore.kernel.org
 help / color / mirror / Atom feed
* libsemanage: Perform access check using euid instead of uid v2
@ 2018-03-06 11:58 Vit Mojzis
  2018-03-06 11:58 ` [PATCH 1/3] libsemanage: remove access() check to make setuid programs work Vit Mojzis
                   ` (2 more replies)
  0 siblings, 3 replies; 29+ messages in thread
From: Vit Mojzis @ 2018-03-06 11:58 UTC (permalink / raw)
  To: selinux

Changes: 
  - replace semanage_copy_file by copy_file_if_exists to make sure "retval" is 0 if the file does not exist
  - restructure if statements to be more clear ("fail" is last part of the statement)
  - replace read test (attempt to open the file) by stat() call

^ permalink raw reply	[flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] libsemanage: replace access(, F_OK) checks to make setuid programs work
@ 2017-06-26 13:55 Stephen Smalley
  2018-02-28 10:15 ` [PATCH 1/3] libsemanage: remove access() check " Vit Mojzis
  0 siblings, 1 reply; 29+ messages in thread
From: Stephen Smalley @ 2017-06-26 13:55 UTC (permalink / raw)
  To: Vit Mojzis, selinux

On Mon, 2017-06-26 at 14:38 +0200, 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().
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
> 
> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
> ---
>  libsemanage/src/direct_api.c | 50 +++++++++++++++++++++++++++++-----
> ----------
>  1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/libsemanage/src/direct_api.c
> b/libsemanage/src/direct_api.c
> index ed11a7c..3d1c354 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -296,10 +296,19 @@ 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)
> +
> +	struct stat sb;

Please add a blank line after declarations before code, and move this
up to the beginning of the function.

> +	if (stat(path, &sb) == 0)
>  		sepol_set_disable_dontaudit(sh->sepolh, 1);
> -	else
> -		sepol_set_disable_dontaudit(sh->sepolh, 0);
> +	else{

Please include a space between else and {.

> +		if (errno == ENOENT){

Please include a space between ) and {.

> +			sepol_set_disable_dontaudit(sh->sepolh, 0);
> +		}
> +		else{

This should be } else { on a single line, with space between else and
{.

> +			ERR(sh, "Unable to access disable_dontaudit
> file at %s: %s\n",path , strerror(errno));
> +			goto err;
> +		}
> +	}
>  
>  	return STATUS_SUCCESS;
>  
> @@ -1114,6 +1123,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);
> @@ -1130,9 +1140,12 @@ 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 && status != ENOENT) {

errno != ENOENT

Also, we should ERR() here since otherwise we won't get any diagnostic
output (in the other error cases, the called function handles this, but
here we have a direct failure in this function).

> +			goto cleanup; //an error in the "stat" call
> +		}
>  
>  		status = semanage_compile_module(sh, &modinfos[i]);
>  		if (status < 0) {
> @@ -1200,7 +1213,8 @@ 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)
> +	struct stat sb;

Empty line between declarations and code, move decls to beginning of
function.

> +	if (stat(path, &sb) == 0)
>  		do_rebuild |= !(sepol_get_disable_dontaudit(sh-
> >sepolh) == 1);
>  	else
>  		do_rebuild |= (sepol_get_disable_dontaudit(sh-
> >sepolh) == 1);

No errno check here or below?

> @@ -1225,7 +1239,7 @@ 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
>  		do_rebuild |= (sepol_get_preserve_tunables(sh-
> >sepolh) == 1);
> @@ -1266,37 +1280,37 @@ static int
> semanage_direct_commit(semanage_handle_t * sh)
>  	 */
>  	if (!do_rebuild) {
>  		path = semanage_path(SEMANAGE_TMP,
> SEMANAGE_STORE_KERNEL);
> -		if (access(path, F_OK) != 0) {
> +		if (stat(path, &sb) != 0) {
>  			do_rebuild = 1;
>  			goto rebuild;
>  		}
>  
>  		path = semanage_path(SEMANAGE_TMP,
> SEMANAGE_STORE_FC);
> -		if (access(path, F_OK) != 0) {
> +		if (stat(path, &sb) != 0) {
>  			do_rebuild = 1;
>  			goto rebuild;
>  		}
>  
>  		path = semanage_path(SEMANAGE_TMP,
> SEMANAGE_STORE_SEUSERS);
> -		if (access(path, F_OK) != 0) {
> +		if (stat(path, &sb) != 0) {
>  			do_rebuild = 1;
>  			goto rebuild;
>  		}
>  
>  		path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
> -		if (access(path, F_OK) != 0) {
> +		if (stat(path, &sb) != 0) {
>  			do_rebuild = 1;
>  			goto rebuild;
>  		}
>  
>  		path = semanage_path(SEMANAGE_TMP,
> SEMANAGE_SEUSERS_LINKED);
> -		if (access(path, F_OK) != 0) {
> +		if (stat(path, &sb) != 0) {
>  			do_rebuild = 1;
>  			goto rebuild;
>  		}
>  
>  		path = semanage_path(SEMANAGE_TMP,
> SEMANAGE_USERS_EXTRA_LINKED);
> -		if (access(path, F_OK) != 0) {
> +		if (stat(path, &sb) != 0) {
>  			do_rebuild = 1;
>  			goto rebuild;
>  		}
> @@ -1431,7 +1445,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(SE
> MANAGE_TMP,
>  								  SE
> MANAGE_STORE_SEUSERS),
> @@ -1444,7 +1458,7 @@ rebuild:
>  		}
>  
>  		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(SE
> MANAGE_TMP,
>  								  SE
> MANAGE_USERS_EXTRA),
> @@ -1785,7 +1799,8 @@ static int
> semanage_direct_extract(semanage_handle_t * sh,
>  		goto cleanup;
>  	}
>  
> -	if (access(module_path, F_OK) != 0) {
> +	struct stat sb;
> +	if (stat(module_path, &sb) != 0) {
>  		ERR(sh, "Module does not exist: %s", module_path);
>  		rc = -1;
>  		goto cleanup;
> @@ -1815,7 +1830,7 @@ 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) {
>  		rc = semanage_compile_module(sh, _modinfo);
>  		if (rc < 0) {
>  			goto cleanup;
> @@ -2790,7 +2805,8 @@ static int
> semanage_direct_install_info(semanage_handle_t *sh,
>  			goto cleanup;
>  		}
>  
> -		if (access(path, F_OK) == 0) {
> +		struct stat sb;
> +		if (stat(path, &sb) == 0) {
>  			ret = unlink(path);
>  			if (ret != 0) {
>  				ERR(sh, "Error while removing cached
> CIL file %s: %s", path, strerror(errno));

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2018-03-19 16:18 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-06 11:58 libsemanage: Perform access check using euid instead of uid v2 Vit Mojzis
2018-03-06 11:58 ` [PATCH 1/3] libsemanage: remove access() check to make setuid programs work Vit Mojzis
2018-03-06 11:58 ` [PATCH 2/3] " Vit Mojzis
2018-03-08 19:54   ` Stephen Smalley
2018-03-06 11:58 ` [PATCH 3/3] libsemanage: replace access() checks " Vit Mojzis
2018-03-07 14:59   ` Stephen Smalley
2018-03-08 20:24     ` Stephen Smalley
2018-03-09 13:14     ` Vit Mojzis
2018-03-09 13:28       ` Stephen Smalley
2018-03-09 13:36         ` Stephen Smalley
2018-03-09 15:21           ` [PATCH] " Vit Mojzis
2018-03-09 15:31             ` Stephen Smalley
2018-03-09 15:39               ` Vit Mojzis
2018-03-09 15:51                 ` Stephen Smalley
2018-03-13 10:58                   ` Petr Lautrbach
2018-03-17  7:26                 ` Petr Lautrbach
2018-03-19 14:46                   ` [PATCH] libsemanage/direct_api.c: Fix iterating over array Vit Mojzis
2018-03-19 15:19                     ` William Roberts
2018-03-19 16:18                       ` William Roberts
  -- strict thread matches above, loose matches on Subject: below --
2017-06-26 13:55 [PATCH 3/3] libsemanage: replace access(, F_OK) checks to make setuid programs work Stephen Smalley
2018-02-28 10:15 ` [PATCH 1/3] libsemanage: remove access() check " Vit Mojzis
2018-02-28 10:15   ` [PATCH 3/3] libsemanage: replace access() checks " Vit Mojzis
2018-02-28 17:50     ` William Roberts
2018-02-28 18:24       ` William Roberts
2018-02-28 18:43         ` Stephen Smalley
2018-02-28 19:07           ` William Roberts
2018-02-28 19:26             ` William Roberts
2018-02-28 19:39               ` Stephen Smalley
2018-02-28 19:44                 ` William Roberts
2018-02-28 20:53                   ` Stephen Smalley
2018-03-01 14:40                     ` Stephen Smalley
2018-03-01 14:46                       ` Stephen Smalley
2018-02-28 18:41     ` Stephen Smalley

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.