From 158163f248631f5f2b51b7a1ff00c3eb2c096400 Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Thu, 20 Dec 2018 13:05:13 +0100

Subject: [PATCH 1/3] dhcp: fix sd_dhcp_client_set_client_id() for infiniband

 addresses

Infiniband addresses are 20 bytes (INFINIBAND_ALEN), but only the last

8 bytes are suitable for putting into the client-id.

This bug had no effect for networkd, because sd_dhcp_client_set_client_id()

has only one caller which always uses ARPHRD_ETHER type.

I was unable to find good references for why this is correct ([1]). Fedora/RHEL

has patches for ISC dhclient that also only use the last 8 bytes ([2], [3]).

RFC 4390 (Dynamic Host Configuration Protocol (DHCP) over InfiniBand) [4] does

not discuss the content of the client-id either.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1658057#c29

[2] https://bugzilla.redhat.com/show_bug.cgi?id=660681

[3] https://src.fedoraproject.org/rpms/dhcp/blob/3ccf3c8d815df4b8e11e1a04850975f099273d5d/f/dhcp-lpf-ib.patch

[4] https://tools.ietf.org/html/rfc4390

https://github.com/systemd/systemd/commit/b9d80714583bf40e354ad0fc364ebfb35a0b3d76

(cherry picked from commit 24a62f90c764c4c84fc95c654b76842dcf48118e)

(cherry picked from commit 56f5edcbd31b91b9d0fd23f9b70d3de04ab70d1a)

---

 src/systemd/src/libsystemd-network/sd-dhcp-client.c | 4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/systemd/src/libsystemd-network/sd-dhcp-client.c b/src/systemd/src/libsystemd-network/sd-dhcp-client.c

index 915894537..ceb1709d1 100644

--- a/src/systemd/src/libsystemd-network/sd-dhcp-client.c

+++ b/src/systemd/src/libsystemd-network/sd-dhcp-client.c

