[ovs-dev] [PATCH] Add Nicira extension to OpenFlow for dropping spoofed ARP packets.
Ben Pfaff
blp at nicira.com
Tue Aug 24 23:03:45 UTC 2010
On Thu, Aug 19, 2010 at 05:53:20PM -0400, Jesse Gross wrote:
> On Thu, Aug 12, 2010 at 5:04 PM, Ben Pfaff <blp at nicira.com> wrote:
>
> > "ARP spoofing" is when a host claims an incorrect association between an
> > IP address and a MAC address for deceptive purposes. OpenFlow by itself
> > can prevent a host from sending out ARP replies from an incorrect MAC
> > address in the Ethernet L2 header, but it cannot control the MAC addresses
> > inside the ARP L3 packet. This commit adds a new action that can be used
> > to drop these spoofed packets.
> >
> > CC: Paul Ingram <paul at nicira.com>
>
> Need a signed-off-by line now for kernel code.
Thank you, I added one.
> > +static bool is_spoofed_arp(struct sk_buff *skb, const struct odp_flow_key
> > *key)
> > +{
> > + struct arp_eth_header *arp;
> > +
> > + if (key->dl_type != htons(ETH_P_ARP))
> > + return false;
> > +
> > + if (skb_network_offset(skb) + sizeof(struct arp_eth_header) >
> > skb->len)
> > + return false;
> >
>
> I think that invalid packets should get dropped here. Since this is
> specifically a security feature, it shouldn't be possible to bypass it with
> weird packets.
>
> This is obviously related to the runt flows set that I still need to look at
> in depth but I think here the case for dropping packets is stronger because
> the intent of the user is more clear.
OK, thanks, I changed the length check here to "return true"
>
> > +
> > + arp = (struct arp_eth_header *)skb_network_header(skb);
> > + return (arp->ar_hrd == htons(ARPHRD_ETHER) &&
> > + arp->ar_pro == htons(ETH_P_IP) &&
> > + arp->ar_hln == ETH_ALEN &&
> > + arp->ar_pln == 4 &&
> > + compare_ether_addr(arp->ar_sha, eth_hdr(skb)->h_source));
> > +}
>
> If ARP fields claim that this isn't Ethernet or the Ethernet address length
> is wrong we should definitely drop - something fishy is going on. Likewise,
> if the protocol is IP, the protocol address length better be 4.
Good point, I changed this to be more pessimistic.
> Even with this and appropriate MAC and IP flows you can still poison
> someone's ARP cache (by responding with someone else's IP). You may not be
> able to see their traffic but you can DoS them.
The IP fields in the ARP packet are part of the flow, so it's the
controller's problem to ensure that the host can't poison someone's ARP
cache, by using flows to restrict the possible responses. We only need
this action because the MAC fields in the ARP packet are not part of the
flow.
> > +
> > + case ODPAT_DROP_SPOOFED_ARP:
> > + if (is_spoofed_arp(skb, key))
> > + goto exit;
> > + break;
> >
>
> This is hopefully unlikely().
Added, thanks.
>
> > + case NXAST_DROP_SPOOFED_ARP:
> > + if (ctx->flow.dl_type == htons(ETH_TYPE_ARP)) {
> > + odp_actions_add(ctx->out, ODPAT_DROP_SPOOFED_ARP);
> > + }
> > + break;
> >
>
> If this is used on a datapath that doesn't support this action (i.e. the
> userspace datapath) it will be silently ignored, right? That makes me
> nervous.
The best I can do is to implement it in the userspace datapath, unless
you have a better idea. I've done that now.
I'm enclosing an updated version for you to look over again. Build
tested only.
--8<--------------------------cut here-------------------------->8--
From: Ben Pfaff <blp at nicira.com>
Date: Tue, 24 Aug 2010 16:00:27 -0700
Subject: [PATCH] Add Nicira extension to OpenFlow for dropping spoofed ARP packets.
"ARP spoofing" is when a host claims an incorrect association between an
IP address and a MAC address for deceptive purposes. OpenFlow by itself
can prevent a host from sending out ARP replies from an incorrect MAC
address in the Ethernet L2 header, but it cannot control the MAC addresses
inside the ARP L3 packet. This commit adds a new action that can be used
to drop these spoofed packets.
CC: Paul Ingram <paul at nicira.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
datapath/actions.c | 36 +++++++++++++++++++++++++++++++
datapath/flow.c | 15 -------------
datapath/flow.h | 16 +++++++++++++
include/openflow/nicira-ext.h | 13 ++++++++++-
include/openvswitch/datapath-protocol.h | 3 +-
lib/dpif-netdev.c | 33 ++++++++++++++++++++++++++++
ofproto/ofproto.c | 6 +++++
7 files changed, 105 insertions(+), 17 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index a6771b6..f7bf871 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -14,6 +14,7 @@
#include <linux/tcp.h>
#include <linux/udp.h>
#include <linux/in6.h>
+#include <linux/if_arp.h>
#include <linux/if_vlan.h>
#include <net/inet_ecn.h>
#include <net/ip.h>
@@ -318,6 +319,35 @@ static struct sk_buff *set_tp_port(struct sk_buff *skb,
return skb;
}
+/**
+ * is_spoofed_arp - check for invalid ARP packet
+ *
+ * @skb: skbuff containing an Ethernet packet, with network header pointing
+ * just past the Ethernet and optional 802.1Q header.
+ * @key: flow key extracted from @skb by flow_extract()
+ *
+ * Returns true if @skb is an invalid Ethernet+IPv4 ARP packet: one with screwy
+ * or truncated header fields or one whose inner and outer Ethernet address
+ * differ.
+ */
+static bool is_spoofed_arp(struct sk_buff *skb, const struct odp_flow_key *key)
+{
+ struct arp_eth_header *arp;
+
+ if (key->dl_type != htons(ETH_P_ARP))
+ return false;
+
+ if (skb_network_offset(skb) + sizeof(struct arp_eth_header) > skb->len)
+ return true;
+
+ arp = (struct arp_eth_header *)skb_network_header(skb);
+ return (arp->ar_hrd != htons(ARPHRD_ETHER) ||
+ arp->ar_pro != htons(ETH_P_IP) ||
+ arp->ar_hln != ETH_ALEN ||
+ arp->ar_pln != 4 ||
+ !compare_ether_addr(arp->ar_sha, eth_hdr(skb)->h_source));
+}
+
static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port)
{
struct dp_port *p;
@@ -484,10 +514,16 @@ int execute_actions(struct datapath *dp, struct sk_buff *skb,
case ODPAT_POP_PRIORITY:
skb->priority = priority;
break;
+
+ case ODPAT_DROP_SPOOFED_ARP:
+ if (unlikely(is_spoofed_arp(skb, key)))
+ goto exit;
+ break;
}
if (!skb)
return -ENOMEM;
}
+exit:
if (prev_port != -1)
do_output(dp, skb, prev_port);
else
diff --git a/datapath/flow.c b/datapath/flow.c
index f769b14..8074b20 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -34,21 +34,6 @@
struct kmem_cache *flow_cache;
static unsigned int hash_seed;
-struct arp_eth_header
-{
- __be16 ar_hrd; /* format of hardware address */
- __be16 ar_pro; /* format of protocol address */
- unsigned char ar_hln; /* length of hardware address */
- unsigned char ar_pln; /* length of protocol address */
- __be16 ar_op; /* ARP opcode (command) */
-
- /* Ethernet+IPv4 specific members. */
- unsigned char ar_sha[ETH_ALEN]; /* sender hardware address */
- unsigned char ar_sip[4]; /* sender IP address */
- unsigned char ar_tha[ETH_ALEN]; /* target hardware address */
- unsigned char ar_tip[4]; /* target IP address */
-} __attribute__((packed));
-
static inline int arphdr_ok(struct sk_buff *skb)
{
int nh_ofs = skb_network_offset(skb);
diff --git a/datapath/flow.h b/datapath/flow.h
index 534c7af..80a5b66 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -14,6 +14,7 @@
#include <linux/types.h>
#include <linux/rcupdate.h>
#include <linux/gfp.h>
+#include <linux/if_ether.h>
#include <linux/jiffies.h>
#include <linux/time.h>
@@ -42,6 +43,21 @@ struct sw_flow {
u8 tcp_flags; /* Union of seen TCP flags. */
};
+struct arp_eth_header
+{
+ __be16 ar_hrd; /* format of hardware address */
+ __be16 ar_pro; /* format of protocol address */
+ unsigned char ar_hln; /* length of hardware address */
+ unsigned char ar_pln; /* length of protocol address */
+ __be16 ar_op; /* ARP opcode (command) */
+
+ /* Ethernet+IPv4 specific members. */
+ unsigned char ar_sha[ETH_ALEN]; /* sender hardware address */
+ unsigned char ar_sip[4]; /* sender IP address */
+ unsigned char ar_tha[ETH_ALEN]; /* target hardware address */
+ unsigned char ar_tip[4]; /* target IP address */
+} __attribute__((packed));
+
extern struct kmem_cache *flow_cache;
struct sw_flow_actions *flow_actions_alloc(size_t n_actions);
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index a80439c..885e01d 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -131,7 +131,18 @@ enum nx_action_subtype {
*/
NXAST_RESUBMIT,
- NXAST_SET_TUNNEL /* Set encapsulating tunnel ID. */
+ /* Set encapsulating tunnel ID. */
+ NXAST_SET_TUNNEL,
+
+ /* Stops processing further actions, if the packet being processed is an
+ * Ethernet+IPv4 ARP packet for which the source Ethernet address inside
+ * the ARP packet differs from the source Ethernet address in the Ethernet
+ * header.
+ *
+ * This is useful because OpenFlow does not provide a way to match on the
+ * Ethernet addresses inside ARP packets, so there is no other way to drop
+ * spoofed ARPs other than sending every packet up to the controller. */
+ NXAST_DROP_SPOOFED_ARP
};
/* Action structure for NXAST_RESUBMIT. */
diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
index c3ec4dc..839c484 100644
--- a/include/openvswitch/datapath-protocol.h
+++ b/include/openvswitch/datapath-protocol.h
@@ -281,7 +281,8 @@ struct odp_flowvec {
#define ODPAT_SET_TUNNEL 13 /* Set the encapsulating tunnel ID. */
#define ODPAT_SET_PRIORITY 14 /* Set skb->priority. */
#define ODPAT_POP_PRIORITY 15 /* Restore original skb->priority. */
-#define ODPAT_N_ACTIONS 16
+#define ODPAT_DROP_SPOOFED_ARP 16 /* Drop ARPs with spoofed source MAC. */
+#define ODPAT_N_ACTIONS 17
struct odp_action_output {
uint16_t type; /* ODPAT_OUTPUT. */
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c17b525..6c81329 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1269,6 +1269,34 @@ dp_netdev_output_control(struct dp_netdev *dp, const struct ofpbuf *packet,
return 0;
}
+/* Returns true if 'packet' is an invalid Ethernet+IPv4 ARP packet: one with
+ * screwy or truncated header fields or one whose inner and outer Ethernet
+ * address differ. */
+static bool
+dp_netdev_is_spoofed_arp(struct ofpbuf *packet, const struct odp_flow_key *key)
+{
+ struct arp_eth_header *arp;
+ struct eth_header *eth;
+ ptrdiff_t l3_size;
+
+ if (key->dl_type != htons(ETH_TYPE_ARP)) {
+ return false;
+ }
+
+ l3_size = (char *) ofpbuf_end(packet) - (char *) packet->l3;
+ if (l3_size < sizeof(struct arp_eth_header)) {
+ return true;
+ }
+
+ eth = packet->l2;
+ arp = packet->l3;
+ return (arp->ar_hrd != htons(ARP_HRD_ETHERNET)
+ || arp->ar_pro != htons(ARP_PRO_IP)
+ || arp->ar_hln != ETH_HEADER_LEN
+ || arp->ar_pln != 4
+ || !eth_addr_equals(arp->ar_sha, eth->eth_src));
+}
+
static int
dp_netdev_execute_actions(struct dp_netdev *dp,
struct ofpbuf *packet, const flow_t *key,
@@ -1329,6 +1357,11 @@ dp_netdev_execute_actions(struct dp_netdev *dp,
case ODPAT_SET_TP_DST:
dp_netdev_set_tp_port(packet, key, &a->tp_port);
break;
+
+ case ODPAT_DROP_SPOOFED_ARP:
+ if (dp_netdev_is_spoofed_arp(packet, key)) {
+ return 0;
+ }
}
}
return 0;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 91ff023..2e1530a 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2631,6 +2631,12 @@ xlate_nicira_action(struct action_xlate_ctx *ctx,
ctx->flow.tun_id = oa->tunnel.tun_id = nast->tun_id;
break;
+ case NXAST_DROP_SPOOFED_ARP:
+ if (ctx->flow.dl_type == htons(ETH_TYPE_ARP)) {
+ odp_actions_add(ctx->out, ODPAT_DROP_SPOOFED_ARP);
+ }
+ break;
+
/* If you add a new action here that modifies flow data, don't forget to
* update the flow key in ctx->flow at the same time. */
--
1.7.1
More information about the dev
mailing list