netdev - Re: [PATCH net-next] net/tls: prevent skb_orphan() from leaking TLS plain text with offload

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+FuTSdN41z5PVfyT5Z-ApnKQ9CYcDSnr4VGZnsgA-iAEK12Ow@mail.gmail.com>
Date:   Wed, 31 Jul 2019 11:57:10 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     David Miller <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>,
        oss-drivers@...ronome.com, Eric Dumazet <edumazet@...gle.com>,
        borisp@...lanox.com, aviadye@...lanox.com, davejwatson@...com,
        John Fastabend <john.fastabend@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Florian Westphal <fw@...len.de>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Subject: Re: [PATCH net-next] net/tls: prevent skb_orphan() from leaking TLS
 plain text with offload

On Tue, Jul 30, 2019 at 5:13 PM Jakub Kicinski
<jakub.kicinski@...ronome.com> wrote:
>
> sk_validate_xmit_skb() and drivers depend on the sk member of
> struct sk_buff to identify segments requiring encryption.
> Any operation which removes or does not preserve the original TLS
> socket such as skb_orphan() or skb_clone() will cause clear text
> leaks.
>
> Make the TCP socket underlying an offloaded TLS connection
> mark all skbs as decrypted, if TLS TX is in offload mode.
> Then in sk_validate_xmit_skb() catch skbs which have no socket
> (or a socket with no validation) and decrypted flag set.
>
> Note that CONFIG_SOCK_VALIDATE_XMIT, CONFIG_TLS_DEVICE and
> sk->sk_validate_xmit_skb are slightly interchangeable right now,
> they all imply TLS offload. The new checks are guarded by
> CONFIG_TLS_DEVICE because that's the option guarding the
> sk_buff->decrypted member.
>
> Second, smaller issue with orphaning is that it breaks
> the guarantee that packets will be delivered to device
> queues in-order. All TLS offload drivers depend on that
> scheduling property. This means skb_orphan_partial()'s
> trick of preserving partial socket references will cause
> issues in the drivers. We need a full orphan, and as a
> result netem delay/throttling will cause all TLS offload
> skbs to be dropped.
>
> Reusing the sk_buff->decrypted flag also protects from
> leaking clear text when incoming, decrypted skb is redirected
> (e.g. by TC).
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> ---
> I'm sending this for net-next because of lack of confidence
> in my own abilities. It should apply cleanly to net... :)
>
>  Documentation/networking/tls-offload.rst |  9 --------
>  include/net/sock.h                       | 28 +++++++++++++++++++++++-
>  net/core/skbuff.c                        |  3 +++
>  net/core/sock.c                          | 22 ++++++++++++++-----
>  net/ipv4/tcp.c                           |  4 +++-
>  net/ipv4/tcp_output.c                    |  3 +++
>  net/tls/tls_device.c                     |  2 ++
>  7 files changed, 55 insertions(+), 16 deletions(-)

>  Redirects leak clear text
>  -------------------------
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 228db3998e46..90f3f552c789 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -814,6 +814,7 @@ enum sock_flags {
>         SOCK_TXTIME,
>         SOCK_XDP, /* XDP is attached */
>         SOCK_TSTAMP_NEW, /* Indicates 64 bit timestamps always */
> +       SOCK_CRYPTO_TX_PLAIN_TEXT, /* Generate skbs with decrypted flag set */
>  };
>
>  #define SK_FLAGS_TIMESTAMP ((1UL << SOCK_TIMESTAMP) | (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE))
> @@ -2480,8 +2481,26 @@ static inline bool sk_fullsock(const struct sock *sk)
>         return (1 << sk->sk_state) & ~(TCPF_TIME_WAIT | TCPF_NEW_SYN_RECV);
>  }
>
> +static inline void sk_set_tx_crypto(const struct sock *sk, struct sk_buff *skb)

nit: skb_.. instead of sk_.. ?

> +{
> +#ifdef CONFIG_TLS_DEVICE
> +       skb->decrypted = sock_flag(sk, SOCK_CRYPTO_TX_PLAIN_TEXT);
> +#endif
> +}

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 0b788df5a75b..9e92684479b9 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3794,6 +3794,9 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>
>                         skb_reserve(nskb, headroom);
>                         __skb_put(nskb, doffset);
> +#ifdef CONFIG_TLS_DEVICE
> +                       nskb->decrypted = head_skb->decrypted;
> +#endif

decrypted is between header_start and headers_end, so
__copy_skb_header just below should take care of this?


> diff --git a/net/core/sock.c b/net/core/sock.c
> index d57b0cc995a0..b0c10b518e65 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1992,6 +1992,22 @@ void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
>  }
>  EXPORT_SYMBOL(skb_set_owner_w);
>
> +static bool can_skb_orphan_partial(const struct sk_buff *skb)
> +{
> +#ifdef CONFIG_TLS_DEVICE
> +       /* Drivers depend on in-order delivery for crypto offload,
> +        * partial orphan breaks out-of-order-OK logic.
> +        */
> +       if (skb->decrypted)
> +               return false;
> +#endif
> +#ifdef CONFIG_INET
> +       if (skb->destructor == tcp_wfree)
> +               return true;
> +#endif
> +       return skb->destructor == sock_wfree;
> +}
> +

Just insert the skb->decrypted check into skb_orphan_partial for less
code churn?

I also think that this is an independent concern from leaking plain
text, so perhaps could be a separate patch.