Skip to content

[dhcp4relay] Reject chained requests with a local giaddr #126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Xichen96 wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[dhcp4relay] Reject chained requests with a local giaddr #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions dhcp4relay/src/dhcp4relay.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,37 @@ void encode_relay_option(pcpp::DhcpLayer *dhcp_pkt, relay_config *config) {
return;
}

enum class LocalAddressCheck {
ERROR,
NOT_LOCAL,
LOCAL
};

static LocalAddressCheck check_local_ipv4_address(uint32_t address) {
struct ifaddrs *ifaddrs_list = nullptr;
if (getifaddrs(&ifaddrs_list) == -1) {
SWSS_LOG_ERROR("[DHCPV4_RELAY] Unable to enumerate local IPv4 addresses: %s",
strerror(errno));
return LocalAddressCheck::ERROR;
}

LocalAddressCheck result = LocalAddressCheck::NOT_LOCAL;
for (struct ifaddrs *ifa = ifaddrs_list; ifa != nullptr; ifa = ifa->ifa_next) {
if (ifa->ifa_addr == nullptr || ifa->ifa_addr->sa_family != AF_INET) {
continue;
}

auto *local_address = reinterpret_cast<struct sockaddr_in *>(ifa->ifa_addr);
if (local_address->sin_addr.s_addr == address) {
result = LocalAddressCheck::LOCAL;
break;
}
}

freeifaddrs(ifaddrs_list);
return result;
}

/**
* @code void from_client(pcpp::DhcpLayer* dhcp_pkt, relay_config *config)
*
Expand Down Expand Up @@ -594,6 +625,26 @@ void from_client(pcpp::DhcpLayer *dhcp_pkt, relay_config &config) {
encode_relay_option(dhcp_pkt, &config);
}
} else {
const uint32_t giaddr = dhcp_pkt->getDhcpHeader()->gatewayIpAddress;
const LocalAddressCheck address_check = check_local_ipv4_address(giaddr);
if (address_check == LocalAddressCheck::ERROR) {
SWSS_LOG_ERROR("[DHCPV4_RELAY] Dropping chained packet because local giaddr validation failed on %s",
config.vlan.c_str());
dhcp_cntr_table.increment_counter(config.vlan, "TX", DHCPv4_MESSAGE_TYPE_DROP);
return;
}
if (address_check == LocalAddressCheck::LOCAL) {
char giaddr_string[INET_ADDRSTRLEN] = {};
struct in_addr giaddr_address = {giaddr};
const char *formatted_giaddr =
inet_ntop(AF_INET, &giaddr_address, giaddr_string, sizeof(giaddr_string));
SWSS_LOG_NOTICE("[DHCPV4_RELAY] Dropping chained packet with local giaddr %s on %s",
formatted_giaddr == nullptr ? "<invalid>" : formatted_giaddr,
config.vlan.c_str());
dhcp_cntr_table.increment_counter(config.vlan, "TX", DHCPv4_MESSAGE_TYPE_DROP);
return;
}

/* If the relay packet is from another relay, we should act based on
configuration of agent_relay_mode.
append - Forward the packet with appending our own relay option.
Expand Down
63 changes: 63 additions & 0 deletions dhcp4relay/test/mock_relay.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -935,6 +935,17 @@ TEST(DHCPRelayTest, to_client) {
to_client(&dhcpLayer, &vlans, "172.22.178.234");
}

static struct ifaddrs *expect_local_address_check(
const std::string &vlan_ip = "192.168.10.10",
const std::string &source_ip = "192.168.1.2") {
struct ifaddrs *mock_ifaddrs = CreateMockIfaddrs(
vlan_ip, "255.255.255.0", "Vlan10", source_ip, "Ethernet12");
EXPECT_GLOBAL_CALL(getifaddrs, getifaddrs(_))
.WillOnce(DoAll(testing::SetArgPointee<0>(mock_ifaddrs), Return(0)));
EXPECT_GLOBAL_CALL(freeifaddrs, freeifaddrs(_)).Times(1);
return mock_ifaddrs;
}

TEST(DHCPRelayTest, from_client) {

pcpp::MacAddress clientMac(std::string("00:0e:86:11:c0:75"));
Expand Down Expand Up @@ -964,6 +975,7 @@ TEST(DHCPRelayTest, from_client) {
m_config.host_mac_addr = "12:32:54:24:95:36";
encode_relay_option(&dhcpLayer, &config);

struct ifaddrs *mock_ifaddrs = expect_local_address_check();
EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).WillOnce([]
(int sock, uint8_t* hdr, struct sockaddr_in target, uint32_t len, in_addr src_ip, bool use_src_ip, bool pad) {
pcpp::dhcp_header* dhcp_hdr = (pcpp::dhcp_header*)hdr;
Expand All @@ -973,6 +985,7 @@ TEST(DHCPRelayTest, from_client) {
return true;
});
from_client(&dhcpLayer, config);
FreeMockIfaddrs(mock_ifaddrs);
}

/* Helper: build a relay-of-relay packet (giaddr already set) with a pre-existing Option 82. */
Expand All @@ -997,6 +1010,48 @@ static relay_config make_relay_of_relay_config(const std::string &agent_relay_mo
return config;
}

TEST(DHCPRelayTest, from_client_relay_of_relay_local_giaddr_is_dropped) {
relay_config config = make_relay_of_relay_config("forward");

pcpp::MacAddress clientMac(std::string("00:0e:86:11:c0:75"));
pcpp::DhcpLayer dhcpLayer(pcpp::DHCP_DISCOVER, clientMac);
dhcpLayer.getDhcpHeader()->hops = 2;
dhcpLayer.getDhcpHeader()->gatewayIpAddress = inet_addr("192.168.1.2");
encode_relay_option(&dhcpLayer, &config);

struct ifaddrs *mock_ifaddrs = expect_local_address_check();
EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).Times(0);
from_client(&dhcpLayer, config);

