Unprivileged: Disable raising of privileges
in [Kernel]
|
Prev: BFS v0.313 CPU scheduler for 2.6.32
Next: ACPI warning from alloc_pages_nodemask on boot (2.6.33 regression)
From: Eric W. Biederman on 31 Dec 2009 04:00 Alan Cox <alan(a)lxorguk.ukuu.org.uk> writes: > On Wed, 30 Dec 2009 13:36:57 -0800 > ebiederm(a)xmission.com (Eric W. Biederman) wrote: > >> Alan Cox <alan(a)lxorguk.ukuu.org.uk> writes: >> >> >> Added bprm->nosuid to make remove the need to add >> >> duplicate error prone checks. This ensures that >> >> the disabling of suid executables is exactly the >> >> same as MNT_NOSUID. >> > >> > Another fine example of why we have security hooks so that we don't get a >> > kernel full of other "random security idea of the day" hacks. >> >> Well it comes from plan 9. Except there they just simply did not >> implement suid. What causes you to think dropping the ability >> to execute suid executables is a random security idea of the day? > > Well to be fair its random regurgitated security idea of every year or > two. > > More to the point - we have security_* hooks so this kind of continuous > security proposal turdstream can stay out of the main part of the kernel. > > Cleaning up the mechanism by which NOSUID is handled in kernel seems a > good idea. Adding wacky new prctls and gunk for it doesn't, and belongs > in whatever security model you are using via the security hooks. I am more than happy to make this a proper system call, instead of a prctl. The way this code is evolving that seems to be the clean way to handle this. No point in hiding the functionality away in a corner in shame. In my book SUID applications are the root of all evil. They are exploitable if you twist their environment in a way they have not hardened themselves against, and simply supporting them prevents a lot of good features from being used by ordinary applications. To get SUID out of my way I have to do something. A disable SUID from this process and it's children is a simple and direct way there. My other path is much more complicated. As this also has security related uses it seems even better as a feature. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Serge E. Hallyn on 31 Dec 2009 10:30 Quoting Eric W. Biederman (ebiederm(a)xmission.com): > > If we can know that a process will never raise > it's priveleges we can enable a lot of features > without privilege (such as unsharing namespaces > and unprivileged mounts) that otherwise would be unsafe, > because they could break assumptions of existing > suid executables. > > To allow this to be used as a sand boxing feature > also disable ptracing other executables without > this new restriction. > > For the moment I have used a per thread flag because > we are out of per process flags. > > To ensure all descendants get this flag I rely on > the default copying of procss structures. > > Added bprm->nosuid to make remove the need to add > duplicate error prone checks. This ensures that > the disabling of suid executables is exactly the > same as MNT_NOSUID. > > Signed-off-by: Eric W. Biederman <ebiederm(a)xmission.com> > --- > arch/x86/include/asm/thread_info.h | 2 ++ > fs/exec.c | 6 ++++-- > include/linux/binfmts.h | 1 + > include/linux/prctl.h | 3 +++ > kernel/ptrace.c | 4 ++++ > kernel/sys.c | 21 +++++++++++++++++++++ > security/commoncap.c | 3 +-- > security/selinux/hooks.c | 2 +- > 8 files changed, 37 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h > index 375c917..e716203 100644 > --- a/arch/x86/include/asm/thread_info.h > +++ b/arch/x86/include/asm/thread_info.h > @@ -82,6 +82,7 @@ struct thread_info { > #define TIF_SYSCALL_EMU 6 /* syscall emulation active */ > #define TIF_SYSCALL_AUDIT 7 /* syscall auditing active */ > #define TIF_SECCOMP 8 /* secure computing */ > +#define TIF_NOSUID 9 /* suid exec permanently disabled */ > #define TIF_MCE_NOTIFY 10 /* notify userspace of an MCE */ > #define TIF_USER_RETURN_NOTIFY 11 /* notify kernel of userspace return */ > #define TIF_NOTSC 16 /* TSC is not accessible in userland */ > @@ -107,6 +108,7 @@ struct thread_info { > #define _TIF_SYSCALL_EMU (1 << TIF_SYSCALL_EMU) > #define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT) > #define _TIF_SECCOMP (1 << TIF_SECCOMP) > +#define _TIF_NOSUID (1 << TIF_NOSUID) > #define _TIF_MCE_NOTIFY (1 << TIF_MCE_NOTIFY) > #define _TIF_USER_RETURN_NOTIFY (1 << TIF_USER_RETURN_NOTIFY) > #define _TIF_NOTSC (1 << TIF_NOTSC) > diff --git a/fs/exec.c b/fs/exec.c > index 632b02e..5cba5ac 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1131,8 +1131,10 @@ int prepare_binprm(struct linux_binprm *bprm) > /* 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)) { > + bprm->nosuid = > + (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) || > + test_tsk_thread_flag(current, TIF_NOSUID); > + if (bprm->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 cd4349b..c3b5a30 100644 > --- a/include/linux/binfmts.h > +++ b/include/linux/binfmts.h > @@ -44,6 +44,7 @@ struct linux_binprm{ > #ifdef __alpha__ > unsigned int taso:1; > #endif > + unsigned int nosuid:1; /* True if suid bits are ignored */ > unsigned int recursion_depth; > struct file * file; > struct cred *cred; /* new credentials */ > diff --git a/include/linux/prctl.h b/include/linux/prctl.h > index a3baeb2..8adc517 100644 > --- a/include/linux/prctl.h > +++ b/include/linux/prctl.h > @@ -102,4 +102,7 @@ > > #define PR_MCE_KILL_GET 34 > > +#define PR_SET_NOSUID 35 > +#define PR_GET_NOSUID 36 > + > #endif /* _LINUX_PRCTL_H */ > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > index 23bd09c..b91040c 100644 > --- a/kernel/ptrace.c > +++ b/kernel/ptrace.c > @@ -152,6 +152,10 @@ int __ptrace_may_access(struct task_struct *task, unsigned int mode) > if (!dumpable && !capable(CAP_SYS_PTRACE)) > return -EPERM; > > + if (test_tsk_thread_flag(current, TIF_NOSUID) && > + !test_tsk_thread_flag(task, TIF_NOSUID)) > + return -EPERM; > + > return security_ptrace_access_check(task, mode); > } > > diff --git a/kernel/sys.c b/kernel/sys.c > index 26a6b73..8731f2a 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1578,6 +1578,27 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > else > error = PR_MCE_KILL_DEFAULT; > break; > + case PR_SET_NOSUID: > + { > + const struct cred *cred = current->cred; > + error = -EINVAL; > + /* Don't support cases that could be unsafe */ > + if ( (cred->uid != cred->suid) || > + (cred->uid != cred->euid) || > + (cred->uid != cred->fsuid) || > + (cred->gid != cred->sgid) || > + (cred->gid != cred->egid) || > + (cred->gid != cred->fsgid) || > + !cap_isclear(cred->cap_permitted) || > + (atomic_read(¤t->signal->count) != 1)) > + break; > + error = 0; > + set_tsk_thread_flag(current, TIF_NOSUID); > + break; > + } > + case PR_GET_NOSUID: > + error = !!test_tsk_thread_flag(current, TIF_NOSUID); > + break; > default: > error = -EINVAL; > break; > diff --git a/security/commoncap.c b/security/commoncap.c > index f800fdb..34500e3 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -389,7 +389,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->nosuid) > return 0; I'm sorry, this may actually not be sufficient. Could you try the following test on a kernel with this patch? : 1. become root 2. do prctl(PR_SET_NOSUID); 3. run bash, and examine your capabilities in /proc/self/status I think the code in security/commoncap.c:457-458 will re-raise your capabilities. > > dentry = dget(bprm->file->f_dentry); > @@ -868,7 +868,6 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, > else > new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS); > goto changed; > - > default: > /* No functionality available - continue with default */ > error = -ENOSYS; > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 7a374c2..bd77a2b 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2147,7 +2147,7 @@ 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) > + if (bprm->nosuid) > new_tsec->sid = old_tsec->sid; > > if (new_tsec->sid == old_tsec->sid) { > -- > 1.6.5.2.143.g8cc62 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Eric W. Biederman on 31 Dec 2009 11:50 "Serge E. Hallyn" <serue(a)us.ibm.com> writes: >> diff --git a/security/commoncap.c b/security/commoncap.c >> index f800fdb..34500e3 100644 >> --- a/security/commoncap.c >> +++ b/security/commoncap.c >> @@ -389,7 +389,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->nosuid) >> return 0; > > I'm sorry, this may actually not be sufficient. > > Could you try the following test on a kernel with this patch? : > > 1. become root > 2. do prctl(PR_SET_NOSUID); > 3. run bash, and examine your capabilities in /proc/self/status > > I think the code in security/commoncap.c:457-458 will re-raise your > capabilities. Right. That is a legitimate issue. I almost guard against it with my test against with my start condition test of cap_isclear(cred->cap_permitted). Which causes this to fail for root in most situations. I will add a test for the securebits, and deny this to root unless the securebits are such that root cannot gain privilege. Thanks for catching this. I figured I might need a uid == 0 exclusion. Where the test was split when I wrote it I wasn't certain where to put it. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alan Cox on 31 Dec 2009 12:10 > Lets step back for a moment. What is the common issue with both. > > The issue is simple. "How to I generically tell the secuirty system > want particular restrictions." You don't. It's not "the security system", its a whole collection of completely different models of security and differing tools. > There is no generic LSM API for application or users to talk to the > LSM and say I want the following restricted. That's a meaningless observation I think because security doesn't work that way. Removing specific features from a specific piece of code generally isn't a security feature - its only meaningful in the context of a more general policy and that policy expression isn't generic. > To control the LSM the applications are expected to know what the LSM. > This has caused items like chrome major issues. ... > Application does not need to be informed what is disabled from it. So why does it cause chrome problems ? There are multiple security models because nobody can agree on what they should look like, just like multiple desktops. Each of them is based on a totally different conceptual model so the idea of a single interface to them is a bit meaningless. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Wagner on 31 Dec 2009 13:00
Alan Cox wrote: >Removing specific features from a specific piece of code >generally isn't a security feature - You lost me there. The ability of a specific piece of code to voluntarily relinquish privileges can be a big benefit to security. It enables privilege-separated software architectures, which are a powerful way to reduce risk. That's the motivation for the disablenetwork proposal that has stimulated all this discussion. I hope this is obvious? Does it need elaboration? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |