From patchwork Tue Jan 15 08:55:17 2019
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
X-Patchwork-Submitter: David Marchand <david.marchand@redhat.com>
X-Patchwork-Id: 1025065
Return-Path: <ovs-dev-bounces@openvswitch.org>
X-Original-To: incoming@patchwork.ozlabs.org
Delivered-To: patchwork-incoming@bilbo.ozlabs.org
Authentication-Results: ozlabs.org;
	spf=pass (mailfrom) smtp.mailfrom=openvswitch.org
	(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;
	envelope-from=ovs-dev-bounces@openvswitch.org;
	receiver=<UNKNOWN>)
Authentication-Results: ozlabs.org;
	dmarc=fail (p=none dis=none) header.from=redhat.com
Received: from mail.linuxfoundation.org (mail.linuxfoundation.org
	[140.211.169.12])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256
	bits)) (No client certificate requested)
	by ozlabs.org (Postfix) with ESMTPS id 43f4Th6jVwz9sCh
	for <incoming@patchwork.ozlabs.org>;
	Tue, 15 Jan 2019 20:16:40 +1100 (AEDT)
Received: from mail.linux-foundation.org (localhost [127.0.0.1])
	by mail.linuxfoundation.org (Postfix) with ESMTP id 6D75D3633;
	Tue, 15 Jan 2019 09:15:58 +0000 (UTC)
X-Original-To: dev@openvswitch.org
Delivered-To: ovs-dev@mail.linuxfoundation.org
Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org
	[172.17.192.35])
	by mail.linuxfoundation.org (Postfix) with ESMTPS id F2FDD3406
	for <dev@openvswitch.org>; Tue, 15 Jan 2019 08:55:28 +0000 (UTC)
X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
	by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 291C1466
	for <dev@openvswitch.org>; Tue, 15 Jan 2019 08:55:28 +0000 (UTC)