EXPECT_EQ(dhcpLayer.getDhcpHeader()->hops, 2);
EXPECT_EQ(dhcpLayer.getDhcpHeader()->gatewayIpAddress, inet_addr("192.168.1.2"));
FreeMockIfaddrs(mock_ifaddrs);
}

TEST(DHCPRelayTest, from_client_relay_of_relay_nonlocal_giaddr_forwards) {
relay_config config = make_relay_of_relay_config("forward");

pcpp::MacAddress clientMac(std::string("00:0e:86:11:c0:75"));
pcpp::DhcpLayer dhcpLayer(pcpp::DHCP_DISCOVER, clientMac);
dhcpLayer.getDhcpHeader()->hops = 2;
dhcpLayer.getDhcpHeader()->gatewayIpAddress = inet_addr("203.0.113.10");
encode_relay_option(&dhcpLayer, &config);

struct ifaddrs *mock_ifaddrs = expect_local_address_check();
EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).WillOnce([]
(int, uint8_t *hdr, struct sockaddr_in, uint32_t, in_addr, bool, bool) {
pcpp::dhcp_header *dhcp_hdr = reinterpret_cast<pcpp::dhcp_header *>(hdr);
EXPECT_EQ(dhcp_hdr->hops, 3);
EXPECT_EQ(dhcp_hdr->gatewayIpAddress, inet_addr("203.0.113.10"));
return true;
});
from_client(&dhcpLayer, config);

EXPECT_EQ(dhcpLayer.getDhcpHeader()->hops, 3);
EXPECT_EQ(dhcpLayer.getDhcpHeader()->gatewayIpAddress, inet_addr("203.0.113.10"));
FreeMockIfaddrs(mock_ifaddrs);
}

/* agent_relay_mode=append: packet already has Option 82; we should append ours and forward. */
TEST(DHCPRelayTest, from_client_relay_of_relay_append) {
relay_config config = make_relay_of_relay_config("append");
Expand All @@ -1008,6 +1063,7 @@ TEST(DHCPRelayTest, from_client_relay_of_relay_append) {
dhcpLayer.getDhcpHeader()->gatewayIpAddress = inet_addr("192.168.1.1");
encode_relay_option(&dhcpLayer, &config);

struct ifaddrs *mock_ifaddrs = expect_local_address_check();
EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).WillOnce([]
(int, uint8_t* hdr, struct sockaddr_in, uint32_t, in_addr, bool, bool) {
pcpp::dhcp_header* dhcp_hdr = (pcpp::dhcp_header*)hdr;
Expand All @@ -1016,6 +1072,7 @@ TEST(DHCPRelayTest, from_client_relay_of_relay_append) {
return true;
});
from_client(&dhcpLayer, config);
FreeMockIfaddrs(mock_ifaddrs);
}

/* agent_relay_mode=replace: existing Option 82 stripped, ours added, packet forwarded. */
Expand All @@ -1028,6 +1085,7 @@ TEST(DHCPRelayTest, from_client_relay_of_relay_replace) {
dhcpLayer.getDhcpHeader()->gatewayIpAddress = inet_addr("192.168.1.1");
encode_relay_option(&dhcpLayer, &config);

struct ifaddrs *mock_ifaddrs = expect_local_address_check();
EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).WillOnce([]
(int, uint8_t* hdr, struct sockaddr_in, uint32_t, in_addr, bool, bool) {
pcpp::dhcp_header* dhcp_hdr = (pcpp::dhcp_header*)hdr;
Expand All @@ -1036,6 +1094,7 @@ TEST(DHCPRelayTest, from_client_relay_of_relay_replace) {
return true;
});
from_client(&dhcpLayer, config);
FreeMockIfaddrs(mock_ifaddrs);
}

/* agent_relay_mode=forward: packet forwarded unchanged, Option 82 not modified. */
Expand All @@ -1048,6 +1107,7 @@ TEST(DHCPRelayTest, from_client_relay_of_relay_forward) {
dhcpLayer.getDhcpHeader()->gatewayIpAddress = inet_addr("192.168.1.1");
encode_relay_option(&dhcpLayer, &config);

struct ifaddrs *mock_ifaddrs = expect_local_address_check();
EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).WillOnce([]
(int, uint8_t* hdr, struct sockaddr_in, uint32_t, in_addr, bool, bool) {
pcpp::dhcp_header* dhcp_hdr = (pcpp::dhcp_header*)hdr;
Expand All @@ -1056,6 +1116,7 @@ TEST(DHCPRelayTest, from_client_relay_of_relay_forward) {
return true;
});
from_client(&dhcpLayer, config);
FreeMockIfaddrs(mock_ifaddrs);
}

/* agent_relay_mode=discard: packet must be dropped, send_udp must NOT be called. */
Expand All @@ -1068,6 +1129,8 @@ TEST(DHCPRelayTest, from_client_relay_of_relay_discard) {
dhcpLayer.getDhcpHeader()->gatewayIpAddress = inet_addr("192.168.1.1");
encode_relay_option(&dhcpLayer, &config);

struct ifaddrs *mock_ifaddrs = expect_local_address_check();
EXPECT_GLOBAL_CALL(send_udp, send_udp(_, _, _, _, _, _, _)).Times(0);
from_client(&dhcpLayer, config);
FreeMockIfaddrs(mock_ifaddrs);
}
Loading