GitLab

Describe the bug Segfault of dhcrelay -a upon receipt of answer packet not intended for us, but with a circuit ID that resolves to a local interface.

To Reproduce Steps to reproduce the behavior:

1. Configure this setup with all relevant routes (think of R1 and R2 as failover partners in a VRRP setup with dynamic routing):
   • R1 has interfaces
     • eth0 with address 10.0.0.1/24 (backbone)
     • eth1 with address 10.0.1.1/24 (clients1)
     • eth2 without address
     • eth3 without address
   • R2 has interfaces
     • eth0 with address 10.0.0.2/24 (backbone)
     • eth1 without address
     • eth2 with address 10.0.2.2/24 (clients2)
     • eth3 with address 10.0.3.2/24 (to DHCP-Server)
2. Run dhcrelay -a -iu eth0 -iu eth3 -id eth1 -id eth2 10.0.3.100 on both R1 and R2
3. A client in 10.0.1.1/24 (clients1) sends a request via relay R1, that relays it to 10.0.3.100. The relayed packet is routed by R2 like a regular IP packet
4. The server 10.0.3.100 replies to R1 via unicast
5. dhcrelay on R2 sees the answer packet and inspects it. It doesn't find a matching interface via giaddr, but via Circuit ID, which contains "eth1". But eth1 doesn't have an address configured that it could use to send the reply, so dhcrelay segfaults.

Expected behavior R2 should ignore the packet.

Environment:

• ISC DHCP version: Tested on 4.3.5-3+deb9u1, but relevant code is present in master
• OS: Debian Stretch x64

Additional Information The segfault itself is caused in dhcrelay.c:

if (send_packet(out, NULL, packet, length, out->addresses[0], &to, htop) < 0) {

Because strip_relay_agent_options() earlier set out to eth1, out->addresses is a NULL reference because eth1 doesn't have any addresses.

The segfault should be avoided by checking that out is not only non-NULL but also has addresses:

--- a/relay/dhcrelay.c
+++ b/relay/dhcrelay.c
@@ -902,7 +902,7 @@ do_relay4(struct interface_info *ip, struct dhcp_packet *packet,
                      strip_relay_agent_options(ip, &out, packet, length)))
                        return;
 
-               if (!out) {
+               if (!out || out->address_count == 0) {
                        log_error("Packet to bogus giaddr %s.\n",
                              inet_ntoa(packet->giaddr));
                        ++bogus_giaddr_drops;

(This whole bug report would have been much easier as a merge request, but I'm not allowed to clone the repository)