[PATCH] signal: fix overflow_uid signal sender

[PATCH] signal: fix overflow_uid signal sender

[Jann Horn]

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 <jann@thejh.net>
---
 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

Re: [PATCH] signal: fix overflow_uid signal sender

[Jann Horn]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] signal: fix overflow_uid signal sender

[Oleg Nesterov]

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.

Re: [PATCH] signal: fix overflow_uid signal sender

[Oleg Nesterov]

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.