[RFC PATCH] selinux: prevent setting a security label on MNT_NOSUID applications

[RFC PATCH] selinux: prevent setting a security label on MNT_NOSUID applications

[Paul Moore]

We presently prevent processes from explicitly setting an arbitrary
security label on new processes when NO_NEW_PRIVS is enabled; in an
attempt for more consistency, this patch extends this to prevent
setting an arbitrary label when the new application lives on a
filesystem mounted with MNT_NOSUID.

Signed-off-by: Paul Moore <pmoore@redhat.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Stephen Smalley <sds@tycho.nsa.gov>
---
 security/selinux/hooks.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 57b0b49..6fafe86 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2106,11 +2106,13 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 		new_tsec->exec_sid = 0;
 
 		/*
-		 * Minimize confusion: if no_new_privs and a transition is
-		 * explicitly requested, then fail the exec.
+		 * Minimize confusion: if no_new_privs or nosuid and a
+		 * transition is explicitly requested, then fail the exec.
 		 */
 		if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
 			return -EPERM;
+		if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
+			return -EACCES;
 	} else {
 		/* Check for a default transition on this program. */
 		rc = security_transition_sid(old_tsec->sid, isec->sid,

Re: [RFC PATCH] selinux: prevent setting a security label on MNT_NOSUID applications

[Andy Lutomirski]

On Wed, May 14, 2014 at 8:58 AM, Paul Moore <pmoore@redhat.com> wrote:
> We presently prevent processes from explicitly setting an arbitrary
> security label on new processes when NO_NEW_PRIVS is enabled; in an
> attempt for more consistency, this patch extends this to prevent
> setting an arbitrary label when the new application lives on a
> filesystem mounted with MNT_NOSUID.
>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Stephen Smalley <sds@tycho.nsa.gov>
> ---

Acked-by: Andy Lutomirski <luto@amacapital.net>

However: would it pay to move the check above this:

                /* Reset exec SID on execve. */
                new_tsec->exec_sid = 0;

I suppose that this shouldn't matter: any correct application already
needs to redo setexeccon if it gets an error from execve.  Fixing this
for real would probably involve moving that line of code into
selinux_bprm_committed_creds.

--Andy

Re: [RFC PATCH] selinux: prevent setting a security label on MNT_NOSUID applications

[Andy Lutomirski]

On Wed, May 14, 2014 at 9:16 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Wed, May 14, 2014 at 8:58 AM, Paul Moore <pmoore@redhat.com> wrote:
>> We presently prevent processes from explicitly setting an arbitrary
>> security label on new processes when NO_NEW_PRIVS is enabled; in an
>> attempt for more consistency, this patch extends this to prevent
>> setting an arbitrary label when the new application lives on a
>> filesystem mounted with MNT_NOSUID.
>>
>> Signed-off-by: Paul Moore <pmoore@redhat.com>
>> CC: Andy Lutomirski <luto@amacapital.net>
>> CC: Stephen Smalley <sds@tycho.nsa.gov>
>> ---
>
> Acked-by: Andy Lutomirski <luto@amacapital.net>
>
> However: would it pay to move the check above this:
>
>                 /* Reset exec SID on execve. */
>                 new_tsec->exec_sid = 0;
>
> I suppose that this shouldn't matter: any correct application already
> needs to redo setexeccon if it gets an error from execve.  Fixing this
> for real would probably involve moving that line of code into
> selinux_bprm_committed_creds.

I'm also unconvinced by the subject line -- would "selinux: return an
error when rejecting settexeccon on MNT_NOSUID applications" be
better?

--Andy

Re: [RFC PATCH] selinux: prevent setting a security label on MNT_NOSUID applications

[Stephen Smalley]

On 05/14/2014 12:16 PM, Andy Lutomirski wrote:
> On Wed, May 14, 2014 at 8:58 AM, Paul Moore <pmoore@redhat.com> wrote:
>> We presently prevent processes from explicitly setting an arbitrary
>> security label on new processes when NO_NEW_PRIVS is enabled; in an
>> attempt for more consistency, this patch extends this to prevent
>> setting an arbitrary label when the new application lives on a
>> filesystem mounted with MNT_NOSUID.
>>
>> Signed-off-by: Paul Moore <pmoore@redhat.com>
>> CC: Andy Lutomirski <luto@amacapital.net>
>> CC: Stephen Smalley <sds@tycho.nsa.gov>
>> ---
> 
> Acked-by: Andy Lutomirski <luto@amacapital.net>
> 
> However: would it pay to move the check above this:
> 
>                 /* Reset exec SID on execve. */
>                 new_tsec->exec_sid = 0;
> 
> I suppose that this shouldn't matter: any correct application already
> needs to redo setexeccon if it gets an error from execve.  Fixing this
> for real would probably involve moving that line of code into
> selinux_bprm_committed_creds.

Shouldn't matter as that is a change to new_tsec (i.e.
bprm->cred->security), which will only be applied to the process if we
reach install_exec_creds(); otherwise it will just be discarded.

Re: [RFC PATCH] selinux: prevent setting a security label on MNT_NOSUID applications

[Andy Lutomirski]

On Wed, May 14, 2014 at 9:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 05/14/2014 12:16 PM, Andy Lutomirski wrote:
>> On Wed, May 14, 2014 at 8:58 AM, Paul Moore <pmoore@redhat.com> wrote:
>>> We presently prevent processes from explicitly setting an arbitrary
>>> security label on new processes when NO_NEW_PRIVS is enabled; in an
>>> attempt for more consistency, this patch extends this to prevent
>>> setting an arbitrary label when the new application lives on a
>>> filesystem mounted with MNT_NOSUID.
>>>
>>> Signed-off-by: Paul Moore <pmoore@redhat.com>
>>> CC: Andy Lutomirski <luto@amacapital.net>
>>> CC: Stephen Smalley <sds@tycho.nsa.gov>
>>> ---
>>
>> Acked-by: Andy Lutomirski <luto@amacapital.net>
>>
>> However: would it pay to move the check above this:
>>
>>                 /* Reset exec SID on execve. */
>>                 new_tsec->exec_sid = 0;
>>
>> I suppose that this shouldn't matter: any correct application already
>> needs to redo setexeccon if it gets an error from execve.  Fixing this
>> for real would probably involve moving that line of code into
>> selinux_bprm_committed_creds.
>
> Shouldn't matter as that is a change to new_tsec (i.e.
> bprm->cred->security), which will only be applied to the process if we
> reach install_exec_creds(); otherwise it will just be discarded.
>
>

