From a60a203b75b02a1a83694a9bf2273ad3dc72be7f Mon Sep 17 00:00:00 2001 From: Tamer Ahmed Date: Tue, 28 Apr 2020 00:57:59 -0700 Subject: [PATCH] [dhcpmon] Filter DHCP O/A Messages of Neighboring Vlans (#4469) * [dhcpmon] Filter DHCP O/A Messages of Neighboring Vlans This code fixes a bug where two or more vlans exist. Cross contamination happens for DHCP packets Offer/Ack when received on shared northbound links. The code filters out those packet based on dst IP equal Vlan loopback IP. signed-off-by: Tamer Ahmed --- src/dhcpmon/src/dhcp_device.c | 185 ++++++++++++++++++++++++---------- src/dhcpmon/src/dhcp_device.h | 38 +++++-- src/dhcpmon/src/dhcp_devman.c | 33 +++++- src/dhcpmon/src/dhcp_devman.h | 19 +++- src/dhcpmon/src/dhcp_mon.c | 10 +- src/dhcpmon/src/dhcp_mon.h | 2 +- src/dhcpmon/src/main.c | 4 +- 7 files changed, 212 insertions(+), 79 deletions(-) diff --git a/src/dhcpmon/src/dhcp_device.c b/src/dhcpmon/src/dhcp_device.c index aa0c0f835c..554a1dcf0a 100644 --- a/src/dhcpmon/src/dhcp_device.c +++ b/src/dhcpmon/src/dhcp_device.c @@ -33,6 +33,23 @@ #define DHCP_START_OFFSET (UDP_START_OFFSET + sizeof(struct udphdr)) /** Start of DHCP Options segment of a captured frame */ #define DHCP_OPTIONS_HEADER_SIZE 240 +/** Offset of DHCP GIADDR */ +#define DHCP_GIADDR_OFFSET 24 + +/** + * DHCP message types + **/ +typedef enum +{ + DHCP_MESSAGE_TYPE_DISCOVER = 1, + DHCP_MESSAGE_TYPE_OFFER = 2, + DHCP_MESSAGE_TYPE_REQUEST = 3, + DHCP_MESSAGE_TYPE_DECLINE = 4, + DHCP_MESSAGE_TYPE_ACK = 5, + DHCP_MESSAGE_TYPE_NAK = 6, + DHCP_MESSAGE_TYPE_RELEASE = 7, + DHCP_MESSAGE_TYPE_INFORM = 8 +} dhcp_message_type; #define OP_LDHA (BPF_LD | BPF_H | BPF_ABS) /** bpf ldh Abs */ #define OP_LDHI (BPF_LD | BPF_H | BPF_IND) /** bpf ldh Ind */ @@ -43,8 +60,9 @@ #define OP_JSET (BPF_JMP | BPF_JSET | BPF_K) /** bpf jset */ #define OP_LDXB (BPF_LDX | BPF_B | BPF_MSH) /** bpf ldxb */ -/** Berkley Packet Fitler program for "udp and (port 67 or port 68)". This program is obtained suing the following - * tcpdump command: 'tcpdump -dd "udp and (port 67 or port 68)"' +/** Berkeley Packet Filter program for "udp and (port 67 or port 68)". + * This program is obtained using the following command tcpdump: + * `tcpdump -dd "udp and (port 67 or port 68)"` */ static struct sock_filter dhcp_bpf_code[] = { {.code = OP_LDHA, .jt = 0, .jf = 0, .k = 0x0000000c}, // (000) ldh [12] @@ -90,47 +108,63 @@ static dhcp_device_counters_t glob_counters_snapshot[DHCP_DIR_COUNT] = { }; /** - * @code handle_dhcp_option_53(context, dhcp_option, dir); + * @code handle_dhcp_option_53(context, dhcp_option, dir, iphdr, dhcphdr); * * @brief handle the logic related to DHCP option 53 * * @param context Device (interface) context * @param dhcp_option pointer to DHCP option buffer space * @param dir packet direction + * @param iphdr pointer to packet IP header + * @param dhcphdr pointer to DHCP header * * @return none */ -static void handle_dhcp_option_53(dhcp_device_context_t *context, const u_char *dhcp_option, dhcp_packet_direction_t dir) +static void handle_dhcp_option_53(dhcp_device_context_t *context, + const u_char *dhcp_option, + dhcp_packet_direction_t dir, + struct ip *iphdr, + uint8_t *dhcphdr) { + in_addr_t giaddr; switch (dhcp_option[2]) { - case 1: + case DHCP_MESSAGE_TYPE_DISCOVER: + giaddr = ntohl(dhcphdr[DHCP_GIADDR_OFFSET] << 24 | dhcphdr[DHCP_GIADDR_OFFSET + 1] << 16 | + dhcphdr[DHCP_GIADDR_OFFSET + 2] << 8 | dhcphdr[DHCP_GIADDR_OFFSET + 3]); context->counters[dir].discover++; - if ((context->is_uplink && dir == DHCP_TX) || (!context->is_uplink && dir == DHCP_RX)) { + if ((context->vlan_ip == giaddr && context->is_uplink && dir == DHCP_TX) || + (!context->is_uplink && dir == DHCP_RX && iphdr->ip_dst.s_addr == INADDR_BROADCAST)) { glob_counters[dir].discover++; } break; - case 2: + case DHCP_MESSAGE_TYPE_OFFER: context->counters[dir].offer++; - if ((!context->is_uplink && dir == DHCP_TX) || (context->is_uplink && dir == DHCP_RX)) { + if ((context->vlan_ip == iphdr->ip_dst.s_addr && context->is_uplink && dir == DHCP_RX) || + (!context->is_uplink && dir == DHCP_TX)) { glob_counters[dir].offer++; } break; - case 3: + case DHCP_MESSAGE_TYPE_REQUEST: + giaddr = ntohl(dhcphdr[DHCP_GIADDR_OFFSET] << 24 | dhcphdr[DHCP_GIADDR_OFFSET + 1] << 16 | + dhcphdr[DHCP_GIADDR_OFFSET + 2] << 8 | dhcphdr[DHCP_GIADDR_OFFSET + 3]); context->counters[dir].request++; - if ((context->is_uplink && dir == DHCP_TX) || (!context->is_uplink && dir == DHCP_RX)) { + if ((context->vlan_ip == giaddr && context->is_uplink && dir == DHCP_TX) || + (!context->is_uplink && dir == DHCP_RX && iphdr->ip_dst.s_addr == INADDR_BROADCAST)) { glob_counters[dir].request++; } break; - case 5: + case DHCP_MESSAGE_TYPE_ACK: context->counters[dir].ack++; - if ((!context->is_uplink && dir == DHCP_TX) || (context->is_uplink && dir == DHCP_RX)) { + if ((context->vlan_ip == iphdr->ip_dst.s_addr && context->is_uplink && dir == DHCP_RX) || + (!context->is_uplink && dir == DHCP_TX)) { glob_counters[dir].ack++; } break; - case 4: // type: Decline - case 6 ... 8: - // type: NAK, Release, Inform + case DHCP_MESSAGE_TYPE_DECLINE: + case DHCP_MESSAGE_TYPE_NAK: + case DHCP_MESSAGE_TYPE_RELEASE: + case DHCP_MESSAGE_TYPE_INFORM: break; default: syslog(LOG_WARNING, "handle_dhcp_option_53(%s): Unknown DHCP option 53 type %d", context->intf, dhcp_option[2]); @@ -146,7 +180,6 @@ static void handle_dhcp_option_53(dhcp_device_context_t *context, const u_char * * @param fd socket to read from * @param event libevent triggered event * @param arg user provided argument for callback (interface context) - * @param packet pointer to packet data * * @return none */ @@ -158,7 +191,9 @@ static void read_callback(int fd, short event, void *arg) while ((event == EV_READ) && ((buffer_sz = recv(fd, context->buffer, context->snaplen, MSG_DONTWAIT)) > 0)) { struct ether_header *ethhdr = (struct ether_header*) context->buffer; + struct ip *iphdr = (struct ip*) (context->buffer + IP_START_OFFSET); struct udphdr *udp = (struct udphdr*) (context->buffer + UDP_START_OFFSET); + uint8_t *dhcphdr = context->buffer + DHCP_START_OFFSET; int dhcp_option_offset = DHCP_START_OFFSET + DHCP_OPTIONS_HEADER_SIZE; if ((buffer_sz > UDP_START_OFFSET + sizeof(struct udphdr) + DHCP_OPTIONS_HEADER_SIZE) && @@ -181,7 +216,7 @@ static void read_callback(int fd, short event, void *arg) { case 53: if (offset < (dhcp_option_sz + 2)) { - handle_dhcp_option_53(context, &dhcp_option[offset], dir); + handle_dhcp_option_53(context, &dhcp_option[offset], dir, iphdr, dhcphdr); } stop_dhcp_processing = 1; // break while loop since we are only interested in Option 53 break; @@ -260,31 +295,21 @@ static void dhcp_print_counters(dhcp_device_counters_t *counters) } /** - * @code init_socket(context, intf, snaplen, base); + * @code init_socket(context, intf); * * @brief initializes socket, bind it to interface and bpf prgram, and * associate with libevent base * * @param context pointer to device (interface) context * @param intf interface name - * @param snaplen length of packet capture - * @param base libevent base * * @return 0 on success, otherwise for failure */ -static int init_socket(dhcp_device_context_t *context, - const char *intf, - size_t snaplen, - struct event_base *base) +static int init_socket(dhcp_device_context_t *context, const char *intf) { int rv = -1; do { - if (snaplen < UDP_START_OFFSET + sizeof(struct udphdr) + DHCP_OPTIONS_HEADER_SIZE) { - syslog(LOG_ALERT, "init_socket(%s): snap length is too low to capture DHCP options", intf); - break; - } - context->sock = socket(AF_PACKET, SOCK_RAW | SOCK_NONBLOCK, htons(ETH_P_ALL)); if (context->sock < 0) { syslog(LOG_ALERT, "socket: failed to open socket with '%s'\n", strerror(errno)); @@ -301,25 +326,6 @@ static int init_socket(dhcp_device_context_t *context, break; } - if (setsockopt(context->sock, SOL_SOCKET, SO_ATTACH_FILTER, &dhcp_sock_bfp, sizeof(dhcp_sock_bfp)) != 0) { - syslog(LOG_ALERT, "setsockopt: failed to attach filter with '%s'\n", strerror(errno)); - break; - } - - context->buffer = (uint8_t *) malloc(snaplen); - if (context->buffer == NULL) { - syslog(LOG_ALERT, "malloc: failed to allocate memory for socket buffer '%s'\n", strerror(errno)); - break; - } - context->snaplen = snaplen; - - struct event *ev = event_new(base, context->sock, EV_READ | EV_PERSIST, read_callback, context); - if (ev == NULL) { - syslog(LOG_ALERT, "event_new: failed to allocate memory for libevent event '%s'\n", strerror(errno)); - break; - } - event_add(ev, NULL); - strncpy(context->intf, intf, sizeof(context->intf) - 1); context->intf[sizeof(context->intf) - 1] = '\0'; @@ -377,15 +383,32 @@ static int initialize_intf_mac_and_ip_addr(dhcp_device_context_t *context) } /** - * @code dhcp_device_init(context, intf, snaplen, is_uplink, base); + * @code dhcp_device_get_ip(context); + * + * @brief Accessor method + * + * @param context pointer to device (interface) context + * + * @return interface IP + */ +int dhcp_device_get_ip(dhcp_device_context_t *context, in_addr_t *ip) +{ + int rv = -1; + + if (context != NULL && ip != NULL) { + *ip = context->ip; + rv = 0; + } + + return rv; +} + +/** + * @code dhcp_device_init(context, intf, is_uplink); * * @brief initializes device (interface) that handles packet capture per interface. */ -int dhcp_device_init(dhcp_device_context_t **context, - const char *intf, - int snaplen, - uint8_t is_uplink, - struct event_base *base) +int dhcp_device_init(dhcp_device_context_t **context, const char *intf, uint8_t is_uplink) { int rv = -1; dhcp_device_context_t *dev_context = NULL; @@ -394,8 +417,8 @@ int dhcp_device_init(dhcp_device_context_t **context, dev_context = (dhcp_device_context_t *) malloc(sizeof(dhcp_device_context_t)); if (dev_context != NULL) { - if ((init_socket(dev_context, intf, snaplen, base) == 0) && - (initialize_intf_mac_and_ip_addr(dev_context) == 0 ) ) { + if ((init_socket(dev_context, intf) == 0) && + (initialize_intf_mac_and_ip_addr(dev_context) == 0)) { dev_context->is_uplink = is_uplink; @@ -414,6 +437,56 @@ int dhcp_device_init(dhcp_device_context_t **context, return rv; } +/** + * @code dhcp_device_start_capture(context, snaplen, base, vlan_ip); + * + * @brief starts packet capture on this interface + */ +int dhcp_device_start_capture(dhcp_device_context_t *context, + size_t snaplen, + struct event_base *base, + in_addr_t vlan_ip) +{ + int rv = -1; + + do { + if (context == NULL) { + syslog(LOG_ALERT, "NULL interface context pointer'\n"); + break; + } + + if (snaplen < UDP_START_OFFSET + sizeof(struct udphdr) + DHCP_OPTIONS_HEADER_SIZE) { + syslog(LOG_ALERT, "dhcp_device_start_capture(%s): snap length is too low to capture DHCP options", context->intf); + break; + } + + context->vlan_ip = vlan_ip; + + context->buffer = (uint8_t *) malloc(snaplen); + if (context->buffer == NULL) { + syslog(LOG_ALERT, "malloc: failed to allocate memory for socket buffer '%s'\n", strerror(errno)); + break; + } + context->snaplen = snaplen; + + if (setsockopt(context->sock, SOL_SOCKET, SO_ATTACH_FILTER, &dhcp_sock_bfp, sizeof(dhcp_sock_bfp)) != 0) { + syslog(LOG_ALERT, "setsockopt: failed to attach filter with '%s'\n", strerror(errno)); + break; + } + + struct event *ev = event_new(base, context->sock, EV_READ | EV_PERSIST, read_callback, context); + if (ev == NULL) { + syslog(LOG_ALERT, "event_new: failed to allocate memory for libevent event '%s'\n", strerror(errno)); + break; + } + event_add(ev, NULL); + + rv = 0; + } while (0); + + return rv; +} + /** * @code dhcp_device_shutdown(context); * diff --git a/src/dhcpmon/src/dhcp_device.h b/src/dhcpmon/src/dhcp_device.h index 04113eeabd..bc1582d46a 100644 --- a/src/dhcpmon/src/dhcp_device.h +++ b/src/dhcpmon/src/dhcp_device.h @@ -49,6 +49,7 @@ typedef struct int sock; /** Raw socket associated with this device/interface */ in_addr_t ip; /** network address of this device (interface) */ uint8_t mac[ETHER_ADDR_LEN]; /** hardware address of this device (interface) */ + in_addr_t vlan_ip; /** Vlan IP address */ uint8_t is_uplink; /** north interface? */ char intf[IF_NAMESIZE]; /** device (interface) name */ uint8_t *buffer; /** buffer used to read socket data */ @@ -60,23 +61,48 @@ typedef struct } dhcp_device_context_t; /** - * @code dhcp_device_init(context, intf, snaplen, timeout_ms, is_uplink, base); + * @code dhcp_device_get_ip(context, ip); + * + * @brief Accessor method + * + * @param context pointer to device (interface) context + * @param ip(out) pointer to device IP + * + * @return 0 on success, otherwise for failure + */ +int dhcp_device_get_ip(dhcp_device_context_t *context, in_addr_t *ip); + +/** + * @code dhcp_device_init(context, intf, is_uplink); * * @brief initializes device (interface) that handles packet capture per interface. * * @param context(inout) pointer to device (interface) context * @param intf interface name - * @param snaplen length of packet capture * @param is_uplink uplink interface - * @param base pointer to libevent base * * @return 0 on success, otherwise for failure */ int dhcp_device_init(dhcp_device_context_t **context, const char *intf, - int snaplen, - uint8_t is_uplink, - struct event_base *base); + uint8_t is_uplink); + +/** + * @code dhcp_device_start_capture(context, snaplen, base, vlan_ip); + * + * @brief starts packet capture on this interface + * + * @param context pointer to device (interface) context + * @param snaplen length of packet capture + * @param base pointer to libevent base + * @param vlan_ip vlan IP address + * + * @return 0 on success, otherwise for failure + */ +int dhcp_device_start_capture(dhcp_device_context_t *context, + size_t snaplen, + struct event_base *base, + in_addr_t vlan_ip); /** * @code dhcp_device_shutdown(context); diff --git a/src/dhcpmon/src/dhcp_devman.c b/src/dhcpmon/src/dhcp_devman.c index c19cbde591..077ed210a2 100644 --- a/src/dhcpmon/src/dhcp_devman.c +++ b/src/dhcpmon/src/dhcp_devman.c @@ -28,6 +28,23 @@ static uint32_t dhcp_num_south_intf = 0; /** dhcp_num_north_intf number of north interfaces */ static uint32_t dhcp_num_north_intf = 0; +/** On Device vlan interface IP address corresponding vlan downlink IP + * This IP is used to filter Offer/Ack packet coming from DHCP server */ +static in_addr_t vlan_ip = 0; + +/** vlan interface name */ +static char vlan_intf[IF_NAMESIZE] = "Undefined"; + +/** + * @code dhcp_devman_get_vlan_intf(); + * + * Accessor method + */ +const char* dhcp_devman_get_vlan_intf() +{ + return vlan_intf; +} + /** * @code dhcp_devman_init(); * @@ -65,7 +82,7 @@ void dhcp_devman_shutdown() } /** - * @code dhcp_devman_add_intf(name, uplink); + * @code dhcp_devman_add_intf(name, is_uplink); * * @brief adds interface to the device manager. */ @@ -84,9 +101,15 @@ int dhcp_devman_add_intf(const char *name, uint8_t is_uplink) assert(dhcp_num_south_intf <= 1); } - LIST_INSERT_HEAD(&intfs, dev, entry); + rv = dhcp_device_init(&dev->dev_context, dev->name, dev->is_uplink); + if (rv == 0 && !is_uplink) { + rv = dhcp_device_get_ip(dev->dev_context, &vlan_ip); - rv = 0; + strncpy(vlan_intf, name, sizeof(vlan_intf) - 1); + vlan_intf[sizeof(vlan_intf) - 1] = '\0'; + } + + LIST_INSERT_HEAD(&intfs, dev, entry); } else { syslog(LOG_ALERT, "malloc: failed to allocate memory for intf '%s'\n", name); @@ -100,14 +123,14 @@ int dhcp_devman_add_intf(const char *name, uint8_t is_uplink) * * @brief start packet capture on the devman interface list */ -int dhcp_devman_start_capture(int snaplen, struct event_base *base) +int dhcp_devman_start_capture(size_t snaplen, struct event_base *base) { int rv = -1; struct intf *int_ptr; if ((dhcp_num_south_intf == 1) && (dhcp_num_north_intf >= 1)) { LIST_FOREACH(int_ptr, &intfs, entry) { - rv = dhcp_device_init(&int_ptr->dev_context, int_ptr->name, snaplen, int_ptr->is_uplink, base); + rv = dhcp_device_start_capture(int_ptr->dev_context, snaplen, base, vlan_ip); if (rv == 0) { syslog(LOG_INFO, "Capturing DHCP packets on interface %s, ip: 0x%08x, mac [%02x:%02x:%02x:%02x:%02x:%02x] \n", diff --git a/src/dhcpmon/src/dhcp_devman.h b/src/dhcpmon/src/dhcp_devman.h index a0753b4b93..2f66fa407c 100644 --- a/src/dhcpmon/src/dhcp_devman.h +++ b/src/dhcpmon/src/dhcp_devman.h @@ -31,29 +31,38 @@ void dhcp_devman_init(); */ void dhcp_devman_shutdown(); +/** + * @code dhcp_devman_get_vlan_intf(); + * + * @brief Accessor method + * + * @return pointer to vlan ip interface name + */ +const char* dhcp_devman_get_vlan_intf(); + /** * @code dhcp_devman_add_intf(name, uplink); * * @brief adds interface to the device manager. * - * @param name interface name - * @param is_uplink true for uplink (north) interface + * @param name interface name + * @param is_uplink true for uplink (north) interface * * @return 0 on success, nonzero otherwise */ int dhcp_devman_add_intf(const char *name, uint8_t is_uplink); /** - * @code dhcp_devman_start_capture(snaplen, timeout_ms); + * @code dhcp_devman_start_capture(snaplen, base); * * @brief start packet capture on the devman interface list * - * @param snaplen packet capture snap length + * @param snaplen packet packet capture snap length * @param base libevent base * * @return 0 on success, nonzero otherwise */ -int dhcp_devman_start_capture(int snaplen, struct event_base *base); +int dhcp_devman_start_capture(size_t snaplen, struct event_base *base); /** * @code dhcp_devman_get_status(); diff --git a/src/dhcpmon/src/dhcp_mon.c b/src/dhcpmon/src/dhcp_mon.c index dc0a7d94f1..623bc46f52 100644 --- a/src/dhcpmon/src/dhcp_mon.c +++ b/src/dhcpmon/src/dhcp_mon.c @@ -36,13 +36,13 @@ static struct event *ev_sigterm; * * @param fd libevent socket * @param event event triggered - * @param arg pointer user provided context (libevent base) + * @param arg pointer to user provided context (libevent base) * * @return none */ static void signal_callback(evutil_socket_t fd, short event, void *arg) { - syslog(LOG_ALERT, "Received signal %d\n", event); + syslog(LOG_ALERT, "Received signal %s\n", strsignal(fd)); dhcp_devman_print_status(); dhcp_mon_stop(); } @@ -67,7 +67,9 @@ static void timeout_callback(evutil_socket_t fd, short event, void *arg) { case DHCP_MON_STATUS_UNHEALTHY: if (++count > dhcp_unhealthy_max_count) { - syslog(LOG_ALERT, "DHCP Relay is not healthy after %d health checks\n", count); + syslog(LOG_ALERT, "dhcpmon detected disparity in DHCP Relay behavior. Failure count: %d for vlan: '%s'\n", + count, dhcp_devman_get_vlan_intf()); + dhcp_devman_print_status(); } break; case DHCP_MON_STATUS_HEALTHY: @@ -151,7 +153,7 @@ void dhcp_mon_shutdown() * * @brief start monitoring DHCP Relay */ -int dhcp_mon_start(int snaplen) +int dhcp_mon_start(size_t snaplen) { int rv = -1; diff --git a/src/dhcpmon/src/dhcp_mon.h b/src/dhcpmon/src/dhcp_mon.h index 44d361b32e..ae8911ab51 100644 --- a/src/dhcpmon/src/dhcp_mon.h +++ b/src/dhcpmon/src/dhcp_mon.h @@ -40,7 +40,7 @@ void dhcp_mon_shutdown(); * * @return 0 upon success, otherwise upon failure */ -int dhcp_mon_start(int snaplen); +int dhcp_mon_start(size_t snaplen); /** * @code dhcp_mon_stop(); diff --git a/src/dhcpmon/src/main.c b/src/dhcpmon/src/main.c index 11eab6ee9e..9d155393a6 100644 --- a/src/dhcpmon/src/main.c +++ b/src/dhcpmon/src/main.c @@ -21,7 +21,7 @@ #include "dhcp_devman.h" /** dhcpmon_default_snaplen: default snap length of packet being captured */ -static const uint32_t dhcpmon_default_snaplen = 65535; +static const size_t dhcpmon_default_snaplen = 65535; /** dhcpmon_default_health_check_window: default value for a time window, during which DHCP DORA packet counts are being * collected */ static const uint32_t dhcpmon_default_health_check_window = 12; @@ -109,7 +109,7 @@ int main(int argc, char **argv) int i; int window_interval = dhcpmon_default_health_check_window; int max_unhealthy_count = dhcpmon_default_unhealthy_max_count; - uint32_t snaplen = dhcpmon_default_snaplen; + size_t snaplen = dhcpmon_default_snaplen; int make_daemon = 0; setlogmask(LOG_UPTO(LOG_INFO));