Re: [PATCH] signal: fix overflow_uid signal sender
On 11/02, Jann Horn wrote:
>
> On Wed, Nov 02, 2016 at 07:16:41PM +0100, Oleg Nesterov wrote:
> > On 10/31, Jann Horn wrote:
> > >
> > > static inline void userns_fixup_signal_uid(struct siginfo *info, struct
> > > task_struct *t)
> > > {
> > > - if (current_user_ns() == task_cred_xxx(t, user_ns))
> > > + if (_user_ns == task_cred_xxx(t, user_ns))
> > > return;
> > >
> > > if (SI_FROMKERNEL(info))
> > > @@ -959,7 +959,7 @@ static inline void userns_fixup_signal_uid(struct
> > > siginfo *info, struct task_str
> > >
> > > rcu_read_lock();
> > > info->si_uid = from_kuid_munged(task_cred_xxx(t, user_ns),
> > > - make_kuid(current_user_ns(),
> > > info->si_uid));
> > > + make_kuid(_user_ns, info->si_uid));
> > > rcu_read_unlock();
> > > }
> > > #else
> > > @@ -1027,7 +1027,8 @@ static int __send_signal(int sig, struct siginfo
> > > *info, struct task_struct *t,
> > > q->info.si_code = SI_USER;
> > > q->info.si_pid = task_tgid_nr_ns(current,
> > > task_active_pid_ns(t));
> > > - q->info.si_uid = from_kuid_munged(current_user_ns(),
> > > current_uid());
> > > + q->info.si_uid = from_kuid(_user_ns,
> > > +current_uid());
> >
> > Looks good to me at first glance, but I think this needs an ack from Eric.
> >
> > I have to admit that I forgot how uid_map/etc actually works, I can't
> > even recall if from_kuid(init_user_ns, xxx) == __kuid_val(xxx) or not,
> > although this doesn't really matter.
>
> Yes, from_kuid(_user_ns, xxx) == __kuid_val(xxx). For values from 0
> to 0xfffe, the uid_map of init_user_ns is an identity map (and it
> can't be changed), and for 0x, which isn't mapped, it returns
> 0x to denote failure.
So perhaps we can add another helper to avoid the unnecessary map_id_up()
from_kuid_init_user_ns(kuid_t kuid)
{
return __kuid_val(kuid);
}
But this is a bit off-topic, let me repeat that your patch looks fine to me.
Oleg.
Re: [PATCH] signal: fix overflow_uid signal sender
On 11/02, Jann Horn wrote:
>
> On Wed, Nov 02, 2016 at 07:16:41PM +0100, Oleg Nesterov wrote:
> > On 10/31, Jann Horn wrote:
> > >
> > > static inline void userns_fixup_signal_uid(struct siginfo *info, struct
> > > task_struct *t)
> > > {
> > > - if (current_user_ns() == task_cred_xxx(t, user_ns))
> > > + if (_user_ns == task_cred_xxx(t, user_ns))
> > > return;
> > >
> > > if (SI_FROMKERNEL(info))
> > > @@ -959,7 +959,7 @@ static inline void userns_fixup_signal_uid(struct
> > > siginfo *info, struct task_str
> > >
> > > rcu_read_lock();
> > > info->si_uid = from_kuid_munged(task_cred_xxx(t, user_ns),
> > > - make_kuid(current_user_ns(),
> > > info->si_uid));
> > > + make_kuid(_user_ns, info->si_uid));
> > > rcu_read_unlock();
> > > }
> > > #else
> > > @@ -1027,7 +1027,8 @@ static int __send_signal(int sig, struct siginfo
> > > *info, struct task_struct *t,
> > > q->info.si_code = SI_USER;
> > > q->info.si_pid = task_tgid_nr_ns(current,
> > > task_active_pid_ns(t));
> > > - q->info.si_uid = from_kuid_munged(current_user_ns(),
> > > current_uid());
> > > + q->info.si_uid = from_kuid(_user_ns,
> > > +current_uid());
> >
> > Looks good to me at first glance, but I think this needs an ack from Eric.
> >
> > I have to admit that I forgot how uid_map/etc actually works, I can't
> > even recall if from_kuid(init_user_ns, xxx) == __kuid_val(xxx) or not,
> > although this doesn't really matter.
>
> Yes, from_kuid(_user_ns, xxx) == __kuid_val(xxx). For values from 0
> to 0xfffe, the uid_map of init_user_ns is an identity map (and it
> can't be changed), and for 0x, which isn't mapped, it returns
> 0x to denote failure.
So perhaps we can add another helper to avoid the unnecessary map_id_up()
from_kuid_init_user_ns(kuid_t kuid)
{
return __kuid_val(kuid);
}
But this is a bit off-topic, let me repeat that your patch looks fine to me.
Oleg.
Re: [PATCH] signal: fix overflow_uid signal sender
On Wed, Nov 02, 2016 at 07:16:41PM +0100, Oleg Nesterov wrote:
> On 10/31, Jann Horn wrote:
> >
> > static inline void userns_fixup_signal_uid(struct siginfo *info, struct
> > task_struct *t)
> > {
> > - if (current_user_ns() == task_cred_xxx(t, user_ns))
> > + if (_user_ns == task_cred_xxx(t, user_ns))
> > return;
> >
> > if (SI_FROMKERNEL(info))
> > @@ -959,7 +959,7 @@ static inline void userns_fixup_signal_uid(struct
> > siginfo *info, struct task_str
> >
> > rcu_read_lock();
> > info->si_uid = from_kuid_munged(task_cred_xxx(t, user_ns),
> > - make_kuid(current_user_ns(),
> > info->si_uid));
> > + make_kuid(_user_ns, info->si_uid));
> > rcu_read_unlock();
> > }
> > #else
> > @@ -1027,7 +1027,8 @@ static int __send_signal(int sig, struct siginfo
> > *info, struct task_struct *t,
> > q->info.si_code = SI_USER;
> > q->info.si_pid = task_tgid_nr_ns(current,
> > task_active_pid_ns(t));
> > - q->info.si_uid = from_kuid_munged(current_user_ns(),
> > current_uid());
> > + q->info.si_uid = from_kuid(_user_ns,
> > + current_uid());
>
> Looks good to me at first glance, but I think this needs an ack from Eric.
>
> I have to admit that I forgot how uid_map/etc actually works, I can't
> even recall if from_kuid(init_user_ns, xxx) == __kuid_val(xxx) or not,
> although this doesn't really matter.
Yes, from_kuid(_user_ns, xxx) == __kuid_val(xxx). For values from 0
to 0xfffe, the uid_map of init_user_ns is an identity map (and it
can't be changed), and for 0x, which isn't mapped, it returns
0x to denote failure.
signature.asc
Description: Digital signature
Re: [PATCH] signal: fix overflow_uid signal sender
On Wed, Nov 02, 2016 at 07:16:41PM +0100, Oleg Nesterov wrote:
> On 10/31, Jann Horn wrote:
> >
> > static inline void userns_fixup_signal_uid(struct siginfo *info, struct
> > task_struct *t)
> > {
> > - if (current_user_ns() == task_cred_xxx(t, user_ns))
> > + if (_user_ns == task_cred_xxx(t, user_ns))
> > return;
> >
> > if (SI_FROMKERNEL(info))
> > @@ -959,7 +959,7 @@ static inline void userns_fixup_signal_uid(struct
> > siginfo *info, struct task_str
> >
> > rcu_read_lock();
> > info->si_uid = from_kuid_munged(task_cred_xxx(t, user_ns),
> > - make_kuid(current_user_ns(),
> > info->si_uid));
> > + make_kuid(_user_ns, info->si_uid));
> > rcu_read_unlock();
> > }
> > #else
> > @@ -1027,7 +1027,8 @@ static int __send_signal(int sig, struct siginfo
> > *info, struct task_struct *t,
> > q->info.si_code = SI_USER;
> > q->info.si_pid = task_tgid_nr_ns(current,
> > task_active_pid_ns(t));
> > - q->info.si_uid = from_kuid_munged(current_user_ns(),
> > current_uid());
> > + q->info.si_uid = from_kuid(_user_ns,
> > + current_uid());
>
> Looks good to me at first glance, but I think this needs an ack from Eric.
>
> I have to admit that I forgot how uid_map/etc actually works, I can't
> even recall if from_kuid(init_user_ns, xxx) == __kuid_val(xxx) or not,
> although this doesn't really matter.
Yes, from_kuid(_user_ns, xxx) == __kuid_val(xxx). For values from 0
to 0xfffe, the uid_map of init_user_ns is an identity map (and it
can't be changed), and for 0x, which isn't mapped, it returns
0x to denote failure.
signature.asc
Description: Digital signature
Re: [PATCH] signal: fix overflow_uid signal sender
On 10/31, Jann Horn wrote:
>
> static inline void userns_fixup_signal_uid(struct siginfo *info, struct
> task_struct *t)
> {
> - if (current_user_ns() == task_cred_xxx(t, user_ns))
> + if (_user_ns == task_cred_xxx(t, user_ns))
> return;
>
> if (SI_FROMKERNEL(info))
> @@ -959,7 +959,7 @@ static inline void userns_fixup_signal_uid(struct siginfo
> *info, struct task_str
>
> rcu_read_lock();
> info->si_uid = from_kuid_munged(task_cred_xxx(t, user_ns),
> - make_kuid(current_user_ns(),
> info->si_uid));
> + make_kuid(_user_ns, info->si_uid));
> rcu_read_unlock();
> }
> #else
> @@ -1027,7 +1027,8 @@ static int __send_signal(int sig, struct siginfo *info,
> struct task_struct *t,
> q->info.si_code = SI_USER;
> q->info.si_pid = task_tgid_nr_ns(current,
> task_active_pid_ns(t));
> - q->info.si_uid = from_kuid_munged(current_user_ns(),
> current_uid());
> + q->info.si_uid = from_kuid(_user_ns,
> +current_uid());
Looks good to me at first glance, but I think this needs an ack from Eric.
I have to admit that I forgot how uid_map/etc actually works, I can't
even recall if from_kuid(init_user_ns, xxx) == __kuid_val(xxx) or not,
although this doesn't really matter.
Oleg.
Re: [PATCH] signal: fix overflow_uid signal sender
On 10/31, Jann Horn wrote:
>
> static inline void userns_fixup_signal_uid(struct siginfo *info, struct
> task_struct *t)
> {
> - if (current_user_ns() == task_cred_xxx(t, user_ns))
> + if (_user_ns == task_cred_xxx(t, user_ns))
> return;
>
> if (SI_FROMKERNEL(info))
> @@ -959,7 +959,7 @@ static inline void userns_fixup_signal_uid(struct siginfo
> *info, struct task_str
>
> rcu_read_lock();
> info->si_uid = from_kuid_munged(task_cred_xxx(t, user_ns),
> - make_kuid(current_user_ns(),
> info->si_uid));
> + make_kuid(_user_ns, info->si_uid));
> rcu_read_unlock();
> }
> #else
> @@ -1027,7 +1027,8 @@ static int __send_signal(int sig, struct siginfo *info,
> struct task_struct *t,
> q->info.si_code = SI_USER;
> q->info.si_pid = task_tgid_nr_ns(current,
> task_active_pid_ns(t));
> - q->info.si_uid = from_kuid_munged(current_user_ns(),
> current_uid());
> + q->info.si_uid = from_kuid(_user_ns,
> +current_uid());
Looks good to me at first glance, but I think this needs an ack from Eric.
I have to admit that I forgot how uid_map/etc actually works, I can't
even recall if from_kuid(init_user_ns, xxx) == __kuid_val(xxx) or not,
although this doesn't really matter.
Oleg.
[PATCH] signal: fix overflow_uid signal sender
This fixes the case where a signal that was sent by a user-namespaced
process appears to come from a different uid. This happens if the following
conditions are met:
- sender is in a user namespace
- sender's uid isn't mapped into sender's user namespace
- target is in the init user namespace
In this case, the sender's uid is first mapped down to the overflowuid via
from_kuid_munged() in sys_kill(), then mapped back up to a kuid (-1) and
down into the init namespace (overflowuid again) in
userns_fixup_signal_uid(). To observe this behavior, run the following
program:
===
$ cat signal_userns_poc.c
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
void handle_sigusr1(int sig, siginfo_t *info, void *uctx) {
printf("got SIGUSR1 from uid %d\n", info->si_uid);
}
int main(void) {
struct sigaction act = {
.sa_sigaction = handle_sigusr1,
.sa_flags = SA_SIGINFO
};
if (sigaction(SIGUSR1, , NULL))
err(1, "sigaction");
pid_t parent = getpid();
pid_t child = fork();
if (child == -1)
err(1, "fork");
if (child == 0) {
if (unshare(CLONE_NEWUSER))
err(1, "unshare");
kill(parent, SIGUSR1);
return 0;
}
int status;
waitpid(child, , 0);
return 0;
}
$ gcc -o signal_userns_poc signal_userns_poc.c -Wall
$ ./signal_userns_poc
got SIGUSR1 from uid 65534
===
While this is probably harmless - incorrect sender uids will always be
either the overflowuid or a uid that was mapped to the overflowuid in the
sender's namespace -, it's still an unnecessary loss of information.
Fix it by always storing the kuid in non-SI_KERNEL signals until the final
uid is computed in userns_fixup_signal_uid().
Signed-off-by: Jann Horn
---
ipc/mqueue.c| 9 +
kernel/signal.c | 22 +++---
2 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 8cbd6e6894d5..8bb0dda68fec 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -73,7 +73,6 @@ struct mqueue_inode_info {
struct sigevent notify;
struct pid *notify_owner;
- struct user_namespace *notify_user_ns;
struct user_struct *user; /* user who created, for accounting */
struct sock *notify_sock;
struct sk_buff *notify_cookie;
@@ -240,7 +239,6 @@ static struct inode *mqueue_get_inode(struct super_block
*sb,
INIT_LIST_HEAD(>e_wait_q[0].list);
INIT_LIST_HEAD(>e_wait_q[1].list);
info->notify_owner = NULL;
- info->notify_user_ns = NULL;
info->qsize = 0;
info->user = NULL; /* set when all is ok */
info->msg_tree = RB_ROOT;
@@ -643,7 +641,7 @@ static void __do_notify(struct mqueue_inode_info *info)
rcu_read_lock();
sig_i.si_pid = task_tgid_nr_ns(current,
ns_of_pid(info->notify_owner));
- sig_i.si_uid = from_kuid_munged(info->notify_user_ns,
current_uid());
+ sig_i.si_uid = from_kuid(_user_ns, current_uid());
rcu_read_unlock();
kill_pid_info(info->notify.sigev_signo,
@@ -656,9 +654,7 @@ static void __do_notify(struct mqueue_inode_info *info)
}
/* after notification unregisters process */
put_pid(info->notify_owner);
- put_user_ns(info->notify_user_ns);
info->notify_owner = NULL;
- info->notify_user_ns = NULL;
}
wake_up(>wait_q);
}
@@ -683,9 +679,7 @@ static void remove_notification(struct mqueue_inode_info
*info)
netlink_sendskb(info->notify_sock, info->notify_cookie);
}
put_pid(info->notify_owner);
- put_user_ns(info->notify_user_ns);
info->notify_owner = NULL;
- info->notify_user_ns = NULL;
}
static int mq_attr_ok(struct ipc_namespace *ipc_ns, struct mq_attr *attr)
@@ -1301,7 +1295,6 @@ SYSCALL_DEFINE2(mq_notify, mqd_t, mqdes,
}
info->notify_owner = get_pid(task_tgid(current));
- info->notify_user_ns = get_user_ns(current_user_ns());
inode->i_atime = inode->i_ctime = current_time(inode);
}
spin_unlock(>lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index 75761acc77cf..b0afbf9a30dd 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -951,7 +951,7 @@ static inline int legacy_queue(struct sigpending *signals,
int sig)
#ifdef CONFIG_USER_NS
static inline void userns_fixup_signal_uid(struct siginfo *info, struct
task_struct *t)
{
- if (current_user_ns() == task_cred_xxx(t, user_ns))
+ if (_user_ns == task_cred_xxx(t, user_ns))
return;
if (SI_FROMKERNEL(info))
@@ -959,7 +959,7 @@ static inline void
[PATCH] signal: fix overflow_uid signal sender
This fixes the case where a signal that was sent by a user-namespaced
process appears to come from a different uid. This happens if the following
conditions are met:
- sender is in a user namespace
- sender's uid isn't mapped into sender's user namespace
- target is in the init user namespace
In this case, the sender's uid is first mapped down to the overflowuid via
from_kuid_munged() in sys_kill(), then mapped back up to a kuid (-1) and
down into the init namespace (overflowuid again) in
userns_fixup_signal_uid(). To observe this behavior, run the following
program:
===
$ cat signal_userns_poc.c
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
void handle_sigusr1(int sig, siginfo_t *info, void *uctx) {
printf("got SIGUSR1 from uid %d\n", info->si_uid);
}
int main(void) {
struct sigaction act = {
.sa_sigaction = handle_sigusr1,
.sa_flags = SA_SIGINFO
};
if (sigaction(SIGUSR1, , NULL))
err(1, "sigaction");
pid_t parent = getpid();
pid_t child = fork();
if (child == -1)
err(1, "fork");
if (child == 0) {
if (unshare(CLONE_NEWUSER))
err(1, "unshare");
kill(parent, SIGUSR1);
return 0;
}
int status;
waitpid(child, , 0);
return 0;
}
$ gcc -o signal_userns_poc signal_userns_poc.c -Wall
$ ./signal_userns_poc
got SIGUSR1 from uid 65534
===
While this is probably harmless - incorrect sender uids will always be
either the overflowuid or a uid that was mapped to the overflowuid in the
sender's namespace -, it's still an unnecessary loss of information.
Fix it by always storing the kuid in non-SI_KERNEL signals until the final
uid is computed in userns_fixup_signal_uid().
Signed-off-by: Jann Horn
---
ipc/mqueue.c| 9 +
kernel/signal.c | 22 +++---
2 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 8cbd6e6894d5..8bb0dda68fec 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -73,7 +73,6 @@ struct mqueue_inode_info {
struct sigevent notify;
struct pid *notify_owner;
- struct user_namespace *notify_user_ns;
struct user_struct *user; /* user who created, for accounting */
struct sock *notify_sock;
struct sk_buff *notify_cookie;
@@ -240,7 +239,6 @@ static struct inode *mqueue_get_inode(struct super_block
*sb,
INIT_LIST_HEAD(>e_wait_q[0].list);
INIT_LIST_HEAD(>e_wait_q[1].list);
info->notify_owner = NULL;
- info->notify_user_ns = NULL;
info->qsize = 0;
info->user = NULL; /* set when all is ok */
info->msg_tree = RB_ROOT;
@@ -643,7 +641,7 @@ static void __do_notify(struct mqueue_inode_info *info)
rcu_read_lock();
sig_i.si_pid = task_tgid_nr_ns(current,
ns_of_pid(info->notify_owner));
- sig_i.si_uid = from_kuid_munged(info->notify_user_ns,
current_uid());
+ sig_i.si_uid = from_kuid(_user_ns, current_uid());
rcu_read_unlock();
kill_pid_info(info->notify.sigev_signo,
@@ -656,9 +654,7 @@ static void __do_notify(struct mqueue_inode_info *info)
}
/* after notification unregisters process */
put_pid(info->notify_owner);
- put_user_ns(info->notify_user_ns);
info->notify_owner = NULL;
- info->notify_user_ns = NULL;
}
wake_up(>wait_q);
}
@@ -683,9 +679,7 @@ static void remove_notification(struct mqueue_inode_info
*info)
netlink_sendskb(info->notify_sock, info->notify_cookie);
}
put_pid(info->notify_owner);
- put_user_ns(info->notify_user_ns);
info->notify_owner = NULL;
- info->notify_user_ns = NULL;
}
static int mq_attr_ok(struct ipc_namespace *ipc_ns, struct mq_attr *attr)
@@ -1301,7 +1295,6 @@ SYSCALL_DEFINE2(mq_notify, mqd_t, mqdes,
}
info->notify_owner = get_pid(task_tgid(current));
- info->notify_user_ns = get_user_ns(current_user_ns());
inode->i_atime = inode->i_ctime = current_time(inode);
}
spin_unlock(>lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index 75761acc77cf..b0afbf9a30dd 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -951,7 +951,7 @@ static inline int legacy_queue(struct sigpending *signals,
int sig)
#ifdef CONFIG_USER_NS
static inline void userns_fixup_signal_uid(struct siginfo *info, struct
task_struct *t)
{
- if (current_user_ns() == task_cred_xxx(t, user_ns))
+ if (_user_ns == task_cred_xxx(t, user_ns))
return;
if (SI_FROMKERNEL(info))
@@ -959,7 +959,7 @@ static inline void userns_fixup_signal_uid(struct siginfo
