On Tue, Jan 8, 2019 at 2:57 PM Darrell Ball <dlu...@gmail.com> wrote:
>
>
> On Tue, Jan 8, 2019 at 2:13 AM David Marchand <david.march...@redhat.com>
> wrote:
>
>> On Tue, Jan 8, 2019 at 5:49 AM Darrell Ball <dlu...@gmail.com> wrote:
>>
>>> On Thu, Dec 20, 2018 at 5:33 AM David Marchand <
>>> david.march...@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.march...@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
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
