From ce8c21737b1b952044219111b2358b1602983138 Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Thu, 4 Jan 2018 16:12:40 +0100

Subject: [PATCH 1/2] dhcp: cleanup handling of ipv4.dhcp-client-id and avoid

 assertion failure

The internal client asserts that the length of the client ID is not more

than MAX_CLIENT_ID_LEN. Avoid that assert by truncating the string.

Also add new nm_dhcp_client_set_client_id_*() setters, that either

set the ID based on a string (in our common dhclient specific

format), or based on the binary data (as obtained from systemd client).

Also, add checks and assertions that the client ID which is

set via nm_dhcp_client_set_client_id() is always of length

of at least 2 (as required by rfc2132, section-9.14).

(cherry picked from commit 686afe531ab3774cd818feda8361de74101971f5)

(cherry picked from commit 41a89aeebac146000de97b778800e755db29568f)

---

 libnm-core/nm-setting-ip4-config.c                 |  2 +-

 src/dhcp/nm-dhcp-client.c                          | 67 ++++++++++++++++++----

 src/dhcp/nm-dhcp-client.h                          |  9 ++-

 src/dhcp/nm-dhcp-dhclient-utils.c                  |  8 ++-

 src/dhcp/nm-dhcp-systemd.c                         | 18 ++----

 src/dhcp/nm-dhcp-utils.c                           |  9 ++-

 src/nm-types.h                                     |  2 +

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

 8 files changed, 88 insertions(+), 28 deletions(-)

diff --git a/libnm-core/nm-setting-ip4-config.c b/libnm-core/nm-setting-ip4-config.c

index 26e7f5056..d9c0cbb14 100644

--- a/libnm-core/nm-setting-ip4-config.c

+++ b/libnm-core/nm-setting-ip4-config.c

@@ -191,7 +191,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)

 		return FALSE;

 	}

