selinux: prevent setting a security label on MNT_NOSUID applications

RFC PATCH In review Paul Moore 3 participants 9 messages May 14, 2014 – May 15, 2014 SELinux security module

Share & tools

Paul Moore

author

2014-05-14 15:58 UTC #0 lore raw

RFC PATCH 1/1 selinux: prevent setting a security label on MNT_NOSUID applications

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 <[email protected]>

CC Andy Lutomirski <[email protected]>

CC Stephen Smalley <[email protected]>

---
 security/selinux/hooks.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

security/selinux/hooks.c +4 -2

index 57b0b49..6fafe86 100644
@@ -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,

Andy Lutomirski

Acked-by on v1 1/1

2014-05-14 16:16 UTC #1 lore raw

Quoted 10 lines · 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 <[email protected]>
CC Andy Lutomirski <[email protected]>
CC Stephen Smalley <[email protected]>
---

Acked-by Andy Lutomirski <[email protected]>

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

Andy Lutomirski

2014-05-14 16:18 UTC #2 lore raw

Quoted 16 lines · Andy Lutomirski

Quoted 10 lines · 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 <[email protected]>
CC Andy Lutomirski <[email protected]>
CC Stephen Smalley <[email protected]>
---
Acked-by Andy Lutomirski <[email protected]>
However: would it pay to move the check above this:

Quoted 7 lines

                /* 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

Stephen Smalley

2014-05-14 16:23 UTC #3 lore raw

In reply to Andy Lutomirski

Quoted 16 lines · Andy Lutomirski

Quoted 10 lines · 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 <[email protected]>
CC Andy Lutomirski <[email protected]>
CC Stephen Smalley <[email protected]>
---
Acked-by Andy Lutomirski <[email protected]>
However: would it pay to move the check above this:

Quoted 7 lines

                /* 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.

Andy Lutomirski

2014-05-14 16:48 UTC #4 lore raw

Quoted 20 lines · Stephen Smalley

Quoted 19 lines · Andy Lutomirski
Quoted 10 lines · 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 <[email protected]>
CC Andy Lutomirski <[email protected]>
CC Stephen Smalley <[email protected]>
---
Acked-by Andy Lutomirski <[email protected]>
However: would it pay to move the check above this:
                /* Reset exec SID on execve. */
                new_tsec->exec_sid = 0;

Quoted 10 lines

Quoted 4 lines
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.

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Stephen Smalley

Acked-by on v1 1/1

2014-05-14 16:28 UTC #5 lore raw

In reply to Paul Moore

Quoted 5 lines · 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.

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 <[email protected]>

Quoted 30 lines

Signed-off-by Paul Moore <[email protected]>

CC Andy Lutomirski <[email protected]>

CC Stephen Smalley <[email protected]>

---
 security/selinux/hooks.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

security/selinux/hooks.c +4 -2

index 57b0b49..6fafe86 100644
@@ -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

author

2014-05-14 21:00 UTC #6 lore raw

Quoted 17 lines · Andy Lutomirski

Quoted 7 lines · Paul Moore
Quoted 6 lines
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.
Quoted 5 lines
Quoted 4 lines
Signed-off-by Paul Moore <[email protected]>
CC Andy Lutomirski <[email protected]>
CC Stephen Smalley <[email protected]>
---
Acked-by Andy Lutomirski <[email protected]>
I'm also unconvinced by the subject line -- would "selinux: return an error when rejecting settexeccon on MNT_NOSUID applications" be better?

...

Quoted 7 lines · Stephen Smalley

Quoted 5 lines · 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.

Quoted 7 lines

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 <[email protected]>

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.
***

Quoted 36 lines

Signed-off-by Paul Moore <[email protected]>

CC Andy Lutomirski <[email protected]>

CC Stephen Smalley <[email protected]>

---

 security/selinux/hooks.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

security/selinux/hooks.c +0 -0

index 57b0b49..6fafe86 100644
@@ -2106,11 +2106,13 @@ static int selinux_bprm_set_creds(struct

linux_binprm *bprm)>

security/selinux/hooks.c +4 -2

 		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,

Signature · 2 lines

paul moore
security and virtualization @ redhat

Andy Lutomirski

2014-05-14 21:27 UTC #7 lore raw

Quoted 44 lines · Paul Moore

Quoted 17 lines · Andy Lutomirski
Quoted 7 lines · Paul Moore
Quoted 6 lines
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.
Quoted 5 lines
Quoted 4 lines
Signed-off-by Paul Moore <[email protected]>
CC Andy Lutomirski <[email protected]>
CC Stephen Smalley <[email protected]>
---
Acked-by Andy Lutomirski <[email protected]>
I'm also unconvinced by the subject line -- would "selinux: return an error when rejecting settexeccon on MNT_NOSUID applications" be better?
...
Quoted 7 lines · Stephen Smalley
Quoted 5 lines · 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.
Quoted 7 lines
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 <[email protected]>
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

Quoted 6 lines

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

Paul Moore

author

2014-05-15 15:14 UTC #8 lore raw

Quoted 45 lines · Andy Lutomirski

Quoted 44 lines · Paul Moore
Quoted 17 lines · Andy Lutomirski
Quoted 11 lines · Paul Moore
Quoted 10 lines
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 <[email protected]>
CC Andy Lutomirski <[email protected]>
CC Stephen Smalley <[email protected]>
---
Acked-by Andy Lutomirski <[email protected]>
I'm also unconvinced by the subject line -- would "selinux: return an error when rejecting settexeccon on MNT_NOSUID applications" be better?
...
Quoted 7 lines · Stephen Smalley
Quoted 5 lines · 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.
Quoted 7 lines
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 <[email protected]>
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

Quoted 8 lines

Quoted 6 lines
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.

Signature · 2 lines

paul moore
security and virtualization @ redhat