| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAG-2HqXokzbsDt5Mt5VRyE=FFpUtfVe3YFhEhxuHv0b1KR7Wmw@mail.gmail.com>
Date: Mon, 17 Jul 2017 14:25:00 +0200
From: Tom Gundersen <teg@...m.no>
To: David Herrmann <dh.herrmann@...il.com>
Cc: netdev <netdev@...r.kernel.org>,
David Miller <davem@...emloft.net>,
Eric Dumazet <eric.dumazet@...il.com>,
Hannes Frederic Sowa <hannes@...essinduktion.org>,
LKML <linux-kernel@...r.kernel.org>,
Alban Crequy <alban.crequy@...labora.co.uk>,
Simon McVittie <simon.mcvittie@...labora.co.uk>
Subject: Re: [PATCH] net/unix: drop obsolete fd-recursion limits
On Mon, Jul 17, 2017 at 11:35 AM, David Herrmann <dh.herrmann@...il.com> wrote:
> All unix sockets now account inflight FDs to the respective sender.
> This was introduced in:
>
> commit 712f4aad406bb1ed67f3f98d04c044191f0ff593
> Author: willy tarreau <w@....eu>
> Date: Sun Jan 10 07:54:56 2016 +0100
>
> unix: properly account for FDs passed over unix sockets
>
> and further refined in:
>
> commit 415e3d3e90ce9e18727e8843ae343eda5a58fad6
> Author: Hannes Frederic Sowa <hannes@...essinduktion.org>
> Date: Wed Feb 3 02:11:03 2016 +0100
>
> unix: correctly track in-flight fds in sending process user_struct
>
> Hence, regardless of the stacking depth of FDs, the total number of
> inflight FDs is limited, and accounted. There is no known way for a
> local user to exceed those limits or exploit the accounting.
>
> Furthermore, the GC logic is independent of the recursion/stacking depth
> as well. It solely depends on the total number of inflight FDs,
> regardless of their layout.
>
> Lastly, the current `recursion_level' suffers a TOCTOU race, since it
> checks and inherits depths only at queue time. If we consider `A<-B' to
> mean `queue-B-on-A', the following sequence circumvents the recursion
> level easily:
>
> A<-B
> B<-C
> C<-D
> ...
> Y<-Z
>
> resulting in:
>
> A<-B<-C<-...<-Z
>
> With all of this in mind, lets drop the recursion limit. It has no
> additional security value, anymore. On the contrary, it randomly
> confuses message brokers that try to forward file-descriptors, since
> any sendmsg(2) call can fail spuriously with ETOOMANYREFS if a client
> maliciously modifies the FD while inflight.
>
> Cc: Alban Crequy <alban.crequy@...labora.co.uk>
> Cc: Simon McVittie <simon.mcvittie@...labora.co.uk>
> Signed-off-by: David Herrmann <dh.herrmann@...il.com>
Reviewed-by: Tom Gundersen <teg@...m.no>
> ---
> include/net/af_unix.h | 1 -
> net/unix/af_unix.c | 24 +-----------------------
> 2 files changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index 678e4d6fa317..3b3194b2fc65 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -58,7 +58,6 @@ struct unix_sock {
> struct list_head link;
> atomic_long_t inflight;
> spinlock_t lock;
> - unsigned char recursion_level;
> unsigned long gc_flags;
> #define UNIX_GC_CANDIDATE 0
> #define UNIX_GC_MAYBE_CYCLE 1
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 7b52a380d710..5c53f22d62e8 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1528,26 +1528,13 @@ static inline bool too_many_unix_fds(struct task_struct *p)
> return false;
> }
>
> -#define MAX_RECURSION_LEVEL 4
> -
> static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
> {
> int i;
> - unsigned char max_level = 0;
>
> if (too_many_unix_fds(current))
> return -ETOOMANYREFS;
>
> - for (i = scm->fp->count - 1; i >= 0; i--) {
> - struct sock *sk = unix_get_socket(scm->fp->fp[i]);
> -
> - if (sk)
> - max_level = max(max_level,
> - unix_sk(sk)->recursion_level);
> - }
> - if (unlikely(max_level > MAX_RECURSION_LEVEL))
> - return -ETOOMANYREFS;
> -
> /*
> * Need to duplicate file references for the sake of garbage
> * collection. Otherwise a socket in the fps might become a
> @@ -1559,7 +1546,7 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
>
> for (i = scm->fp->count - 1; i >= 0; i--)
> unix_inflight(scm->fp->user, scm->fp->fp[i]);
> - return max_level;
> + return 0;
> }
>
> static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool send_fds)
> @@ -1649,7 +1636,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> struct sk_buff *skb;
> long timeo;
> struct scm_cookie scm;
> - int max_level;
> int data_len = 0;
> int sk_locked;
>
> @@ -1701,7 +1687,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> err = unix_scm_to_skb(&scm, skb, true);
> if (err < 0)
> goto out_free;
> - max_level = err + 1;
>
> skb_put(skb, len - data_len);
> skb->data_len = data_len;
> @@ -1819,8 +1804,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> __net_timestamp(skb);
> maybe_add_creds(skb, sock, other);
> skb_queue_tail(&other->sk_receive_queue, skb);
> - if (max_level > unix_sk(other)->recursion_level)
> - unix_sk(other)->recursion_level = max_level;
> unix_state_unlock(other);
> other->sk_data_ready(other);
> sock_put(other);
> @@ -1855,7 +1838,6 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
> int sent = 0;
> struct scm_cookie scm;
> bool fds_sent = false;
> - int max_level;
> int data_len;
>
> wait_for_unix_gc();
> @@ -1905,7 +1887,6 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
> kfree_skb(skb);
> goto out_err;
> }
> - max_level = err + 1;
> fds_sent = true;
>
> skb_put(skb, size - data_len);
> @@ -1925,8 +1906,6 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
>
> maybe_add_creds(skb, sock, other);
> skb_queue_tail(&other->sk_receive_queue, skb);
> - if (max_level > unix_sk(other)->recursion_level)
> - unix_sk(other)->recursion_level = max_level;
> unix_state_unlock(other);
> other->sk_data_ready(other);
> sent += size;
> @@ -2324,7 +2303,6 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
> last_len = last ? last->len : 0;
> again:
> if (skb == NULL) {
> - unix_sk(sk)->recursion_level = 0;
> if (copied >= target)
> goto unlock;
>
> --
> 2.13.2
>
Powered by blists - more mailing lists