-	if (priv->dhcp_client_id && !strlen (priv->dhcp_client_id)) {

+	if (priv->dhcp_client_id && !priv->dhcp_client_id[0]) {

 		g_set_error_literal (error,

 		                     NM_CONNECTION_ERROR,

 		                     NM_CONNECTION_ERROR_INVALID_PROPERTY,

diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c

index 20ea092f0..1f19db148 100644

--- a/src/dhcp/nm-dhcp-client.c

+++ b/src/dhcp/nm-dhcp-client.c

@@ -186,19 +186,69 @@ nm_dhcp_client_get_client_id (NMDhcpClient *self)

 	return NM_DHCP_CLIENT_GET_PRIVATE (self)->client_id;

 }

+static void

+_set_client_id (NMDhcpClient *self, GBytes *client_id, gboolean take)

+{

+	NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE (self);

+

+	nm_assert (!client_id || g_bytes_get_size (client_id) >= 2);

+

+	if (   priv->client_id == client_id

+	    || (   priv->client_id

+	        && g_bytes_equal (priv->client_id, client_id))) {

+		if (take && client_id)

+			g_bytes_unref (client_id);

+		return;

+	}

+

+	if (priv->client_id)

+		g_bytes_unref (priv->client_id);

+	priv->client_id = client_id;

+	if (!take && client_id)

+		g_bytes_ref (client_id);

+}

+

 void

 nm_dhcp_client_set_client_id (NMDhcpClient *self, GBytes *client_id)

 {

-	NMDhcpClientPrivate *priv;

+	g_return_if_fail (NM_IS_DHCP_CLIENT (self));

+	g_return_if_fail (!client_id || g_bytes_get_size (client_id) >= 2);

+

+	_set_client_id (self, client_id, FALSE);

+}

+

+void

+nm_dhcp_client_set_client_id_bin (NMDhcpClient *self,

+                                  guint8 type,

+                                  const guint8 *client_id,

+                                  gsize len)

+{

+	guint8 *buf;

+	GBytes *b;

 	g_return_if_fail (NM_IS_DHCP_CLIENT (self));

+	g_return_if_fail (client_id);

+	g_return_if_fail (len > 0);

+

+	buf = g_malloc (len + 1);

+	buf[0] = type;

+	memcpy (buf + 1, client_id, len);

+	b = g_bytes_new_take (buf, len + 1);

+	_set_client_id (self, b, TRUE);

+}

-	priv = NM_DHCP_CLIENT_GET_PRIVATE (self);

+void

+nm_dhcp_client_set_client_id_str (NMDhcpClient *self,

+                                  const char *dhcp_client_id)

+{

+	g_return_if_fail (NM_IS_DHCP_CLIENT (self));

+	g_return_if_fail (!dhcp_client_id || dhcp_client_id[0]);

-	if (priv->client_id && client_id && g_bytes_equal (priv->client_id, client_id))

-		return;

-	g_clear_pointer (&priv->client_id, g_bytes_unref);

-	priv->client_id = client_id ? g_bytes_ref (client_id) : NULL;

+	_set_client_id (self,

+	                dhcp_client_id

+	                  ? nm_dhcp_utils_client_id_string_to_bytes (dhcp_client_id)

+	                  : NULL,

+	                TRUE);

 }

 const char *

@@ -448,7 +498,6 @@ nm_dhcp_client_start_ip4 (NMDhcpClient *self,

                           const char *last_ip4_address)

 {

 	NMDhcpClientPrivate *priv;

-	gs_unref_bytes GBytes *tmp = NULL;

 	g_return_val_if_fail (NM_IS_DHCP_CLIENT (self), FALSE);

@@ -462,9 +511,7 @@ nm_dhcp_client_start_ip4 (NMDhcpClient *self,

 	else

 		_LOGI ("activation: beginning transaction (timeout in %u seconds)", (guint) priv->timeout);

-	if (dhcp_client_id)

-		tmp = nm_dhcp_utils_client_id_string_to_bytes (dhcp_client_id);

-	nm_dhcp_client_set_client_id (self, tmp);

+	nm_dhcp_client_set_client_id_str (self, dhcp_client_id);

 	g_clear_pointer (&priv->hostname, g_free);

 	priv->hostname = g_strdup (hostname);

diff --git a/src/dhcp/nm-dhcp-client.h b/src/dhcp/nm-dhcp-client.h

index 02804002f..2c6341682 100644

--- a/src/dhcp/nm-dhcp-client.h

+++ b/src/dhcp/nm-dhcp-client.h

@@ -173,7 +173,14 @@ gboolean nm_dhcp_client_handle_event (gpointer unused,

                                       const char *reason,

                                       NMDhcpClient *self);

-void nm_dhcp_client_set_client_id (NMDhcpClient *self, GBytes *client_id);

+void nm_dhcp_client_set_client_id (NMDhcpClient *self,

+                                   GBytes *client_id);

+void nm_dhcp_client_set_client_id_bin (NMDhcpClient *self,

+                                       guint8 type,

+                                       const guint8 *client_id,

+                                       gsize len);

+void nm_dhcp_client_set_client_id_str (NMDhcpClient *self,

+                                       const char *dhcp_client_id);

 /*****************************************************************************

  * Client data

diff --git a/src/dhcp/nm-dhcp-dhclient-utils.c b/src/dhcp/nm-dhcp-dhclient-utils.c

index e63e6a869..4df90d76a 100644

--- a/src/dhcp/nm-dhcp-dhclient-utils.c

+++ b/src/dhcp/nm-dhcp-dhclient-utils.c

@@ -178,7 +178,7 @@ read_client_id (const char *str)

 	gs_free char *s = NULL;

 	char *p;

-	g_assert (!strncmp (str, CLIENTID_TAG, NM_STRLEN (CLIENTID_TAG)));

+	nm_assert (!strncmp (str, CLIENTID_TAG, NM_STRLEN (CLIENTID_TAG)));

 	str += NM_STRLEN (CLIENTID_TAG);

 	while (g_ascii_isspace (*str))

@@ -198,6 +198,9 @@ read_client_id (const char *str)

 	if (s[strlen (s) - 1] == ';')

 		s[strlen (s) - 1] = '\0';

+	if (!s[0])

+		return NULL;

+

 	return nm_dhcp_utils_client_id_string_to_bytes (s);

 }

@@ -329,8 +332,7 @@ nm_dhcp_dhclient_create_config (const char *interface,

 					continue;

 				/* Otherwise capture and return the existing client id */

-				if (out_new_client_id)

-					*out_new_client_id = read_client_id (p);

+				NM_SET_OUT (out_new_client_id, read_client_id (p));

 			}

 			/* Override config file hostname and use one from the connection */

diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c

index 9b1a44332..f79b7cb1e 100644

--- a/src/dhcp/nm-dhcp-systemd.c

+++ b/src/dhcp/nm-dhcp-systemd.c

@@ -489,19 +489,13 @@ _save_client_id (NMDhcpSystemd *self,

                  const uint8_t *client_id,

                  size_t len)

 {

-	gs_unref_bytes GBytes *b = NULL;

-	gs_free char *buf = NULL;

-

 	g_return_if_fail (self != NULL);

 	g_return_if_fail (client_id != NULL);

 	g_return_if_fail (len > 0);

 	if (!nm_dhcp_client_get_client_id (NM_DHCP_CLIENT (self))) {

-		buf = g_malloc (len + 1);

-		buf[0] = type;

-		memcpy (buf + 1, client_id, len);

-		b = g_bytes_new (buf, len + 1);

-		nm_dhcp_client_set_client_id (NM_DHCP_CLIENT (self), b);

+		nm_dhcp_client_set_client_id_bin (NM_DHCP_CLIENT (self),

+		                                  type, client_id, len);

 	}

 }

@@ -543,7 +537,7 @@ bound4_handle (NMDhcpSystemd *self)

 		add_requests_to_options (options, dhcp4_requests);

 		dhcp_lease_save (lease, priv->lease_file);

-		sd_dhcp_client_get_client_id(priv->client4, &type, &client_id, &client_id_len);

+		sd_dhcp_client_get_client_id (priv->client4, &type, &client_id, &client_id_len);

 		if (client_id)

 			_save_client_id (self, type, client_id, client_id_len);

@@ -691,14 +685,14 @@ ip4_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const char *last

 	override_client_id = nm_dhcp_client_get_client_id (client);

 	if (override_client_id) {

 		client_id = g_bytes_get_data (override_client_id, &client_id_len);

-		g_assert (client_id && client_id_len);

+		nm_assert (client_id && client_id_len >= 2);

 		sd_dhcp_client_set_client_id (priv->client4,

 		                              client_id[0],

 		                              client_id + 1,

-		                              client_id_len - 1);

+		                              NM_MIN (client_id_len - 1, _NM_SD_MAX_CLIENT_ID_LEN));

 	} else if (lease) {

 		r = sd_dhcp_lease_get_client_id (lease, (const void **) &client_id, &client_id_len);

-		if (r == 0 && client_id_len) {

+		if (r == 0 && client_id_len >= 2) {

 			sd_dhcp_client_set_client_id (priv->client4,

 			                              client_id[0],

 			                              client_id + 1,

diff --git a/src/dhcp/nm-dhcp-utils.c b/src/dhcp/nm-dhcp-utils.c

index 4b2d57b90..50ca2abe2 100644

--- a/src/dhcp/nm-dhcp-utils.c

+++ b/src/dhcp/nm-dhcp-utils.c

@@ -750,8 +750,15 @@ nm_dhcp_utils_client_id_string_to_bytes (const char *client_id)

 	g_return_val_if_fail (client_id && client_id[0], NULL);

 	/* Try as hex encoded */

-	if (strchr (client_id, ':'))

+	if (strchr (client_id, ':')) {

 		bytes = nm_utils_hexstr2bin (client_id);

+

+		/* the result must be at least two bytes long,

+		 * because @client_id contains a delimiter

+		 * but nm_utils_hexstr2bin() does not allow

+		 * leading nor trailing delimiters. */

+		nm_assert (!bytes || g_bytes_get_size (bytes) >= 2);

+	}

 	if (!bytes) {

 		/* Fall back to string */

 		len = strlen (client_id);

diff --git a/src/nm-types.h b/src/nm-types.h

index cc657397e..02163f87e 100644

--- a/src/nm-types.h

+++ b/src/nm-types.h

@@ -25,6 +25,8 @@

 #error "nm-utils-private.h" must not be used outside of libnm-core/. Do you want "nm-core-internal.h"?

 #endif

+#define _NM_SD_MAX_CLIENT_ID_LEN (sizeof (guint32) + 128)

+

 /* core */

 typedef struct _NMExportedObject     NMExportedObject;

 typedef struct _NMActiveConnection   NMActiveConnection;

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

index fc6ad422d..5eab00502 100644

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

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

@@ -311,6 +311,7 @@ int sd_dhcp_client_set_client_id(

         assert_return(client, -EINVAL);

         assert_return(data, -EINVAL);

         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) {

-- 

2.14.3

From 161a554575f0e043ecf2d50b6108fa102fa9a982 Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Tue, 9 Jan 2018 15:55:23 +0100

Subject: [PATCH 2/2] dhcp: fix check for client-id in _set_client_id()

Fixes: 686afe531ab3774cd818feda8361de74101971f5

(cherry picked from commit 0e1fb1dbd282f546820c1ab8326cf4cf550ae2ae)

(cherry picked from commit 8998ce629da8954a15c3cfc9aadb0d74bbcc116b)

---

 src/dhcp/nm-dhcp-client.c | 1 +

 1 file changed, 1 insertion(+)

diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c

index 1f19db148..ea3938d63 100644

--- a/src/dhcp/nm-dhcp-client.c

+++ b/src/dhcp/nm-dhcp-client.c

@@ -195,6 +195,7 @@ _set_client_id (NMDhcpClient *self, GBytes *client_id, gboolean take)

 	if (   priv->client_id == client_id

 	    || (   priv->client_id

+	        && client_id

 	        && g_bytes_equal (priv->client_id, client_id))) {

 		if (take && client_id)

 			g_bytes_unref (client_id);

-- 

2.14.3