[RFC] Clean up MNT_NOSUID handling in exec

[Andrew Lutomirski]

On Tue, Apr 20, 2010 at 10:25 PM, Serge E. Hallyn <serue@xxxxxxxxxx> wrote:
> Quoting Andrew Lutomirski (luto@xxxxxxx):
>> On Tue, Apr 20, 2010 at 8:37 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
>> >
>> > At least in the case of SELinux, context transitions upon execve are
>> > already disabled in the nosuid case, and Eric's patch updated the
>> > SELinux test accordingly.
>>
>> I don't see that code in current -linus, nor do I see where SELinux
>> affects dumpability.  What's supposed to happen?  I'm writing a patch
>> right now to clean this stuff up.
>
> check out security/selinux/hooks.c:selinux_bprm_set_creds()
>
>        if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
>                new_tsec->sid = old_tsec->sid;
>
> I assume that's it?

*sigh*, that's it, of course.

But it seems odd that if you ask the system to change sid (i.e.
exec_sid) and then you exec something that's nosuid, your request gets
silently ignored.  Presumably either the request should be respected
or some error should occur.

Also, the FILE__EXECUTE_NO_TRANS check looks bogus in the nosuid case
-- you can't trust security labels if nosuid.

Here's a compile-tested patch (against current -git) that might make
this all it bit more sane, and as a bonus it makes it easier to reuse
the NOSUID mechanism down the road for some prctl flag.  Thoughts?
SECINITSID_UNLABELED might want to be SECINITSID_FILE.


diff --git a/fs/exec.c b/fs/exec.c
index 49cdaa1..9b34234 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1151,11 +1151,14 @@ int prepare_binprm(struct linux_binprm *bprm)
 	if (bprm->file->f_op == NULL)
 		return -EACCES;

+	/* Check this exactly once to avoid races. */
+	bprm->file_nosuid = !!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID);
+
 	/* clear any previous set[ug]id data from a previous binary */
 	bprm->cred->euid = current_euid();
 	bprm->cred->egid = current_egid();

-	if (!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)) {
+	if (!bprm->file_nosuid) {
 		/* Set-uid? */
 		if (mode & S_ISUID) {
 			bprm->per_clear |= PER_CLEAR_ON_SETID;
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index c809e28..ff6be04 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -36,6 +36,8 @@ struct linux_binprm{
 	struct mm_struct *mm;
 	unsigned long p; /* current top of mem */
 	unsigned int
+		file_nosuid:1,  /* true if file's security state should
+				   be ignored. */
 		cred_prepared:1,/* true if creds already prepared (multiple
 				 * preps happen for interpreters) */
 		cap_effective:1;/* true if has elevated effective capabilities,
diff --git a/security/commoncap.c b/security/commoncap.c
index 6166973..4836913 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -390,7 +390,7 @@ static int get_file_caps(struct linux_binprm
*bprm, bool *effective)
 	if (!file_caps_enabled)
 		return 0;

-	if (bprm->file->f_vfsmnt->mnt_flags & MNT_NOSUID)
+	if (bprm->file_nosuid)
 		return 0;

 	dentry = dget(bprm->file->f_dentry);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5feecb4..762ac00 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2100,7 +2100,7 @@ static int selinux_bprm_set_creds(struct
linux_binprm *bprm)
 {
 	const struct task_security_struct *old_tsec;
 	struct task_security_struct *new_tsec;
-	struct inode_security_struct *isec;
+	u32 isid;
 	struct common_audit_data ad;
 	struct inode *inode = bprm->file->f_path.dentry->d_inode;
 	int rc;
@@ -2116,7 +2116,9 @@ static int selinux_bprm_set_creds(struct
linux_binprm *bprm)

 	old_tsec = current_security();
 	new_tsec = bprm->cred->security;
-	isec = inode->i_security;
+	isid = ((struct inode_security_struct*)inode->i_security)->sid;
+	if (bprm->file_nosuid)
+		isid = SECINITSID_UNLABELED;

 	/* Default to the current task SID. */
 	new_tsec->sid = old_tsec->sid;
@@ -2133,7 +2135,7 @@ static int selinux_bprm_set_creds(struct
linux_binprm *bprm)
 		new_tsec->exec_sid = 0;
 	} else {
 		/* Check for a default transition on this program. */
-		rc = security_transition_sid(old_tsec->sid, isec->sid,
+		rc = security_transition_sid(old_tsec->sid, isid,
 					     SECCLASS_PROCESS, &new_tsec->sid);
 		if (rc)
 			return rc;
@@ -2142,11 +2144,8 @@ static int selinux_bprm_set_creds(struct
linux_binprm *bprm)
 	COMMON_AUDIT_DATA_INIT(&ad, FS);
 	ad.u.fs.path = bprm->file->f_path;

-	if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
-		new_tsec->sid = old_tsec->sid;
-
 	if (new_tsec->sid == old_tsec->sid) {
-		rc = avc_has_perm(old_tsec->sid, isec->sid,
+		rc = avc_has_perm(old_tsec->sid, isid,
 				  SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, &ad);
 		if (rc)
 			return rc;
@@ -2157,7 +2156,7 @@ static int selinux_bprm_set_creds(struct
linux_binprm *bprm)
 		if (rc)
 			return rc;

-		rc = avc_has_perm(new_tsec->sid, isec->sid,
+		rc = avc_has_perm(new_tsec->sid, isid,
 				  SECCLASS_FILE, FILE__ENTRYPOINT, &ad);
 		if (rc)
 			return rc;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/