diff mbox series

[net,1/2] ipv4: tcp: send zero IPID in SYNACK messages

Message ID 20220126200518.990670-2-eric.dumazet@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ipv4: less uses of shared IP generator | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 4 this patch: 5
netdev/cc_maintainers warning 1 maintainers not CCed: yoshfuji@linux-ipv6.org
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn fail Errors and warnings before: 9 this patch: 10
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 21 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Eric Dumazet Jan. 26, 2022, 8:05 p.m. UTC 
From: Eric Dumazet <edumazet@google.com>

In commit 431280eebed9 ("ipv4: tcp: send zero IPID for RST and
ACK sent in SYN-RECV and TIME-WAIT state") we took care of some
ctl packets sent by TCP.

It turns out we need to use a similar strategy for SYNACK packets.

By default, they carry IP_DF and IPID==0, but there are ways
to ask them to use the hashed IP ident generator and thus
be used to build off-path attacks.
(Ref: Off-Path TCP Exploits of the Mixed IPID Assignment)

One of this way is to force (before listener is started)
echo 1 >/proc/sys/net/ipv4/ip_no_pmtu_disc

Another way is using forged ICMP ICMP_FRAG_NEEDED
with a very small MTU (like 68) to force a false return from
ip_dont_fragment()

In this patch, ip_build_and_send_pkt() uses the following
heuristics.

1) Most SYNACK packets are smaller than IPV4_MIN_MTU and therefore
can use IP_DF regardless of the listener or route pmtu setting.

2) In case the SYNACK packet is bigger than IPV4_MIN_MTU,
we use prandom_u32() generator instead of the IPv4 hashed ident one.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Ray Che <xijiache@gmail.com>
Cc: Geoff Alexander <alexandg@cs.unm.edu>
Cc: Willy Tarreau <w@1wt.eu>
---
 net/ipv4/ip_output.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

David Ahern Jan. 26, 2022, 8:35 p.m. UTC | #1 
On 1/26/22 1:05 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> In commit 431280eebed9 ("ipv4: tcp: send zero IPID for RST and
> ACK sent in SYN-RECV and TIME-WAIT state") we took care of some
> ctl packets sent by TCP.
> 
> It turns out we need to use a similar strategy for SYNACK packets.
> 
> By default, they carry IP_DF and IPID==0, but there are ways
> to ask them to use the hashed IP ident generator and thus
> be used to build off-path attacks.
> (Ref: Off-Path TCP Exploits of the Mixed IPID Assignment)
> 
> One of this way is to force (before listener is started)
> echo 1 >/proc/sys/net/ipv4/ip_no_pmtu_disc
> 
> Another way is using forged ICMP ICMP_FRAG_NEEDED
> with a very small MTU (like 68) to force a false return from
> ip_dont_fragment()
> 
> In this patch, ip_build_and_send_pkt() uses the following
> heuristics.
> 
> 1) Most SYNACK packets are smaller than IPV4_MIN_MTU and therefore
> can use IP_DF regardless of the listener or route pmtu setting.
> 
> 2) In case the SYNACK packet is bigger than IPV4_MIN_MTU,
> we use prandom_u32() generator instead of the IPv4 hashed ident one.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Ray Che <xijiache@gmail.com>
> Cc: Geoff Alexander <alexandg@cs.unm.edu>
> Cc: Willy Tarreau <w@1wt.eu>
> ---
>  net/ipv4/ip_output.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 


Reviewed-by: David Ahern <dsahern@kernel.org>
Jakub Kicinski Jan. 27, 2022, 12:56 a.m. UTC | #2 
On Wed, 26 Jan 2022 12:05:17 -0800 Eric Dumazet wrote:
> +		/* TCP packets here are SYNACK with fat IPv4/TCP options.
> +		 * Avoid using the hashed IP ident generator.
> +		 */
> +		if (sk->sk_protocol == IPPROTO_TCP)
> +			iph->id = prandom_u32();

