[ovs-dev,v3,2/3] conntrack: Allow specified alg port numbers.

Message ID 1512403987-112612-3-git-send-email-dlu998@gmail.com
State Accepted
Headers show
Series
  • conntrack: Alg improvements.
Related show

Commit Message

Darrell Ball Dec. 4, 2017, 4:13 p.m.
Algs can use variable control port numbers for servers.
The main use case is a kind of feeble security measure; the
thinking being by some is that it obscures the alg traffic.
It is really not very effective, but the kernel has this
capability. This patch mimics the capability.

Signed-off-by: Darrell Ball <dlu998@gmail.com>
---
 lib/conntrack.c        | 39 +++++++++++++++++++++++++++------------
 lib/conntrack.h        |  8 ++++----
 lib/dpif-netdev.c      |  4 ++--
 tests/test-conntrack.c |  6 +++---
 4 files changed, 36 insertions(+), 21 deletions(-)

Comments

Aaron Conole Dec. 6, 2017, 6:46 p.m. | #1
Darrell Ball <dlu998@gmail.com> writes:

> Algs can use variable control port numbers for servers.
> The main use case is a kind of feeble security measure; the
> thinking being by some is that it obscures the alg traffic.
> It is really not very effective, but the kernel has this
> capability. This patch mimics the capability.
>
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> ---

Looks like the only thing that changed was a bit of context from the 1/3
change.

Acked-by: Aaron Conole <aconole@redhat.com>

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index b370384..61f3a79 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -462,23 +462,37 @@  is_ftp_ctl(const enum ct_alg_ctl_type ct_alg_ctl)
 }
 
 static enum ct_alg_ctl_type
