[PATCH] Smack: Use secureexec with SMACK64EXEC

[PATCH] Smack: Use secureexec with SMACK64EXEC

[Jarkko Sakkinen]

SMACK64EXEC extended attribute allows switching to
another security context when executing a file.

This patch enables secureexec bit in ELF auxiliary
vector so that code cannot be injected from executers
security context.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
---
 security/smack/smack.h     |    5 +++++
 security/smack/smack_lsm.c |   15 ++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/security/smack/smack.h b/security/smack/smack.h
index 2b6c6a5..e41fb07 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -181,6 +181,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 b9c5e14..3ea018d 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -465,12 +465,24 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
 
 	isp = dp->d_inode->i_security;
 
-	if (isp->smk_task != NULL)
+	if (isp->smk_task != NULL) {
 		tsp->smk_task = isp->smk_task;
+		bprm->unsafe = SMK_SECUREEXEC_NEEDED;
+	}
 
 	return 0;
 }
 
+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
  */
@@ -3409,6 +3421,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: Use secureexec with SMACK64EXEC

[Stephen Smalley]

On Tue, 2011-09-20 at 15:37 +0300, Jarkko Sakkinen wrote:
> SMACK64EXEC extended attribute allows switching to
> another security context when executing a file.
> 
> This patch enables secureexec bit in ELF auxiliary
> vector so that code cannot be injected from executers
> security context.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
> ---
>  security/smack/smack.h     |    5 +++++
>  security/smack/smack_lsm.c |   15 ++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 2b6c6a5..e41fb07 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -181,6 +181,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 b9c5e14..3ea018d 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -465,12 +465,24 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
>  
>  	isp = dp->d_inode->i_security;
>  
> -	if (isp->smk_task != NULL)
> +	if (isp->smk_task != NULL) {
>  		tsp->smk_task = isp->smk_task;
> +		bprm->unsafe = SMK_SECUREEXEC_NEEDED;
> +	}
>  
>  	return 0;
>  }
>  
> +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
>   */

bprm->unsafe isn't private to your security module, unlike e.g.
bprm->cred->security.  And it isn't intended to indicate that a
secureexec is being performed, but instead as an indicator that a
credential-changing exec may be unsafe.  Which you presently ignore.
Defining and setting a new flag in it will have interesting side
effects, e.g. consider cap_bprm_secureexec, not to mention being a
layering violation and a source of future conflicts.

Why can't your bprm_secureexec hook just test isp->smk_task directly?
It can reach it from the bprm.  Or if you don't like testing it twice,
then you could always add a flag to your struct referenced by
bprm->cred->security, i.e. the smack_task struct.

BTW, there is a lot more to do if you want SMACK64EXEC to be safe.

-- 
Stephen Smalley
National Security Agency

RE: [PATCH] Smack: Use secureexec with SMACK64EXEC

[Ryan Ware]

> -----Original Message-----
> 
> 
> bprm->unsafe isn't private to your security module, unlike e.g.
> bprm->cred->security.  And it isn't intended to indicate that a
> secureexec is being performed, but instead as an indicator that a
> credential-changing exec may be unsafe.  Which you presently ignore.
> Defining and setting a new flag in it will have interesting side effects,
> e.g. consider cap_bprm_secureexec, not to mention being a layering
> violation and a source of future conflicts.
> 
> Why can't your bprm_secureexec hook just test isp->smk_task directly?
> It can reach it from the bprm.  Or if you don't like testing it twice,
> then you could always add a flag to your struct referenced by
> bprm->cred->security, i.e. the smack_task struct.
> 
> BTW, there is a lot more to do if you want SMACK64EXEC to be safe.

Thanks for the feedback Stephen.  Could you be more detailed on what else you feel needs to be in place to make SMACK64EXEC safe?

Ryan

Re: [PATCH] Smack: Use secureexec with SMACK64EXEC

[Sakkinen, Jarkko]

On Wed, Sep 21, 2011 at 8:15 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> bprm->unsafe isn't private to your security module, unlike e.g.
> bprm->cred->security.  And it isn't intended to indicate that a
> secureexec is being performed, but instead as an indicator that a
> credential-changing exec may be unsafe.  Which you presently ignore.
> Defining and setting a new flag in it will have interesting side
> effects, e.g. consider cap_bprm_secureexec, not to mention being a
> layering violation and a source of future conflicts.
>
> Why can't your bprm_secureexec hook just test isp->smk_task directly?
> It can reach it from the bprm.  Or if you don't like testing it twice,
> then you could always add a flag to your struct referenced by
> bprm->cred->security, i.e. the smack_task struct.

Thank you. You're absolutely right on this and yes, I can
safely just use isp->smk_task. No need for that flag.

BTW, do you know why AppArmor does use similar flag
AA_SECURE_X_NEEDED?

> BTW, there is a lot more to do if you want SMACK64EXEC to be safe.

Can you open up this a bit?

/Jarkko

Re: [PATCH] Smack: Use secureexec with SMACK64EXEC

[Stephen Smalley]

On Thu, 2011-09-22 at 10:25 +0300, Sakkinen, Jarkko wrote:
> On Wed, Sep 21, 2011 at 8:15 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > bprm->unsafe isn't private to your security module, unlike e.g.
> > bprm->cred->security.  And it isn't intended to indicate that a
> > secureexec is being performed, but instead as an indicator that a
> > credential-changing exec may be unsafe.  Which you presently ignore.
> > Defining and setting a new flag in it will have interesting side
> > effects, e.g. consider cap_bprm_secureexec, not to mention being a
> > layering violation and a source of future conflicts.
> >
> > Why can't your bprm_secureexec hook just test isp->smk_task directly?
> > It can reach it from the bprm.  Or if you don't like testing it twice,
> > then you could always add a flag to your struct referenced by
> > bprm->cred->security, i.e. the smack_task struct.
> 
> Thank you. You're absolutely right on this and yes, I can
> safely just use isp->smk_task. No need for that flag.
> 
> BTW, do you know why AppArmor does use similar flag
> AA_SECURE_X_NEEDED?

No idea.  Sounds wrong to me.

> > BTW, there is a lot more to do if you want SMACK64EXEC to be safe.
> 
> Can you open up this a bit?

I'd suggest studying the SELinux bprm hooks. You don't necessarily have
to do everything we do, but you at least should understand why we do it
and what tradeoff you are making by not doing it.

-- 
Stephen Smalley
National Security Agency