Re: [PATCH] exec: Make suid_dumpable apply to SUID/SGID binaries irrespective of invoking users

[ebiederm@xmission.com (Eric W. Biederman)]

Waiman Long <longman@redhat.com> writes:

> The begin_new_exec() function checks for SUID or SGID binaries by
> comparing effective uid and gid against real uid and gid and using
> the suid_dumpable sysctl parameter setting only if either one of them
> differs.
>
> In the special case that the uid and/or gid of the SUID/SGID binaries
> matches the id's of the user invoking it, the suid_dumpable is not
> used and SUID_DUMP_USER will be used instead. The documentation for the
> suid_dumpable sysctl parameter does not include that exception and so
> this will be an undocumented behavior.
>
> Eliminate this undocumented behavior by adding a flag in the linux_binprm
> structure to designate a SUID/SGID binary and use it for determining
> if the suid_dumpable setting should be applied or not.

I see that you are making the code match the documentation.
What harm/problems does this mismatch cause in practice?
What is the motivation for this change?

I am trying to see the motivation but all I can see is that
in the case where suid and sgid do nothing in practice the code
does not change dumpable.  The point of dumpable is to refuse to
core dump when it is not safe.  In this case since nothing happened
in practice it is safe.

So how does this matter in practice.  If there isn't a good
motivation my feel is that it is the documentation that needs to be
updated rather than the code.

There are a lot of warts to the suid/sgid handling during exec.  This
just doesn't look like one of them.

Eric


> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  fs/exec.c               | 6 +++---
>  include/linux/binfmts.h | 5 ++++-
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 537d92c41105..60e02e678fb6 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1344,9 +1344,7 @@ int begin_new_exec(struct linux_binprm * bprm)
>  	 * is wrong, but userspace depends on it. This should be testing
>  	 * bprm->secureexec instead.
>  	 */
> -	if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||
> -	    !(uid_eq(current_euid(), current_uid()) &&
> -	      gid_eq(current_egid(), current_gid())))
> +	if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP || bprm->is_sugid)
>  		set_dumpable(current->mm, suid_dumpable);
>  	else
>  		set_dumpable(current->mm, SUID_DUMP_USER);
> @@ -1619,11 +1617,13 @@ static void bprm_fill_uid(struct linux_binprm *bprm, struct file *file)
>  	if (mode & S_ISUID) {
>  		bprm->per_clear |= PER_CLEAR_ON_SETID;
>  		bprm->cred->euid = uid;
> +		bprm->is_sugid = 1;
>  	}
>  
>  	if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
>  		bprm->per_clear |= PER_CLEAR_ON_SETID;
>  		bprm->cred->egid = gid;
> +		bprm->is_sugid = 1;
>  	}
>  }
>  
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 049cf9421d83..6d9893c59085 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -41,7 +41,10 @@ struct linux_binprm {
>  		 * Set when errors can no longer be returned to the
>  		 * original userspace.
>  		 */
> -		point_of_no_return:1;
> +		point_of_no_return:1,
> +
> +		/* Is a SUID or SGID binary? */
> +		is_sugid:1;
>  #ifdef __alpha__
>  	unsigned int taso:1;
>  #endif