Is it worth marking this as (__force __be32) to avoid the false
positive sparse warning?
kernel test robot Jan. 27, 2022, 1 a.m. UTC | #3 
Hi Eric,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Eric-Dumazet/ipv4-less-uses-of-shared-IP-generator/20220127-040810
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 429c3be8a5e2695b5b92a6a12361eb89eb185495
config: i386-randconfig-s002 (https://download.01.org/0day-ci/archive/20220127/202201270807.HsUjGLC8-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/37d3b618591c7c736c2ad3b3febe12779e01369c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-Dumazet/ipv4-less-uses-of-shared-IP-generator/20220127-040810
        git checkout 37d3b618591c7c736c2ad3b3febe12779e01369c
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash net/ipv4/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> net/ipv4/ip_output.c:175:33: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __be16 [usertype] id @@     got unsigned int @@
   net/ipv4/ip_output.c:175:33: sparse:     expected restricted __be16 [usertype] id
   net/ipv4/ip_output.c:175:33: sparse:     got unsigned int
   net/ipv4/ip_output.c: note: in included file (through include/net/ip.h):
   include/net/route.h:373:48: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned int [usertype] key @@     got restricted __be32 [usertype] daddr @@
   include/net/route.h:373:48: sparse:     expected unsigned int [usertype] key
   include/net/route.h:373:48: sparse:     got restricted __be32 [usertype] daddr
   include/net/route.h:373:48: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned int [usertype] key @@     got restricted __be32 [usertype] daddr @@
   include/net/route.h:373:48: sparse:     expected unsigned int [usertype] key
   include/net/route.h:373:48: sparse:     got restricted __be32 [usertype] daddr

vim +175 net/ipv4/ip_output.c

   140	
   141	/*
   142	 *		Add an ip header to a skbuff and send it out.
   143	 *
   144	 */
   145	int ip_build_and_send_pkt(struct sk_buff *skb, const struct sock *sk,
   146				  __be32 saddr, __be32 daddr, struct ip_options_rcu *opt,
   147				  u8 tos)
   148	{
   149		struct inet_sock *inet = inet_sk(sk);
   150		struct rtable *rt = skb_rtable(skb);
   151		struct net *net = sock_net(sk);
   152		struct iphdr *iph;
   153	
   154		/* Build the IP header. */
   155		skb_push(skb, sizeof(struct iphdr) + (opt ? opt->opt.optlen : 0));
   156		skb_reset_network_header(skb);
   157		iph = ip_hdr(skb);
   158		iph->version  = 4;
   159		iph->ihl      = 5;
   160		iph->tos      = tos;
   161		iph->ttl      = ip_select_ttl(inet, &rt->dst);
   162		iph->daddr    = (opt && opt->opt.srr ? opt->opt.faddr : daddr);
   163		iph->saddr    = saddr;
   164		iph->protocol = sk->sk_protocol;
   165		/* Do not bother generating IPID for small packets (eg SYNACK) */
   166		if (skb->len <= IPV4_MIN_MTU || ip_dont_fragment(sk, &rt->dst)) {
   167			iph->frag_off = htons(IP_DF);
   168			iph->id = 0;
   169		} else {
   170			iph->frag_off = 0;
   171			/* TCP packets here are SYNACK with fat IPv4/TCP options.
   172			 * Avoid using the hashed IP ident generator.
   173			 */
   174			if (sk->sk_protocol == IPPROTO_TCP)
 > 175				iph->id = prandom_u32();
   176			else
   177				__ip_select_ident(net, iph, 1);
   178		}
   179	
   180		if (opt && opt->opt.optlen) {
   181			iph->ihl += opt->opt.optlen>>2;
   182			ip_options_build(skb, &opt->opt, daddr, rt, 0);
   183		}
   184	
   185		skb->priority = sk->sk_priority;
   186		if (!skb->mark)
   187			skb->mark = sk->sk_mark;
   188	
   189		/* Send it out. */
   190		return ip_local_out(net, skb->sk, skb);
   191	}
   192	EXPORT_SYMBOL_GPL(ip_build_and_send_pkt);
   193	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Eric Dumazet Jan. 27, 2022, 1:02 a.m. UTC | #4 
On Wed, Jan 26, 2022 at 4:56 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 26 Jan 2022 12:05:17 -0800 Eric Dumazet wrote:
> > +             /* TCP packets here are SYNACK with fat IPv4/TCP options.
> > +              * Avoid using the hashed IP ident generator.
> > +              */
> > +             if (sk->sk_protocol == IPPROTO_TCP)
> > +                     iph->id = prandom_u32();
>
> Is it worth marking this as (__force __be32) to avoid the false
> positive sparse warning?

Sure thing, we can add this (I guess this needs to be __be16 ?)
diff mbox series

Patch

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index e331c8d4e6cfc4f2199a7877d8257b3b3b519561..6529484e8a36e1d9aef942879a5d2d18cecf2dc9 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -162,12 +162,19 @@  int ip_build_and_send_pkt(struct sk_buff *skb, const struct sock *sk,
 	iph->daddr    = (opt && opt->opt.srr ? opt->opt.faddr : daddr);
 	iph->saddr    = saddr;
 	iph->protocol = sk->sk_protocol;
-	if (ip_dont_fragment(sk, &rt->dst)) {
+	/* Do not bother generating IPID for small packets (eg SYNACK) */
+	if (skb->len <= IPV4_MIN_MTU || ip_dont_fragment(sk, &rt->dst)) {
 		iph->frag_off = htons(IP_DF);
 		iph->id = 0;
 	} else {
 		iph->frag_off = 0;
-		__ip_select_ident(net, iph, 1);
+		/* TCP packets here are SYNACK with fat IPv4/TCP options.
+		 * Avoid using the hashed IP ident generator.
+		 */
+		if (sk->sk_protocol == IPPROTO_TCP)
+			iph->id = prandom_u32();
+		else
+			__ip_select_ident(net, iph, 1);
 	}
 
 	if (opt && opt->opt.optlen) {