Date:	Mon, 26 May 2014 10:19:56 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	"Jorge Boncompte [DTI2]" <jorge@...2.net>,
	Jiri Benc <jbenc@...hat.com>,
	David Miller <davem@...emloft.net>,
	Vivek Goyal <vgoyal@...hat.com>,
	Simo Sorce <ssorce@...hat.com>,
	"security@...nel.org" <security@...nel.org>,
	Network Development <netdev@...r.kernel.org>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	Michael Kerrisk-manpages <mtk.manpages@...il.com>
Subject: Re: [RFC][PATCH 2/1] netlink: Use the credential at the time the
 destination address was set.

On Sun, May 25, 2014 at 10:36 PM, Eric W. Biederman
<ebiederm@...ssion.com> wrote:
>
> When sending a message over a netlink socket and then checking to see if
> the person is authorized to send the message it is important that we verify
> both the sender of the message and the whoever it is that set the destination
> of the message both have permission.  Otherwise it becomes possible for an
> unpriivleged user to set the message destination and trick an suid process
> to write to the socket and change the network connection.
>
> For netlink sockets socket() sets the default destination address to 0 (the kernel)
> so we need to remember the credentials when a socket is created.
>
> For netlink sockets connect() changes the default destination address so
> we need to remember the credentials of the last changer of the default destination
> with connect.
>
> This results is there always being a valid remembered credential on the socket
> and so that credential is unconditionally freed in netlink_release().
>
> This change makes the semantics of the permission checks of netlink sockets
> make sense,and removes the possibility of an unprivileged user getting access
> to a root own socket and changing the destination address with connect.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
> ---
>
> This winds up continuig to grab credentials when socket() is called
> because we actually set the destination address in socket() for netlink
> sockets, but now we update those credentials when connect() is called.
>
>  include/linux/netlink.h  |  2 +-
>  net/netlink/af_netlink.c | 27 ++++++++++++++++++++++++---
>  net/netlink/af_netlink.h |  1 +
>  3 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index f289d085f87f..4f4607c0a1a1 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -19,7 +19,7 @@ enum netlink_skb_flags {
>         NETLINK_SKB_MMAPED      = 0x1,          /* Packet data is mmaped */
>         NETLINK_SKB_TX          = 0x2,          /* Packet was sent by userspace */
>         NETLINK_SKB_DELIVERED   = 0x4,          /* Packet was delivered */
> -       NETLINK_SKB_DST         = 0x8,          /* Packet not socket destination */
> +       NETLINK_SKB_DST         = 0x8,          /* Dst set in sendto or sendmsg */
>  };
>
>  struct netlink_skb_parms {
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 15c731f03fa6..5b886ed6a648 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1191,6 +1191,7 @@ static int __netlink_create(struct net *net, struct socket *sock,
>                 mutex_init(nlk->cb_mutex);
>         }
>         init_waitqueue_head(&nlk->wait);
> +       nlk->cred = get_current_cred();
>  #ifdef CONFIG_NETLINK_MMAP
>         mutex_init(&nlk->pg_vec_lock);
>  #endif
> @@ -1291,6 +1292,7 @@ static int netlink_release(struct socket *sock)
>                                 NETLINK_URELEASE, &n);
>         }
>
> +       put_cred(nlk->cred);
>         module_put(nlk->module);
>
>         netlink_table_grab();
> @@ -1377,9 +1379,17 @@ retry:
>  bool __netlink_ns_capable(const struct netlink_skb_parms *nsp,
>                         struct user_namespace *user_ns, int cap)
>  {
> -       return ((nsp->flags & NETLINK_SKB_DST) ||
> -               file_ns_capable(nsp->sk->sk_socket->file, user_ns, cap)) &&
> -               ns_capable(user_ns, cap);
> +       const struct cred *cred;
> +       bool capable;
> +
> +       rcu_read_lock();
> +       cred = nlk_sk(nsp->sk)->cred;
> +       capable = ((nsp->flags & NETLINK_SKB_DST) ||
> +                  security_capable(cred, user_ns, cap)) &&
> +                       ns_capable(user_ns, cap);
> +       rcu_read_unlock();
> +
> +       return capable;
>  }
>  EXPORT_SYMBOL(__netlink_ns_capable);
>
> @@ -1569,14 +1579,20 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr,
>         struct sock *sk = sock->sk;
>         struct netlink_sock *nlk = nlk_sk(sk);
>         struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr;
> +       const struct cred *old_cred;
>
>         if (alen < sizeof(addr->sa_family))
>                 return -EINVAL;
>
>         if (addr->sa_family == AF_UNSPEC) {
> +               lock_sock(sk);
>                 sk->sk_state    = NETLINK_UNCONNECTED;
>                 nlk->dst_portid = 0;
>                 nlk->dst_group  = 0;
> +               old_cred = nlk->cred;
> +               nlk->cred = get_current_cred();
> +               put_cred(old_cred);
> +               release_sock(sk);
>                 return 0;

I'm confused about the locking.  Either lock_sock prevents concurrent
sendmsg, in which case the rcu_read_lock should be unnecessary, or it
doesn't, in which case I think this is racy, because sendmsg could see
a cred and dst_whatever that don't match.  Admittedly, exploiting that
race is probably extremely difficult.

If the RCU protection is needed, shouldn't this use rcu_assign_pointer
and rcu_dereference?


--Andy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists