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.
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