Received: from smtp.corp.redhat.com
	(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mx1.redhat.com (Postfix) with ESMTPS id 94FFC85A07;
	Tue, 15 Jan 2019 08:55:27 +0000 (UTC)
Received: from dmarchan.remote.csb (ovpn-117-97.ams2.redhat.com
	[10.36.117.97])
	by smtp.corp.redhat.com (Postfix) with ESMTP id A8B5D6031B;
	Tue, 15 Jan 2019 08:55:26 +0000 (UTC)
From: David Marchand <david.marchand@redhat.com>
To: dev@openvswitch.org
Date: Tue, 15 Jan 2019 09:55:17 +0100
Message-Id: <1547542519-26269-2-git-send-email-david.marchand@redhat.com>
In-Reply-To: <1547542519-26269-1-git-send-email-david.marchand@redhat.com>
References: <1547047989-28356-1-git-send-email-david.marchand@redhat.com>
	<1547542519-26269-1-git-send-email-david.marchand@redhat.com>
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16
	(mx1.redhat.com [10.5.110.26]);
	Tue, 15 Jan 2019 08:55:27 +0000 (UTC)
X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_HI
	autolearn=ham version=3.3.1
X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on
	smtp1.linux-foundation.org
Subject: [ovs-dev] [PATCH v4 1/3] conntrack: fix tcp seq adjustments when
	mangling commands
X-BeenThere: ovs-dev@openvswitch.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: <ovs-dev.openvswitch.org>
List-Unsubscribe: <https://mail.openvswitch.org/mailman/options/ovs-dev>,
	<mailto:ovs-dev-request@openvswitch.org?subject=unsubscribe>
List-Archive: <http://mail.openvswitch.org/pipermail/ovs-dev/>
List-Post: <mailto:ovs-dev@openvswitch.org>
List-Help: <mailto:ovs-dev-request@openvswitch.org?subject=help>
List-Subscribe: <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>,
	<mailto:ovs-dev-request@openvswitch.org?subject=subscribe>
MIME-Version: 1.0
Sender: ovs-dev-bounces@openvswitch.org
Errors-To: ovs-dev-bounces@openvswitch.org

The ftp alg deals with packets in two ways for the command connection:
either they are inspected and can be mangled when nat is enabled
(CT_FTP_CTL_INTEREST) or they just go through without being modified
(CT_FTP_CTL_OTHER).

For CT_FTP_CTL_INTEREST packets, we must both adjust the packet tcp seq
number by the connection current offset, then prepare for the next
packets by setting an accumulated offset in the ct object.  However,
this was not done for multiple CT_FTP_CTL_INTEREST packets for the same
connection.
This is relevant for handling multiple child data connections that also
need natting.

The tests are updated so that some ftp+NAT tests send multiple port
commands or other similar commands for a single control connection.
Wget is not able to do this, so switch to lftp.

Co-authored-by: Darrell Ball <dlu998@gmail.com>
Signed-off-by: Darrell Ball <dlu998@gmail.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changelog since v3:
- added two more 'ls' commands in the ftp command connection
- compressed two lines wrt the ip_len variable

Changelog since v2:
- skip tests relying on lftp when absent
- removed unneeded temp seq_skew variable by moving conn_seq_skew_set
  at the end of the function and rely on the value in the conn object,
  then removed unneeded do_seq_skew_adj

---
 Vagrantfile             |  9 ++++--
 Vagrantfile-FreeBSD     |  2 +-
 lib/conntrack.c         | 74 ++++++++++++++++++++++++-------------------------
 tests/atlocal.in        |  3 ++
 tests/system-traffic.at | 14 +++++++++-
 5 files changed, 59 insertions(+), 43 deletions(-)

diff --git a/Vagrantfile b/Vagrantfile
index 0192f66..fbd772a 100644
--- a/Vagrantfile
+++ b/Vagrantfile
@@ -12,7 +12,8 @@ dnf -y install autoconf automake openssl-devel libtool \
                python-twisted python-zope-interface \
                desktop-file-utils groff graphviz rpmdevtools nc curl \
                wget python-six pyftpdlib checkpolicy selinux-policy-devel \
-               libcap-ng-devel kernel-devel-`uname -r` ethtool python-tftpy
+               libcap-ng-devel kernel-devel-`uname -r` ethtool python-tftpy \
+               lftp
 echo "search extra update built-in" >/etc/depmod.d/search_path.conf
 SCRIPT
 
@@ -28,7 +29,8 @@ aptitude -y install -R \
                 wget python-six ethtool \
                 libcap-ng-dev libssl-dev python-dev openssl \
                 python-pyftpdlib python-flake8 python-tftpy \
-                linux-headers-`uname -r`
+                linux-headers-`uname -r` \
+                lftp
 SCRIPT
 
 $bootstrap_centos = <<SCRIPT
@@ -37,7 +39,8 @@ yum -y install autoconf automake openssl-devel libtool \
                python-twisted-core python-zope-interface \
                desktop-file-utils groff graphviz rpmdevtools nc curl \
                wget python-six pyftpdlib checkpolicy selinux-policy-devel \
-               libcap-ng-devel kernel-devel-`uname -r` ethtool net-tools
+               libcap-ng-devel kernel-devel-`uname -r` ethtool net-tools \
+               lftp
 SCRIPT
 
 $configure_ovs = <<SCRIPT
diff --git a/Vagrantfile-FreeBSD b/Vagrantfile-FreeBSD
index 8f00abe..52599ee 100644
--- a/Vagrantfile-FreeBSD
+++ b/Vagrantfile-FreeBSD
@@ -12,7 +12,7 @@ Vagrant.require_version ">=1.7.0"
 $bootstrap_freebsd = <<SCRIPT
 sed  -e 's/\#DEFAULT_ALWAYS_YES = false/DEFAULT_ALWAYS_YES = true/g' -e 's/\#ASSUME_ALWAYS_YES = false/ASSUME_ALWAYS_YES = true/g' /usr/local/etc/pkg.conf > /tmp/pkg.conf
 mv -f /tmp/pkg.conf /usr/local/etc/pkg.conf
-pkg install automake libtool wget python py27-six gmake
+pkg install automake libtool wget python py27-six gmake lftp
 SCRIPT
 
 $configure_ovs = <<SCRIPT
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 6f6021a..ae549bd 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -3170,9 +3170,8 @@ repl_ftp_v6_addr(struct dp_packet *pkt, struct ct_addr v6_addr_rep,
 
 static void
 handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
-               struct dp_packet *pkt,
-               const struct conn *conn_for_expectation,
-               long long now, enum ftp_ctl_pkt ftp_ctl, bool nat)
+               struct dp_packet *pkt, const struct conn *ec, long long now,
+               enum ftp_ctl_pkt ftp_ctl, bool nat)
 {
     struct ip_header *l3_hdr = dp_packet_l3(pkt);
     ovs_be32 v4_addr_rep = 0;
@@ -3180,31 +3179,24 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
     size_t addr_offset_from_ftp_data_start = 0;
     size_t addr_size = 0;
     char *ftp_data_start;
-    bool do_seq_skew_adj = true;
     enum ct_alg_mode mode = CT_FTP_MODE_ACTIVE;
 
     if (detect_ftp_ctl_type(ctx, pkt) != ftp_ctl) {
         return;
     }
 
-    if (!nat || !conn_for_expectation->seq_skew) {
-        do_seq_skew_adj = false;
-    }
-
     struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt);
     int64_t seq_skew = 0;
 