-get_alg_ctl_type(const struct dp_packet *pkt)
+get_alg_ctl_type(const struct dp_packet *pkt, ovs_be16 tp_src, ovs_be16 tp_dst,
+                 const char *helper)
 {
-    uint8_t ip_proto = get_ip_proto(pkt);
-    struct udp_header *uh = dp_packet_l4(pkt);
-    struct tcp_header *th = dp_packet_l4(pkt);
-
     /* CT_IPPORT_FTP/TFTP is used because IPPORT_FTP/TFTP in not defined
      * in OSX, at least in in.h. Since these values will never change, remove
      * the external dependency. */
     enum { CT_IPPORT_FTP = 21 };
     enum { CT_IPPORT_TFTP = 69 };
+    uint8_t ip_proto = get_ip_proto(pkt);
+    struct udp_header *uh = dp_packet_l4(pkt);
+    struct tcp_header *th = dp_packet_l4(pkt);
+    ovs_be16 ftp_src_port = htons(CT_IPPORT_FTP);
+    ovs_be16 ftp_dst_port = htons(CT_IPPORT_FTP);
+    ovs_be16 tftp_dst_port = htons(CT_IPPORT_TFTP);
+
+    if (OVS_UNLIKELY(tp_dst)) {
+        if (helper && !strncmp(helper, "ftp", strlen("ftp"))) {
+            ftp_dst_port = tp_dst;
+        } else if (helper && !strncmp(helper, "tftp", strlen("tftp"))) {
+            tftp_dst_port = tp_dst;
+        }
+    } else if (OVS_UNLIKELY(tp_src)) {
+        if (helper && !strncmp(helper, "ftp", strlen("ftp"))) {
+            ftp_src_port = tp_src;
+        }
+    }
 
-    if (ip_proto == IPPROTO_UDP && uh->udp_dst == htons(CT_IPPORT_TFTP)) {
+    if (ip_proto == IPPROTO_UDP && uh->udp_dst == tftp_dst_port) {
         return CT_ALG_CTL_TFTP;
     } else if (ip_proto == IPPROTO_TCP &&
-               (th->tcp_src == htons(CT_IPPORT_FTP) ||
-                th->tcp_dst == htons(CT_IPPORT_FTP))) {
+               (th->tcp_src == ftp_src_port || th->tcp_dst == ftp_dst_port)) {
         return CT_ALG_CTL_FTP;
     }
     return CT_ALG_CTL_NONE;
@@ -1134,7 +1148,7 @@  process_one(struct conntrack *ct, struct dp_packet *pkt,
             bool force, bool commit, long long now, const uint32_t *setmark,
             const struct ovs_key_ct_labels *setlabel,
             const struct nat_action_info_t *nat_action_info,
-            const char *helper)
+            ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper)
 {
     struct conn *conn;
     unsigned bucket = hash_to_bucket(ctx->hash);
@@ -1181,7 +1195,8 @@  process_one(struct conntrack *ct, struct dp_packet *pkt,
     struct conn conn_for_un_nat_copy;
     conn_for_un_nat_copy.conn_type = CT_CONN_TYPE_DEFAULT;
 
-    enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt);
+    enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt, tp_src, tp_dst,
+                                                       helper);
 
     if (OVS_LIKELY(conn)) {
         if (OVS_LIKELY(!conn_update_state_alg(ct, pkt, ctx, conn,
@@ -1264,7 +1279,7 @@  conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
                   ovs_be16 dl_type, bool force, bool commit, uint16_t zone,
                   const uint32_t *setmark,
                   const struct ovs_key_ct_labels *setlabel,
-                  const char *helper,
+                  ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper,
                   const struct nat_action_info_t *nat_action_info,
                   long long now)
 {
@@ -1279,7 +1294,7 @@  conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
             continue;
         }
         process_one(ct, packet, &ctx, zone, force, commit, now, setmark,
-                    setlabel, nat_action_info, helper);
+                    setlabel, nat_action_info, tp_src, tp_dst, helper);
     }
 
     return 0;
diff --git a/lib/conntrack.h b/lib/conntrack.h
index fbeef1c..990f6c2 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -90,11 +90,11 @@  struct nat_action_info_t {
 void conntrack_init(struct conntrack *);
 void conntrack_destroy(struct conntrack *);
 
-int conntrack_execute(struct conntrack *, struct dp_packet_batch *,
-                      ovs_be16 dl_type, bool force, bool commit,
-                      uint16_t zone, const uint32_t *setmark,
+int conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
+                      ovs_be16 dl_type, bool force, bool commit, uint16_t zone,
+                      const uint32_t *setmark,
                       const struct ovs_key_ct_labels *setlabel,
-                      const char *helper,
+                      ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper,
                       const struct nat_action_info_t *nat_action_info,
                       long long now);
 
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index db78318..f0e5eb3 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -5635,8 +5635,8 @@  dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
         }
 
         conntrack_execute(&dp->conntrack, packets_, aux->flow->dl_type, force,
-                          commit, zone, setmark, setlabel, helper,
-                          nat_action_info_ref, now);
+                          commit, zone, setmark, setlabel, aux->flow->tp_src,
+                          aux->flow->tp_dst, helper, nat_action_info_ref, now);
         break;
     }
 
diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c
index b27d181..76bca2e 100644
--- a/tests/test-conntrack.c
+++ b/tests/test-conntrack.c
@@ -90,7 +90,7 @@  ct_thread_main(void *aux_)
     ovs_barrier_block(&barrier);
     for (i = 0; i < n_pkts; i += batch_size) {
         conntrack_execute(&ct, pkt_batch, dl_type, false, true, 0, NULL, NULL,
-                          NULL, NULL, now);
+                          0, 0, NULL, NULL, now);
     }
     ovs_barrier_block(&barrier);
     destroy_packets(pkt_batch);
@@ -174,7 +174,7 @@  pcap_batch_execute_conntrack(struct conntrack *ct,
 
         if (flow.dl_type != dl_type) {
             conntrack_execute(ct, &new_batch, dl_type, false, true, 0,
-                              NULL, NULL, NULL, NULL, now);
+                              NULL, NULL, 0, 0, NULL, NULL, now);
             dp_packet_batch_init(&new_batch);
         }
         new_batch.packets[new_batch.count++] = packet;;
@@ -182,7 +182,7 @@  pcap_batch_execute_conntrack(struct conntrack *ct,
 
     if (!dp_packet_batch_is_empty(&new_batch)) {
         conntrack_execute(ct, &new_batch, dl_type, false, true, 0, NULL, NULL,
-                          NULL, NULL, now);
+                          0, 0, NULL, NULL, now);
     }
 
 }