Re: [PATCH] Smack: fix domain transfer issues

[Stephen Smalley <sds@tycho.nsa.gov>]

On Wed, 2011-09-28 at 13:48 +0300, Jarkko Sakkinen wrote:
> When domain changes, Smack should check for ptracing
> and shared state. Additionally, it should clear unsafe
> personality bits and turn on the secureexec bit. This
> patch addresses these issues.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
> ---
>  security/smack/smack.h     |    5 ++++
>  security/smack/smack_lsm.c |   46 ++++++++++++++++++++++++++++++++++---------
>  2 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 174d3be..7b37615 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -187,6 +187,11 @@ struct smack_known {
>  #define SMK_NUM_ACCESS_TYPE 4
>  
>  /*
> + * Passed in the bprm->unsafe field
> + */
> +#define SMK_SECUREEXEC_NEEDED 0x8000

Did I miss something or did you find a rationale for using bprm->unsafe
in this manner?  It isn't private to your security module yet you are
claiming a bit for your own private use without reserving it in any way
globally (consider implications for any future stacking), and setting
new bits in it will have side effects on the capabilities logic.  I
already mentioned this on your first patch and you seemed to acknowledge
it then.  Pass it via bprm->cred->security if you need to pass it as a
flag, or re-test the condition in the secureexec hook otherwise.

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2e71c3f..b3766ac 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -455,22 +462,40 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
>  	if (bprm->cred_prepared)
>  		return 0;
>  
> -	if (bprm->file == NULL || bprm->file->f_dentry == NULL)
> -		return 0;
> -
> -	dp = bprm->file->f_dentry;
> +	isp = inode->i_security;
>  
> -	if (dp->d_inode == NULL)
> +	if (isp->smk_task == NULL || isp->smk_task == tsp->smk_task)
>  		return 0;
>  
> -	isp = dp->d_inode->i_security;
> +	if (bprm->unsafe & LSM_UNSAFE_SHARE)
> +		return -EPERM;
>  
> -	if (isp->smk_task != NULL)
> -		tsp->smk_task = isp->smk_task;
> +	if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP))
> +		return -EPERM;

Why not just:
	if (bprm->unsafe)
		return -EPERM;
if you aren't going to distinguish them via permission checks or
anything?

I take it you've decided you don't need any of the other checks or
sanitization applied by SELinux?

-- 
Stephen Smalley
National Security Agency