@@ -309,7 +309,9 @@ int sd_dhcp_client_set_client_id(

                 break;

         case ARPHRD_INFINIBAND:

-                if (data_len != INFINIBAND_ALEN)

+                /* Infiniband addresses are 20 bytes (INFINIBAND_ALEN), however only

+                 * the last 8 bytes are stable and suitable for putting into the client-id. */

+                if (data_len != 8)

                         return -EINVAL;

                 break;

-- 

2.20.1

From 68aa4fe865ea76bc4d03194d3eb9a5e6c09b08d9 Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Wed, 19 Dec 2018 10:05:37 +0100

Subject: [PATCH 2/3] dhcp: don't enforce hardware address length for

 sd_dhcp_client_set_client_id()

sd_dhcp_client_set_client_id() is the only API for setting a raw client-id.

All other setters are more restricted and only allow to set a type 255 DUID.

Also, dhcp4_set_client_identifier() is the only caller, which already

does:

                r = sd_dhcp_client_set_client_id(link->dhcp_client,

                                                 ARPHRD_ETHER,

                                                 (const uint8_t *) &link->mac,

                                                 sizeof(link->mac));

and hence ensures that the data length is indeed ETH_ALEN.

Drop additional input validation from sd_dhcp_client_set_client_id(). The client-id

is an opaque blob, and if a caller wishes to set type 1 (ethernet) or type 32

(infiniband) with unexpected address length, it should be allowed. The actual

client-id is not relevant to the DHCP client, and it's the responsibility of the

caller to generate a suitable client-id.

For example, in NetworkManager you can configure all the bytes of the

client-id, including such _invalid_ settings. I think it makes sense,

to allow the user to fully configure the identifier. Even if such configuration

would be rejected, it would be the responsibility of the higher layers (including

a sensible error message to the user) and not fail later during

sd_dhcp_client_set_client_id().

Still log a debug message if the length is unexpected.

https://github.com/systemd/systemd/commit/bfda0d0f09666e2476c9abd0280c4b9fa82b968c

(cherry picked from commit 0d5fec574117a59c6aa9767a1948a5e3d1a70531)

(cherry picked from commit a46745a36a825d0b14dc13d276ed192df44e0ca0)

---

 .../src/libsystemd-network/sd-dhcp-client.c   | 29 +++++++------------

 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/src/systemd/src/libsystemd-network/sd-dhcp-client.c b/src/systemd/src/libsystemd-network/sd-dhcp-client.c

index ceb1709d1..94ee88e36 100644

--- a/src/systemd/src/libsystemd-network/sd-dhcp-client.c

+++ b/src/systemd/src/libsystemd-network/sd-dhcp-client.c

@@ -301,29 +301,22 @@ int sd_dhcp_client_set_client_id(

         assert_return(data_len > 0 && data_len <= MAX_CLIENT_ID_LEN, -EINVAL);

         G_STATIC_ASSERT_EXPR (_NM_SD_MAX_CLIENT_ID_LEN == MAX_CLIENT_ID_LEN);

-        switch (type) {

-

-        case ARPHRD_ETHER:

-                if (data_len != ETH_ALEN)

-                        return -EINVAL;

-                break;

-

-        case ARPHRD_INFINIBAND:

-                /* Infiniband addresses are 20 bytes (INFINIBAND_ALEN), however only

-                 * the last 8 bytes are stable and suitable for putting into the client-id. */

-                if (data_len != 8)

-                        return -EINVAL;

-                break;

-

-        default:

-                break;

-        }

-

         if (client->client_id_len == data_len + sizeof(client->client_id.type) &&

             client->client_id.type == type &&

             memcmp(&client->client_id.raw.data, data, data_len) == 0)

                 return 0;

+        /* For hardware types, log debug message about unexpected data length.

+         *

+         * Note that infiniband's INFINIBAND_ALEN is 20 bytes long, but only

+         * last last 8 bytes of the address are stable and suitable to put into

+         * the client-id. The caller is advised to account for that. */

+        if ((type == ARPHRD_ETHER && data_len != ETH_ALEN) ||

+            (type == ARPHRD_INFINIBAND && data_len != 8))

+                log_dhcp_client(client, "Changing client ID to hardware type %u with "

+                                "unexpected address length %zu",

+                                type, data_len);

+

         if (!IN_SET(client->state, DHCP_STATE_INIT, DHCP_STATE_STOPPED)) {

                 log_dhcp_client(client, "Changing client ID on running DHCP "

                                 "client, restarting");

-- 

2.20.1

From 7bf24a0786a721619a4ab2b734d74718fbbaa793 Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Thu, 20 Dec 2018 11:56:02 +0100

Subject: [PATCH 3/3] dhcp6: don't enforce DUID content for

 sd_dhcp6_client_set_duid()

There are various functions to set the DUID of a DHCPv6 client.

However, none of them allows to set arbitrary data. The closest is

sd_dhcp6_client_set_duid(), which would still do validation of the

DUID's content via dhcp_validate_duid_len().

Relax the validation and only log a debug message if the DUID

does not validate.

Note that dhcp_validate_duid_len() already is not very strict. For example

with DUID_TYPE_LLT it only ensures that the length is suitable to contain

hwtype and time. It does not further check that the length of hwaddr is non-zero

or suitable for hwtype. Also, non-well-known DUID types are accepted for

extensibility. Why reject certain DUIDs but allowing clearly wrong formats

otherwise?

The validation and failure should happen earlier, when accepting the

unsuitable DUID. At that point, there is more context of what is wrong,

and a better failure reason (or warning) can be reported to the user. Rejecting

the DUID when setting up the DHCPv6 client seems not optimal, in particular

because the DHCPv6 client does not care about actual content of the

DUID and treats it as opaque blob.

Also, NetworkManager (which uses this code) allows to configure the entire

binary DUID in binary. It intentionally does not validate the binary

content any further. Hence, it needs to be able to set _invalid_ DUIDs,

provided that some basic constraints are satisfied (like the maximum length).

sd_dhcp6_client_set_duid() has two callers: both set the DUID obtained

from link_get_duid(), which comes from configuration.

`man networkd.conf` says: "The configured DHCP DUID should conform to

the specification in RFC 3315, RFC 6355.". It does not not state that

it MUST conform.

Note that dhcp_validate_duid_len() has another caller: DHCPv4's

dhcp_client_set_iaid_duid_internal(). In this case, continue with

strict validation, as the callers are more controlled. Also, there is

already sd_dhcp_client_set_client_id() which can be used to bypass

this check and set arbitrary client identifiers.

https://github.com/systemd/systemd/commit/ab4a88bc29e31754ec50c4a865058ee36f6284a6

(cherry picked from commit d65ee3bb18fd27072113065c189e76d8abe16b25)

(cherry picked from commit 87baa2678a2bb67896da1382f0ddef2f54ef585d)

---

 src/systemd/src/libsystemd-network/dhcp-identifier.c |  8 +++++++-

 src/systemd/src/libsystemd-network/dhcp-identifier.h |  2 +-

 src/systemd/src/libsystemd-network/sd-dhcp-client.c  |  2 +-

 src/systemd/src/libsystemd-network/sd-dhcp6-client.c | 10 +++++++---

 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/systemd/src/libsystemd-network/dhcp-identifier.c b/src/systemd/src/libsystemd-network/dhcp-identifier.c

index f18713e8d..b1afbf009 100644

--- a/src/systemd/src/libsystemd-network/dhcp-identifier.c

+++ b/src/systemd/src/libsystemd-network/dhcp-identifier.c

@@ -25,13 +25,19 @@

 #define APPLICATION_ID SD_ID128_MAKE(a5,0a,d1,12,bf,60,45,77,a2,fb,74,1a,b1,95,5b,03)

 #define USEC_2000       ((usec_t) 946684800000000) /* 2000-01-01 00:00:00 UTC */

-int dhcp_validate_duid_len(uint16_t duid_type, size_t duid_len) {

+int dhcp_validate_duid_len(uint16_t duid_type, size_t duid_len, bool strict) {

         struct duid d;

         assert_cc(sizeof(d.raw) >= MAX_DUID_LEN);

         if (duid_len > MAX_DUID_LEN)

                 return -EINVAL;

+        if (!strict) {

+                /* Strict validation is not requested. We only ensure that the

+                 * DUID is not too long. */

+                return 0;

+        }

+

         switch (duid_type) {

         case DUID_TYPE_LLT:

                 if (duid_len <= sizeof(d.llt))

diff --git a/src/systemd/src/libsystemd-network/dhcp-identifier.h b/src/systemd/src/libsystemd-network/dhcp-identifier.h

index 64315d3a3..e6834039a 100644

--- a/src/systemd/src/libsystemd-network/dhcp-identifier.h

+++ b/src/systemd/src/libsystemd-network/dhcp-identifier.h

@@ -52,7 +52,7 @@ struct duid {

         };

 } _packed_;

-int dhcp_validate_duid_len(uint16_t duid_type, size_t duid_len);

+int dhcp_validate_duid_len(uint16_t duid_type, size_t duid_len, bool strict);

 int dhcp_identifier_set_duid_llt(struct duid *duid, usec_t t, const uint8_t *addr, size_t addr_len, uint16_t arp_type, size_t *len);

 int dhcp_identifier_set_duid_ll(struct duid *duid, const uint8_t *addr, size_t addr_len, uint16_t arp_type, size_t *len);

 int dhcp_identifier_set_duid_en(struct duid *duid, size_t *len);

diff --git a/src/systemd/src/libsystemd-network/sd-dhcp-client.c b/src/systemd/src/libsystemd-network/sd-dhcp-client.c

index 94ee88e36..0c385a84e 100644

--- a/src/systemd/src/libsystemd-network/sd-dhcp-client.c

+++ b/src/systemd/src/libsystemd-network/sd-dhcp-client.c

@@ -357,7 +357,7 @@ static int dhcp_client_set_iaid_duid_internal(

         assert_return(duid_len == 0 || duid != NULL, -EINVAL);

         if (duid != NULL) {

-                r = dhcp_validate_duid_len(duid_type, duid_len);

+                r = dhcp_validate_duid_len(duid_type, duid_len, true);

                 if (r < 0)

                         return r;

         }

diff --git a/src/systemd/src/libsystemd-network/sd-dhcp6-client.c b/src/systemd/src/libsystemd-network/sd-dhcp6-client.c

index 7dfb2acee..f0cd2f977 100644

--- a/src/systemd/src/libsystemd-network/sd-dhcp6-client.c

+++ b/src/systemd/src/libsystemd-network/sd-dhcp6-client.c

@@ -197,9 +197,13 @@ static int dhcp6_client_set_duid_internal(

         assert_return(IN_SET(client->state, DHCP6_STATE_STOPPED), -EBUSY);

         if (duid != NULL) {

-                r = dhcp_validate_duid_len(duid_type, duid_len);

-                if (r < 0)

-                        return r;

+                r = dhcp_validate_duid_len(duid_type, duid_len, true);

+                if (r < 0) {

+                        r = dhcp_validate_duid_len(duid_type, duid_len, false);

+                        if (r < 0)

+                                return r;

+                        log_dhcp6_client(client, "Setting DUID of type %u with unexpected content", duid_type);

+                }

                 client->duid.type = htobe16(duid_type);

                 memcpy(&client->duid.raw.data, duid, duid_len);

-- 

2.20.1