[PATCH] tcp: verify the checksum of the first data segment in a new connection

[PATCH] tcp: verify the checksum of the first data segment in a new connection

[Frank van der Linden]

commit 079096f103fa ("tcp/dccp: install syn_recv requests into ehash
table") introduced an optimization for the handling of child sockets
created for a new TCP connection.

But this optimization passes any data associated with the last ACK of the
connection handshake up the stack without verifying its checksum, because it
calls tcp_child_process(), which in turn calls tcp_rcv_state_process()
directly.  These lower-level processing functions do not do any checksum
verification.

Insert a tcp_checksum_complete call in the TCP_NEW_SYN_RECEIVE path to
fix this.

Signed-off-by: Frank van der Linden <fllinden@amazon.com>
---
 net/ipv4/tcp_ipv4.c | 8 +++++++-
 net/ipv6/tcp_ipv6.c | 8 +++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index f70586b50838..1ec4c0d4aba5 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1703,7 +1703,13 @@ int tcp_v4_rcv(struct sk_buff *skb)
 			th = (const struct tcphdr *)skb->data;
 			iph = ip_hdr(skb);
 			tcp_v4_fill_cb(skb, iph, th);
-			nsk = tcp_check_req(sk, skb, req, false, &req_stolen);
+
+			if (tcp_checksum_complete(skb)) {
+				__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
+			} else {
+				nsk = tcp_check_req(sk, skb, req, false,
+						    &req_stolen);
+			}
 		}
 		if (!nsk) {
 			reqsk_put(req);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 6d664d83cd16..a12b694d3d1e 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1486,7 +1486,13 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 			th = (const struct tcphdr *)skb->data;
 			hdr = ipv6_hdr(skb);
 			tcp_v6_fill_cb(skb, hdr, th);
-			nsk = tcp_check_req(sk, skb, req, false, &req_stolen);
+
+			if (tcp_checksum_complete(skb)) {
+				__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
+			} else {
+				nsk = tcp_check_req(sk, skb, req, false,
+						    &req_stolen);
+			}
 		}
 		if (!nsk) {
 			reqsk_put(req);
-- 
2.14.4

Re: [PATCH] tcp: verify the checksum of the first data segment in a new connection

[van der Linden, Frank]

A few comments on this one:

- obviously this is fairly serious, as it can let corrupted data all the way up to the application
- I am not nuts about the patch itself, the code feels a bit cluttered, but it's the least invasive way
  I could think of. Probably some refactoring is needed at some point.
- here's how to easily reproduce it:

On the sender, set up artificial packet corruption:

#!/bin/sh
tc qdisc add dev eth0 root handle 1: prio
tc qdisc add dev eth0 parent 1:3 netem corrupt 50.0%
tc filter add dev eth0 protocol ip parent 1:0 prio 3 u32 match ip dst $DSTADDR flowid 10:3


Then, run the following on the sender:

while :; do dd if=/dev/zero of=/dev/stdout bs=4096 count=4 | nc $DSTADDR 10000; sleep 1; done

..and this on the receiver:

uname -r; grep ^Tcp /proc/net/snmp; ttl=$((${SECONDS} + 300)); while [[ ${SECONDS} -lt ${ttl} ]]; do out="foo.$(date +%s)"; nc -l 10000 > "${out}"; md5=$(md5sum "${out}"|cut -d\  -f 1); echo -n "${md5}"; if [[ "${md5}" != "ce338fe6899778aacfc28414f2d9498b" ]]; then echo " corrupted"; else echo; fi; done; grep ^Tcp /proc/net/snmp

[obviously, if you change the size / content, the md5 sum has to be changed here]

This reproduces it fairly quickly (within 20 seconds) for us, on 4.14.x kernels. 4.17 kernels were verified to still have the issue.

Frank

On 6/11/18, 4:15 PM, "Frank van der Linden" <fllinden@amazon.com> wrote:

    commit 079096f103fa ("tcp/dccp: install syn_recv requests into ehash
    table") introduced an optimization for the handling of child sockets
    created for a new TCP connection.
    
    But this optimization passes any data associated with the last ACK of the
    connection handshake up the stack without verifying its checksum, because it
    calls tcp_child_process(), which in turn calls tcp_rcv_state_process()
    directly.  These lower-level processing functions do not do any checksum
    verification.
    
    Insert a tcp_checksum_complete call in the TCP_NEW_SYN_RECEIVE path to
    fix this.
    
    Signed-off-by: Frank van der Linden <fllinden@amazon.com>
    ---
     net/ipv4/tcp_ipv4.c | 8 +++++++-
     net/ipv6/tcp_ipv6.c | 8 +++++++-
     2 files changed, 14 insertions(+), 2 deletions(-)
    
    diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
    index f70586b50838..1ec4c0d4aba5 100644
    --- a/net/ipv4/tcp_ipv4.c
    +++ b/net/ipv4/tcp_ipv4.c
    @@ -1703,7 +1703,13 @@ int tcp_v4_rcv(struct sk_buff *skb)
     			th = (const struct tcphdr *)skb->data;
     			iph = ip_hdr(skb);
     			tcp_v4_fill_cb(skb, iph, th);
    -			nsk = tcp_check_req(sk, skb, req, false, &req_stolen);
    +
    +			if (tcp_checksum_complete(skb)) {
    +				__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
    +			} else {
    +				nsk = tcp_check_req(sk, skb, req, false,
    +						    &req_stolen);
    +			}
     		}
     		if (!nsk) {
     			reqsk_put(req);
    diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
    index 6d664d83cd16..a12b694d3d1e 100644
    --- a/net/ipv6/tcp_ipv6.c
    +++ b/net/ipv6/tcp_ipv6.c
    @@ -1486,7 +1486,13 @@ static int tcp_v6_rcv(struct sk_buff *skb)
     			th = (const struct tcphdr *)skb->data;
     			hdr = ipv6_hdr(skb);
     			tcp_v6_fill_cb(skb, hdr, th);
    -			nsk = tcp_check_req(sk, skb, req, false, &req_stolen);
    +
    +			if (tcp_checksum_complete(skb)) {
    +				__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
    +			} else {
    +				nsk = tcp_check_req(sk, skb, req, false,
    +						    &req_stolen);
    +			}
     		}
     		if (!nsk) {
     			reqsk_put(req);
    -- 
    2.14.4
    
    

Re: [PATCH] tcp: verify the checksum of the first data segment in a new connection

[Eric Dumazet]

On 06/11/2018 04:25 PM, van der Linden, Frank wrote:
> A few comments on this one:
> 
> - obviously this is fairly serious, as it can let corrupted data all the way up to the application

Sure, although anyone relying on CRC checksum for ensuring TCP data integrity
has big troubles ;)

I would rather have a refined version of this patch doing a "goto csum_error" 
so that we properly increment TCP_MIB_CSUMERRORS and TCP_MIB_INERRS 

Thanks !

Re: [PATCH] tcp: verify the checksum of the first data segment in a new connection

[van der Linden, Frank]

Yeah, true, it's missing INERRS in this case. I'll fix it up a bit.

Frank

On 6/11/18, 4:38 PM, "Eric Dumazet" <eric.dumazet@gmail.com> wrote:

    
    
    On 06/11/2018 04:25 PM, van der Linden, Frank wrote:
    > A few comments on this one:
    > 
    > - obviously this is fairly serious, as it can let corrupted data all the way up to the application
    
    Sure, although anyone relying on CRC checksum for ensuring TCP data integrity
    has big troubles ;)
    
    I would rather have a refined version of this patch doing a "goto csum_error" 
    so that we properly increment TCP_MIB_CSUMERRORS and TCP_MIB_INERRS 
    
    Thanks !
    
    

Re: [PATCH] tcp: verify the checksum of the first data segment in a new connection

[van der Linden, Frank]

Resubmitted. The various release/deref requirements in that path make a straight "goto csum_error" impossible without duplicating some lines, but this is 2nd best.

Frank

On 6/11/18, 4:43 PM, "van der Linden, Frank" <fllinden@amazon.com> wrote:

    Yeah, true, it's missing INERRS in this case. I'll fix it up a bit.
    
    Frank
    
    On 6/11/18, 4:38 PM, "Eric Dumazet" <eric.dumazet@gmail.com> wrote:
    
        
        
        On 06/11/2018 04:25 PM, van der Linden, Frank wrote:
        > A few comments on this one:
        > 
        > - obviously this is fairly serious, as it can let corrupted data all the way up to the application
        
        Sure, although anyone relying on CRC checksum for ensuring TCP data integrity
        has big troubles ;)
        
        I would rather have a refined version of this patch doing a "goto csum_error" 
        so that we properly increment TCP_MIB_CSUMERRORS and TCP_MIB_INERRS 
        
        Thanks !