[peter at p12n.org: dhcp 3.0.3 theoretical alignment bug]

[Andrew Pollock]

Hi,

Spent some time investigating a bug in 3.0.2 that seemed to be partially
fixed (but without anything appropriate that I could see in the release
notes in 3.0.3).

regards

Andrew

----- Forwarded message from Peter Samuelson <peter at p12n.org> -----

From: Peter Samuelson <peter at p12n.org>
To: dhcp-server at isc.org
Cc: apollock at debian.org
Subject: dhcp 3.0.3 theoretical alignment bug
Date: Tue, 30 Aug 2005 01:11:09 -0500
User-Agent: Mutt/1.5.9i


So, I was helping someone track down a pointer alignment bug in dhcp
3.0.2, and it turned out to be fixed in dhcp 3.0.3.  However, there is
another bug of exactly the same type, in the same function, *not* fixed
in 3.0.3.  Why it does not trigger in the same situation, I don't know.

The 3.0.2 bug was in decode_udp_ip_header(): 'struct ip *ip', which was
changed to a stack variable 'struct ip ip' and copied from the input
buffer, for proper struct alignment.  The related variable 'struct
udphdr *udp' was not changed in the same way and I think it should have
been.

Practical result: the UDP checksum in the input buffer is no longer
zeroed.  I didn't check to see if this mattered.


diff -urNad dhcp-3.0.3/common/packet.c dhcp3-3.0.3/common/packet.c
--- dhcp-3.0.3/common/packet.c
+++ dhcp-3.0.3/common/packet.c
@@ -219,7 +219,7 @@
 {
   unsigned char *data;
   struct ip ip;
-  struct udphdr *udp;
+  struct udphdr udp;
   u_int32_t ip_len = (buf [bufix] & 0xf) << 2;
   u_int32_t sum, usum;
   static int ip_packets_seen;
@@ -233,7 +233,7 @@
   int ignore = 0;
 
   memcpy(&ip, buf + bufix, sizeof (struct ip));
-  udp = (struct udphdr *)(buf + bufix + ip_len);
+  memcpy(&udp, buf + bufix + ip_len, sizeof (struct udphdr));
 
 #ifdef USERLAND_FILTER
   /* Is it a UDP packet? */
@@ -241,13 +241,13 @@
 	  return -1;
 
   /* Is it to the port we're serving? */
-  if (udp -> uh_dport != local_port)
+  if (udp.uh_dport != local_port)
 	  return -1;
 #endif /* USERLAND_FILTER */
 
-  ulen = ntohs (udp -> uh_ulen);
-  if (ulen < sizeof *udp ||
-      ((unsigned char *)udp) + ulen > buf + bufix + buflen) {
+  ulen = ntohs (udp.uh_ulen);
+  if (ulen < sizeof udp ||
+      ((unsigned char *)&udp) + ulen > buf + bufix + buflen) {
 	  log_info ("bogus UDP packet length: %d", ulen);
 	  return -1;
   }
@@ -281,8 +281,8 @@
      header and the data.   If the UDP checksum field is zero, we're
      not supposed to do a checksum. */
 
-  data = buf + bufix + ip_len + sizeof *udp;
-  len = ulen - sizeof *udp;
+  data = buf + bufix + ip_len + sizeof udp;
+  len = ulen - sizeof udp;
   ++udp_packets_length_checked;
   if (len + data > buf + bufix + buflen) {
 	  ++udp_packets_length_overflow;
@@ -302,14 +302,14 @@
 	  log_debug ("accepting packet with data after udp payload.");
   if (len + data > buf + bufix + buflen) {
 	  log_debug ("dropping packet with bogus uh_ulen %ld",
-		     (long)(len + sizeof *udp));
+		     (long)(len + sizeof udp));
 	  return -1;
   }
 
-  usum = udp -> uh_sum;
-  udp -> uh_sum = 0;
+  usum = udp.uh_sum;
+  udp.uh_sum = 0;
 
-  sum = wrapsum (checksum ((unsigned char *)udp, sizeof *udp,
+  sum = wrapsum (checksum ((unsigned char *)&udp, sizeof udp,
 			   checksum (data, len,
 				     checksum ((unsigned char *)
 					       &ip.ip_src,
@@ -330,8 +330,8 @@
   }
 
   /* Copy out the port... */
-  memcpy (&from -> sin_port, &udp -> uh_sport, sizeof udp -> uh_sport);
+  memcpy (&from -> sin_port, &udp.uh_sport, sizeof udp.uh_sport);
 
-  return ip_len + sizeof *udp;
+  return ip_len + sizeof udp;
 }
 #endif /* PACKET_DECODING */



----- End forwarded message -----