| Message ID | 1545312814-23634-2-git-send-email-david.marchand@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fixes for ftp alg in userspace dp | expand |
Thanks for the multiple seq_skew changes David. Its easy enough to support although I wonder if it is standard and the added usefulness seems minimal. comments inline On Thu, Dec 20, 2018 at 5:33 AM David Marchand <david.marchand@redhat.com> wrote: > 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. > > The tests are updated so that ftp+NAT checks send multiple commands in a > single tcp command connection: wget is not able to do this, so switch to > lftp. > > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > Vagrantfile | 9 ++++++--- > Vagrantfile-FreeBSD | 2 +- > lib/conntrack.c | 52 > ++++++++++++++++++++++++++----------------------- > tests/system-traffic.at | 11 ++++++++++- > 4 files changed, 45 insertions(+), 29 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..2e4141a 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -3214,30 +3214,34 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > pkt->md.ct_state |= CS_TRACKED | CS_INVALID; > return; > } else if (rc == CT_FTP_CTL_INTEREST) { > - 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 (seq_skew) { > - ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); > - ip_len += 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 (seq_skew) { > - ip_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); > + seq_skew = conn_for_expectation->seq_skew; > + if (nat) { > + uint16_t ip_len; > + int64_t new_skew; > + > + if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { > + new_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, > ftp_data_start, > + > addr_offset_from_ftp_data_start, > + addr_size, mode); > + if (new_skew) { > + ip_len = > ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); > + ip_len += new_skew; > + nh6->ip6_ctlun.ip6_un1.ip6_un1_plen = > htons(ip_len); > + conn_seq_skew_set(ct, &conn_for_expectation->key, > now, > + new_skew + seq_skew, > ctx->reply); > + } > + } else { > + new_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 (new_skew) { > + ip_len += new_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, > + new_skew + seq_skew, > ctx->reply); > + } > } > } > } else { > 1/ I don't think we need another local variable since conn_for_expectation->seq_skew keeps the previous seq_skew, so we can just use it. 2/ The "nat" check is semantically correct around the repl_ftp_v4/6_addr() calls if (nat) { seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep, ftp_data_start, addr_offset_from_ftp_data_start); } This yields: @@ -3217,27 +3225,37 @@ 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; 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); + seq_skew + conn_for_expectation->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 = ntohs(l3_hdr->ip_tot_len); ip_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); + seq_skew + conn_for_expectation->seq_skew, + ctx->reply); } } } else { (END) > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 4c52431..3734497 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -4240,7 +4240,16 @@ 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 > +]) > + NS_CHECK_EXEC([at_ns0], [lftp -f ftp.cmd > lftp.log]) > A check is needed for running outside of vagrant which is how I ran it: see atlocal.in and add "find_command lftp" then check for it in system_traffic.at > > dnl Discards CLOSE_WAIT and CLOSING > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], > [0], [$4]) > -- > 1.8.3.1 > >
On Tue, Jan 8, 2019 at 5:49 AM Darrell Ball <dlu998@gmail.com> wrote: > On Thu, Dec 20, 2018 at 5:33 AM David Marchand <david.marchand@redhat.com> > wrote: > >> 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. >> >> The tests are updated so that ftp+NAT checks send multiple commands in a >> single tcp command connection: wget is not able to do this, so switch to >> lftp. >> >> Signed-off-by: David Marchand <david.marchand@redhat.com> >> --- >> Vagrantfile | 9 ++++++--- >> Vagrantfile-FreeBSD | 2 +- >> lib/conntrack.c | 52 >> ++++++++++++++++++++++++++----------------------- >> tests/system-traffic.at | 11 ++++++++++- >> 4 files changed, 45 insertions(+), 29 deletions(-) >> >> >> [snip] > diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 6f6021a..2e4141a 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -3214,30 +3214,34 @@ handle_ftp_ctl(struct conntrack *ct, const struct >> conn_lookup_ctx *ctx, >> pkt->md.ct_state |= CS_TRACKED | CS_INVALID; >> return; >> } else if (rc == CT_FTP_CTL_INTEREST) { >> - 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 (seq_skew) { >> - ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); >> - ip_len += 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 (seq_skew) { >> - ip_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); >> + seq_skew = conn_for_expectation->seq_skew; >> + if (nat) { >> + uint16_t ip_len; >> + int64_t new_skew; >> + >> + if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { >> + new_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, >> ftp_data_start, >> + >> addr_offset_from_ftp_data_start, >> + addr_size, mode); >> + if (new_skew) { >> + ip_len = >> ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); >> + ip_len += new_skew; >> + nh6->ip6_ctlun.ip6_un1.ip6_un1_plen = >> htons(ip_len); >> + conn_seq_skew_set(ct, >> &conn_for_expectation->key, now, >> + new_skew + seq_skew, >> ctx->reply); >> + } >> + } else { >> + new_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 (new_skew) { >> + ip_len += new_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, >> + new_skew + seq_skew, >> ctx->reply); >> + } >> } >> } >> } else { >> > > > 1/ I don't think we need another local variable > since conn_for_expectation->seq_skew > keeps the previous seq_skew, so we can just use it. > Nop, because we need both the current "tcp skew" value to mangle the current packet tcp seq number and an updated value to update the ip length etc... and store the new value in the conn object for next packets. See below. > 2/ The "nat" check is semantically correct around the repl_ftp_v4/6_addr() > calls > > if (nat) { > seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep, > ftp_data_start, > addr_offset_from_ftp_data_start); > } > > This yields: > > @@ -3217,27 +3225,37 @@ 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; > 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); > + seq_skew + conn_for_expectation->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 = ntohs(l3_hdr->ip_tot_len); > ip_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); > + seq_skew + conn_for_expectation->seq_skew, > + ctx->reply); > } > } > } else { > (END) > Let me take an example with the test CHECK_FTP_SNAT_POST_RECIRC() + address 10.1.1.240. When dealing with the first mangle operation (client sending its first "PORT xxx" for the first "ls" command), the conn tcp seq skew value is at 0. repl_ftp_v4_addr() returns 2 since it replaced 10.1.1.1 with 10.1.1.240. The tcp seq numbers is kept untouched in the "if (do_seq_skew_adj && seq_skew != 0)" block later in this function. When dealing with the second mangle operation (second "PORT xxx"), the current conn tcp skew value is at 2. repl_ftp_v4_addr() returns again 2 since it replaced 10.1.1.1 with 10.1.1.240. The tcp seq number is updated by 2 in the "if (do_seq_skew_adj && seq_skew != 0)" block later in this function. Now, you could argue that the code you propose can work so far, but if we add a new command again in this session, the tcp seq numbers sequence is broken. The command packet tcp seq number would be updated by the repl_ftp_v4_addr() return value 2 and not by the accumulated value which would be 4 for it to work. diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index 4c52431..3734497 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -4240,7 +4240,16 @@ 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 >> +]) >> + NS_CHECK_EXEC([at_ns0], [lftp -f ftp.cmd > lftp.log]) >> > > A check is needed for running outside of vagrant which is how I ran it: > > see atlocal.in and add "find_command lftp" > then check for it in system_traffic.at > > Yes, done.
On Tue, Jan 8, 2019 at 2:13 AM David Marchand <david.marchand@redhat.com> wrote: > On Tue, Jan 8, 2019 at 5:49 AM Darrell Ball <dlu998@gmail.com> wrote: > >> On Thu, Dec 20, 2018 at 5:33 AM David Marchand <david.marchand@redhat.com> >> wrote: >> >>> 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. >>> >>> The tests are updated so that ftp+NAT checks send multiple commands in a >>> single tcp command connection: wget is not able to do this, so switch to >>> lftp. >>> >>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>> --- >>> Vagrantfile | 9 ++++++--- >>> Vagrantfile-FreeBSD | 2 +- >>> lib/conntrack.c | 52 >>> ++++++++++++++++++++++++++----------------------- >>> tests/system-traffic.at | 11 ++++++++++- >>> 4 files changed, 45 insertions(+), 29 deletions(-) >>> >>> >>> > [snip] > > >> diff --git a/lib/conntrack.c b/lib/conntrack.c >>> index 6f6021a..2e4141a 100644 >>> --- a/lib/conntrack.c >>> +++ b/lib/conntrack.c >>> @@ -3214,30 +3214,34 @@ handle_ftp_ctl(struct conntrack *ct, const >>> struct conn_lookup_ctx *ctx, >>> pkt->md.ct_state |= CS_TRACKED | CS_INVALID; >>> return; >>> } else if (rc == CT_FTP_CTL_INTEREST) { >>> - 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 (seq_skew) { >>> - ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); >>> - ip_len += 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 (seq_skew) { >>> - ip_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); >>> + seq_skew = conn_for_expectation->seq_skew; >>> + if (nat) { >>> + uint16_t ip_len; >>> + int64_t new_skew; >>> + >>> + if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { >>> + new_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, >>> ftp_data_start, >>> + >>> addr_offset_from_ftp_data_start, >>> + addr_size, mode); >>> + if (new_skew) { >>> + ip_len = >>> ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); >>> + ip_len += new_skew; >>> + nh6->ip6_ctlun.ip6_un1.ip6_un1_plen = >>> htons(ip_len); >>> + conn_seq_skew_set(ct, >>> &conn_for_expectation->key, now, >>> + new_skew + seq_skew, >>> ctx->reply); >>> + } >>> + } else { >>> + new_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 (new_skew) { >>> + ip_len += new_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, >>> + new_skew + seq_skew, >>> ctx->reply); >>> + } >>> } >>> } >>> } else { >>> >> >> >> 1/ I don't think we need another local variable >> since conn_for_expectation->seq_skew >> keeps the previous seq_skew, so we can just use it. >> > > Nop, because we need both the current "tcp skew" value to mangle the > current packet tcp seq number and an updated value to update the ip length > etc... and store the new value in the conn object for next packets. > See below. > > >> 2/ The "nat" check is semantically correct around the >> repl_ftp_v4/6_addr() calls >> >> if (nat) { >> seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep, >> ftp_data_start, >> addr_offset_from_ftp_data_start); >> } >> >> This yields: >> >> @@ -3217,27 +3225,37 @@ 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; >> 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); >> + seq_skew + conn_for_expectation->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 = ntohs(l3_hdr->ip_tot_len); >> ip_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); >> + seq_skew + conn_for_expectation->seq_skew, >> + ctx->reply); >> } >> } >> } else { >> (END) >> > > Let me take an example with the test CHECK_FTP_SNAT_POST_RECIRC() + > address 10.1.1.240. > > When dealing with the first mangle operation (client sending its first > "PORT xxx" for the first "ls" command), the conn tcp seq skew value is at 0. > repl_ftp_v4_addr() returns 2 since it replaced 10.1.1.1 with 10.1.1.240. > The tcp seq numbers is kept untouched in the "if (do_seq_skew_adj && > seq_skew != 0)" block later in this function. > > When dealing with the second mangle operation (second "PORT xxx"), the > current conn tcp skew value is at 2. > repl_ftp_v4_addr() returns again 2 since it replaced 10.1.1.1 with > 10.1.1.240. > The tcp seq number is updated by 2 in the "if (do_seq_skew_adj && seq_skew > != 0)" block later in this function. > > Now, you could argue that the code you propose can work so far, but if we > add a new command again in this session, the tcp seq numbers sequence is > broken. > The command packet tcp seq number would be updated by the > repl_ftp_v4_addr() return value 2 and not by the accumulated value which > would be 4 for it to work. > 1/ Cool; I took a second look. I also increased the "ls" commands to 4 for better testing; see below 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 ]) 2/ I think adding multiple sequence skew support is a mini-feature, even has exploit implications and deserves some refactoring of the function (see below). Sending and handling multiple port cmds in lieu of one, which has the same effect, does not seem like an optimization, although I know lftp is doing it. @@ -3170,9 +3178,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; @@ -3187,24 +3194,22 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, return; } - if (!nat || !conn_for_expectation->seq_skew) { + if (!nat || !ec->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); } @@ -3214,65 +3219,67 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, pkt->md.ct_state |= CS_TRACKED | CS_INVALID; return; } else if (rc == CT_FTP_CTL_INTEREST) { + 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)); 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 (do_seq_skew_adj && 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 +3297,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 (END) > > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>> index 4c52431..3734497 100644 >>> --- a/tests/system-traffic.at >>> +++ b/tests/system-traffic.at >>> @@ -4240,7 +4240,16 @@ 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 >>> +]) >>> + NS_CHECK_EXEC([at_ns0], [lftp -f ftp.cmd > lftp.log]) >>> >> >> A check is needed for running outside of vagrant which is how I ran it: >> >> see atlocal.in and add "find_command lftp" >> then check for it in system_traffic.at >> >> > Yes, done. > > > -- > David Marchand >
On Tue, Jan 8, 2019 at 2:57 PM Darrell Ball <dlu998@gmail.com> wrote: > > > On Tue, Jan 8, 2019 at 2:13 AM David Marchand <david.marchand@redhat.com> > wrote: > >> On Tue, Jan 8, 2019 at 5:49 AM Darrell Ball <dlu998@gmail.com> wrote: >> >>> On Thu, Dec 20, 2018 at 5:33 AM David Marchand < >>> david.marchand@redhat.com> wrote: >>> >>>> 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. >>>> >>>> The tests are updated so that ftp+NAT checks send multiple commands in a >>>> single tcp command connection: wget is not able to do this, so switch to >>>> lftp. >>>> >>>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>>> --- >>>> Vagrantfile | 9 ++++++--- >>>> Vagrantfile-FreeBSD | 2 +- >>>> lib/conntrack.c | 52 >>>> ++++++++++++++++++++++++++----------------------- >>>> tests/system-traffic.at | 11 ++++++++++- >>>> 4 files changed, 45 insertions(+), 29 deletions(-) >>>> >>>> >>>> >> [snip] >> >> >>> diff --git a/lib/conntrack.c b/lib/conntrack.c >>>> index 6f6021a..2e4141a 100644 >>>> --- a/lib/conntrack.c >>>> +++ b/lib/conntrack.c >>>> @@ -3214,30 +3214,34 @@ handle_ftp_ctl(struct conntrack *ct, const >>>> struct conn_lookup_ctx *ctx, >>>> pkt->md.ct_state |= CS_TRACKED | CS_INVALID; >>>> return; >>>> } else if (rc == CT_FTP_CTL_INTEREST) { >>>> - 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 (seq_skew) { >>>> - ip_len = >>>> ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); >>>> - ip_len += 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 (seq_skew) { >>>> - ip_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); >>>> + seq_skew = conn_for_expectation->seq_skew; >>>> + if (nat) { >>>> + uint16_t ip_len; >>>> + int64_t new_skew; >>>> + >>>> + if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { >>>> + new_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, >>>> ftp_data_start, >>>> + >>>> addr_offset_from_ftp_data_start, >>>> + addr_size, mode); >>>> + if (new_skew) { >>>> + ip_len = >>>> ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); >>>> + ip_len += new_skew; >>>> + nh6->ip6_ctlun.ip6_un1.ip6_un1_plen = >>>> htons(ip_len); >>>> + conn_seq_skew_set(ct, >>>> &conn_for_expectation->key, now, >>>> + new_skew + seq_skew, >>>> ctx->reply); >>>> + } >>>> + } else { >>>> + new_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 (new_skew) { >>>> + ip_len += new_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, >>>> + new_skew + seq_skew, >>>> ctx->reply); >>>> + } >>>> } >>>> } >>>> } else { >>>> >>> >>> >>> 1/ I don't think we need another local variable >>> since conn_for_expectation->seq_skew >>> keeps the previous seq_skew, so we can just use it. >>> >> >> Nop, because we need both the current "tcp skew" value to mangle the >> current packet tcp seq number and an updated value to update the ip length >> etc... and store the new value in the conn object for next packets. >> See below. >> >> >>> 2/ The "nat" check is semantically correct around the >>> repl_ftp_v4/6_addr() calls >>> >>> if (nat) { >>> seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep, >>> ftp_data_start, >>> addr_offset_from_ftp_data_start); >>> } >>> >>> This yields: >>> >>> @@ -3217,27 +3225,37 @@ 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; >>> 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); >>> + seq_skew + conn_for_expectation->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 = ntohs(l3_hdr->ip_tot_len); >>> ip_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); >>> + seq_skew + conn_for_expectation->seq_skew, >>> + ctx->reply); >>> } >>> } >>> } else { >>> (END) >>> >> >> Let me take an example with the test CHECK_FTP_SNAT_POST_RECIRC() + >> address 10.1.1.240. >> >> When dealing with the first mangle operation (client sending its first >> "PORT xxx" for the first "ls" command), the conn tcp seq skew value is at 0. >> repl_ftp_v4_addr() returns 2 since it replaced 10.1.1.1 with 10.1.1.240. >> The tcp seq numbers is kept untouched in the "if (do_seq_skew_adj && >> seq_skew != 0)" block later in this function. >> >> When dealing with the second mangle operation (second "PORT xxx"), the >> current conn tcp skew value is at 2. >> repl_ftp_v4_addr() returns again 2 since it replaced 10.1.1.1 with >> 10.1.1.240. >> The tcp seq number is updated by 2 in the "if (do_seq_skew_adj && >> seq_skew != 0)" block later in this function. >> >> Now, you could argue that the code you propose can work so far, but if we >> add a new command again in this session, the tcp seq numbers sequence is >> broken. >> The command packet tcp seq number would be updated by the >> repl_ftp_v4_addr() return value 2 and not by the accumulated value which >> would be 4 for it to work. >> > > > 1/ Cool; I took a second look. > I also increased the "ls" commands to 4 for better testing; see below > > 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 > ]) > > 2/ I think adding multiple sequence skew support is a mini-feature, even > has exploit implications > and deserves some refactoring of the function (see below). Sending > and handling multiple port cmds > in lieu of one, which has the same effect, does not seem like an > optimization, although I know lftp > is doing it. > > @@ -3170,9 +3178,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; > @@ -3187,24 +3194,22 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > return; > } > > - if (!nat || !conn_for_expectation->seq_skew) { > + if (!nat || !ec->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); > } > @@ -3214,65 +3219,67 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > pkt->md.ct_state |= CS_TRACKED | CS_INVALID; > return; > } else if (rc == CT_FTP_CTL_INTEREST) { > + > 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)); > 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 (do_seq_skew_adj && ec->seq_skew != 0) { > s/if (do_seq_skew_adj && ec->seq_skew != 0) {/if (do_seq_skew_adj) {/ since "ec->seq_skew != 0" is now redundant > + 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 +3297,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 > (END) > > > > >> >> >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>>> index 4c52431..3734497 100644 >>>> --- a/tests/system-traffic.at >>>> +++ b/tests/system-traffic.at >>>> @@ -4240,7 +4240,16 @@ 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 >>>> +]) >>>> + NS_CHECK_EXEC([at_ns0], [lftp -f ftp.cmd > lftp.log]) >>>> >>> >>> A check is needed for running outside of vagrant which is how I ran it: >>> >>> see atlocal.in and add "find_command lftp" >>> then check for it in system_traffic.at >>> >>> >> Yes, done. >> >> >> -- >> David Marchand >> >
Hello, On Wed, Jan 9, 2019 at 4:51 AM Darrell Ball <dlu998@gmail.com> wrote: > On Tue, Jan 8, 2019 at 2:57 PM Darrell Ball <dlu998@gmail.com> wrote: > >> On Tue, Jan 8, 2019 at 2:13 AM David Marchand <david.marchand@redhat.com> >> wrote: >> >>> >>> Let me take an example with the test CHECK_FTP_SNAT_POST_RECIRC() + >>> address 10.1.1.240. >>> >>> When dealing with the first mangle operation (client sending its first >>> "PORT xxx" for the first "ls" command), the conn tcp seq skew value is at 0. >>> repl_ftp_v4_addr() returns 2 since it replaced 10.1.1.1 with 10.1.1.240. >>> The tcp seq numbers is kept untouched in the "if (do_seq_skew_adj && >>> seq_skew != 0)" block later in this function. >>> >>> When dealing with the second mangle operation (second "PORT xxx"), the >>> current conn tcp skew value is at 2. >>> repl_ftp_v4_addr() returns again 2 since it replaced 10.1.1.1 with >>> 10.1.1.240. >>> The tcp seq number is updated by 2 in the "if (do_seq_skew_adj && >>> seq_skew != 0)" block later in this function. >>> >>> Now, you could argue that the code you propose can work so far, but if >>> we add a new command again in this session, the tcp seq numbers sequence is >>> broken. >>> The command packet tcp seq number would be updated by the >>> repl_ftp_v4_addr() return value 2 and not by the accumulated value which >>> would be 4 for it to work. >>> >> >> @@ -3170,9 +3178,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; >> @@ -3187,24 +3194,22 @@ handle_ftp_ctl(struct conntrack *ct, const struct >> conn_lookup_ctx *ctx, >> return; >> } >> >> - if (!nat || !conn_for_expectation->seq_skew) { >> + if (!nat || !ec->seq_skew) { >> do_seq_skew_adj = false; >> } >> >> > How about removing this boolean ? See below. > 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); >> } >> @@ -3214,65 +3219,67 @@ handle_ftp_ctl(struct conntrack *ct, const struct >> conn_lookup_ctx *ctx, >> pkt->md.ct_state |= CS_TRACKED | CS_INVALID; >> return; >> } else if (rc == CT_FTP_CTL_INTEREST) { >> + >> 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)); >> 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 (do_seq_skew_adj && ec->seq_skew != 0) { >> > > s/if (do_seq_skew_adj && ec->seq_skew != 0) {/if (do_seq_skew_adj) {/ > > since "ec->seq_skew != 0" is now redundant > Since the connection seq_skew is updated as the last thing in the function, we do not need the do_seq_skew_adj boolean anymore. We can change the test as "if (nat && ec->seq_skew != 0) {" [snip] > >> >> @@ -3290,6 +3297,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 >> (END) >> >> Will send a v3 with this change today.
On Wed, Jan 9, 2019 at 12:53 AM David Marchand <david.marchand@redhat.com> wrote: > Hello, > > On Wed, Jan 9, 2019 at 4:51 AM Darrell Ball <dlu998@gmail.com> wrote: > >> On Tue, Jan 8, 2019 at 2:57 PM Darrell Ball <dlu998@gmail.com> wrote: >> >>> On Tue, Jan 8, 2019 at 2:13 AM David Marchand <david.marchand@redhat.com> >>> wrote: >>> >>>> >>>> Let me take an example with the test CHECK_FTP_SNAT_POST_RECIRC() + >>>> address 10.1.1.240. >>>> >>>> When dealing with the first mangle operation (client sending its first >>>> "PORT xxx" for the first "ls" command), the conn tcp seq skew value is at 0. >>>> repl_ftp_v4_addr() returns 2 since it replaced 10.1.1.1 with 10.1.1.240. >>>> The tcp seq numbers is kept untouched in the "if (do_seq_skew_adj && >>>> seq_skew != 0)" block later in this function. >>>> >>>> When dealing with the second mangle operation (second "PORT xxx"), the >>>> current conn tcp skew value is at 2. >>>> repl_ftp_v4_addr() returns again 2 since it replaced 10.1.1.1 with >>>> 10.1.1.240. >>>> The tcp seq number is updated by 2 in the "if (do_seq_skew_adj && >>>> seq_skew != 0)" block later in this function. >>>> >>>> Now, you could argue that the code you propose can work so far, but if >>>> we add a new command again in this session, the tcp seq numbers sequence is >>>> broken. >>>> The command packet tcp seq number would be updated by the >>>> repl_ftp_v4_addr() return value 2 and not by the accumulated value which >>>> would be 4 for it to work. >>>> >>> >>> @@ -3170,9 +3178,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; >>> @@ -3187,24 +3194,22 @@ handle_ftp_ctl(struct conntrack *ct, const >>> struct conn_lookup_ctx *ctx, >>> return; >>> } >>> >>> - if (!nat || !conn_for_expectation->seq_skew) { >>> + if (!nat || !ec->seq_skew) { >>> do_seq_skew_adj = false; >>> } >>> >>> >> > How about removing this boolean ? > See below. > > >> 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); >>> } >>> @@ -3214,65 +3219,67 @@ handle_ftp_ctl(struct conntrack *ct, const >>> struct conn_lookup_ctx *ctx, >>> pkt->md.ct_state |= CS_TRACKED | CS_INVALID; >>> return; >>> } else if (rc == CT_FTP_CTL_INTEREST) { >>> + >>> 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)); >>> 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 (do_seq_skew_adj && ec->seq_skew != 0) { >>> >> >> s/if (do_seq_skew_adj && ec->seq_skew != 0) {/if (do_seq_skew_adj) {/ >> >> since "ec->seq_skew != 0" is now redundant >> > > Since the connection seq_skew is updated as the last thing in the > function, we do not need the do_seq_skew_adj boolean anymore. > We can change the test as "if (nat && ec->seq_skew != 0) {" > sure; do_seq_skew_adj is not needed anymore. > [snip] > > >> >>> >>> @@ -3290,6 +3297,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 >>> (END) >>> >>> > Will send a v3 with this change today. > > > -- > David Marchand >
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..2e4141a 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -3214,30 +3214,34 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, pkt->md.ct_state |= CS_TRACKED | CS_INVALID; return; } else if (rc == CT_FTP_CTL_INTEREST) { - 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 (seq_skew) { - ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); - ip_len += 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 (seq_skew) { - ip_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); + seq_skew = conn_for_expectation->seq_skew; + if (nat) { + uint16_t ip_len; + int64_t new_skew; + + if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { + new_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, ftp_data_start, + addr_offset_from_ftp_data_start, + addr_size, mode); + if (new_skew) { + ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); + ip_len += new_skew; + nh6->ip6_ctlun.ip6_un1.ip6_un1_plen = htons(ip_len); + conn_seq_skew_set(ct, &conn_for_expectation->key, now, + new_skew + seq_skew, ctx->reply); + } + } else { + new_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 (new_skew) { + ip_len += new_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, + new_skew + seq_skew, ctx->reply); + } } } } else { diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 4c52431..3734497 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -4240,7 +4240,16 @@ 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 +]) + 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])
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. The tests are updated so that ftp+NAT checks send multiple commands in a single tcp command connection: wget is not able to do this, so switch to lftp. Signed-off-by: David Marchand <david.marchand@redhat.com> --- Vagrantfile | 9 ++++++--- Vagrantfile-FreeBSD | 2 +- lib/conntrack.c | 52 ++++++++++++++++++++++++++----------------------- tests/system-traffic.at | 11 ++++++++++- 4 files changed, 45 insertions(+), 29 deletions(-)