Buffer Overflow in DnssdServer::AdvertiseOperational

[mchlswyr]

Setup

I compiled for target_os="linux" target_cpu="arm". My system has many kernel network interfaces, some of which have MAC addresses longer than 8 bytes. I am on an older commit, 796ba98, however this bug still appears to be present in the code as of 00e3dfb.

Problem

Expected Behaviour

During commissioning the node receives its NOC and then begins its operational advertisement by calling AdvertiseOperational. This function allocates a buffer of kPrimaryMACAddressLength (8) bytes in its stack frame, and then it calls GetPrimaryMACAddress, which calls GetPrimaryWiFiMACAddress. GetPrimaryWiFiMACAddress then populates the 8-byte buffer with the MAC address of a network interface.

Actual Behaviour

When an interface's MAC address length is greater than 8 bytes, and it is chosen in GetPrimaryWiFiMACAddress, then memcpy will write past the end of AdvertiseOperational's stack frame. This can result in a segmentation fault or, when stack protection is enabled, stack smashing:

*** stack smashing detected ***: terminated
Aborted (core dumped)

Reason

The problem occurs because mac->sll_halen is not checked before the call to memcpy; the destination is 8 bytes, yet more than 8 bytes will be copied from the source when mac->sll_halen is greater than 8.

Proposed Solution

The ifaddrs returned by getifaddrs are filtered by sa_family to obtain a sockaddr_ll. These sockaddr_lls also need to be filtered such that the resulting sockaddr_lls have a MAC address of length 8 bytes or less. The sockaddr_lls can also be filtered such that the resulting sockaddr_lls are WiFi or Ethernet interfaces. Here is what I added to ConfigurationManagerImpl.cpp to solve this issue:

...
#include <linux/if_arp.h>
// #include <netpacket/packet.h>
...
CHIP_ERROR ConfigurationManagerImpl::GetPrimaryWiFiMACAddress(uint8_t * buf)
{
...
            struct sockaddr_ll * mac = (struct sockaddr_ll *) addr->ifa_addr;

            if ((mac->sll_hatype == ARPHRD_IEEE80211 || mac->sll_hatype == ARPHRD_ETHER) && mac->sll_halen <= 8) {
                memcpy(buf, mac->sll_addr, mac->sll_halen);
                found = true;
                break;
            }
...
}

I am not sure what exactly this MAC address is used for, but based on how the interfaces are currently being filtered, a WiFi or Ethernet interface's MAC address may not be needed. In any case, the call to memcpy should occur only after ensuring that mac->sll_halen <= 8.

[bzbarsky-apple]

My system has many kernel network interfaces, some of which have MAC addresses longer than 8 bytes.

@mchlswyr I'm a little curious about this part. What does man packet on your system say about the definition of struct sockaddr_ll and its sll_addr field? What I've seen in the past is:

    unsigned char  sll_addr[8];  /* Physical layer address */