dhcp: ignore padding of 'chaddr' in DHCP server response

The "chaddr" field is 16 bytes long, with "hlen" being the
length of the address.

https://tools.ietf.org/html/rfc2131#section-4.3.1 says:

    The server MUST return to the client:

    ...

    o Any parameters specific to this client (as identified by
      the contents of 'chaddr' or 'client identifier' in the DHCPDISCOVER
      or DHCPREQUEST message), e.g., as configured by the network
      administrator,

It's not clear, whether only the first 'hlen' bytes of 'chaddr'
must correspond or all 16 bytes.

Note that https://tools.ietf.org/html/rfc4390#section-2.1 says for IPoIB

    "chaddr" (client hardware address) field MUST be zeroed.

with having "hlen" zero. This indicates that at least in this case, the
bytes after "hlen" would matter.

As the DHCP client always sets the trailing bytes to zero, we would expect
that the server also replies as such and we could just compare all 16 bytes.
However, let's be liberal and accept any padding here.

This in practice only changes behavior for infiniband, where we
previously would enforce that the first ETH_ALEN bytes are zero.
That seems arbitrary for IPoIB. We should either check all bytes or
none of them. Let's do the latter and don't enforce RFC 4390 in this
regard.

2 files changed, 11 insertions, 11 deletions

diff --git a/src/libsystemd-network/dhcp-network.c b/src/libsystemd-network/dhcp-network.c
index 0e5b4147a9..b62eed0dd4 100644
--- a/src/libsystemd-network/dhcp-network.c
+++ b/src/libsystemd-network/dhcp-network.c
@@ -50,12 +50,16 @@ static int _bind_raw_socket(int ifindex, union sockaddr_union *link,
                 BPF_STMT(BPF_LD + BPF_B + BPF_ABS, offsetof(DHCPPacket, dhcp.htype)),  /* A <- DHCP header type */
                 BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, arp_type, 1, 0),                   /* header type == arp_type ? */
                 BPF_STMT(BPF_RET + BPF_K, 0),                                          /* ignore */
-                BPF_STMT(BPF_LD + BPF_B + BPF_ABS, offsetof(DHCPPacket, dhcp.hlen)),   /* A <- MAC address length */
-                BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, dhcp_hlen, 1, 0),                  /* address length == dhcp_hlen ? */
-                BPF_STMT(BPF_RET + BPF_K, 0),                                          /* ignore */
                 BPF_STMT(BPF_LD + BPF_W + BPF_ABS, offsetof(DHCPPacket, dhcp.xid)),    /* A <- client identifier */
                 BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, xid, 1, 0),                        /* client identifier == xid ? */
                 BPF_STMT(BPF_RET + BPF_K, 0),                                          /* ignore */
+                BPF_STMT(BPF_LD + BPF_B + BPF_ABS, offsetof(DHCPPacket, dhcp.hlen)),   /* A <- MAC address length */
+                BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, dhcp_hlen, 1, 0),                  /* address length == dhcp_hlen ? */
+                BPF_STMT(BPF_RET + BPF_K, 0),                                          /* ignore */
+
+                /* We only support MAC address length to be either 0 or 6 (ETH_ALEN). Optionally
+                 * compare chaddr for ETH_ALEN bytes. */
+                BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ETH_ALEN, 0, 12),                                  /* A (the MAC address length) == ETH_ALEN ? */
                 BPF_STMT(BPF_LD + BPF_IMM, unaligned_read_be32(&eth_mac->ether_addr_octet[0])),        /* A <- 4 bytes of client's MAC */
                 BPF_STMT(BPF_MISC + BPF_TAX, 0),                                                       /* X <- A */
                 BPF_STMT(BPF_LD + BPF_W + BPF_ABS, offsetof(DHCPPacket, dhcp.chaddr)),                 /* A <- 4 bytes of MAC from dhcp.chaddr */
@@ -68,6 +72,7 @@ static int _bind_raw_socket(int ifindex, union sockaddr_union *link,
                 BPF_STMT(BPF_ALU + BPF_XOR + BPF_X, 0),                                                /* A xor X */
                 BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, 0, 1, 0),                                          /* A == 0 ? */
                 BPF_STMT(BPF_RET + BPF_K, 0),                                                          /* ignore */
+
                 BPF_STMT(BPF_LD + BPF_W + BPF_ABS, offsetof(DHCPPacket, dhcp.magic)),  /* A <- DHCP magic cookie */
                 BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, DHCP_MAGIC_COOKIE, 1, 0),          /* cookie == DHCP magic cookie ? */
                 BPF_STMT(BPF_RET + BPF_K, 0),                                          /* ignore */
diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c
index 35fc88ef91..ece163124a 100644
--- a/src/libsystemd-network/sd-dhcp-client.c
+++ b/src/libsystemd-network/sd-dhcp-client.c
@@ -1676,8 +1676,7 @@ static int client_receive_message_udp(
 
         sd_dhcp_client *client = userdata;
         _cleanup_free_ DHCPMessage *message = NULL;
-        const struct ether_addr zero_mac = {};
-        const struct ether_addr *expected_chaddr = NULL;
+        const uint8_t *expected_chaddr = NULL;
         uint8_t expected_hlen = 0;
         ssize_t len, buflen;
 
@@ -1722,11 +1721,7 @@ static int client_receive_message_udp(
 
         if (client->arp_type == ARPHRD_ETHER) {
                 expected_hlen = ETH_ALEN;
-                expected_chaddr = (const struct ether_addr *) &client->mac_addr;
-        } else {
-               /* Non-Ethernet links expect zero chaddr */
-               expected_hlen = 0;
-               expected_chaddr = &zero_mac;
+                expected_chaddr = &client->mac_addr[0];
         }
 
         if (message->hlen != expected_hlen) {
@@ -1734,7 +1729,7 @@ static int client_receive_message_udp(
                 return 0;
         }
 
-        if (memcmp(&message->chaddr[0], expected_chaddr, ETH_ALEN)) {
+        if (expected_hlen > 0 && memcmp(&message->chaddr[0], expected_chaddr, expected_hlen)) {
                 log_dhcp_client(client, "Received chaddr does not match expected: ignoring");
                 return 0;
         }