| Message ID | 1544895448-14499-2-git-send-email-david.marchand@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | fixes for ftp alg in userspace dp | expand |
Thanks for working on this.
1/
What is the use case for multiple adjustments?
This code has been tested externally to Vmware as well.
Also multiple adjustments may be indicative of an exploit attempt or other
problem, so lets delineate
the use case first; please add a 'real' test case for this.
2/
IF we end up supporting multiple adjustments, as it stands now the patch
fails these tests
conntrack - NAT
96: conntrack - FTP NAT postrecirc seqadj FAILED (
system-traffic.at:4391)
98: conntrack - FTP NAT orig tuple seqadj FAILED (
system-traffic.at:4515)
The following alternative patch would correctly and succinctly implement
multiple adjustments.
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 6f6021a..f283db2 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -3225,7 +3225,8 @@ handle_ftp_ctl(struct conntrack *ct, const struct
conn_lookup_ctx *ctx,
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,
@@ -3237,7 +3238,8 @@ handle_ftp_ctl(struct conntrack *ct, const struct
conn_lookup_ctx *ctx,
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 {
On Sat, Dec 15, 2018 at 9:38 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 (CT_FTP_CTL_INTEREST) or they just go through
> without being modified (CT_FTP_CTL_OTHER).
>
> In both cases, the tcp seq/ack must be adjusted by the current offset
> that has been introduced in previous mangle operations and prepare for
> the next packets by setting an accumulated offset.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> lib/conntrack.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 974f985..d08d0ea 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -3185,11 +3185,9 @@ handle_ftp_ctl(struct conntrack *ct, const struct
> conn_lookup_ctx *ctx,
> }
>
> struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt);
> - int64_t seq_skew = 0;
> + int64_t seq_skew = conn_for_expectation->seq_skew;
>
> - 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,
> @@ -3208,35 +3206,36 @@ handle_ftp_ctl(struct conntrack *ct, const struct
> conn_lookup_ctx *ctx,
> return;
> } else if (rc == CT_FTP_CTL_INTEREST) {
> uint16_t ip_len;
> + int64_t new_skew;
>
> if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
> - seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep,
> ftp_data_start,
> + new_skew = repl_ftp_v6_addr(pkt, v6_addr_rep,
> ftp_data_start,
>
> addr_offset_from_ftp_data_start,
> addr_size, mode);
> - if (seq_skew) {
> + if (new_skew) {
> ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen);
> - ip_len += seq_skew;
> + 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,
> - seq_skew, ctx->reply);
> + new_skew + seq_skew, ctx->reply);
> }
> } else {
> - seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep,
> ftp_data_start,
> + 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 (seq_skew) {
> - ip_len += seq_skew;
> + 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,
> - seq_skew, ctx->reply);
> + new_skew + seq_skew, ctx->reply);
> }
> }
> } else {
> OVS_NOT_REACHED();
> }
> - } else {
> + } else if (ftp_ctl == CT_FTP_CTL_INVALID) {
> OVS_NOT_REACHED();
> }
>
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Hello Darrell, On Wed, Dec 19, 2018 at 9:16 AM Darrell Ball <dlu998@gmail.com> wrote: > 1/ > What is the use case for multiple adjustments? > This code has been tested externally to Vmware as well. > Also multiple adjustments may be indicative of an exploit attempt or other > problem, so lets delineate > the use case first; please add a 'real' test case for this. > Connect a ftp client to a server with nat (with the nat triggering a tcp seq ajustement), then enter several commands and watch the tcp seq numbers on the command connection. See patch 5 for the test, I put it later in the series to avoid test failures. > 2/ > IF we end up supporting multiple adjustments, as it stands now the patch > fails these tests > conntrack - NAT > > 96: conntrack - FTP NAT postrecirc seqadj FAILED ( > system-traffic.at:4391) > 98: conntrack - FTP NAT orig tuple seqadj FAILED ( > system-traffic.at:4515) > Argh, indeed, I guess patch 2 should come first, will check.
On Wed, Dec 19, 2018 at 12:57 AM David Marchand <david.marchand@redhat.com> wrote: > Hello Darrell, > > On Wed, Dec 19, 2018 at 9:16 AM Darrell Ball <dlu998@gmail.com> wrote: > >> 1/ >> What is the use case for multiple adjustments? >> This code has been tested externally to Vmware as well. >> Also multiple adjustments may be indicative of an exploit attempt or >> other problem, so lets delineate >> the use case first; please add a 'real' test case for this. >> > > Connect a ftp client to a server with nat (with the nat triggering a tcp > seq ajustement), then enter several commands and watch the tcp seq numbers > on the command connection. > See patch 5 for the test, I put it later in the series to avoid test > failures. > fold patch 5 into patch 1 and I'll take another look. > > >> 2/ >> IF we end up supporting multiple adjustments, as it stands now the patch >> fails these tests >> conntrack - NAT >> >> 96: conntrack - FTP NAT postrecirc seqadj FAILED ( >> system-traffic.at:4391) >> 98: conntrack - FTP NAT orig tuple seqadj FAILED ( >> system-traffic.at:4515) >> > > Argh, indeed, I guess patch 2 should come first, will check. > I am not sure Patch 2 is needed at all; it breaks the common code path intent as well. > > > -- > David Marchand >
diff --git a/lib/conntrack.c b/lib/conntrack.c index 974f985..d08d0ea 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -3185,11 +3185,9 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, } struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt); - int64_t seq_skew = 0; + int64_t seq_skew = conn_for_expectation->seq_skew; - 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, @@ -3208,35 +3206,36 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, return; } else if (rc == CT_FTP_CTL_INTEREST) { uint16_t ip_len; + int64_t new_skew; if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { - seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, ftp_data_start, + new_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, ftp_data_start, addr_offset_from_ftp_data_start, addr_size, mode); - if (seq_skew) { + if (new_skew) { ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); - ip_len += seq_skew; + 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, - seq_skew, ctx->reply); + new_skew + seq_skew, ctx->reply); } } else { - seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep, ftp_data_start, + 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 (seq_skew) { - ip_len += seq_skew; + 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, - seq_skew, ctx->reply); + new_skew + seq_skew, ctx->reply); } } } else { OVS_NOT_REACHED(); } - } else { + } else if (ftp_ctl == CT_FTP_CTL_INVALID) { OVS_NOT_REACHED(); }
The ftp alg deals with packets in two ways for the command connection: either they are inspected (CT_FTP_CTL_INTEREST) or they just go through without being modified (CT_FTP_CTL_OTHER). In both cases, the tcp seq/ack must be adjusted by the current offset that has been introduced in previous mangle operations and prepare for the next packets by setting an accumulated offset. Signed-off-by: David Marchand <david.marchand@redhat.com> --- lib/conntrack.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)