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

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 e52f096173..74c19b6a1c 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 25490741ff..1959f05234 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 3c2ab028a8..3583281dc3 100644
--- a/src/dhcp/nm-dhcp-client.h
+++ b/src/dhcp/nm-dhcp-client.h
@@ -174,7 +174,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 e63e6a8696..4df90d76a0 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 2e552cd4ff..abb2c36cb8 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 4b2d57b90f..50ca2abe2d 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 cc657397ea..02163f87e6 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 c04c792e2d..a88908e44f 100644
--- a/src/systemd/src/libsystemd-network/sd-dhcp-client.c
+++ b/src/systemd/src/libsystemd-network/sd-dhcp-client.c
@@ -312,6 +312,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) {