From d9488417e76c6d88dd4f97709b8826e1566db829 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Jun 2018 11:36:40 +0200 Subject: device: obtain current MAC address from platform for generating ipv4.dhcp-client-id=mac In practice, there should be no difference between peeking into the platform cache, or using the cached value from nm_device_get_hw_address(). Prefer the hardware address from the platform, because: - we also pass the current MAC address to nm_dhcp_manager_start_ip4(). For not particularly strong reason, it uses the MAC address obtained from platform. At the least, it makes sense that we use the same addresses for the client-id as well. - ipv6.dhcp-duid also gets the address from platform. Again, no strong reason either way, but they should behave similar in this regard. --- src/devices/nm-device.c | 50 +++++++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 66bd419447..95e56b9cea 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7018,7 +7018,9 @@ get_dhcp_timeout (NMDevice *self, int addr_family) } static GBytes * -dhcp4_get_client_id (NMDevice *self, NMConnection *connection) +dhcp4_get_client_id (NMDevice *self, + NMConnection *connection, + GBytes *hwaddr) { NMSettingIPConfig *s_ip4; const char *client_id; @@ -7041,33 +7043,41 @@ dhcp4_get_client_id (NMDevice *self, NMConnection *connection) if ( (is_mac = nm_streq (client_id, "mac")) || nm_streq (client_id, "perm-mac")) { - const char *hwaddr; - char addr_buf[NM_UTILS_HWADDR_LEN_MAX]; - gsize addr_len; - guint8 addr_type; - - hwaddr = is_mac - ? nm_device_get_hw_address (self) - : nm_device_get_permanent_hw_address (self); - if (!hwaddr) - return NULL; + char hwaddr_bin_buf[NM_UTILS_HWADDR_LEN_MAX]; + guint8 hwaddr_type; + const guint8 *hwaddr_bin; + gsize hwaddr_len; + + if (is_mac) { + if (!hwaddr) + return NULL; + hwaddr_bin = g_bytes_get_data (hwaddr, &hwaddr_len); + } else { + const char *hwaddr_str; - if (!_nm_utils_hwaddr_aton (hwaddr, addr_buf, sizeof (addr_buf), &addr_len)) - g_return_val_if_reached (NULL); + hwaddr_str = nm_device_get_permanent_hw_address (self); + if (!hwaddr_str) + return NULL; + + if (!_nm_utils_hwaddr_aton (hwaddr_str, hwaddr_bin_buf, sizeof (hwaddr_bin_buf), &hwaddr_len)) + g_return_val_if_reached (NULL); + + hwaddr_bin = (const guint8 *) hwaddr_bin_buf; + } - switch (addr_len) { + switch (hwaddr_len) { case ETH_ALEN: - addr_type = ARPHRD_ETHER; + hwaddr_type = ARPHRD_ETHER; break; default: /* unsupported type. */ return NULL; } - client_id_buf = g_malloc (addr_len + 1); - client_id_buf[0] = addr_type; - memcpy (&client_id_buf[1], addr_buf, addr_len); - return g_bytes_new_take (client_id_buf, addr_len + 1); + client_id_buf = g_malloc (hwaddr_len + 1); + client_id_buf[0] = hwaddr_type; + memcpy (&client_id_buf[1], hwaddr_bin, hwaddr_len); + return g_bytes_new_take (client_id_buf, hwaddr_len + 1); } if (nm_streq (client_id, "stable")) { @@ -7130,7 +7140,7 @@ dhcp4_start (NMDevice *self) hwaddr = nm_platform_link_get_address_as_bytes (nm_device_get_platform (self), nm_device_get_ip_ifindex (self)); - client_id = dhcp4_get_client_id (self, connection); + client_id = dhcp4_get_client_id (self, connection, hwaddr); g_warn_if_fail (priv->dhcp4.client == NULL); priv->dhcp4.client = nm_dhcp_manager_start_ip4 (nm_dhcp_manager_get (), -- cgit v1.2.1 From 5df4c17ba14f8604277aa338f2e26ad264012e1b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Jun 2018 11:55:35 +0200 Subject: device: handle failure to generate ipv4.dhcp-client-id by fallback to random client-id First of all, generating the client-id is not expected to fail. If it fails, something is already very wrong. Maybe, a failure to generate a client-id should result in failing the activation. However, let's not go full measure in this question. Instead: - ensure that we log a warning and a reason why the client-id could not be generated. - fallback to a random client id. Clearly, we were unable to generate the requested client-id, hence, we should fallback to a default value which does not make the host easily identifyable. Of course, that means that the generated DHCP client-id is not at all stable. But note that something is already very wrong, so when handling the error we should do something conservative (that is, protecting the users privacy). This is also what happens for a failure to generate the ipv6.dhcp-duid. --- src/devices/nm-device.c | 83 +++++++++++++++++++++++++++++++------------------ 1 file changed, 53 insertions(+), 30 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 95e56b9cea..ab8565c2dd 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7017,6 +7017,18 @@ get_dhcp_timeout (NMDevice *self, int addr_family) return timeout ?: NM_DHCP_TIMEOUT_DEFAULT; } +static GBytes * +dhcp4_get_client_id_mac (const guint8 *hwaddr /* ETH_ALEN bytes */) +{ + guint8 *client_id_buf; + guint8 hwaddr_type = ARPHRD_ETHER; + + client_id_buf = g_malloc (ETH_ALEN + 1); + client_id_buf[0] = hwaddr_type; + memcpy (&client_id_buf[1], hwaddr, ETH_ALEN); + return g_bytes_new_take (client_id_buf, ETH_ALEN + 1); +} + static GBytes * dhcp4_get_client_id (NMDevice *self, NMConnection *connection, @@ -7026,7 +7038,10 @@ dhcp4_get_client_id (NMDevice *self, const char *client_id; gs_free char *client_id_default = NULL; guint8 *client_id_buf; - gboolean is_mac; + const char *fail_reason; + guint8 hwaddr_bin_buf[NM_UTILS_HWADDR_LEN_MAX]; + const guint8 *hwaddr_bin; + gsize hwaddr_len; s_ip4 = nm_connection_get_setting_ip4_config (connection); client_id = nm_setting_ip4_config_get_dhcp_client_id (NM_SETTING_IP4_CONFIG (s_ip4)); @@ -7041,43 +7056,40 @@ dhcp4_get_client_id (NMDevice *self, if (!client_id) return NULL; - if ( (is_mac = nm_streq (client_id, "mac")) - || nm_streq (client_id, "perm-mac")) { - char hwaddr_bin_buf[NM_UTILS_HWADDR_LEN_MAX]; - guint8 hwaddr_type; - const guint8 *hwaddr_bin; - gsize hwaddr_len; - - if (is_mac) { - if (!hwaddr) - return NULL; - hwaddr_bin = g_bytes_get_data (hwaddr, &hwaddr_len); - } else { - const char *hwaddr_str; + if (nm_streq (client_id, "mac")) { + if (!hwaddr) { + fail_reason = "failed to get current MAC address"; + goto out_fail; + } - hwaddr_str = nm_device_get_permanent_hw_address (self); - if (!hwaddr_str) - return NULL; + hwaddr_bin = g_bytes_get_data (hwaddr, &hwaddr_len); + if (hwaddr_len != ETH_ALEN) { + fail_reason = "MAC address is not ethernet"; + goto out_fail; + } + + return dhcp4_get_client_id_mac (hwaddr_bin); + } - if (!_nm_utils_hwaddr_aton (hwaddr_str, hwaddr_bin_buf, sizeof (hwaddr_bin_buf), &hwaddr_len)) - g_return_val_if_reached (NULL); + if (nm_streq (client_id, "perm-mac")) { + const char *hwaddr_str; - hwaddr_bin = (const guint8 *) hwaddr_bin_buf; + hwaddr_str = nm_device_get_permanent_hw_address (self); + if (!hwaddr_str) { + fail_reason = "failed to get permanent MAC address"; + goto out_fail; } - switch (hwaddr_len) { - case ETH_ALEN: - hwaddr_type = ARPHRD_ETHER; - break; - default: + if (!_nm_utils_hwaddr_aton (hwaddr_str, hwaddr_bin_buf, sizeof (hwaddr_bin_buf), &hwaddr_len)) + g_return_val_if_reached (NULL); + + if (hwaddr_len != ETH_ALEN) { /* unsupported type. */ - return NULL; + fail_reason = "MAC address is not ethernet"; + goto out_fail; } - client_id_buf = g_malloc (hwaddr_len + 1); - client_id_buf[0] = hwaddr_type; - memcpy (&client_id_buf[1], hwaddr_bin, hwaddr_len); - return g_bytes_new_take (client_id_buf, hwaddr_len + 1); + return dhcp4_get_client_id_mac (hwaddr_bin_buf); } if (nm_streq (client_id, "stable")) { @@ -7117,6 +7129,17 @@ dhcp4_get_client_id (NMDevice *self, } return nm_dhcp_utils_client_id_string_to_bytes (client_id); + +out_fail: + { + _LOGW (LOGD_DEVICE | LOGD_DHCP4 | LOGD_IP4, + "ipv4.dhcp-client-id: failure to generate client id (%s). Use random client id", + fail_reason); + client_id_buf = g_malloc (1 + 15); + client_id_buf[0] = 0; + nm_utils_random_bytes (&client_id_buf[1], 15); + return g_bytes_new_take (client_id_buf, 1 + 15); + } } static NMActStageReturn -- cgit v1.2.1 From 8bb1aed2ad1a0beb8f7036a0f3b54da6fc5b0ba5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Jun 2018 12:32:59 +0200 Subject: device: fix enforcing ipv6.dhcp-duid for binary DUID --- src/devices/nm-device.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index ab8565c2dd..4198a9dd68 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7826,7 +7826,6 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp gsize len = sizeof (sha256_digest); NMDhcpDuidEnforce duid_enforce = NM_DHCP_DUID_ENFORCE_NEVER; - s_ip6 = nm_connection_get_setting_ip6_config (connection); duid = nm_setting_ip6_config_get_dhcp_duid (NM_SETTING_IP6_CONFIG (s_ip6)); @@ -7841,6 +7840,8 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp goto end; } + duid_enforce = NM_DHCP_DUID_ENFORCE_ALWAYS; + if (!_nm_utils_dhcp_duid_valid (duid, &duid_out)) { duid_error = "invalid duid"; goto end; @@ -7887,8 +7888,6 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp g_checksum_free (sum); } - duid_enforce = NM_DHCP_DUID_ENFORCE_ALWAYS; - #define EPOCH_DATETIME_THREE_YEARS (356 * 24 * 3600 * 3) if (nm_streq0 (duid, "ll")) { duid_out = generate_duid_ll (g_bytes_get_data (hwaddr, NULL)); -- cgit v1.2.1 From 6d06a0e1b0c2732155a61289736fc95e2275db98 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Jun 2018 12:27:03 +0200 Subject: device: handle failure in generate_duid_from_machine_id() in dhcp6_get_duid() dhcp6_get_duid() already handles failure to generate the DUID in a sensible manner. No reason to duplicate the error handling in generate_duid_from_machine_id(). Especially, because generate_duid_from_machine_id() used to cache the random DUID in memory and reuse it from then on. There is no reason to do that, /etc/machine-id must be available to NetworkManager. We still handle such a grave error gracefully by generating a random DUID. --- src/devices/nm-device.c | 40 ++++++++++++++++------------------------ src/nm-core-utils.c | 6 +++--- 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 4198a9dd68..db3b15a53b 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7797,18 +7797,14 @@ generate_duid_from_machine_id (void) return g_bytes_ref (global_duid); machine_id_s = nm_utils_machine_id_read (); - if (nm_utils_machine_id_parse (machine_id_s, uuid)) { - /* Hash the machine ID so it's not leaked to the network */ - sum = g_checksum_new (G_CHECKSUM_SHA256); - g_checksum_update (sum, (const guchar *) &uuid, sizeof (uuid)); - g_checksum_get_digest (sum, sha256_digest, &len); - g_checksum_free (sum); - } else { - nm_log_warn (LOGD_IP6, "global duid: failed to read " SYSCONFDIR "/machine-id " - "or " LOCALSTATEDIR "/lib/dbus/machine-id to generate " - "DHCPv6 DUID; creating non-persistent random DUID."); - nm_utils_random_bytes (sha256_digest, len); - } + if (!nm_utils_machine_id_parse (machine_id_s, uuid)) + return NULL; + + /* Hash the machine ID so it's not leaked to the network */ + sum = g_checksum_new (G_CHECKSUM_SHA256); + g_checksum_update (sum, (const guchar *) &uuid, sizeof (uuid)); + g_checksum_get_digest (sum, sha256_digest, &len); + g_checksum_free (sum); global_duid = generate_duid_uuid (sha256_digest, len); return g_bytes_ref (global_duid); @@ -7824,7 +7820,7 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp GBytes *duid_out = NULL; guint8 sha256_digest[32]; gsize len = sizeof (sha256_digest); - NMDhcpDuidEnforce duid_enforce = NM_DHCP_DUID_ENFORCE_NEVER; + NMDhcpDuidEnforce duid_enforce = NM_DHCP_DUID_ENFORCE_ALWAYS; s_ip6 = nm_connection_get_setting_ip6_config (connection); duid = nm_setting_ip6_config_get_dhcp_duid (NM_SETTING_IP6_CONFIG (s_ip6)); @@ -7836,12 +7832,13 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp } if (!duid || nm_streq (duid, "lease")) { + duid_enforce = NM_DHCP_DUID_ENFORCE_NEVER; duid_out = generate_duid_from_machine_id (); + if (!duid_out) + duid_error = "failure to read machine-id"; goto end; } - duid_enforce = NM_DHCP_DUID_ENFORCE_ALWAYS; - if (!_nm_utils_dhcp_duid_valid (duid, &duid_out)) { duid_error = "invalid duid"; goto end; @@ -7868,11 +7865,8 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp gsize secret_key_len; stable_id = _get_stable_id (self, connection, &stable_type); - if (!stable_id) { - nm_assert_not_reached (); - duid_error = "cannot retrieve the stable id"; - goto end; - } + if (!stable_id) + g_return_val_if_reached (NULL); salted_header = htonl (670531087 + stable_type); @@ -7933,8 +7927,7 @@ end: if (!duid_out) { guint8 uuid[16]; - if (duid_error) - _LOGW (LOGD_IP6, "duid-gen (%s): %s. Fallback to random DUID-UUID.", duid, duid_error); + _LOGW (LOGD_IP6, "duid-gen (%s): %s. Fallback to random DUID-UUID.", duid, duid_error); nm_utils_random_bytes (uuid, sizeof (uuid)); duid_out = generate_duid_uuid (uuid, sizeof (uuid)); @@ -7945,7 +7938,6 @@ end: (duid_enforce == NM_DHCP_DUID_ENFORCE_ALWAYS) ? "enforcing" : "fallback"); NM_SET_OUT (out_enforce, duid_enforce); - return duid_out; } @@ -7956,7 +7948,7 @@ dhcp6_start_with_link_ready (NMDevice *self, NMConnection *connection) NMSettingIPConfig *s_ip6; gs_unref_bytes GBytes *hwaddr = NULL; gs_unref_bytes GBytes *duid = NULL; - NMDhcpDuidEnforce enforce_duid; + NMDhcpDuidEnforce enforce_duid = NM_DHCP_DUID_ENFORCE_NEVER; const NMPlatformIP6Address *ll_addr = NULL; diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 09b465a07e..54ccdb836d 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -2521,20 +2521,20 @@ nm_utils_machine_id_read (void) */ if ( !g_file_get_contents ("/etc/machine-id", &contents, NULL, NULL) && !g_file_get_contents (LOCALSTATEDIR "/lib/dbus/machine-id", &contents, NULL, NULL)) - return FALSE; + return NULL; contents = g_strstrip (contents); for (i = 0; i < 32; i++) { if (!g_ascii_isxdigit (contents[i])) - return FALSE; + return NULL; if (contents[i] >= 'A' && contents[i] <= 'F') { /* canonicalize to lower-case */ contents[i] = 'a' + (contents[i] - 'A'); } } if (contents[i] != '\0') - return FALSE; + return NULL; return g_steal_pointer (&contents); } -- cgit v1.2.1 From 374d147421a963a31f15c344c0ada4a39e7884f0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Jun 2018 12:27:03 +0200 Subject: device: refactor generate_duid_from_machine_id() to have a straight forward code path Previously, there were two blocks if (NM_IN_SET (duid, "ll", "llt") preprocess_hwaddr() else if (NM_IN_SET (duid, "stable-ll", "stable-llt", "stable-uuid")) preprecess_stable_id() if (nm_streq (duid, "ll") generate_ll() else if (nm_streq (duid, "llt")) generate_llt() else if (nm_streq (duid, "stable-ll") generate_stable_ll() ... That is, the latter block depends on the execution of the previous block, while the previous block is guarded by a particular condition, slighlty different than the condition in the later block. It is confusing to follow. Instead, check for our cases one by one, and when we determined a particular DUID type, process it within the same block of code. Now the code consists of individual blocks, that all end with a "goto out*". That means, it's easier to understand the flow of the code. Also, don't initialize duid_error variable and separate between "out_error" and "out_good". This allows that the compiler gives a warning if we missed ot initialize duid_error. --- src/devices/nm-device.c | 105 +++++++++++++++++++++++++++--------------------- 1 file changed, 59 insertions(+), 46 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index db3b15a53b..a084563534 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7816,8 +7816,8 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp NMSettingIPConfig *s_ip6; const char *duid; gs_free char *duid_default = NULL; - const char *duid_error = NULL; - GBytes *duid_out = NULL; + const char *duid_error; + GBytes *duid_out; guint8 sha256_digest[32]; gsize len = sizeof (sha256_digest); NMDhcpDuidEnforce duid_enforce = NM_DHCP_DUID_ENFORCE_ALWAYS; @@ -7834,29 +7834,49 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp if (!duid || nm_streq (duid, "lease")) { duid_enforce = NM_DHCP_DUID_ENFORCE_NEVER; duid_out = generate_duid_from_machine_id (); - if (!duid_out) + if (!duid_out) { duid_error = "failure to read machine-id"; - goto end; + goto out_fail; + } + goto out_good; } if (!_nm_utils_dhcp_duid_valid (duid, &duid_out)) { duid_error = "invalid duid"; - goto end; + goto out_fail; } if (duid_out) - goto end; + goto out_good; if (NM_IN_STRSET (duid, "ll", "llt")) { if (!hwaddr) { duid_error = "missing link-layer address"; - goto end; + goto out_fail; } if (g_bytes_get_size (hwaddr) != ETH_ALEN) { duid_error = "unsupported link-layer address"; - goto end; + goto out_fail; + } + + if (nm_streq (duid, "ll")) { + duid_out = generate_duid_ll (g_bytes_get_data (hwaddr, NULL)); + } else { + gint64 time; + + time = nm_utils_secret_key_get_timestamp (); + if (!time) { + duid_error = "cannot retrieve the secret key timestamp"; + goto out_fail; + } + + duid_out = generate_duid_llt (g_bytes_get_data (hwaddr, NULL), time); } - } else if (NM_IN_STRSET (duid, "stable-llt", "stable-ll", "stable-uuid")) { + + goto out_good; + } + + if (NM_IN_STRSET (duid, "stable-ll", "stable-llt", "stable-uuid")) { NMUtilsStableType stable_type; const char *stable_id = NULL; guint32 salted_header; @@ -7880,51 +7900,42 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp g_checksum_get_digest (sum, sha256_digest, &len); g_checksum_free (sum); - } -#define EPOCH_DATETIME_THREE_YEARS (356 * 24 * 3600 * 3) - if (nm_streq0 (duid, "ll")) { - duid_out = generate_duid_ll (g_bytes_get_data (hwaddr, NULL)); - - } else if (nm_streq0 (duid, "llt")) { - gint64 time; - - time = nm_utils_secret_key_get_timestamp (); - if (!time) { - duid_error = "cannot retrieve the secret key timestamp"; - goto end; - } + if (nm_streq (duid, "stable-ll")) { + duid_out = generate_duid_ll (sha256_digest); + } else if (nm_streq (duid, "stable-llt")) { + gint64 time; - duid_out = generate_duid_llt (g_bytes_get_data (hwaddr, NULL), time); - } else if (nm_streq0 (duid, "stable-ll")) { - duid_out = generate_duid_ll (sha256_digest); +#define EPOCH_DATETIME_THREE_YEARS (356 * 24 * 3600 * 3) - } else if (nm_streq0 (duid, "stable-llt")) { - gint64 time; + /* We want a variable time between the secret_key timestamp and three years + * before. Let's compute the time (in seconds) from 0 to 3 years; then we'll + * subtract it from the secret_key timestamp. + */ + time = nm_utils_secret_key_get_timestamp (); + if (!time) { + duid_error = "cannot retrieve the secret key timestamp"; + goto out_fail; + } + /* don't use too old timestamps. They cannot be expressed in DUID-LLT and + * would all be truncated to zero. */ + time = NM_MAX (time, EPOCH_DATETIME_200001010000 + EPOCH_DATETIME_THREE_YEARS); + time -= (unaligned_read_be32 (&sha256_digest[ETH_ALEN]) % EPOCH_DATETIME_THREE_YEARS); - /* We want a variable time between the secret_key timestamp and three years - * before. Let's compute the time (in seconds) from 0 to 3 years; then we'll - * subtract it from the secret_key timestamp. - */ - time = nm_utils_secret_key_get_timestamp (); - if (!time) { - duid_error = "cannot retrieve the secret key timestamp"; - goto end; + duid_out = generate_duid_llt (sha256_digest, time); + } else { + nm_assert (nm_streq (duid, "stable-uuid")); + duid_out = generate_duid_uuid (sha256_digest, len); } - /* don't use too old timestamps. They cannot be expressed in DUID-LLT and - * would all be truncated to zero. */ - time = NM_MAX (time, EPOCH_DATETIME_200001010000 + EPOCH_DATETIME_THREE_YEARS); - time -= (unaligned_read_be32 (&sha256_digest[ETH_ALEN]) % EPOCH_DATETIME_THREE_YEARS); - duid_out = generate_duid_llt (sha256_digest, time); - - } else if (nm_streq0 (duid, "stable-uuid")) { - duid_out = generate_duid_uuid (sha256_digest, len); + goto out_good; } - duid_error = "generation failed"; -end: - if (!duid_out) { + g_return_val_if_reached (NULL); + +out_fail: + nm_assert (!duid_out && duid_error); + { guint8 uuid[16]; _LOGW (LOGD_IP6, "duid-gen (%s): %s. Fallback to random DUID-UUID.", duid, duid_error); @@ -7933,6 +7944,8 @@ end: duid_out = generate_duid_uuid (uuid, sizeof (uuid)); } +out_good: + nm_assert (duid_out); _LOGD (LOGD_IP6, "DUID gen: '%s' (%s)", nm_dhcp_utils_duid_to_string (duid_out), (duid_enforce == NM_DHCP_DUID_ENFORCE_ALWAYS) ? "enforcing" : "fallback"); -- cgit v1.2.1 From 67ffd17b6cd6c7da4723075cac4ee07ad1c0da85 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Jun 2018 13:03:15 +0200 Subject: device: unify logging of ipv6.dhcp-duid by giving common prefix For better or worse, the logging done for ipv4.dhcp-client-id is prefixed with ipv4.dhcp-client-id. Let ipv6.dhcp-duid follow that pattern. Also, generate_duid_from_machine_id() would log at two places, it should use the same logging prefix. Also, it logs the value of "duid" variable. Ensure, that "duid" is not %NULL at that point. Also, fix leak of nm_dhcp_utils_duid_to_string() value during logging. --- src/devices/nm-device.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index a084563534..07e2aba3e1 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7821,6 +7821,7 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp guint8 sha256_digest[32]; gsize len = sizeof (sha256_digest); NMDhcpDuidEnforce duid_enforce = NM_DHCP_DUID_ENFORCE_ALWAYS; + gs_free char *logstr1 = NULL; s_ip6 = nm_connection_get_setting_ip6_config (connection); duid = nm_setting_ip6_config_get_dhcp_duid (NM_SETTING_IP6_CONFIG (s_ip6)); @@ -7829,9 +7830,11 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, NMDhcp duid_default = nm_config_data_get_connection_default (NM_CONFIG_GET_DATA, "ipv6.dhcp-duid", self); duid = duid_default; + if (!duid) + duid = "lease"; } - if (!duid || nm_streq (duid, "lease")) { + if (nm_streq (duid, "lease")) { duid_enforce = NM_DHCP_DUID_ENFORCE_NEVER; duid_out = generate_duid_from_machine_id (); if (!duid_out) { @@ -7938,7 +7941,9 @@ out_fail: { guint8 uuid[16]; - _LOGW (LOGD_IP6, "duid-gen (%s): %s. Fallback to random DUID-UUID.", duid, duid_error); + _LOGW (LOGD_IP6 | LOGD_DHCP6, + "ipv6.dhcp-duid: failure to generate %s DUID: %s. Fallback to random DUID-UUID.", + duid, duid_error); nm_utils_random_bytes (uuid, sizeof (uuid)); duid_out = generate_duid_uuid (uuid, sizeof (uuid)); @@ -7946,9 +7951,11 @@ out_fail: out_good: nm_assert (duid_out); - _LOGD (LOGD_IP6, "DUID gen: '%s' (%s)", - nm_dhcp_utils_duid_to_string (duid_out), - (duid_enforce == NM_DHCP_DUID_ENFORCE_ALWAYS) ? "enforcing" : "fallback"); + _LOGD (LOGD_IP6 | LOGD_DHCP6, + "ipv6.dhcp-duid: generate %s DUID '%s' (%s)", + duid, + (logstr1 = nm_dhcp_utils_duid_to_string (duid_out)), + (duid_enforce == NM_DHCP_DUID_ENFORCE_ALWAYS) ? "enforcing" : "fallback"); NM_SET_OUT (out_enforce, duid_enforce); return duid_out; -- cgit v1.2.1 From 988cecb6d36f9a866dd2b43549fb8c15e354b4ff Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Jun 2018 13:46:12 +0200 Subject: device: log generated ipv4.dhcp-client-id in mode --- src/devices/nm-device.c | 48 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 07e2aba3e1..060e59d700 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7042,6 +7042,8 @@ dhcp4_get_client_id (NMDevice *self, guint8 hwaddr_bin_buf[NM_UTILS_HWADDR_LEN_MAX]; const guint8 *hwaddr_bin; gsize hwaddr_len; + GBytes *result; + gs_free char *logstr1 = NULL; s_ip4 = nm_connection_get_setting_ip4_config (connection); client_id = nm_setting_ip4_config_get_dhcp_client_id (NM_SETTING_IP4_CONFIG (s_ip4)); @@ -7049,12 +7051,17 @@ dhcp4_get_client_id (NMDevice *self, if (!client_id) { client_id_default = nm_config_data_get_connection_default (NM_CONFIG_GET_DATA, "ipv4.dhcp-client-id", self); - if (client_id_default && client_id_default[0]) + if (client_id_default && client_id_default[0]) { + /* a non-empty client-id is always valid, see nm_dhcp_utils_client_id_string_to_bytes(). */ client_id = client_id_default; + } } - if (!client_id) + if (!client_id) { + _LOGD (LOGD_DEVICE | LOGD_DHCP4 | LOGD_IP4, + "ipv4.dhcp-client-id: no explicity client-id configured"); return NULL; + } if (nm_streq (client_id, "mac")) { if (!hwaddr) { @@ -7068,7 +7075,8 @@ dhcp4_get_client_id (NMDevice *self, goto out_fail; } - return dhcp4_get_client_id_mac (hwaddr_bin); + result = dhcp4_get_client_id_mac (hwaddr_bin); + goto out_good; } if (nm_streq (client_id, "perm-mac")) { @@ -7089,7 +7097,8 @@ dhcp4_get_client_id (NMDevice *self, goto out_fail; } - return dhcp4_get_client_id_mac (hwaddr_bin_buf); + result = dhcp4_get_client_id_mac (hwaddr_bin_buf); + goto out_good; } if (nm_streq (client_id, "stable")) { @@ -7125,21 +7134,30 @@ dhcp4_get_client_id (NMDevice *self, client_id_buf = g_malloc (1 + 15); client_id_buf[0] = 0; memcpy (&client_id_buf[1], buf, 15); - return g_bytes_new_take (client_id_buf, 1 + 15); + result = g_bytes_new_take (client_id_buf, 1 + 15); + goto out_good; } - return nm_dhcp_utils_client_id_string_to_bytes (client_id); + result = nm_dhcp_utils_client_id_string_to_bytes (client_id); + goto out_good; out_fail: - { - _LOGW (LOGD_DEVICE | LOGD_DHCP4 | LOGD_IP4, - "ipv4.dhcp-client-id: failure to generate client id (%s). Use random client id", - fail_reason); - client_id_buf = g_malloc (1 + 15); - client_id_buf[0] = 0; - nm_utils_random_bytes (&client_id_buf[1], 15); - return g_bytes_new_take (client_id_buf, 1 + 15); - } + nm_assert (fail_reason); + _LOGW (LOGD_DEVICE | LOGD_DHCP4 | LOGD_IP4, + "ipv4.dhcp-client-id: failure to generate client id (%s). Use random client id", + fail_reason); + client_id_buf = g_malloc (1 + 15); + client_id_buf[0] = 0; + nm_utils_random_bytes (&client_id_buf[1], 15); + result = g_bytes_new_take (client_id_buf, 1 + 15); + +out_good: + nm_assert (result); + _LOGD (LOGD_DEVICE | LOGD_DHCP4 | LOGD_IP4, + "ipv4.dhcp-client-id: use \"%s\" client ID: %s", + client_id, + (logstr1 = nm_dhcp_utils_duid_to_string (result))); + return result; } static NMActStageReturn -- cgit v1.2.1 From 92b8161578a546fe3dc86701a471938cfbd520fd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Jun 2018 13:19:40 +0200 Subject: libnm: avoid allocating GBytes in _nm_utils_dhcp_duid_valid() In a lot of cases, we don't require the GBytes out-argument. This is the case when called from NMSettingIP6Config's verify(). Avoid allocating the GBytes instance and also don't heap allocate the temporary buffer in that case. Also, being called from NMSettingIP6Config's verify(), at which point the string value contains untrusted data. Of course, we do very badly in general protecting against the user creating huge settings, which could trick NetworkManage to allocate large amounts of memory (and being killed by glib's out of memory handling). We should handle such cases better in general, but just avoid it here. Since we know that the buffer must hold at most 128+2 bytes, we can stack allocate it. Later, in case we really need to return the value, we can create a GBytes instance of the right size. --- libnm-core/nm-utils.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 861b283e45..eb705cbe0a 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -4448,11 +4448,10 @@ _nm_utils_inet6_is_token (const struct in6_addr *in6addr) gboolean _nm_utils_dhcp_duid_valid (const char *duid, GBytes **out_duid_bin) { + guint8 duid_arr[128 + 2]; gsize duid_len; - gs_unref_bytes GBytes *duid_bin = NULL; - if (out_duid_bin) - *out_duid_bin = NULL; + NM_SET_OUT (out_duid_bin, NULL); if (!duid) return FALSE; @@ -4466,20 +4465,16 @@ _nm_utils_dhcp_duid_valid (const char *duid, GBytes **out_duid_bin) return TRUE; } - duid_bin = nm_utils_hexstr2bin (duid); - if (!duid_bin) - return FALSE; - - duid_len = g_bytes_get_size (duid_bin); - /* MAX DUID lenght is 128 octects + the type code (2 octects). */ - if ( duid_len <= 2 - || duid_len > (128 + 2)) - return FALSE; - - if (out_duid_bin) - *out_duid_bin = g_steal_pointer (&duid_bin); + if (_str2bin (duid, FALSE, ":", duid_arr, sizeof (duid_arr), &duid_len)) { + /* MAX DUID length is 128 octects + the type code (2 octects). */ + if ( duid_len > 2 + && duid_len <= (128 + 2)) { + NM_SET_OUT (out_duid_bin, g_bytes_new (duid_arr, duid_len)); + return TRUE; + } + } - return TRUE; + return FALSE; } /** -- cgit v1.2.1 From fd878d826129141043e7b30e6521b23544c8967e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 11 Jun 2018 13:08:00 +0200 Subject: examples: add ipv4.dhcp-client-id and ipv6.dhcp-duid to 30-anon.conf example --- examples/nm-conf.d/30-anon.conf | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/examples/nm-conf.d/30-anon.conf b/examples/nm-conf.d/30-anon.conf index 28a9ae701a..3e879fc2d4 100644 --- a/examples/nm-conf.d/30-anon.conf +++ b/examples/nm-conf.d/30-anon.conf @@ -1,39 +1,44 @@ # Example configuration snippet for NetworkManager to # overwrite some default value for more privacy. -# Put it for example to /etc/NetworkManager/conf.d/30-anon.conf +# Drop this file for example to /etc/NetworkManager/conf.d/30-anon.conf # # See man NetworkManager.conf(5) for how default values # work. See man nm-settings(5) for the connection properties. # # -# This enables privacy setting by default. The defaults +# This enables some privacy setting by default. The defaults # apply only to settings that do not explicitly configure # a per-connection override. # That means, if the connection profile has # # $ nmcli connection show "$CON_NAME" | -# grep '^\(connection.stable-id\|ipv6.addr-gen-mode\|ipv6.ip6-privacy\|802-11-wireless.cloned-mac-address\|802-11-wireless.mac-address-randomization\|802-3-ethernet.cloned-mac-address\)' +# grep '^\(connection.stable-id\|ipv6.addr-gen-mode\|ipv6.ip6-privacy\|802-11-wireless.cloned-mac-address\|802-11-wireless.mac-address-randomization\|802-3-ethernet.cloned-mac-address\|ipv4.dhcp-client-id\|ipv6.dhcp-duid\)' # connection.stable-id: -- # 802-3-ethernet.cloned-mac-address: -- # 802-11-wireless.cloned-mac-address: -- # 802-11-wireless.mac-address-randomization:default +# ipv4.dhcp-client-id: -- # ipv6.ip6-privacy: -1 (unknown) # ipv6.addr-gen-mode: stable-privacy +# ipv6.dhcp-duid: -- # # then the default values are inherited and thus both the MAC -# address and the IPv6 host identifier are randomized. +# address, IPv6 host identifier, and DHCP identifiers are randomized. # Also, ipv6 private addresses (RFC4941) are used in # addition. # # +# The connection's stable-id is really a token associated with the identity +# of the connection. It means, by setting it to different values, different +# addresses and DHCP options are generated. # For some profiles it can make sense to reuse the same stable-id -# (and thus MAC address and IPv6 host identifier) for the duration +# (and thus share MAC address and IPv6 host identifier) for the duration # of the current boot, but still exclusive to the connection profile. # Thus, explicitly set the stable-id like: # # $ nmcli connection modify "$CON_NAME" connection.stable-id '${CONNECTION}/${BOOT}' # -# ... or keep it stable accross reboots, still distinct per profile: +# ... or keep it stable accross reboots, but still distinct per profile: # # $ nmcli connection modify "$CON_NAME" connection.stable-id '${CONNECTION}' # @@ -53,3 +58,23 @@ connection.stable-id=${RANDOM} ethernet.cloned-mac-address=stable wifi.cloned-mac-address=stable ipv6.ip6-privacy=2 + +# RFC 7844 "DHCP Anonymity Profiles" mandates in combination with +# MAC address randomization: +# connection.stable-id=${RANDOM} +# ethernet.cloned-mac-address=stable +# wifi.cloned-mac-address=stable +# ipv4.dhcp-client-id=mac +# ipv6.dhcp-duid=ll +# In case, the interface cannot use MAC address randomization, +# RFC 7844 recomments +# connection.stable-id=${RANDOM} +# ipv4.dhcp-client-id=stable +# ipv6.dhcp-duid=stable-llt +# See https://tools.ietf.org/html/rfc7844#section-3.5 +# https://tools.ietf.org/html/rfc7844#section-4.3 +# +# In this example however, the defaults are set to a stable identifier +# depending on the connection.stable-id. +ipv4.dhcp-client-id=stable +ipv6.dhcp-duid=stable-uuid -- cgit v1.2.1