bin/39345: buffer overflow in dhcpcd leading to incorrect hardware address information

[Taylor R Campbell <campbell%mumble.net@localhost>]

>Number:         39345
>Category:       bin
>Synopsis:       buffer overflow in dhcpcd leading to incorrect hardware 
>address information
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Aug 12 23:15:00 +0000 2008
>Originator:     Taylor R Campbell <campbell%mumble.net@localhost>
>Release:        NetBSD 4.99.72
>Organization:
>Environment:
System: NetBSD icon.localdomain 4.99.72 NetBSD 4.99.72 (RIAXEN3_DOMU) #6: Sat 
Aug  9 03:34:22 UTC 2008  
riastradh%smalltalk.localdomain@localhost:/home/riastradh/netbsd/current/obj/sys/arch/i386/compile/RIAXEN3_DOMU
 i386
Architecture: i386
Machine: i386
>Description:

        `do_interface' in net.c of dhcpcd statically allocates a struct
        sockaddr_dl and copies sizeof(struct sockaddr_dl) bytes into it
        from a struct ifreq's ifr_addr.  Then it copies into a buffer
        for the hardware address as many bytes as the sockaddr_dl's
        sdl_alen specifies, which may exceed the bounds of the
        statically allocated struct sockaddr_dl, leading to random
        bytes at the end of the hardware address.

>How-To-Repeat:

        When I run dhcpcd to configure an interface, it chooses a
        random last byte of the hardware address.  This sometimes leads
        to a failure to configure the interface by DHCP at all.

>Fix:

        Apply either of the following patches to
        src/external/bsd/dist/dhcpcd/dist/net.c:

--- net.c       27 Jul 2008 18:06:30 +0000      1.1.1.1
+++ net.c       12 Aug 2008 22:55:15 +0000      
@@ -197,7 +197,7 @@
        struct sockaddr_in netmask;
 
 #ifdef AF_LINK
-       struct sockaddr_dl sdl;
+       struct sockaddr_dl *sdl;
 #endif
 
        if ((s = socket(AF_INET, SOCK_DGRAM, 0)) == -1)
@@ -244,10 +244,10 @@
 
 #ifdef AF_LINK
                if (hwaddr && hwlen && ifr->ifr_addr.sa_family == AF_LINK) {
-                       memcpy(&sdl, &ifr->ifr_addr, sizeof(sdl));
-                       *hwlen = sdl.sdl_alen;
-                       memcpy(hwaddr, sdl.sdl_data + sdl.sdl_nlen,
-                              (size_t)sdl.sdl_alen);
+                       sdl = (struct sockaddr_dl *)&ifr->ifr_addr;
+                       *hwlen = sdl->sdl_alen;
+                       memcpy(hwaddr, sdl->sdl_data + sdl->sdl_nlen,
+                              (size_t)sdl->sdl_alen);
                        retval = 1;
                        break;
                }

        The code originally copied the contents of the ifr_addr into a
        local buffer known to be aligned for a struct sockaddr_dl.  The
        above patch may cause access to misaligned struct fields;
        although I doubt this, the following patch avoids it.

--- net.c       27 Jul 2008 18:06:30 +0000      1.1.1.1
+++ net.c       12 Aug 2008 23:00:43 +0000      
@@ -197,7 +197,7 @@
        struct sockaddr_in netmask;
 
 #ifdef AF_LINK
-       struct sockaddr_dl sdl;
+       struct sockaddr_dl *sdl;
 #endif
 
        if ((s = socket(AF_INET, SOCK_DGRAM, 0)) == -1)
@@ -244,10 +244,12 @@
 
 #ifdef AF_LINK
                if (hwaddr && hwlen && ifr->ifr_addr.sa_family == AF_LINK) {
-                       memcpy(&sdl, &ifr->ifr_addr, sizeof(sdl));
-                       *hwlen = sdl.sdl_alen;
-                       memcpy(hwaddr, sdl.sdl_data + sdl.sdl_nlen,
-                              (size_t)sdl.sdl_alen);
+                       sdl = xmalloc(ifr->ifr_addr.sa_len);
+                       memcpy(sdl, &ifr->ifr_addr, ifr->ifr_addr.sa_len);
+                       *hwlen = sdl->sdl_alen;
+                       memcpy(hwaddr, sdl->sdl_data + sdl->sdl_nlen,
+                              (size_t)sdl->sdl_alen);
+                       free(sdl);
                        retval = 1;
                        break;
                }