netdev - Re: [PATCH net v3] vti: fix spd lookup: match plaintext pkt, not ipsec pkt

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: <20131121121246.GD31491@secunet.com>
Date:	Thu, 21 Nov 2013 13:12:46 +0100
From:	Steffen Klassert <steffen.klassert@...unet.com>
To:	Christophe Gouault <christophe.gouault@...nd.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	netdev@...r.kernel.org, Saurabh Mohan <saurabh.mohan@...tta.com>,
	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net v3] vti: fix spd lookup: match plaintext pkt, not
 ipsec pkt

On Wed, Nov 06, 2013 at 09:05:53AM +0100, Christophe Gouault wrote:
>  
> @@ -133,7 +134,13 @@ static int vti_rcv(struct sk_buff *skb)
>  		 * only match policies with this mark.
>  		 */
>  		skb->mark = be32_to_cpu(tunnel->parms.o_key);
> +		/* The packet is decrypted, but not yet decapsulated.
> +		 * Temporarily make network_header point to the inner header
> +		 * for policy check.
> +		 */
> +		skb_reset_network_header(skb);
>  		ret = xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb);

If we do it like this, we would do an input policy check even for
packets that should be forwarded. I think that's a bit odd.

If we really change to match plaintext traffic, we should do
it like Fan Du proposed. Remove the policy check here and
let the further layers do the policy enforcement. All we
have to do is to set the skb mark, then the lookup should
match the vti policy.

It is already clear that this packet was IPsec transformed
when it enters vti_rcv, so deferring the policy check should
be ok.

>  
>  	if (skb->protocol != htons(ETH_P_IP))
> @@ -173,17 +182,35 @@ static netdev_tx_t vti_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	tos = old_iph->tos;
>  
> +	/* SPD lookup: we must provide a dst_entry to xfrm_lookup, normally the
> +	 * route to the final destination. However this route is a route via
> +	 * the vti interface. Now vti interfaces typically have the NOXFRM
> +	 * flag, hence xfrm_lookup would bypass IPsec.
> +	 *
> +	 * Therefore, we feed xfrm_lookup with a route to the vti tunnel remote
> +	 * endpoint instead.
> +	 */
>  	memset(&fl4, 0, sizeof(fl4));
>  	flowi4_init_output(&fl4, tunnel->parms.link,
>  			   be32_to_cpu(tunnel->parms.o_key), RT_TOS(tos),
>  			   RT_SCOPE_UNIVERSE,
>  			   IPPROTO_IPIP, 0,
>  			   dst, tiph->saddr, 0, 0);
> -	rt = ip_route_output_key(dev_net(dev), &fl4);
> +	rt = __ip_route_output_key(tunnel->net, &fl4);
>  	if (IS_ERR(rt)) {
>  		dev->stats.tx_carrier_errors++;
>  		goto tx_error_icmp;
>  	}
> +
> +	memset(&fl, 0, sizeof(fl));
> +	/* Temporarily mark the skb with the tunnel o_key, to look up
> +	 * for a policy with this mark, matching the plaintext traffic.
> +	 */
> +	skb->mark = be32_to_cpu(tunnel->parms.o_key);
> +	__xfrm_decode_session(skb, &fl, AF_INET, 0);
> +	skb->mark = oldmark;
> +	rt = (struct rtable *)xfrm_lookup(tunnel->net, &rt->dst, &fl, NULL, 0);
> +

I think we should not do such a workarround for the NOXFRM case.
In particular because this is not really typical, it is not the
default and it is not documented that it should be like that.

--
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