On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
> Commit-ID: bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> Gitweb: http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> Author: Kees Cook <[email protected]>
> AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
> Committer: Thomas Gleixner <[email protected]>
> CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
>
> futex: Do not leak robust list to unprivileged process
>
> It was possible to extract the robust list head address from a setuid
> process if it had used set_robust_list(), allowing an ASLR info leak. This
> changes the permission checks to be the same as those used for similar
> info that comes out of /proc.
>
> Running a setuid program that uses robust futexes would have had:
> cred->euid != pcred->euid
> cred->euid == pcred->uid
> so the old permissions check would allow it. I'm not aware of any setuid
> programs that use robust futexes, so this is just a preventative measure.
>
I'm not sure this change prevents the unprivileged process.
Please refer to LTP test, recently I saw that this change broke
the following test.
https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
if (seteuid(1) == -1)
tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
TEST(retval = syscall(__NR_get_robust_list, 1,
(struct robust_list_head *)&head,
&len_ptr));
We set the euid to an unprivileged user, and expect to FAIL with EPERM,
without this patch, it FAIL as we expected, but with it, this call succeed.
Seems that we leaked the check of (cred->euid == pcred->euid && cred->euid == pcred->uid),
I'm not sure which one is right, can you please give an explanation?
Thanks in advance,
Wanlong Gao
> (This patch is based on changes from grsecurity.)
>
> Signed-off-by: Kees Cook <[email protected]>
> Cc: Darren Hart <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Jiri Kosina <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Serge E. Hallyn <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> kernel/futex.c | 36 +++++++++++++-----------------------
> kernel/futex_compat.c | 36 +++++++++++++-----------------------
> 2 files changed, 26 insertions(+), 46 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 72efa1e..d701be5 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -59,6 +59,7 @@
> #include <linux/magic.h>
> #include <linux/pid.h>
> #include <linux/nsproxy.h>
> +#include <linux/ptrace.h>
>
> #include <asm/futex.h>
>
> @@ -2443,40 +2444,29 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
> {
> struct robust_list_head __user *head;
> unsigned long ret;
> - const struct cred *cred = current_cred(), *pcred;
> + struct task_struct *p;
>
> if (!futex_cmpxchg_enabled)
> return -ENOSYS;
>
> + rcu_read_lock();
> +
> + ret = -ESRCH;
> if (!pid)
> - head = current->robust_list;
> + p = current;
> else {
> - struct task_struct *p;
> -
> - ret = -ESRCH;
> - rcu_read_lock();
> p = find_task_by_vpid(pid);
> if (!p)
> goto err_unlock;
> - ret = -EPERM;
> - pcred = __task_cred(p);
> - /* If victim is in different user_ns, then uids are not
> - comparable, so we must have CAP_SYS_PTRACE */
> - if (cred->user->user_ns != pcred->user->user_ns) {
> - if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> - goto err_unlock;
> - goto ok;
> - }
> - /* If victim is in same user_ns, then uids are comparable */
> - if (cred->euid != pcred->euid &&
> - cred->euid != pcred->uid &&
> - !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> - goto err_unlock;
> -ok:
> - head = p->robust_list;
> - rcu_read_unlock();
> }
>
> + ret = -EPERM;
> + if (!ptrace_may_access(p, PTRACE_MODE_READ))
> + goto err_unlock;
> +
> + head = p->robust_list;
> + rcu_read_unlock();
> +
> if (put_user(sizeof(*head), len_ptr))
> return -EFAULT;
> return put_user(head, head_ptr);
> diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
> index 5f9e689..a9642d5 100644
> --- a/kernel/futex_compat.c
> +++ b/kernel/futex_compat.c
> @@ -10,6 +10,7 @@
> #include <linux/compat.h>
> #include <linux/nsproxy.h>
> #include <linux/futex.h>
> +#include <linux/ptrace.h>
>
> #include <asm/uaccess.h>
>
> @@ -136,40 +137,29 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
> {
> struct compat_robust_list_head __user *head;
> unsigned long ret;
> - const struct cred *cred = current_cred(), *pcred;
> + struct task_struct *p;
>
> if (!futex_cmpxchg_enabled)
> return -ENOSYS;
>
> + rcu_read_lock();
> +
> + ret = -ESRCH;
> if (!pid)
> - head = current->compat_robust_list;
> + p = current;
> else {
> - struct task_struct *p;
> -
> - ret = -ESRCH;
> - rcu_read_lock();
> p = find_task_by_vpid(pid);
> if (!p)
> goto err_unlock;
> - ret = -EPERM;
> - pcred = __task_cred(p);
> - /* If victim is in different user_ns, then uids are not
> - comparable, so we must have CAP_SYS_PTRACE */
> - if (cred->user->user_ns != pcred->user->user_ns) {
> - if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> - goto err_unlock;
> - goto ok;
> - }
> - /* If victim is in same user_ns, then uids are comparable */
> - if (cred->euid != pcred->euid &&
> - cred->euid != pcred->uid &&
> - !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> - goto err_unlock;
> -ok:
> - head = p->compat_robust_list;
> - rcu_read_unlock();
> }
>
> + ret = -EPERM;
> + if (!ptrace_may_access(p, PTRACE_MODE_READ))
> + goto err_unlock;
> +
> + head = p->compat_robust_list;
> + rcu_read_unlock();
> +
> if (put_user(sizeof(*head), len_ptr))
> return -EFAULT;
> return put_user(ptr_to_compat(head), head_ptr);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Quoting Wanlong Gao ([email protected]):
> On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
> > Commit-ID: bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> > Gitweb: http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> > Author: Kees Cook <[email protected]>
> > AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
> > Committer: Thomas Gleixner <[email protected]>
> > CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
> >
> > futex: Do not leak robust list to unprivileged process
> >
> > It was possible to extract the robust list head address from a setuid
> > process if it had used set_robust_list(), allowing an ASLR info leak. This
> > changes the permission checks to be the same as those used for similar
> > info that comes out of /proc.
> >
> > Running a setuid program that uses robust futexes would have had:
> > cred->euid != pcred->euid
> > cred->euid == pcred->uid
> > so the old permissions check would allow it. I'm not aware of any setuid
> > programs that use robust futexes, so this is just a preventative measure.
> >
>
> I'm not sure this change prevents the unprivileged process.
> Please refer to LTP test, recently I saw that this change broke
> the following test.
>
> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
> if (seteuid(1) == -1)
> tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
>
> TEST(retval = syscall(__NR_get_robust_list, 1,
> (struct robust_list_head *)&head,
> &len_ptr));
>
> We set the euid to an unprivileged user, and expect to FAIL with EPERM,
> without this patch, it FAIL as we expected, but with it, this call succeed.
This relates to a question I asked - I believe in this thread, maybe in
another thread - about ptrace_may_access. That code goes back further than
our git history, and for so long has used current->uid and ->gid, not
euid and gid, for permission checks. I asked if that's what we really
want, but at the same am not sure we want to change something that's
been like that for so long.
But that's why it succeeded - you changed your euid, not your uid.
> Seems that we leaked the check of (cred->euid == pcred->euid && cred->euid == pcred->uid),
> I'm not sure which one is right, can you please give an explanation?
>
>
> Thanks in advance,
> Wanlong Gao
>
> > (This patch is based on changes from grsecurity.)
> >
> > Signed-off-by: Kees Cook <[email protected]>
> > Cc: Darren Hart <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Jiri Kosina <[email protected]>
> > Cc: Eric W. Biederman <[email protected]>
> > Cc: David Howells <[email protected]>
> > Cc: Serge E. Hallyn <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Link: http://lkml.kernel.org/r/[email protected]
> > Signed-off-by: Thomas Gleixner <[email protected]>
> > ---
> > kernel/futex.c | 36 +++++++++++++-----------------------
> > kernel/futex_compat.c | 36 +++++++++++++-----------------------
> > 2 files changed, 26 insertions(+), 46 deletions(-)
> >
> > diff --git a/kernel/futex.c b/kernel/futex.c
> > index 72efa1e..d701be5 100644
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -59,6 +59,7 @@
> > #include <linux/magic.h>
> > #include <linux/pid.h>
> > #include <linux/nsproxy.h>
> > +#include <linux/ptrace.h>
> >
> > #include <asm/futex.h>
> >
> > @@ -2443,40 +2444,29 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
> > {
> > struct robust_list_head __user *head;
> > unsigned long ret;
> > - const struct cred *cred = current_cred(), *pcred;
> > + struct task_struct *p;
> >
> > if (!futex_cmpxchg_enabled)
> > return -ENOSYS;
> >
> > + rcu_read_lock();
> > +
> > + ret = -ESRCH;
> > if (!pid)
> > - head = current->robust_list;
> > + p = current;
> > else {
> > - struct task_struct *p;
> > -
> > - ret = -ESRCH;
> > - rcu_read_lock();
> > p = find_task_by_vpid(pid);
> > if (!p)
> > goto err_unlock;
> > - ret = -EPERM;
> > - pcred = __task_cred(p);
> > - /* If victim is in different user_ns, then uids are not
> > - comparable, so we must have CAP_SYS_PTRACE */
> > - if (cred->user->user_ns != pcred->user->user_ns) {
> > - if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> > - goto err_unlock;
> > - goto ok;
> > - }
> > - /* If victim is in same user_ns, then uids are comparable */
> > - if (cred->euid != pcred->euid &&
> > - cred->euid != pcred->uid &&
> > - !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> > - goto err_unlock;
> > -ok:
> > - head = p->robust_list;
> > - rcu_read_unlock();
> > }
> >
> > + ret = -EPERM;
> > + if (!ptrace_may_access(p, PTRACE_MODE_READ))
> > + goto err_unlock;
> > +
> > + head = p->robust_list;
> > + rcu_read_unlock();
> > +
> > if (put_user(sizeof(*head), len_ptr))
> > return -EFAULT;
> > return put_user(head, head_ptr);
> > diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
> > index 5f9e689..a9642d5 100644
> > --- a/kernel/futex_compat.c
> > +++ b/kernel/futex_compat.c
> > @@ -10,6 +10,7 @@
> > #include <linux/compat.h>
> > #include <linux/nsproxy.h>
> > #include <linux/futex.h>
> > +#include <linux/ptrace.h>
> >
> > #include <asm/uaccess.h>
> >
> > @@ -136,40 +137,29 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
> > {
> > struct compat_robust_list_head __user *head;
> > unsigned long ret;
> > - const struct cred *cred = current_cred(), *pcred;
> > + struct task_struct *p;
> >
> > if (!futex_cmpxchg_enabled)
> > return -ENOSYS;
> >
> > + rcu_read_lock();
> > +
> > + ret = -ESRCH;
> > if (!pid)
> > - head = current->compat_robust_list;
> > + p = current;
> > else {
> > - struct task_struct *p;
> > -
> > - ret = -ESRCH;
> > - rcu_read_lock();
> > p = find_task_by_vpid(pid);
> > if (!p)
> > goto err_unlock;
> > - ret = -EPERM;
> > - pcred = __task_cred(p);
> > - /* If victim is in different user_ns, then uids are not
> > - comparable, so we must have CAP_SYS_PTRACE */
> > - if (cred->user->user_ns != pcred->user->user_ns) {
> > - if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> > - goto err_unlock;
> > - goto ok;
> > - }
> > - /* If victim is in same user_ns, then uids are comparable */
> > - if (cred->euid != pcred->euid &&
> > - cred->euid != pcred->uid &&
> > - !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
> > - goto err_unlock;
> > -ok:
> > - head = p->compat_robust_list;
> > - rcu_read_unlock();
> > }
> >
> > + ret = -EPERM;
> > + if (!ptrace_may_access(p, PTRACE_MODE_READ))
> > + goto err_unlock;
> > +
> > + head = p->compat_robust_list;
> > + rcu_read_unlock();
> > +
> > if (put_user(sizeof(*head), len_ptr))
> > return -EFAULT;
> > return put_user(ptr_to_compat(head), head_ptr);
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
>
>
On 06/19/2012 10:24 AM, Serge Hallyn wrote:
> Quoting Wanlong Gao ([email protected]):
>> On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
>>> Commit-ID: bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
>>> Gitweb: http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
>>> Author: Kees Cook <[email protected]>
>>> AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
>>> Committer: Thomas Gleixner <[email protected]>
>>> CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
>>>
>>> futex: Do not leak robust list to unprivileged process
>>>
>>> It was possible to extract the robust list head address from a setuid
>>> process if it had used set_robust_list(), allowing an ASLR info leak. This
>>> changes the permission checks to be the same as those used for similar
>>> info that comes out of /proc.
>>>
>>> Running a setuid program that uses robust futexes would have had:
>>> cred->euid != pcred->euid
>>> cred->euid == pcred->uid
>>> so the old permissions check would allow it. I'm not aware of any setuid
>>> programs that use robust futexes, so this is just a preventative measure.
>>>
>>
>> I'm not sure this change prevents the unprivileged process.
>> Please refer to LTP test, recently I saw that this change broke
>> the following test.
>>
>> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
>> if (seteuid(1) == -1)
>> tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
>>
>> TEST(retval = syscall(__NR_get_robust_list, 1,
>> (struct robust_list_head *)&head,
>> &len_ptr));
>>
>> We set the euid to an unprivileged user, and expect to FAIL with EPERM,
>> without this patch, it FAIL as we expected, but with it, this call succeed.
>
> This relates to a question I asked - I believe in this thread, maybe in
> another thread - about ptrace_may_access. That code goes back further than
> our git history, and for so long has used current->uid and ->gid, not
> euid and gid, for permission checks. I asked if that's what we really
> want, but at the same am not sure we want to change something that's
> been like that for so long.
>
> But that's why it succeeded - you changed your euid, not your uid.
Yeah, I known what I'm doing. I just wonder which is the right thing.
Should we check euid or uid ? You mean that checking uid instead of
checking euid for a long time, right?
Thanks,
Wanlong Gao
>
>> Seems that we leaked the check of (cred->euid == pcred->euid && cred->euid == pcred->uid),
>> I'm not sure which one is right, can you please give an explanation?
>>
>>
>> Thanks in advance,
>> Wanlong Gao
>>
>>> (This patch is based on changes from grsecurity.)
>>>
>>> Signed-off-by: Kees Cook <[email protected]>
>>> Cc: Darren Hart <[email protected]>
>>> Cc: Peter Zijlstra <[email protected]>
>>> Cc: Jiri Kosina <[email protected]>
>>> Cc: Eric W. Biederman <[email protected]>
>>> Cc: David Howells <[email protected]>
>>> Cc: Serge E. Hallyn <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Link: http://lkml.kernel.org/r/[email protected]
>>> Signed-off-by: Thomas Gleixner <[email protected]>
>>> ---
>>> kernel/futex.c | 36 +++++++++++++-----------------------
>>> kernel/futex_compat.c | 36 +++++++++++++-----------------------
>>> 2 files changed, 26 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/kernel/futex.c b/kernel/futex.c
>>> index 72efa1e..d701be5 100644
>>> --- a/kernel/futex.c
>>> +++ b/kernel/futex.c
>>> @@ -59,6 +59,7 @@
>>> #include <linux/magic.h>
>>> #include <linux/pid.h>
>>> #include <linux/nsproxy.h>
>>> +#include <linux/ptrace.h>
>>>
>>> #include <asm/futex.h>
>>>
>>> @@ -2443,40 +2444,29 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
>>> {
>>> struct robust_list_head __user *head;
>>> unsigned long ret;
>>> - const struct cred *cred = current_cred(), *pcred;
>>> + struct task_struct *p;
>>>
>>> if (!futex_cmpxchg_enabled)
>>> return -ENOSYS;
>>>
>>> + rcu_read_lock();
>>> +
>>> + ret = -ESRCH;
>>> if (!pid)
>>> - head = current->robust_list;
>>> + p = current;
>>> else {
>>> - struct task_struct *p;
>>> -
>>> - ret = -ESRCH;
>>> - rcu_read_lock();
>>> p = find_task_by_vpid(pid);
>>> if (!p)
>>> goto err_unlock;
>>> - ret = -EPERM;
>>> - pcred = __task_cred(p);
>>> - /* If victim is in different user_ns, then uids are not
>>> - comparable, so we must have CAP_SYS_PTRACE */
>>> - if (cred->user->user_ns != pcred->user->user_ns) {
>>> - if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
>>> - goto err_unlock;
>>> - goto ok;
>>> - }
>>> - /* If victim is in same user_ns, then uids are comparable */
>>> - if (cred->euid != pcred->euid &&
>>> - cred->euid != pcred->uid &&
>>> - !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
>>> - goto err_unlock;
>>> -ok:
>>> - head = p->robust_list;
>>> - rcu_read_unlock();
>>> }
>>>
>>> + ret = -EPERM;
>>> + if (!ptrace_may_access(p, PTRACE_MODE_READ))
>>> + goto err_unlock;
>>> +
>>> + head = p->robust_list;
>>> + rcu_read_unlock();
>>> +
>>> if (put_user(sizeof(*head), len_ptr))
>>> return -EFAULT;
>>> return put_user(head, head_ptr);
>>> diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
>>> index 5f9e689..a9642d5 100644
>>> --- a/kernel/futex_compat.c
>>> +++ b/kernel/futex_compat.c
>>> @@ -10,6 +10,7 @@
>>> #include <linux/compat.h>
>>> #include <linux/nsproxy.h>
>>> #include <linux/futex.h>
>>> +#include <linux/ptrace.h>
>>>
>>> #include <asm/uaccess.h>
>>>
>>> @@ -136,40 +137,29 @@ compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
>>> {
>>> struct compat_robust_list_head __user *head;
>>> unsigned long ret;
>>> - const struct cred *cred = current_cred(), *pcred;
>>> + struct task_struct *p;
>>>
>>> if (!futex_cmpxchg_enabled)
>>> return -ENOSYS;
>>>
>>> + rcu_read_lock();
>>> +
>>> + ret = -ESRCH;
>>> if (!pid)
>>> - head = current->compat_robust_list;
>>> + p = current;
>>> else {
>>> - struct task_struct *p;
>>> -
>>> - ret = -ESRCH;
>>> - rcu_read_lock();
>>> p = find_task_by_vpid(pid);
>>> if (!p)
>>> goto err_unlock;
>>> - ret = -EPERM;
>>> - pcred = __task_cred(p);
>>> - /* If victim is in different user_ns, then uids are not
>>> - comparable, so we must have CAP_SYS_PTRACE */
>>> - if (cred->user->user_ns != pcred->user->user_ns) {
>>> - if (!ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
>>> - goto err_unlock;
>>> - goto ok;
>>> - }
>>> - /* If victim is in same user_ns, then uids are comparable */
>>> - if (cred->euid != pcred->euid &&
>>> - cred->euid != pcred->uid &&
>>> - !ns_capable(pcred->user->user_ns, CAP_SYS_PTRACE))
>>> - goto err_unlock;
>>> -ok:
>>> - head = p->compat_robust_list;
>>> - rcu_read_unlock();
>>> }
>>>
>>> + ret = -EPERM;
>>> + if (!ptrace_may_access(p, PTRACE_MODE_READ))
>>> + goto err_unlock;
>>> +
>>> + head = p->compat_robust_list;
>>> + rcu_read_unlock();
>>> +
>>> if (put_user(sizeof(*head), len_ptr))
>>> return -EFAULT;
>>> return put_user(ptr_to_compat(head), head_ptr);
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>>
>>
>>
>
Quoting Wanlong Gao ([email protected]):
> On 06/19/2012 10:24 AM, Serge Hallyn wrote:
> > Quoting Wanlong Gao ([email protected]):
> >> On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
> >>> Commit-ID: bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> >>> Gitweb: http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> >>> Author: Kees Cook <[email protected]>
> >>> AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
> >>> Committer: Thomas Gleixner <[email protected]>
> >>> CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
> >>>
> >>> futex: Do not leak robust list to unprivileged process
> >>>
> >>> It was possible to extract the robust list head address from a setuid
> >>> process if it had used set_robust_list(), allowing an ASLR info leak. This
> >>> changes the permission checks to be the same as those used for similar
> >>> info that comes out of /proc.
> >>>
> >>> Running a setuid program that uses robust futexes would have had:
> >>> cred->euid != pcred->euid
> >>> cred->euid == pcred->uid
> >>> so the old permissions check would allow it. I'm not aware of any setuid
> >>> programs that use robust futexes, so this is just a preventative measure.
> >>>
> >>
> >> I'm not sure this change prevents the unprivileged process.
> >> Please refer to LTP test, recently I saw that this change broke
> >> the following test.
> >>
> >> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
> >> if (seteuid(1) == -1)
> >> tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
> >>
> >> TEST(retval = syscall(__NR_get_robust_list, 1,
> >> (struct robust_list_head *)&head,
> >> &len_ptr));
> >>
> >> We set the euid to an unprivileged user, and expect to FAIL with EPERM,
> >> without this patch, it FAIL as we expected, but with it, this call succeed.
> >
> > This relates to a question I asked - I believe in this thread, maybe in
> > another thread - about ptrace_may_access. That code goes back further than
> > our git history, and for so long has used current->uid and ->gid, not
> > euid and gid, for permission checks. I asked if that's what we really
> > want, but at the same am not sure we want to change something that's
> > been like that for so long.
> >
> > But that's why it succeeded - you changed your euid, not your uid.
>
> Yeah, I known what I'm doing.
Didn't mean to offend :)
> I just wonder which is the right thing.
> Should we check euid or uid ? You mean that checking uid instead of
> checking euid for a long time, right?
Yup, and I agree it seems wrong.
-serge
On 06/19/2012 11:13 AM, Serge Hallyn wrote:
> Quoting Wanlong Gao ([email protected]):
>> On 06/19/2012 10:24 AM, Serge Hallyn wrote:
>>> Quoting Wanlong Gao ([email protected]):
>>>> On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
>>>>> Commit-ID: bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
>>>>> Gitweb: http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
>>>>> Author: Kees Cook <[email protected]>
>>>>> AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
>>>>> Committer: Thomas Gleixner <[email protected]>
>>>>> CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
>>>>>
>>>>> futex: Do not leak robust list to unprivileged process
>>>>>
>>>>> It was possible to extract the robust list head address from a setuid
>>>>> process if it had used set_robust_list(), allowing an ASLR info leak. This
>>>>> changes the permission checks to be the same as those used for similar
>>>>> info that comes out of /proc.
>>>>>
>>>>> Running a setuid program that uses robust futexes would have had:
>>>>> cred->euid != pcred->euid
>>>>> cred->euid == pcred->uid
>>>>> so the old permissions check would allow it. I'm not aware of any setuid
>>>>> programs that use robust futexes, so this is just a preventative measure.
>>>>>
>>>>
>>>> I'm not sure this change prevents the unprivileged process.
>>>> Please refer to LTP test, recently I saw that this change broke
>>>> the following test.
>>>>
>>>> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
>>>> if (seteuid(1) == -1)
>>>> tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
>>>>
>>>> TEST(retval = syscall(__NR_get_robust_list, 1,
>>>> (struct robust_list_head *)&head,
>>>> &len_ptr));
>>>>
>>>> We set the euid to an unprivileged user, and expect to FAIL with EPERM,
>>>> without this patch, it FAIL as we expected, but with it, this call succeed.
>>>
>>> This relates to a question I asked - I believe in this thread, maybe in
>>> another thread - about ptrace_may_access. That code goes back further than
>>> our git history, and for so long has used current->uid and ->gid, not
>>> euid and gid, for permission checks. I asked if that's what we really
>>> want, but at the same am not sure we want to change something that's
>>> been like that for so long.
>>>
>>> But that's why it succeeded - you changed your euid, not your uid.
>>
>> Yeah, I known what I'm doing.
>
> Didn't mean to offend :)
Sorry for my poor words, I didn't mean that, either. ;)
>
>> I just wonder which is the right thing.
>> Should we check euid or uid ? You mean that checking uid instead of
>> checking euid for a long time, right?
>
> Yup, and I agree it seems wrong.
Are there any other places where also switch checking uid instead of euid ?
In this place, anyway, this syscall is already marked as deprecated.
Thanks,
Wanlong Gao
>
> -serge
>
Quoting Wanlong Gao ([email protected]):
> On 06/19/2012 11:13 AM, Serge Hallyn wrote:
> > Quoting Wanlong Gao ([email protected]):
> >> On 06/19/2012 10:24 AM, Serge Hallyn wrote:
> >>> Quoting Wanlong Gao ([email protected]):
> >>>> On 03/29/2012 05:55 PM, tip-bot for Kees Cook wrote:
> >>>>> Commit-ID: bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> >>>>> Gitweb: http://git.kernel.org/tip/bdbb776f882f5ad431aa1e694c69c1c3d6a4a5b8
> >>>>> Author: Kees Cook <[email protected]>
> >>>>> AuthorDate: Mon, 19 Mar 2012 16:12:53 -0700
> >>>>> Committer: Thomas Gleixner <[email protected]>
> >>>>> CommitDate: Thu, 29 Mar 2012 11:37:17 +0200
> >>>>>
> >>>>> futex: Do not leak robust list to unprivileged process
> >>>>>
> >>>>> It was possible to extract the robust list head address from a setuid
> >>>>> process if it had used set_robust_list(), allowing an ASLR info leak. This
> >>>>> changes the permission checks to be the same as those used for similar
> >>>>> info that comes out of /proc.
> >>>>>
> >>>>> Running a setuid program that uses robust futexes would have had:
> >>>>> cred->euid != pcred->euid
> >>>>> cred->euid == pcred->uid
> >>>>> so the old permissions check would allow it. I'm not aware of any setuid
> >>>>> programs that use robust futexes, so this is just a preventative measure.
> >>>>>
> >>>>
> >>>> I'm not sure this change prevents the unprivileged process.
> >>>> Please refer to LTP test, recently I saw that this change broke
> >>>> the following test.
> >>>>
> >>>> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/get_robust_list/get_robust_list01.c#L155
> >>>> if (seteuid(1) == -1)
> >>>> tst_brkm(TBROK|TERRNO, cleanup, "seteuid(1) failed");
> >>>>
> >>>> TEST(retval = syscall(__NR_get_robust_list, 1,
> >>>> (struct robust_list_head *)&head,
> >>>> &len_ptr));
> >>>>
> >>>> We set the euid to an unprivileged user, and expect to FAIL with EPERM,
> >>>> without this patch, it FAIL as we expected, but with it, this call succeed.
> >>>
> >>> This relates to a question I asked - I believe in this thread, maybe in
> >>> another thread - about ptrace_may_access. That code goes back further than
> >>> our git history, and for so long has used current->uid and ->gid, not
> >>> euid and gid, for permission checks. I asked if that's what we really
> >>> want, but at the same am not sure we want to change something that's
> >>> been like that for so long.
> >>>
> >>> But that's why it succeeded - you changed your euid, not your uid.
> >>
> >> Yeah, I known what I'm doing.
> >
> > Didn't mean to offend :)
>
> Sorry for my poor words, I didn't mean that, either. ;)
>
> >
> >> I just wonder which is the right thing.
> >> Should we check euid or uid ? You mean that checking uid instead of
> >> checking euid for a long time, right?
> >
> > Yup, and I agree it seems wrong.
>
> Are there any other places where also switch checking uid instead of euid ?
> In this place, anyway, this syscall is already marked as deprecated.
This isn't just this syscall, though, it's ptrace_may_access() which is
used in quite a few places (20 at quick glance).
-serge