On Tue, Jan 8, 2019 at 2:13 AM David Marchand <[email protected]> wrote:
> On Tue, Jan 8, 2019 at 5:49 AM Darrell Ball <[email protected]> wrote: > >> On Thu, Dec 20, 2018 at 5:33 AM David Marchand <[email protected]> >> 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 <[email protected]> >>> --- >>> 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
