[ovs-dev] [PATCH v2] ovn: ARP reply security vulnerability.

nickcooper-zhangtonghao nickcooper-zhangtonghao at opencloud.tech
Tue Aug 16 15:43:02 UTC 2016


The the logical routers check only the "arp.op == 2" for ARP replies
and then use ARP replies to populate the logical router's ARP table.
If we continue to send ARP replies, which have different "arp.spa" and
"arp.sha", to logical router, the MAC_Binding table will continue to
increase. That may reduce system performance and cause instability and crashes.

So,
1. When logical router receive an ARP reply packet, we should check the
   packet's "arp.tpa" and "arp.spa".
2. The logical router uses a cache to store the ARP request. Only when
   logical routers send an ARP request packet, and get a corresponding
   ARP reply, will the 'ovn-controller' update MAC_Binding table of SB
   database.

Signed-off-by: nickcooper-zhangtonghao <nickcooper-zhangtonghao at opencloud.tech>
---
 ovn/controller/pinctrl.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++-
 ovn/northd/ovn-northd.c  |  22 ++++---
 2 files changed, 166 insertions(+), 8 deletions(-)

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 8a759cd..bba7b29 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -58,12 +58,18 @@ static void pinctrl_handle_put_mac_binding(const struct flow *md,
                                            const struct flow *headers,
                                            bool is_arp);
 static void init_put_mac_bindings(void);
+static void init_arp_request_caches(void);
 static void destroy_put_mac_bindings(void);
+static void destroy_arp_request_caches(void);
 static void run_put_mac_bindings(struct controller_ctx *,
                                  const struct lport_index *lports);
 static void wait_put_mac_bindings(struct controller_ctx *);
 static void flush_put_mac_bindings(void);

+static void flush_arp_request_caches(void);
+static void pinctrl_handle_arp_request_cache(uint32_t dp_key,
+                                             uint32_t port_key,
+                                             const char *ip_s);
 static void init_send_garps(void);
 static void destroy_send_garps(void);
 static void send_garp_wait(void);
@@ -85,6 +91,7 @@ pinctrl_init(void)
     swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION);
     conn_seq_no = 0;
     init_put_mac_bindings();
+    init_arp_request_caches();
     init_send_garps();
 }
 
@@ -185,6 +192,17 @@ pinctrl_handle_arp(const struct flow *ip_flow, const struct match *md,
     enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
     queue_msg(ofputil_encode_packet_out(&po, proto));
 
+    /* The logical router uses a cache to store the ARP request.
+     * Only when logical routers send an ARP request packet, and get a
+     * corresponding ARP reply, will the 'ovn-controller' update
+     * mac_binding table of SB database. */
+    uint32_t dp_key = ntohll(md->flow.metadata);
+    uint32_t port_key = md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0];
+    char ip_s[INET_ADDRSTRLEN];
+    inet_ntop(AF_INET, &ip_flow->nw_dst, ip_s, sizeof(ip_s));
+
+    pinctrl_handle_arp_request_cache(dp_key, port_key, ip_s);
+
 exit:
     dp_packet_uninit(&packet);
     ofpbuf_uninit(&ofpacts);
@@ -801,6 +819,7 @@ pinctrl_destroy(void)
 {
     rconn_destroy(swconn);
     destroy_put_mac_bindings();
+    destroy_arp_request_caches();
     destroy_send_garps();
 }
 
@@ -830,8 +849,22 @@ struct put_mac_binding {
     struct eth_addr mac;
 };
 
+/* Buffered "arp_request_cache" operation. */
+struct arp_request_cache {
+    struct hmap_node hmap_node; /* In 'arp_request_caches'. */
+
+    long long int timestamp;    /* In milliseconds. */
+
+    /* Key. */
+    uint32_t dp_key;
+    uint32_t port_key;
+    char ip_s[INET6_ADDRSTRLEN + 1];
+};
+
 /* Contains "struct put_mac_binding"s. */
 static struct hmap put_mac_bindings;
+/* Contains "struct arp_request_cache"s. */
+static struct hmap arp_request_caches;
 
 static void
 init_put_mac_bindings(void)
@@ -862,6 +895,100 @@ pinctrl_find_put_mac_binding(uint32_t dp_key, uint32_t port_key,
 }
 
 static void
