netdev - Re: [v3,net-next,2/2] tls: Use correct sk->sk

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: <20180223215248.GA20989@roeck-us.net>
Date:   Fri, 23 Feb 2018 13:52:48 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Ilya Lesokhin <ilyal@...lanox.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, davejwatson@...com,
        aviadye@...lanox.com, Boris Pismenny <borisp@...lanox.com>
Subject: Re: [v3,net-next,2/2] tls: Use correct sk->sk_prot for IPV6

Hi Ilya,

On Mon, Sep 04, 2017 at 01:14:01PM +0300, Ilya Lesokhin wrote:
> The tls ulp overrides sk->prot with a new tls specific proto structs.
> The tls specific structs were previously based on the ipv4 specific
> tcp_prot sturct.
> As a result, attaching the tls ulp to an ipv6 tcp socket replaced
> some ipv6 callback with the ipv4 equivalents.
> 
> This patch adds ipv6 tls proto structs and uses them when
> attached to ipv6 sockets.
> 

Do you plan to update this patch with the missing TCPv6 support ?
As far as I can see, the part that was accepted upstream does not fix
the TCPv6 protocol issue which triggers CVE-2018-5703.

If adding IPv6 support is not possible/acceptable, would it make sense
to limit TLS support to TCPv4, ie add something like

	if (sk->sk_prot != &tcp_prot)
		return -EINVAL;

to tls_init() ?

Thanks,
Guenter

> Fixes: 3c4d7559159b ('tls: kernel TLS support')
> Signed-off-by: Boris Pismenny <borisp@...lanox.com>
> Signed-off-by: Ilya Lesokhin <ilyal@...lanox.com>
> ---
>  net/tls/Kconfig    |  1 +
>  net/tls/tls_main.c | 50 ++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/net/tls/Kconfig b/net/tls/Kconfig
> index eb58303..7e9cf8b 100644
> --- a/net/tls/Kconfig
> +++ b/net/tls/Kconfig
> @@ -7,6 +7,7 @@ config TLS
>  	select CRYPTO
>  	select CRYPTO_AES
>  	select CRYPTO_GCM
> +	select IPV6
>  	default n
>  	---help---
>  	Enable kernel support for TLS protocol. This allows symmetric
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index 60aff60..33c499e 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -40,13 +40,25 @@
>  #include <linux/sched/signal.h>
>  
>  #include <net/tls.h>
> +#include <net/transp_v6.h>
>  
>  MODULE_AUTHOR("Mellanox Technologies");
>  MODULE_DESCRIPTION("Transport Layer Security Support");
>  MODULE_LICENSE("Dual BSD/GPL");
>  
> -static struct proto tls_base_prot;
> -static struct proto tls_sw_prot;
> +enum {
> +	TLSV4,
> +	TLSV6,
> +	TLS_NUM_PROTS,
> +};
> +
> +enum {
> +	TLS_BASE_TX,
> +	TLS_SW_TX,
> +	TLS_NUM_CONFIG,
> +};
> +
> +static struct proto tls_prots[TLS_NUM_PROTS][TLS_NUM_CONFIG];
>  
>  int wait_on_pending_writer(struct sock *sk, long *timeo)
>  {
> @@ -342,6 +354,7 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
>  	struct tls_context *ctx = tls_get_ctx(sk);
>  	struct proto *prot = NULL;
>  	int rc = 0;
> +	int ip_ver = sk->sk_family == AF_INET6 ? TLSV6 : TLSV4;
>  
>  	if (!optval || (optlen < sizeof(*crypto_info))) {
>  		rc = -EINVAL;
> @@ -396,7 +409,7 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
>  
>  	/* currently SW is default, we will have ethtool in future */
>  	rc = tls_set_sw_offload(sk, ctx);
> -	prot = &tls_sw_prot;
> +	prot = &tls_prots[ip_ver][TLS_SW_TX];
>  	if (rc)
>  		goto err_crypto_info;
>  
> @@ -443,6 +456,12 @@ static int tls_init(struct sock *sk)
>  	struct inet_connection_sock *icsk = inet_csk(sk);
>  	struct tls_context *ctx;
>  	int rc = 0;
> +	int ip_ver = TLSV4;
> +
> +	if (sk->sk_prot == &tcpv6_prot)
> +		ip_ver = TLSV6;
> +	else if (sk->sk_prot != &tcp_prot)
> +		return -EINVAL;
>  
>  	/* allocate tls context */
>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> @@ -453,7 +472,8 @@ static int tls_init(struct sock *sk)
>  	icsk->icsk_ulp_data = ctx;
>  	ctx->setsockopt = sk->sk_prot->setsockopt;
>  	ctx->getsockopt = sk->sk_prot->getsockopt;
> -	sk->sk_prot = &tls_base_prot;
> +
> +	sk->sk_prot = &tls_prots[ip_ver][TLS_BASE_TX];
>  out:
>  	return rc;
>  }
> @@ -464,16 +484,22 @@ static int tls_init(struct sock *sk)
>  	.init			= tls_init,
>  };
>  
> +static void build_protos(struct proto *prot, struct proto *base)
> +{
> +	prot[TLS_BASE_TX] = *base;
> +	prot[TLS_BASE_TX].setsockopt = tls_setsockopt;
> +	prot[TLS_BASE_TX].getsockopt = tls_getsockopt;
> +
> +	prot[TLS_SW_TX] = prot[TLS_BASE_TX];
> +	prot[TLS_SW_TX].close		= tls_sk_proto_close;
> +	prot[TLS_SW_TX].sendmsg		= tls_sw_sendmsg;
> +	prot[TLS_SW_TX].sendpage	= tls_sw_sendpage;
> +}
> +
>  static int __init tls_register(void)
>  {
> -	tls_base_prot			= tcp_prot;
> -	tls_base_prot.setsockopt	= tls_setsockopt;
> -	tls_base_prot.getsockopt	= tls_getsockopt;
> -
> -	tls_sw_prot			= tls_base_prot;
> -	tls_sw_prot.sendmsg		= tls_sw_sendmsg;
> -	tls_sw_prot.sendpage            = tls_sw_sendpage;
> -	tls_sw_prot.close               = tls_sk_proto_close;
> +	build_protos(tls_prots[TLSV4], &tcp_prot);
> +	build_protos(tls_prots[TLSV6], &tcpv6_prot);
>  
>  	tcp_register_ulp(&tcp_tls_ulp_ops);
>