[PATCH] Smack: fix domain transfer issues

[PATCH] Smack: fix domain transfer issues

[Jarkko Sakkinen]

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
+
+/*
  * Smack audit data; is empty if CONFIG_AUDIT not set
  * to save some stack
  */
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
@@ -5,12 +5,13 @@
  *
  *  Authors:
  *	Casey Schaufler <casey@schaufler-ca.com>
- *	Jarkko Sakkinen <ext-jarkko.2.sakkinen@nokia.com>
+ *	Jarkko Sakkinen <jarkko.sakkinen@intel.com>
  *
  *  Copyright (C) 2007 Casey Schaufler <casey@schaufler-ca.com>
  *  Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
  *                Paul Moore <paul@paul-moore.com>
  *  Copyright (C) 2010 Nokia Corporation
+ *  Copyright (C) 2011 Intel Corporation.
  *
  *	This program is free software; you can redistribute it and/or modify
  *	it under the terms of the GNU General Public License version 2,
@@ -441,11 +442,17 @@ static int smack_sb_umount(struct vfsmount *mnt, int flags)
  * BPRM hooks
  */
 
+/**
+ * smack_bprm_set_creds - Smack exec that handles the domain transfer.
+ * @bprm: binprm for exec
+ *
+ * Returns 0 on success.
+ */
 static int smack_bprm_set_creds(struct linux_binprm *bprm)
 {
+	struct inode *inode = bprm->file->f_path.dentry->d_inode;
 	struct task_smack *tsp = bprm->cred->security;
 	struct inode_smack *isp;
-	struct dentry *dp;
 	int rc;
 
 	rc = cap_bprm_set_creds(bprm);
@@ -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;
+
+	tsp->smk_task = isp->smk_task;
+	bprm->per_clear |= PER_CLEAR_ON_SETID;
+	bprm->unsafe |= SMK_SECUREEXEC_NEEDED;
 
 	return 0;
 }
 
+/**
+ * smack_bprm_secureexec - Return the decision to use secureexec.
+ * @bprm: binprm for exec
+ *
+ * Returns 0 on success.
+ */
+static int smack_bprm_secureexec(struct linux_binprm *bprm)
+{
+	int ret = cap_bprm_secureexec(bprm);
+
+	if (!ret && (bprm->unsafe & SMK_SECUREEXEC_NEEDED))
+		ret = 1;
+
+	return ret;
+}
+
 /*
  * Inode hooks
  */
@@ -3452,6 +3477,7 @@ struct security_operations smack_ops = {
 	.sb_umount = 			smack_sb_umount,
 
 	.bprm_set_creds =		smack_bprm_set_creds,
+	.bprm_secureexec =		smack_bprm_secureexec,
 
 	.inode_alloc_security = 	smack_inode_alloc_security,
 	.inode_free_security = 		smack_inode_free_security,
-- 
1.7.4.1

Re: [PATCH] Smack: fix domain transfer issues

[Stephen Smalley]

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

Re: [PATCH] Smack: fix domain transfer issues

[Jarkko Sakkinen]

On Wed, 28 Sep 2011, Stephen Smalley wrote:
> 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.

You got your point. I'll add smk_flags field to
the struct task_smack (our task security blob).

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

I guess my purpose was to leave those visible because
plan is to add permission checks at least for the
ptrace case in the future. Now that I think of it,
it does not justify their existence in the code.
I'll revise this.

Even in the current form, there should be check that
you described to protect this code against situation
where new LSM_UNSAFE flag is added and support has not
yet been implemented to Smack.

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

MNT_NOSUID should be checked. Also, I'll plan to
implement permission check for ptrace but in the
scope of this patch.

Thanks for informative reply!

/Jarkko

Re: [PATCH] Smack: fix domain transfer issues

[Stephen Smalley]

On Thu, 2011-09-29 at 11:26 +0300, Jarkko Sakkinen wrote:
> MNT_NOSUID should be checked.

Doubtful, as Smack and capabilities are completely orthogonal, right?
Even for SELinux, the nosuid check is a bit of a nuisance.

>  Also, I'll plan to
> implement permission check for ptrace but in the
> scope of this patch.

Still no transition or entrypoint checks, open file revalidation, parent
death signal clearing, ...?

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] Smack: fix domain transfer issues

[Jarkko Sakkinen]

On Thu, 29 Sep 2011, Stephen Smalley wrote:

> On Thu, 2011-09-29 at 11:26 +0300, Jarkko Sakkinen wrote:
>> MNT_NOSUID should be checked.
>
> Doubtful, as Smack and capabilities are completely orthogonal, right?
> Even for SELinux, the nosuid check is a bit of a nuisance.

What I'm planning to do is to not switch
domain if filesystem is mounted with nosuid.
Same logic as prepare_binprm does for suid
and sgid bits.

>>  Also, I'll plan to
>> implement permission check for ptrace but in the
>> scope of this patch.
>
> Still no transition or entrypoint checks, open file revalidation, parent
> death signal clearing, ...?

I've already added death signal clearing to the
next-to-be-submitted revision of this patch.
I'm planning to implemented flushing of
non-permissible files and signals as two separate
patches later on (in the near future however).

/Jarkko

Re: [PATCH] Smack: fix domain transfer issues

[Stephen Smalley]

On Thu, 2011-09-29 at 16:57 +0300, Jarkko Sakkinen wrote:
> On Thu, 29 Sep 2011, Stephen Smalley wrote:
> 
> > On Thu, 2011-09-29 at 11:26 +0300, Jarkko Sakkinen wrote:
> >> MNT_NOSUID should be checked.
> >
> > Doubtful, as Smack and capabilities are completely orthogonal, right?
> > Even for SELinux, the nosuid check is a bit of a nuisance.
> 
> What I'm planning to do is to not switch
> domain if filesystem is mounted with nosuid.
> Same logic as prepare_binprm does for suid
> and sgid bits.

I'm not sure that is required since a Smack label change doesn't alter
the allowable capabilities, and doing so will create conflicts for users
who will have to choose between supporting Smack label transitions on a
filesystem and mounting it nosuid.  We've seen such issues for SELinux.
Having a separate flag for label transitions would be better.

Also, if it is possible for an exec label to be ignored/overridden, then
you might want to consider an equivalent to the SELinux execute_no_trans
check.

> I've already added death signal clearing to the
> next-to-be-submitted revision of this patch.
> I'm planning to implemented flushing of
> non-permissible files and signals as two separate
> patches later on (in the near future however).

I'd view the lack of equivalents to the transition and entrypoint checks
as more critical, as well as the lack of any control over the
relationship between the file access control label and its exec label.
How can you determine what labels are reachable from a given label?  How
can you determine what programs can be used to enter a given label?  How
can you determine who can modify a program that can be used to enter a
given label?

-- 
Stephen Smalley
National Security Agency