Re: [PATCH net-next] net/tls: prevent skb_orphan() from leaking TLS plain text with offload
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2019-07-31 15:57:53
On Tue, Jul 30, 2019 at 5:13 PM Jakub Kicinski [off-list ref] 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 <redacted> --- 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(-)
quoted hunk ↗ jump to hunk
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
+}quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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.