Re: [lwip-devel] [bug #36492] Static Analysis on code 1.4.0

[Mason]

Mason wrote:

>> bayard wrote:
>>
>>> ABR : Buffer overflow, array index of 'hwaddr' may be out of bounds.
>>> Array 'hwaddr' of size 6 may use index value(s) 6..15 :
>>> lwip/src/core/dhcp.c : 1683 : Critical : Analyze
>>
>> I assume line 1698 in the 1.4.x branch.
>>
>>   for (i = 0; i < DHCP_CHADDR_LEN; i++) {
>>     /* copy netif hardware address, pad with zeroes */
>>     dhcp->msg_out->chaddr[i] = (i < netif->hwaddr_len) ? netif->hwaddr[i] : 
>> 0/* pad byte*/;
>>   }
> 
> Simon, Kieran,
> 
> I have a patch for dhcp_create_msg.
> 
> Instead of setting several fields to 0 (sometimes using loops
> to clear arrays one byte (!!) at a time), I propose clearing
> the whole struct, and then writing only non-zero fields.
> 
> Would you accept it?

There was a copy/paste bug. The complete patch was:

--- dhcp.c.orig 2012-05-14 14:10:21.578125000 +0200
+++ dhcp.c      2012-05-23 12:35:20.880351400 +0200
@@ -1672,39 +1672,21 @@
               ("transaction id xid(%"X32_F")\n", xid));
 
   dhcp->msg_out = (struct dhcp_msg *)dhcp->p_out->payload;
+  memset(dhcp->msg_out, 0, sizeof *dhcp->msg_out);
 
   dhcp->msg_out->op = DHCP_BOOTREQUEST;
   /* TODO: make link layer independent */
   dhcp->msg_out->htype = DHCP_HTYPE_ETH;
   dhcp->msg_out->hlen = netif->hwaddr_len;
-  dhcp->msg_out->hops = 0;
   dhcp->msg_out->xid = htonl(dhcp->xid);
-  dhcp->msg_out->secs = 0;
-  /* we don't need the broadcast flag since we can receive unicast traffic
-     before being fully configured! */
-  dhcp->msg_out->flags = 0;
-  ip_addr_set_zero(&dhcp->msg_out->ciaddr);
   /* set ciaddr to netif->ip_addr based on message_type and state */
   if ((message_type == DHCP_INFORM) || (message_type == DHCP_DECLINE) ||
       ((message_type == DHCP_REQUEST) && /* DHCP_BOUND not used for sending! */
        ((dhcp->state==DHCP_RENEWING) || dhcp->state==DHCP_REBINDING))) {
     ip_addr_copy(dhcp->msg_out->ciaddr, netif->ip_addr);
   }
-  ip_addr_set_zero(&dhcp->msg_out->yiaddr);
-  ip_addr_set_zero(&dhcp->msg_out->siaddr);
-  ip_addr_set_zero(&dhcp->msg_out->giaddr);
-  for (i = 0; i < DHCP_CHADDR_LEN; i++) {
-    /* copy netif hardware address, pad with zeroes */
-    dhcp->msg_out->chaddr[i] = (i < netif->hwaddr_len) ? netif->hwaddr[i] : 
0/* pad byte*/;
-  }
-  for (i = 0; i < DHCP_SNAME_LEN; i++) {
-    dhcp->msg_out->sname[i] = 0;
-  }
-  for (i = 0; i < DHCP_FILE_LEN; i++) {
-    dhcp->msg_out->file[i] = 0;
-  }
+  MEMCPY(dhcp->msg_out->chaddr, netif->hwaddr, netif->hwaddr_len);
   dhcp->msg_out->cookie = PP_HTONL(DHCP_MAGIC_COOKIE);
-  dhcp->options_out_len = 0;
   /* fill options field with an incrementing array (for debugging purposes) */
   for (i = 0; i < DHCP_OPTIONS_LEN; i++) {
     dhcp->msg_out->options[i] = (u8_t)i; /* for debugging only, no matter if 
truncated */


-- 
Regards.