diff --git a/dhcp4relay/src/dhcp4relay.cpp b/dhcp4relay/src/dhcp4relay.cpp index 324a498..c8bc846 100644 --- a/dhcp4relay/src/dhcp4relay.cpp +++ b/dhcp4relay/src/dhcp4relay.cpp @@ -455,11 +455,20 @@ int prepare_vlan_sockets(relay_config &config) { uint8_t encode_tlv(uint8_t *buf, uint8_t t, uint8_t l, uint8_t *v) { *buf = t; *(buf + DHCP_SUB_OPT_TLV_LENGTH_OFFSET) = l; - memcpy((buf + DHCP_SUB_OPT_TLV_HEADER_LEN), v, l); + /* VSS-Control (suboption 152) has a zero-length payload; calling memcpy + with its null payload pointer is unsafe even when the copy length is zero. */ + if (l > 0) { + memcpy((buf + DHCP_SUB_OPT_TLV_HEADER_LEN), v, l); + } return (l + DHCP_SUB_OPT_TLV_HEADER_LEN); } +static bool is_vss_required(const relay_config &config, const std::string &client_vrf) { + return ((config.vrf_selection_opt == "enable") && (client_vrf != "default") && + (config.vrf != client_vrf)); +} + std::string get_mac_address(const std::string &ifname) { std::string path = "/sys/class/net/" + ifname + "/address"; std::ifstream file(path); @@ -535,8 +544,7 @@ void encode_relay_option(pcpp::DhcpLayer *dhcp_pkt, relay_config *config) { /* | 151 | vrf_len | 0 | vrf_name | */ uint8_t vss_buf[32] = {0}; /* Enable VSS only if client and server are in two different VRF's */ - if ((config->vrf_selection_opt == "enable") && (vrf != "default") && - (config->vrf != vrf)) { + if (is_vss_required(*config, vrf)) { uint8_t zero_encode = 0; memcpy(vss_buf, &zero_encode, sizeof(uint8_t)); memcpy((vss_buf + 1), (uint8_t *)vrf.c_str(), (uint8_t)vrf.length()); @@ -544,6 +552,11 @@ void encode_relay_option(pcpp::DhcpLayer *dhcp_pkt, relay_config *config) { offset = encode_tlv((buf + buf_offset), OPTION82_SUBOPT_VIRTUAL_SUBNET, (uint8_t)(vrf.length() + 1), vss_buf); buf_offset += offset; + + /* RFC 6607 requires VSS-Control whenever VSS is included. */ + offset = encode_tlv((buf + buf_offset), OPTION82_SUBOPT_VIRTUAL_SUBNET_CONTROL, + 0, nullptr); + buf_offset += offset; } /* We shouldn't append relay information if packet size is exceeding MTU size */ @@ -664,7 +677,7 @@ uint8_t *decode_tlv(const uint8_t *buf, uint8_t t, uint8_t &l, uint32_t options_ while (temp && ((offset + DHCP_SUB_OPT_TLV_HEADER_LEN) <= options_total_size)) { len = *(temp + DHCP_SUB_OPT_TLV_LENGTH_OFFSET); - if ((offset + DHCP_SUB_OPT_TLV_LENGTH_OFFSET + len) > options_total_size) { + if ((offset + DHCP_SUB_OPT_TLV_HEADER_LEN + len) > options_total_size) { /* Malformed packet */ SWSS_LOG_ERROR("[DHCPV4_INFO] Failed to decode relay agent sub-option %d" " exceeded total option len %d offset %d sub-option len %d", @@ -684,6 +697,66 @@ uint8_t *decode_tlv(const uint8_t *buf, uint8_t t, uint8_t &l, uint32_t options_ return NULL; } +static bool is_valid_tlv_buffer(const uint8_t *buf, uint32_t options_total_size) { + uint32_t offset = 0; + + while (offset < options_total_size) { + if ((options_total_size - offset) < DHCP_SUB_OPT_TLV_HEADER_LEN) { + return false; + } + + const auto len = buf[offset + DHCP_SUB_OPT_TLV_LENGTH_OFFSET]; + if (len > (options_total_size - offset - DHCP_SUB_OPT_TLV_HEADER_LEN)) { + return false; + } + + offset += DHCP_SUB_OPT_TLV_HEADER_LEN + len; + } + + return true; +} + +bool validate_vss_reply(const uint8_t *options_ptr, uint32_t options_size, + const relay_config &config, const std::string &src_ip) { + auto client_vrf_itr = vlan_vrf_map.find(config.vlan); + if ((client_vrf_itr == vlan_vrf_map.end()) || + !is_vss_required(config, client_vrf_itr->second)) { + return true; + } + + if ((options_ptr == nullptr) || !is_valid_tlv_buffer(options_ptr, options_size)) { + SWSS_LOG_WARN("[DHCPV4_RELAY] Dropping server reply for %s from %s: " + "malformed relay agent sub-options", + config.vlan.c_str(), src_ip.c_str()); + return false; + } + + uint8_t vss_control_len = 0; + auto vss_control_ptr = decode_tlv(options_ptr, OPTION82_SUBOPT_VIRTUAL_SUBNET_CONTROL, + vss_control_len, options_size); + if (vss_control_ptr != nullptr) { + SWSS_LOG_WARN("[DHCPV4_RELAY] Dropping server reply for %s from %s: " + "server did not process VSS sub-option", + config.vlan.c_str(), src_ip.c_str()); + return false; + } + + const auto &client_vrf = client_vrf_itr->second; + uint8_t vss_len = 0; + auto vss_ptr = decode_tlv(options_ptr, OPTION82_SUBOPT_VIRTUAL_SUBNET, + vss_len, options_size); + if ((vss_ptr == nullptr) || (vss_len != (client_vrf.length() + 1)) || + (vss_ptr[0] != 0) || + (memcmp(vss_ptr + 1, client_vrf.data(), client_vrf.length()) != 0)) { + SWSS_LOG_WARN("[DHCPV4_RELAY] Dropping server reply for %s from %s: " + "missing or mismatched VSS sub-option", + config.vlan.c_str(), src_ip.c_str()); + return false; + } + + return true; +} + /** * @code void to_client(pcpp::DhcpLayer* dhcp_pkt, std::unordered_map *vlans, std::string src_ip); @@ -790,6 +863,12 @@ void to_client(pcpp::DhcpLayer *dhcp_pkt, std::unordered_mapsecond; dhcp_cntr_table.increment_counter(config.vlan, "RX", (int)dhcp_pkt->getMessageType()); + + if (!validate_vss_reply((const uint8_t *)options_ptr, agent_option_size, config, src_ip)) { + dhcp_cntr_table.increment_counter(config.vlan, "TX", DHCPv4_MESSAGE_TYPE_DROP); + return; + } + /* TODO: Also check it is matching remote ID*/ memcpy(&target_addr.sin_addr, &broadcast_addr, sizeof(struct in_addr)); diff --git a/dhcp4relay/src/dhcp4relay.h b/dhcp4relay/src/dhcp4relay.h index 1654ed6..c5bb743 100644 --- a/dhcp4relay/src/dhcp4relay.h +++ b/dhcp4relay/src/dhcp4relay.h @@ -60,6 +60,7 @@ extern int config_pipe[2]; #define OPTION82_SUBOPT_LINK_SELECTION 5 #define OPTION82_SUBOPT_SERVER_OVERRIDE 11 #define OPTION82_SUBOPT_VIRTUAL_SUBNET 151 +#define OPTION82_SUBOPT_VIRTUAL_SUBNET_CONTROL 152 #define DHCP_ETHERNET_HDR_LEN 14 #define DHCP_IP_HDR_LEN 20 @@ -322,3 +323,7 @@ void pkt_in_callback(evutil_socket_t fd, short event, void *arg); void config_event_callback(evutil_socket_t fd, short event, void *arg); uint8_t *decode_tlv(const uint8_t *buf, uint8_t t, uint8_t &l, uint32_t options_total_size); uint8_t encode_tlv(uint8_t *buf, uint8_t t, uint8_t l, uint8_t *v); +#ifdef UNIT_TEST +bool validate_vss_reply(const uint8_t *options_ptr, uint32_t options_size, + const relay_config &config, const std::string &src_ip); +#endif diff --git a/dhcp4relay/test/mock_relay.cpp b/dhcp4relay/test/mock_relay.cpp index 510b923..bb8446a 100644 --- a/dhcp4relay/test/mock_relay.cpp +++ b/dhcp4relay/test/mock_relay.cpp @@ -100,6 +100,24 @@ TEST(EncodeDecodeTLV, EncodeAndDecode) { EXPECT_EQ(decoded_value[0], 0x11); EXPECT_EQ(decoded_value[1], 0x22); EXPECT_EQ(decoded_value[2], 0x33); + + uint8_t empty_buffer[2] = {}; + encoded_length = encode_tlv(empty_buffer, OPTION82_SUBOPT_VIRTUAL_SUBNET_CONTROL, + 0, nullptr); + EXPECT_EQ(encoded_length, 2); + EXPECT_EQ(empty_buffer[0], OPTION82_SUBOPT_VIRTUAL_SUBNET_CONTROL); + EXPECT_EQ(empty_buffer[1], 0); + + decoded_value = decode_tlv(empty_buffer, OPTION82_SUBOPT_VIRTUAL_SUBNET_CONTROL, + length, sizeof(empty_buffer)); + ASSERT_NE(decoded_value, nullptr); + EXPECT_EQ(length, 0); + + uint8_t truncated_buffer[2] = {OPTION82_SUBOPT_VIRTUAL_SUBNET, 1}; + decoded_value = decode_tlv(truncated_buffer, OPTION82_SUBOPT_VIRTUAL_SUBNET, + length, sizeof(truncated_buffer)); + EXPECT_EQ(decoded_value, nullptr); + EXPECT_EQ(length, 0); } TEST(sock, sock_open) { @@ -820,6 +838,13 @@ TEST(DHCPRelayTest, encode_relay_option) { memcpy((vss_buf + 1), (uint8_t*)vlan_vrf_map["Vlan10"].c_str(), (uint8_t)vlan_vrf_map["Vlan10"].length()); EXPECT_EQ(memcmp(vss_buf, vrf_ptr, 6), 0); + + uint8_t vss_control_len = 1; + auto vss_control_ptr = decode_tlv((const uint8_t *)options_ptr, + OPTION82_SUBOPT_VIRTUAL_SUBNET_CONTROL, + vss_control_len, agent_option_size); + ASSERT_NE(vss_control_ptr, nullptr); + EXPECT_EQ(vss_control_len, 0); } TEST(DHCPRelayTest, encode_relay_option_server_client_same_vrf) { @@ -898,6 +923,54 @@ TEST(DHCPRelayTest, encode_relay_option_server_client_same_vrf) { vrf_len, agent_option_size); EXPECT_EQ((uintptr_t)vrf_ptr, NULL); + + uint8_t vss_control_len = 0; + auto vss_control_ptr = decode_tlv((const uint8_t *)options_ptr, + OPTION82_SUBOPT_VIRTUAL_SUBNET_CONTROL, + vss_control_len, agent_option_size); + EXPECT_EQ((uintptr_t)vss_control_ptr, NULL); +} + +static uint32_t build_vss_reply_options(uint8_t *options, const std::string &vrf, + bool include_vss, bool include_vss_control) { + uint32_t offset = 0; + if (include_vss) { + std::vector vss_data(vrf.length() + 1, 0); + memcpy(vss_data.data() + 1, vrf.data(), vrf.length()); + offset += encode_tlv(options + offset, OPTION82_SUBOPT_VIRTUAL_SUBNET, + static_cast(vss_data.size()), vss_data.data()); + } + if (include_vss_control) { + offset += encode_tlv(options + offset, OPTION82_SUBOPT_VIRTUAL_SUBNET_CONTROL, + 0, nullptr); + } + return offset; +} + +TEST(DHCPRelayTest, validate_vss_reply) { + relay_config config = {}; + config.vlan = "Vlan10"; + config.vrf = "Vrf03"; + config.vrf_selection_opt = "enable"; + vlan_vrf_map["Vlan10"] = "Vrf01"; + + uint8_t options[32] = {}; + auto options_size = build_vss_reply_options(options, "Vrf01", true, false); + EXPECT_TRUE(validate_vss_reply(options, options_size, config, "192.0.2.1")); + + options_size = build_vss_reply_options(options, "Vrf01", true, true); + EXPECT_FALSE(validate_vss_reply(options, options_size, config, "192.0.2.1")); + + options_size = build_vss_reply_options(options, "Vrf01", true, false); + options[options_size++] = OPTION82_SUBOPT_VIRTUAL_SUBNET_CONTROL; + options[options_size++] = 1; + EXPECT_FALSE(validate_vss_reply(options, options_size, config, "192.0.2.1")); + + options_size = build_vss_reply_options(options, "Vrf02", true, false); + EXPECT_FALSE(validate_vss_reply(options, options_size, config, "192.0.2.1")); + + options_size = build_vss_reply_options(options, "Vrf01", false, false); + EXPECT_FALSE(validate_vss_reply(options, options_size, config, "192.0.2.1")); } TEST(DHCPRelayTest, to_client) { @@ -924,6 +997,8 @@ TEST(DHCPRelayTest, to_client) { relay_config config = {}; config.phy_interface = "Ethernet12"; config.vlan = "Vlan10"; + config.vrf = "Vrf01"; + config.client_sock = 1; config.link_selection_opt = "enable"; config.server_id_override_opt = "enable"; config.link_address.sin_addr.s_addr = inet_addr("192.168.10.10");