| Message ID | 87oa1eavfx.fsf_-_@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 17, 2016 at 9:05 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was > overlooked. This can result in incorrect behavior when an application > like strace traces an exec of a setuid executable. > > Further PT_PTRACE_CAP does not have enough information for making good > security decisions as it does not report which user namespace the > capability is in. This has already allowed one mistake through > insufficient granulariy. > > I found this issue when I was testing another corner case of exec and > discovered that I could not get strace to set PT_PTRACE_CAP even when > running strace as root with a full set of caps. > > This change fixes the above issue with strace allowing stracing as > root a setuid executable without disabling setuid. More fundamentaly > this change allows what is allowable at all times, by using the correct > information in it's decision. > > Cc: stable@vger.kernel.org > Fixes: 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12") > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > [...] > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 348f51b0ec92..8fe58255d219 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -1656,6 +1656,7 @@ struct task_struct { > struct list_head cpu_timers[3]; > > /* process credentials */ > + const struct cred __rcu *ptracer_cred; /* Tracer's dredentials at attach */ Typo: credentials. -Kees
On Thu, Nov 17, 2016 at 9:05 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was > overlooked. This can result in incorrect behavior when an application > like strace traces an exec of a setuid executable. > > Further PT_PTRACE_CAP does not have enough information for making good > security decisions as it does not report which user namespace the > capability is in. This has already allowed one mistake through > insufficient granulariy. > > I found this issue when I was testing another corner case of exec and > discovered that I could not get strace to set PT_PTRACE_CAP even when > running strace as root with a full set of caps. > > This change fixes the above issue with strace allowing stracing as > root a setuid executable without disabling setuid. More fundamentaly > this change allows what is allowable at all times, by using the correct > information in it's decision. > > Cc: stable@vger.kernel.org > Fixes: 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12") > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > fs/exec.c | 2 +- > include/linux/capability.h | 1 + > include/linux/ptrace.h | 1 - > include/linux/sched.h | 1 + > kernel/capability.c | 20 ++++++++++++++++++++ > kernel/ptrace.c | 12 +++++++----- > 6 files changed, 30 insertions(+), 7 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 6fcfb3f7b137..fdec760bfac3 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1401,7 +1401,7 @@ static void check_unsafe_exec(struct linux_binprm *bprm) > unsigned n_fs; > > if (p->ptrace) { > - if (p->ptrace & PT_PTRACE_CAP) > + if (ptracer_capable(p, current_user_ns())) IIRC PT_PTRACE_CAP was added to prevent TOCTOU races. What prevents that type of race now? For that matter, what guarantees that we've already switched to new creds here and will continue to do so in the future? --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andy Lutomirski <luto@amacapital.net> writes: > On Thu, Nov 17, 2016 at 9:05 AM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was >> overlooked. This can result in incorrect behavior when an application >> like strace traces an exec of a setuid executable. >> >> Further PT_PTRACE_CAP does not have enough information for making good >> security decisions as it does not report which user namespace the >> capability is in. This has already allowed one mistake through >> insufficient granulariy. >> >> I found this issue when I was testing another corner case of exec and >> discovered that I could not get strace to set PT_PTRACE_CAP even when >> running strace as root with a full set of caps. >> >> This change fixes the above issue with strace allowing stracing as >> root a setuid executable without disabling setuid. More fundamentaly >> this change allows what is allowable at all times, by using the correct >> information in it's decision. >> >> Cc: stable@vger.kernel.org >> Fixes: 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12") >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> --- >> fs/exec.c | 2 +- >> include/linux/capability.h | 1 + >> include/linux/ptrace.h | 1 - >> include/linux/sched.h | 1 + >> kernel/capability.c | 20 ++++++++++++++++++++ >> kernel/ptrace.c | 12 +++++++----- >> 6 files changed, 30 insertions(+), 7 deletions(-) >> >> diff --git a/fs/exec.c b/fs/exec.c >> index 6fcfb3f7b137..fdec760bfac3 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -1401,7 +1401,7 @@ static void check_unsafe_exec(struct linux_binprm *bprm) >> unsigned n_fs; >> >> if (p->ptrace) { >> - if (p->ptrace & PT_PTRACE_CAP) >> + if (ptracer_capable(p, current_user_ns())) > > IIRC PT_PTRACE_CAP was added to prevent TOCTOU races. What prevents > that type of race now? For that matter, what guarantees that we've > already switched to new creds here and will continue to do so in the > future? Because instead of capturing a single bit we now capture tracers entire credentials in tsk->ptracer_cred. As such tsk->ptracer_cred never changes except when ptracing begins or ends, and we remain safe for TOCTOU races. We do hold cred_guard_mutex here so that guarantees we get a new ptracer. So the worst that can happen here is our tracer detaches and ptracer_capable will uncondintionally return true. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Kees Cook <keescook@chromium.org> writes: > On Thu, Nov 17, 2016 at 9:05 AM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was >> overlooked. This can result in incorrect behavior when an application >> like strace traces an exec of a setuid executable. >> >> Further PT_PTRACE_CAP does not have enough information for making good >> security decisions as it does not report which user namespace the >> capability is in. This has already allowed one mistake through >> insufficient granulariy. >> >> I found this issue when I was testing another corner case of exec and >> discovered that I could not get strace to set PT_PTRACE_CAP even when >> running strace as root with a full set of caps. >> >> This change fixes the above issue with strace allowing stracing as >> root a setuid executable without disabling setuid. More fundamentaly >> this change allows what is allowable at all times, by using the correct >> information in it's decision. >> >> Cc: stable@vger.kernel.org >> Fixes: 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12") >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> --- >> [...] >> diff --git a/include/linux/sched.h b/include/linux/sched.h >> index 348f51b0ec92..8fe58255d219 100644 >> --- a/include/linux/sched.h >> +++ b/include/linux/sched.h >> @@ -1656,6 +1656,7 @@ struct task_struct { >> struct list_head cpu_timers[3]; >> >> /* process credentials */ >> + const struct cred __rcu *ptracer_cred; /* Tracer's dredentials at attach */ > > Typo: credentials. Thank you, fixed. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/exec.c b/fs/exec.c index 6fcfb3f7b137..fdec760bfac3 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1401,7 +1401,7 @@ static void check_unsafe_exec(struct linux_binprm *bprm) unsigned n_fs; if (p->ptrace) { - if (p->ptrace & PT_PTRACE_CAP) + if (ptracer_capable(p, current_user_ns())) bprm->unsafe |= LSM_UNSAFE_PTRACE_CAP; else bprm->unsafe |= LSM_UNSAFE_PTRACE; diff --git a/include/linux/capability.h b/include/linux/capability.h index dbc21c719ce6..d6088e2a7668 100644 --- a/include/linux/capability.h +++ b/include/linux/capability.h @@ -242,6 +242,7 @@ static inline bool ns_capable_noaudit(struct user_namespace *ns, int cap) #endif /* CONFIG_MULTIUSER */ extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap); extern bool file_ns_capable(const struct file *file, struct user_namespace *ns, int cap); +extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns); /* audit system wants to get cap info from files as well */ extern int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps); diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h index 504c98a278d4..e13bfdf7f314 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -19,7 +19,6 @@ #define PT_SEIZED 0x00010000 /* SEIZE used, enable new behavior */ #define PT_PTRACED 0x00000001 #define PT_DTRACE 0x00000002 /* delayed trace (used on m68k, i386) */ -#define PT_PTRACE_CAP 0x00000004 /* ptracer can follow suid-exec */ #define PT_OPT_FLAG_SHIFT 3 /* PT_TRACE_* event enable flags */ diff --git a/include/linux/sched.h b/include/linux/sched.h index 348f51b0ec92..8fe58255d219 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1656,6 +1656,7 @@ struct task_struct { struct list_head cpu_timers[3]; /* process credentials */ + const struct cred __rcu *ptracer_cred; /* Tracer's dredentials at attach */ const struct cred __rcu *real_cred; /* objective and real subjective task * credentials (COW) */ const struct cred __rcu *cred; /* effective (overridable) subjective task diff --git a/kernel/capability.c b/kernel/capability.c index 00411c82dac5..dfa0e4528b0b 100644 --- a/kernel/capability.c +++ b/kernel/capability.c @@ -473,3 +473,23 @@ bool capable_wrt_inode_uidgid(const struct inode *inode, int cap) kgid_has_mapping(ns, inode->i_gid); } EXPORT_SYMBOL(capable_wrt_inode_uidgid); + +/** + * ptracer_capable - Determine if the ptracer holds CAP_SYS_PTRACE in the namespace + * @tsk: The task that may be ptraced + * @ns: The user namespace to search for CAP_SYS_PTRACE in + * + * Return true if the task that is ptracing the current task had CAP_SYS_PTRACE + * in the specified user namespace. + */ +bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns) +{ + int ret = 0; /* An absent tracer adds no restrictions */ + const struct cred *cred; + rcu_read_lock(); + cred = rcu_dereference(tsk->ptracer_cred); + if (cred) + ret = security_capable_noaudit(cred, ns, CAP_SYS_PTRACE); + rcu_read_unlock(); + return (ret == 0); +} diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 44a25a1e6e83..982505497680 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -39,6 +39,9 @@ void __ptrace_link(struct task_struct *child, struct task_struct *new_parent) BUG_ON(!list_empty(&child->ptrace_entry)); list_add(&child->ptrace_entry, &new_parent->ptraced); child->parent = new_parent; + rcu_read_lock(); + child->ptracer_cred = get_cred(__task_cred(new_parent)); + rcu_read_unlock(); } /** @@ -71,12 +74,16 @@ void __ptrace_link(struct task_struct *child, struct task_struct *new_parent) */ void __ptrace_unlink(struct task_struct *child) { + const struct cred *old_cred; BUG_ON(!child->ptrace); clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE); child->parent = child->real_parent; list_del_init(&child->ptrace_entry); + old_cred = child->ptracer_cred; + child->ptracer_cred = NULL; + put_cred(old_cred); spin_lock(&child->sighand->siglock); child->ptrace = 0; @@ -326,11 +333,6 @@ static int ptrace_attach(struct task_struct *task, long request, task_lock(task); retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS); - if (!retval) { - struct mm_struct *mm = task->mm; - if (mm && ns_capable(mm->user_ns, CAP_SYS_PTRACE)) - flags |= PT_PTRACE_CAP; - } task_unlock(task); if (retval) goto unlock_creds;
When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was overlooked. This can result in incorrect behavior when an application like strace traces an exec of a setuid executable. Further PT_PTRACE_CAP does not have enough information for making good security decisions as it does not report which user namespace the capability is in. This has already allowed one mistake through insufficient granulariy. I found this issue when I was testing another corner case of exec and discovered that I could not get strace to set PT_PTRACE_CAP even when running strace as root with a full set of caps. This change fixes the above issue with strace allowing stracing as root a setuid executable without disabling setuid. More fundamentaly this change allows what is allowable at all times, by using the correct information in it's decision. Cc: stable@vger.kernel.org Fixes: 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12") Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 2 +- include/linux/capability.h | 1 + include/linux/ptrace.h | 1 - include/linux/sched.h | 1 + kernel/capability.c | 20 ++++++++++++++++++++ kernel/ptrace.c | 12 +++++++----- 6 files changed, 30 insertions(+), 7 deletions(-)