[BusyBox] udhcpc transaction id and chaddr checking

[Tobias Krawutschke]

Hello,

Setup: Multiple embedded systems running busybox dhcpc, and a DHCP
Server configured to assign fixed IPs to the Boards.

I ran into a problem that a board sometimes took a IP Address not
assigned to it and so having two boards with the same IP Address. A
solution was mentioned in the README.udhcpc, section: note on udhcpc's
random seed.

This decreased the probability of accidently multiple xids, but it did 
not
eliminate the problem and with an increasing number of boards this 
probability
also increases.

Another possibility would be to prevent the DHCP Server from sending
out the offer and ack as broadcasts. Then the ethernet header would
guarantee the delivery to the right addresses. A third option is to
check the chaddr field in the DHCP Header what this patch does.

I added right after the check of the xid to check the chaddr
field. This field is filled in by the client and must be copied by the
server. It is a configurable option (CONFIG_UDHCPC_CHECKACCURATE) and
increases size a little bit:

$ arm-linux-size busybox.without-checkaccurate 
   text	   data	    bss	    dec	    hex	filename
 346981	   3484	  28676	 379141	  5c905	busybox.without-checkaccurate

$ arm-linux-size busybox.with-checkaccurate    
   text	   data	    bss	    dec	    hex	filename
 347009	   3484	  28676	 379169	  5c921	busybox.with-checkaccurate

I am uncertain if this change is ok to do. I found nothing in RFC 2131
regarding SHOULD/MUST/ etc to check the chaddr field. So is this
a bug or not?

I verified that dhclient (3.0.2) is checking chaddr, too but I did not
get in to deep of this software - i checked that it compares chaddr
in three functions: dhcpack, dhcpoffer, dhcpnak.  Maybe I
missed something and I appreciate your comments.

Funny, while writing this, I see that this is an assigned bug
(260). For Month or longer this is undiscoverd, now it comes up...
I won't start a new patch request, as this fix from keithsmith is the 
same, aside from he is confident it is a bug.

Tobias


-------------- next part --------------
Index: networking/udhcp/dhcpc.c
===================================================================
--- networking/udhcp/dhcpc.c	(revision 10247)
+++ networking/udhcp/dhcpc.c	(working copy)
@@ -32,6 +32,7 @@
 #include <string.h>
 #include <sys/ioctl.h>
 #include <net/if.h>
+#include <netinet/ether.h> 
 #include <errno.h>
 
 #include "dhcpd.h"
@@ -440,6 +441,15 @@
 				continue;
 			}
 
+#ifdef CONFIG_UDHCPC_CHECKCHADDR
+			DEBUG(LOG_DEBUG, "Received chaddr: %s",ether_ntoa((struct ether_addr *) packet.chaddr));  
+			DEBUG(LOG_DEBUG, "     Our chaddr: %s",ether_ntoa((struct ether_addr *) client_config.arp));
+
+			if (memcmp(client_config.arp,packet.chaddr,6)) {
+ 			  DEBUG(LOG_DEBUG, "Ignoring packet with chaddr not for us: %s", ether_ntoa((struct ether_addr *) packet.chaddr));
+			  continue;
+			}
+#endif
 			if ((message = get_option(&packet, DHCP_MESSAGE_TYPE)) == NULL) {
 				DEBUG(LOG_ERR, "couldnt get option from packet -- ignoring");
 				continue;
Index: networking/udhcp/Config.in
===================================================================
--- networking/udhcp/Config.in	(revision 10247)
+++ networking/udhcp/Config.in	(working copy)
@@ -26,6 +26,23 @@
 
 	  See http://udhcp.busybox.net for further details.
 
+config CONFIG_UDHCPC_CHECKCHADDR
+	bool "Check accurately DHCP Packets"
+	default n
+	help
+	  Incoming DHCP packets are checked if the chaddr field and 
+	  the xid is set (normaly only the xid). 
+
+	  This prevents from multiple IP-Address assignments if there
+	  are equal transactions ID's by accident on the network.
+
+	  Multiple embedded systems with bad random number generators
+	  can lead to this problem. See also README.udhcpc: note on random 
+	  seed.
+	  
+	  See http://udhcp.busybox.net for further details.
+
+
 config CONFIG_DUMPLEASES
 	bool "Lease display utility (dumpleases)"
 	default n