-    if (ftp_ctl == CT_FTP_CTL_OTHER) {
-        seq_skew = conn_for_expectation->seq_skew;
-    } else if (ftp_ctl == CT_FTP_CTL_INTEREST) {
+    if (ftp_ctl == CT_FTP_CTL_INTEREST) {
         enum ftp_ctl_pkt rc;
         if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
-            rc = process_ftp_ctl_v6(ct, pkt, conn_for_expectation,
+            rc = process_ftp_ctl_v6(ct, pkt, ec,
                                     &v6_addr_rep, &ftp_data_start,
                                     &addr_offset_from_ftp_data_start,
                                     &addr_size, &mode);
         } else {
-            rc = process_ftp_ctl_v4(ct, pkt, conn_for_expectation,
+            rc = process_ftp_ctl_v4(ct, pkt, ec,
                                     &v4_addr_rep, &ftp_data_start,
                                     &addr_offset_from_ftp_data_start);
         }
@@ -3217,62 +3209,63 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
             uint16_t ip_len;
 
             if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
-                seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, ftp_data_start,
-                                            addr_offset_from_ftp_data_start,
-                                            addr_size, mode);
+                if (nat) {
+                    seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep,
+                                   ftp_data_start,
+                                   addr_offset_from_ftp_data_start,
+                                   addr_size, mode);
+                }
+
                 if (seq_skew) {
-                    ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen);
-                    ip_len += seq_skew;
+                    ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen) +
+                        seq_skew;
                     nh6->ip6_ctlun.ip6_un1.ip6_un1_plen = htons(ip_len);
-                    conn_seq_skew_set(ct, &conn_for_expectation->key, now,
-                                      seq_skew, ctx->reply);
                 }
             } else {
-                seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep, ftp_data_start,
-                                            addr_offset_from_ftp_data_start);
-                ip_len = ntohs(l3_hdr->ip_tot_len);
+                if (nat) {
+                    seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep,
+                                   ftp_data_start,
+                                   addr_offset_from_ftp_data_start);
+                }
                 if (seq_skew) {
-                    ip_len += seq_skew;
+                    ip_len = ntohs(l3_hdr->ip_tot_len) + seq_skew;
                     l3_hdr->ip_csum = recalc_csum16(l3_hdr->ip_csum,
                                           l3_hdr->ip_tot_len, htons(ip_len));
                     l3_hdr->ip_tot_len = htons(ip_len);
-                    conn_seq_skew_set(ct, &conn_for_expectation->key, now,
-                                      seq_skew, ctx->reply);
                 }
             }
         } else {
             OVS_NOT_REACHED();
         }
-    } else {
-        OVS_NOT_REACHED();
     }
 
     struct tcp_header *th = dp_packet_l4(pkt);
 
