bad memcpy() in dhcp_socket.c, line 253

[harridu]

There is a bad memcpy() in prepare_dhcp(), dhcp_socket.c that works only for sizeof(id) <=4:

:
/**
 * DHCP message format, with a minimum size options buffer
 */
typedef struct __attribute__((packed)) {
	:
	uint8_t client_hw_addr[6];
	:
} dhcp_t;
:
:
	identity = transaction->get_identity(transaction);
	chunk = identity->get_encoding(identity);
	/* magic bytes, a locally administered unicast MAC */
	dhcp->client_hw_addr[0] = 0x7A;
	dhcp->client_hw_addr[1] = 0xA7;
	/* with ID specific postfix */
	if (this->identity_lease)
	{
		id = htonl(chunk_hash_static(chunk));
	}
	else
	{
		id = transaction->get_id(transaction);
	}
	memcpy(&dhcp->client_hw_addr[2], &id, sizeof(id));

	dhcp->magic_cookie = htonl(0x63825363);

[tobiasbrunner]

Which technically is fine because id is an uint32_t. So it's not really a bug (our static analyzers would have complained a long time ago if that was the case).

What fix would you suggest? Setting the length to 4 or base it directly on client_hw_addr would similarly be an issue if id was smaller than 4. So maybe min(sizeof(dhcp->client_hw_addr)-2, sizeof(id))? Or even use a dynamic offset based on the size of id in relation to the size of client_hw_addr? That seems overly complicated for something that's not really an issue in the first place.

[harridu]

Sorry, you are right, I was too blind to see.

[tobiasbrunner]

No worries. It's nice the code gets looked at :)