Indeed.  Sorry for the early-morning failure to read the code.

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC PATCH] selinux: prevent setting a security label on MNT_NOSUID applications

[Stephen Smalley]

On 05/14/2014 11:58 AM, Paul Moore wrote:
> We presently prevent processes from explicitly setting an arbitrary
> security label on new processes when NO_NEW_PRIVS is enabled; in an
> attempt for more consistency, this patch extends this to prevent
> setting an arbitrary label when the new application lives on a
> filesystem mounted with MNT_NOSUID.

It is never arbitrary (the new value is always controlled by policy),
and it isn't set on "new processes" per se but rather transitioned to
upon an exec.  Anyway, the point of the change is to return an error
rather than silently ignore any /proc/self/attr/exec value when
executing from a nosuid mount.

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Stephen Smalley <sds@tycho.nsa.gov>
> ---
>  security/selinux/hooks.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 57b0b49..6fafe86 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2106,11 +2106,13 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>  		new_tsec->exec_sid = 0;
>  
>  		/*
> -		 * Minimize confusion: if no_new_privs and a transition is
> -		 * explicitly requested, then fail the exec.
> +		 * Minimize confusion: if no_new_privs or nosuid and a
> +		 * transition is explicitly requested, then fail the exec.
>  		 */
>  		if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
>  			return -EPERM;
> +		if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
> +			return -EACCES;
>  	} else {
>  		/* Check for a default transition on this program. */
>  		rc = security_transition_sid(old_tsec->sid, isec->sid,
> 
> 

Re: [RFC PATCH] selinux: prevent setting a security label on MNT_NOSUID applications

[Paul Moore]

On Wednesday, May 14, 2014 09:18:35 AM Andy Lutomirski wrote:
> > On Wed, May 14, 2014 at 8:58 AM, Paul Moore <pmoore@redhat.com> wrote:
> >> We presently prevent processes from explicitly setting an arbitrary
> >> security label on new processes when NO_NEW_PRIVS is enabled; in an
> >> attempt for more consistency, this patch extends this to prevent
> >> setting an arbitrary label when the new application lives on a
> >> filesystem mounted with MNT_NOSUID.
> >> 
> >> Signed-off-by: Paul Moore <pmoore@redhat.com>
> >> CC: Andy Lutomirski <luto@amacapital.net>
> >> CC: Stephen Smalley <sds@tycho.nsa.gov>
> >> ---
> > 
> Acked-by: Andy Lutomirski <luto@amacapital.net>
>
> I'm also unconvinced by the subject line -- would "selinux: return an
> error when rejecting settexeccon on MNT_NOSUID applications" be
> better?

...

On Wednesday, May 14, 2014 12:28:24 PM Stephen Smalley wrote:
> On 05/14/2014 11:58 AM, Paul Moore wrote:
> > We presently prevent processes from explicitly setting an arbitrary
> > security label on new processes when NO_NEW_PRIVS is enabled; in an
> > attempt for more consistency, this patch extends this to prevent
> > setting an arbitrary label when the new application lives on a
> > filesystem mounted with MNT_NOSUID.
> 
> It is never arbitrary (the new value is always controlled by policy),
> and it isn't set on "new processes" per se but rather transitioned to
> upon an exec.  Anyway, the point of the change is to return an error
> rather than silently ignore any /proc/self/attr/exec value when
> executing from a nosuid mount.
> 
> Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

I had a feeling while writing the patch description yesterday that someone was 
not going to be happy with the text ... does the subject/description below 
sound better to you guys?

***
selinux: reject setexeccon() on MNT_NOSUID applications with -EACCES

We presently prevent processes from using setexecon() to set the security 
label of exec()'d processes when NO_NEW_PRIVS is enabled by returning an 
error; however, we silently ignore setexeccon() when exec()'ing from a nosuid 
mounted filesystem.  This patch makes things a bit more consistent by 
returning an error in the setexeccon()/nosuid case.
***

> > Signed-off-by: Paul Moore <pmoore@redhat.com>
> > CC: Andy Lutomirski <luto@amacapital.net>
> > CC: Stephen Smalley <sds@tycho.nsa.gov>
> > ---
> > 
> >  security/selinux/hooks.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 57b0b49..6fafe86 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2106,11 +2106,13 @@ static int selinux_bprm_set_creds(struct
> > linux_binprm *bprm)> 
> >  		new_tsec->exec_sid = 0;
> >  		
> >  		/*
> > 
> > -		 * Minimize confusion: if no_new_privs and a transition is
> > -		 * explicitly requested, then fail the exec.
> > +		 * Minimize confusion: if no_new_privs or nosuid and a
> > +		 * transition is explicitly requested, then fail the exec.
> > 
> >  		 */
> >  		
> >  		if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
> >  		
> >  			return -EPERM;
> > 
> > +		if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
> > +			return -EACCES;
> > 
> >  	} else {
> >  	
> >  		/* Check for a default transition on this program. */
> >  		rc = security_transition_sid(old_tsec->sid, isec->sid,

-- 
paul moore
security and virtualization @ redhat

Re: [RFC PATCH] selinux: prevent setting a security label on MNT_NOSUID applications

[Andy Lutomirski]

On Wed, May 14, 2014 at 2:00 PM, Paul Moore <pmoore@redhat.com> wrote:
> On Wednesday, May 14, 2014 09:18:35 AM Andy Lutomirski wrote:
>> > On Wed, May 14, 2014 at 8:58 AM, Paul Moore <pmoore@redhat.com> wrote:
>> >> We presently prevent processes from explicitly setting an arbitrary
>> >> security label on new processes when NO_NEW_PRIVS is enabled; in an
>> >> attempt for more consistency, this patch extends this to prevent
>> >> setting an arbitrary label when the new application lives on a
>> >> filesystem mounted with MNT_NOSUID.
>> >>
>> >> Signed-off-by: Paul Moore <pmoore@redhat.com>
>> >> CC: Andy Lutomirski <luto@amacapital.net>
>> >> CC: Stephen Smalley <sds@tycho.nsa.gov>
>> >> ---
>> >
>> Acked-by: Andy Lutomirski <luto@amacapital.net>
>>
>> I'm also unconvinced by the subject line -- would "selinux: return an
>> error when rejecting settexeccon on MNT_NOSUID applications" be
>> better?
>
> ...
>
> On Wednesday, May 14, 2014 12:28:24 PM Stephen Smalley wrote:
>> On 05/14/2014 11:58 AM, Paul Moore wrote:
>> > We presently prevent processes from explicitly setting an arbitrary
>> > security label on new processes when NO_NEW_PRIVS is enabled; in an
>> > attempt for more consistency, this patch extends this to prevent
>> > setting an arbitrary label when the new application lives on a
>> > filesystem mounted with MNT_NOSUID.
>>
>> It is never arbitrary (the new value is always controlled by policy),
>> and it isn't set on "new processes" per se but rather transitioned to
>> upon an exec.  Anyway, the point of the change is to return an error
>> rather than silently ignore any /proc/self/attr/exec value when
>> executing from a nosuid mount.
>>
>> Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>
>
> I had a feeling while writing the patch description yesterday that someone was
> not going to be happy with the text ... does the subject/description below
> sound better to you guys?
>
> ***
> selinux: reject setexeccon() on MNT_NOSUID applications with -EACCES
>
> We presently prevent processes from using setexecon() to set the security
> label of exec()'d processes when NO_NEW_PRIVS is enabled by returning an
> error; however, we silently ignore setexeccon() when exec()'ing from a nosuid
> mounted filesystem.  This patch makes things a bit more consistent by
> returning an error in the setexeccon()/nosuid case.
> ***

Looks good to me.

--Andy

Re: [RFC PATCH] selinux: prevent setting a security label on MNT_NOSUID applications

[Paul Moore]

On Wednesday, May 14, 2014 02:26:59 PM Andy Lutomirski wrote:
> On Wed, May 14, 2014 at 2:00 PM, Paul Moore <pmoore@redhat.com> wrote:
> > On Wednesday, May 14, 2014 09:18:35 AM Andy Lutomirski wrote:
> >> > On Wed, May 14, 2014 at 8:58 AM, Paul Moore <pmoore@redhat.com> wrote:
> >> >> We presently prevent processes from explicitly setting an arbitrary
> >> >> security label on new processes when NO_NEW_PRIVS is enabled; in an
> >> >> attempt for more consistency, this patch extends this to prevent
> >> >> setting an arbitrary label when the new application lives on a
> >> >> filesystem mounted with MNT_NOSUID.
> >> >> 
> >> >> Signed-off-by: Paul Moore <pmoore@redhat.com>
> >> >> CC: Andy Lutomirski <luto@amacapital.net>
> >> >> CC: Stephen Smalley <sds@tycho.nsa.gov>
> >> >> ---
> >> 
> >> Acked-by: Andy Lutomirski <luto@amacapital.net>
> >> 
> >> I'm also unconvinced by the subject line -- would "selinux: return an
> >> error when rejecting settexeccon on MNT_NOSUID applications" be
> >> better?
> > 
> > ...
> > 
> > On Wednesday, May 14, 2014 12:28:24 PM Stephen Smalley wrote:
> >> On 05/14/2014 11:58 AM, Paul Moore wrote:
> >> > We presently prevent processes from explicitly setting an arbitrary
> >> > security label on new processes when NO_NEW_PRIVS is enabled; in an
> >> > attempt for more consistency, this patch extends this to prevent
> >> > setting an arbitrary label when the new application lives on a
> >> > filesystem mounted with MNT_NOSUID.
> >> 
> >> It is never arbitrary (the new value is always controlled by policy),
> >> and it isn't set on "new processes" per se but rather transitioned to
> >> upon an exec.  Anyway, the point of the change is to return an error
> >> rather than silently ignore any /proc/self/attr/exec value when
> >> executing from a nosuid mount.
> >> 
> >> Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>
> > 
> > I had a feeling while writing the patch description yesterday that someone
> > was not going to be happy with the text ... does the subject/description
> > below sound better to you guys?
> > 
> > ***
> > selinux: reject setexeccon() on MNT_NOSUID applications with -EACCES
> > 
> > We presently prevent processes from using setexecon() to set the security
> > label of exec()'d processes when NO_NEW_PRIVS is enabled by returning an
> > error; however, we silently ignore setexeccon() when exec()'ing from a
> > nosuid mounted filesystem.  This patch makes things a bit more consistent
> > by returning an error in the setexeccon()/nosuid case.
> > ***
> 
> Looks good to me.

Okay, I expect if Stephen had a strong objection to the new text he would have 
commented by now.  I'm going to toss this into #next with the new text now.

-- 
paul moore
security and virtualization @ redhat