+init_arp_request_caches(void)
+{
+    hmap_init(&arp_request_caches);
+}
+
+static void
+destroy_arp_request_caches(void)
+{
+    flush_arp_request_caches();
+    hmap_destroy(&arp_request_caches);
+}
+
+static bool
+pinctrl_find_delete_arp_request_cache(uint32_t dp_key, uint32_t port_key,
+                             const char *ip_s)
+{
+    struct arp_request_cache *arc, *next;
+    HMAP_FOR_EACH_SAFE (arc, next, hmap_node, &arp_request_caches) {
+        if (arc->dp_key == dp_key
+            && arc->port_key == port_key
+            && !strcmp(arc->ip_s, ip_s)) {
+            hmap_remove(&arp_request_caches, &arc->hmap_node);
+            free(arc);
+            return true;
+        }
+    }
+
+    return false;
+}
+
+static struct arp_request_cache *
+pinctrl_find_arp_request_cache(uint32_t dp_key, uint32_t port_key,
+                             const char *ip_s, uint32_t hash)
+{
+    struct arp_request_cache *arc;
+    HMAP_FOR_EACH_WITH_HASH (arc, hmap_node, hash, &arp_request_caches) {
+        if (arc->dp_key == dp_key
+            && arc->port_key == port_key
+            && !strcmp(arc->ip_s, ip_s)) {
+            return arc;
+        }
+    }
+    return NULL;
+}
+
+static void
+pinctrl_insert_arp_request_cache(uint32_t dp_key, uint32_t port_key,
+                             const char *ip_s, uint32_t hash)
+{
+    if (hmap_count(&arp_request_caches) >= 1000) {
+        VLOG_INFO("The ARP request cache is full.  Request will be dropped");
+        return;
+    }
+
+    struct arp_request_cache *arc = xmalloc(sizeof *arc);
+    hmap_insert(&arp_request_caches, &arc->hmap_node, hash);
+    arc->dp_key = dp_key;
+    arc->port_key = port_key;
+    ovs_strlcpy(arc->ip_s, ip_s, sizeof arc->ip_s);
+    arc->timestamp = time_msec();
+}
+
+static void
+pinctrl_handle_arp_request_cache(uint32_t dp_key, uint32_t port_key,
+                             const char *ip_s)
+{
+
+    uint32_t hash = hash_string(ip_s, hash_2words(dp_key, port_key));
+
+    struct arp_request_cache *arc
+        = pinctrl_find_arp_request_cache(dp_key, port_key, ip_s, hash);
+
+    if (!arc) {
+        pinctrl_insert_arp_request_cache(dp_key, port_key, ip_s, hash);
+        return;
+    }
+    arc->timestamp = time_msec();
+}
+
+static void
+run_update_arp_request_cache(void)
+{
+    struct arp_request_cache *arc, *next;
+    HMAP_FOR_EACH_SAFE (arc, next, hmap_node, &arp_request_caches) {
+        /* If the ARP request has been send 10s, and the reply is not received,
+         * remove the ARP request from the cache.*/
+        if (time_msec() > arc->timestamp + 10000) {
+            hmap_remove(&arp_request_caches, &arc->hmap_node);
+            free(arc);
+        }
+    }
+}
+
+static void
 pinctrl_handle_put_mac_binding(const struct flow *md,
                                const struct flow *headers, bool is_arp)
 {
@@ -876,6 +1003,17 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
         ovs_be128 ip6 = hton128(flow_get_xxreg(md, 0));
         inet_ntop(AF_INET6, &ip6, ip_s, sizeof(ip_s));
     }
+
+    /* Check to see whether the corresponding ARP request exists in the cache.
+     * If we don't find it, return directly. */
+    if (is_arp) {
+        bool update = pinctrl_find_delete_arp_request_cache(dp_key, port_key, ip_s);
+
+        if (!update) {
+            return;
+        }
+    }
+
     uint32_t hash = hash_string(ip_s, hash_2words(dp_key, port_key));
     struct put_mac_binding *pmb
         = pinctrl_find_put_mac_binding(dp_key, port_key, ip_s, hash);
@@ -951,6 +1089,9 @@ run_put_mac_bindings(struct controller_ctx *ctx,
         return;
     }
 
+    /* The ARP request expiring will be removed.*/
+    run_update_arp_request_cache();
+
     const struct put_mac_binding *pmb;
     HMAP_FOR_EACH (pmb, hmap_node, &put_mac_bindings) {
         run_put_mac_binding(ctx, lports, pmb);
@@ -974,7 +1115,16 @@ flush_put_mac_bindings(void)
         free(pmb);
     }
 }
-
+
+static void
+flush_arp_request_caches(void)
+{
+    struct arp_request_cache *arc;
+    HMAP_FOR_EACH_POP (arc, hmap_node, &arp_request_caches) {
+        free(arc);
+    }
+}
+
 /*
  * Send gratuitous ARP for vif on localnet.
  *
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index ac0d8f7..02be5e6 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -3143,11 +3143,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                       "ip4.dst == 0.0.0.0/8",
                       "drop;");
 
-        /* ARP reply handling.  Use ARP replies to populate the logical
-         * router's ARP table. */
-        ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90, "arp.op == 2",
-                      "put_arp(inport, arp.spa, arp.sha);");
-
         /* Drop Ethernet local broadcast.  By definition this traffic should
          * not be forwarded.*/
         ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 50,
@@ -3215,9 +3210,22 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                           ds_cstr(&match), ds_cstr(&actions));
         }
 
-        /* ARP reply.  These flows reply to ARP requests for the router's own
-         * IP address. */
         for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
+            /* ARP reply handling.  Use ARP replies to populate the logical
+             * router's ARP table. The logical router doesn't support proxy ARP */
+            ds_clear(&match);
+            ds_put_format(&match,
+                          "inport == %s && arp.op == 2 && arp.tpa == %s "
+                          "&& arp.spa == %s/%d",
+                          op->json_key, op->lrp_networks.ipv4_addrs[i].addr_s,
+                          op->lrp_networks.ipv4_addrs[i].network_s,
+                          op->lrp_networks.ipv4_addrs[i].plen);
+
+            ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90,
+                        ds_cstr(&match), "put_arp(inport, arp.spa, arp.sha);");
+
+            /* ARP reply.  These flows reply to ARP requests for the router's own
+             * IP address. */
             ds_clear(&match);
             ds_put_format(&match,
                           "inport == %s && arp.tpa == %s && arp.op == 1",
-- 
1.8.3.1






More information about the dev mailing list