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 <sched.h>
#include <err.h>
#include <unistd.h>
#include <signal.h>
#include <stdio.h>
#include <sys/wait.h>
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, &act, 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, &status, 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 <[email protected]>
---
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(&info->e_wait_q[0].list);
INIT_LIST_HEAD(&info->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(&init_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(&info->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(&info->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 (&init_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(&init_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(&init_user_ns,
+ current_uid());
break;
case (unsigned long) SEND_SIG_PRIV:
q->info.si_signo = sig;
@@ -1609,8 +1610,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
*/
rcu_read_lock();
info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(tsk->parent));
- info.si_uid = from_kuid_munged(task_cred_xxx(tsk->parent, user_ns),
- task_uid(tsk));
+ info.si_uid = from_kuid(&init_user_ns, task_uid(tsk));
rcu_read_unlock();
task_cputime(tsk, &utime, &stime);
@@ -1695,7 +1695,7 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
*/
rcu_read_lock();
info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(parent));
- info.si_uid = from_kuid_munged(task_cred_xxx(parent, user_ns), task_uid(tsk));
+ info.si_uid = from_kuid(&init_user_ns, task_uid(tsk));
rcu_read_unlock();
task_cputime(tsk, &utime, &stime);
@@ -1904,7 +1904,7 @@ static void ptrace_do_notify(int signr, int exit_code, int why)
info.si_signo = signr;
info.si_code = exit_code;
info.si_pid = task_pid_vnr(current);
- info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
+ info.si_uid = from_kuid(&init_user_ns, current_uid());
/* Let the debugger run. */
ptrace_stop(exit_code, why, 1, &info);
@@ -2113,8 +2113,8 @@ static int ptrace_signal(int signr, siginfo_t *info)
info->si_code = SI_USER;
rcu_read_lock();
info->si_pid = task_pid_vnr(current->parent);
- info->si_uid = from_kuid_munged(current_user_ns(),
- task_uid(current->parent));
+ info->si_uid = from_kuid(&init_user_ns,
+ task_uid(current->parent));
rcu_read_unlock();
}
@@ -2852,7 +2852,7 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
info.si_errno = 0;
info.si_code = SI_USER;
info.si_pid = task_tgid_vnr(current);
- info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
+ info.si_uid = from_kuid(&init_user_ns, current_uid());
return kill_something_info(sig, &info, pid);
}
@@ -2895,7 +2895,7 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
info.si_errno = 0;
info.si_code = SI_TKILL;
info.si_pid = task_tgid_vnr(current);
- info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
+ info.si_uid = from_kuid(&init_user_ns, current_uid());
return do_send_specific(tgid, pid, sig, &info);
}
--
2.1.4
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 (&init_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(&init_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(&init_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.
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 (&init_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(&init_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(&init_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(&init_user_ns, xxx) == __kuid_val(xxx). For values from 0
to 0xfffffffe, the uid_map of init_user_ns is an identity map (and it
can't be changed), and for 0xffffffff, which isn't mapped, it returns
0xffffffff to denote failure.
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 (&init_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(&init_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(&init_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(&init_user_ns, xxx) == __kuid_val(xxx). For values from 0
> to 0xfffffffe, the uid_map of init_user_ns is an identity map (and it
> can't be changed), and for 0xffffffff, which isn't mapped, it returns
> 0xffffffff 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.