-    if (do_seq_skew_adj && seq_skew != 0) {
-        if (ctx->reply != conn_for_expectation->seq_skew_dir) {
+    if (nat && ec->seq_skew != 0) {
+        if (ctx->reply != ec->seq_skew_dir) {
 
             uint32_t tcp_ack = ntohl(get_16aligned_be32(&th->tcp_ack));
 
-            if ((seq_skew > 0) && (tcp_ack < seq_skew)) {
+            if ((ec->seq_skew > 0) && (tcp_ack < ec->seq_skew)) {
                 /* Should not be possible; will be marked invalid. */
                 tcp_ack = 0;
-            } else if ((seq_skew < 0) && (UINT32_MAX - tcp_ack < -seq_skew)) {
-                tcp_ack = (-seq_skew) - (UINT32_MAX - tcp_ack);
+            } else if ((ec->seq_skew < 0) &&
+                       (UINT32_MAX - tcp_ack < -ec->seq_skew)) {
+                tcp_ack = (-ec->seq_skew) - (UINT32_MAX - tcp_ack);
             } else {
-                tcp_ack -= seq_skew;
+                tcp_ack -= ec->seq_skew;
             }
             ovs_be32 new_tcp_ack = htonl(tcp_ack);
             put_16aligned_be32(&th->tcp_ack, new_tcp_ack);
         } else {
             uint32_t tcp_seq = ntohl(get_16aligned_be32(&th->tcp_seq));
-            if ((seq_skew > 0) && (UINT32_MAX - tcp_seq < seq_skew)) {
-                tcp_seq = seq_skew - (UINT32_MAX - tcp_seq);
-            } else if ((seq_skew < 0) && (tcp_seq < -seq_skew)) {
+            if ((ec->seq_skew > 0) && (UINT32_MAX - tcp_seq < ec->seq_skew)) {
+                tcp_seq = ec->seq_skew - (UINT32_MAX - tcp_seq);
+            } else if ((ec->seq_skew < 0) && (tcp_seq < -ec->seq_skew)) {
                 /* Should not be possible; will be marked invalid. */
                 tcp_seq = 0;
             } else {
-                tcp_seq += seq_skew;
+                tcp_seq += ec->seq_skew;
             }
             ovs_be32 new_tcp_seq = htonl(tcp_seq);
             put_16aligned_be32(&th->tcp_seq, new_tcp_seq);
@@ -3290,6 +3283,11 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
     uint8_t pad = dp_packet_l2_pad_size(pkt);
     th->tcp_csum = csum_finish(
         csum_continue(tcp_csum, th, tail - (char *) th - pad));
+
+    if (seq_skew) {
+        conn_seq_skew_set(ct, &ec->key, now, seq_skew + ec->seq_skew,
+                          ctx->reply);
+    }
 }
 
 static void
diff --git a/tests/atlocal.in b/tests/atlocal.in
index 388f79e..5eff0a0 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -193,6 +193,9 @@ fi
 # Set HAVE_TCPDUMP
 find_command tcpdump
 
+# Set HAVE_LFTP
+find_command lftp
+
 CURL_OPT="-g -v --max-time 1 --retry 2 --retry-delay 1 --connect-timeout 1"
 
 # Determine whether "diff" supports "normal" diffs.  (busybox diff does not.)
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index b2ab800..14ce7d2 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -4226,6 +4226,7 @@ dnl NAT, using the provided flow table.
 m4_define([CHECK_FTP_NAT],
    [AT_SETUP([conntrack - FTP NAT $1])
     AT_SKIP_IF([test $HAVE_FTP = no])
+    AT_SKIP_IF([test $HAVE_LFTP = no])
     CHECK_CONNTRACK()
     CHECK_CONNTRACK_NAT()
     CHECK_CONNTRACK_ALG()
@@ -4246,7 +4247,18 @@ m4_define([CHECK_FTP_NAT],
     OVS_START_L7([at_ns1], [ftp])
 
     dnl FTP requests from p0->p1 should work fine.
-    NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.2 -4 --no-passive-ftp -t 3 -T 1 --retry-connrefused -v --server-response --no-remove-listing -o wget0.log -d])
+    AT_DATA([ftp.cmd], [dnl
+set net:max-retries 1
+set net:timeout 1
+set ftp:passive-mode off
+cache off
+connect ftp://anonymous:@10.1.1.2
+ls
+ls
+ls
+ls
+])
+    NS_CHECK_EXEC([at_ns0], [lftp -f ftp.cmd > lftp.log])
 
     dnl Discards CLOSE_WAIT and CLOSING
     AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [$4])