From 16bf47f8ca520b61fffcbb689788bf9f4305b066 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 2 Jan 2022 10:44:02 +0100 Subject: [PATCH] libnm: automatically clear secret string for direct string properties Let's sprinkle some snake ointment. This is questionable, because we copy secrets all over the place where we their deallocation (and clearing) is not in our control. For example, the GValue setter/getter copies the string (but does not clean the secret). Also, when converting the property to a GVariant, we won't clear it. So this does not catch a lot of cases. Still, if we can with relative ease avoid leaking the string at some places, do it. --- src/libnm-core-impl/nm-setting.c | 26 ++++++++++++++++++------ src/libnm-core-impl/tests/test-setting.c | 23 +++++++++++++++++---- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/libnm-core-impl/nm-setting.c b/src/libnm-core-impl/nm-setting.c index 5d8db7cc12..ccf80e9714 100644 --- a/src/libnm-core-impl/nm-setting.c +++ b/src/libnm-core-impl/nm-setting.c @@ -8,11 +8,12 @@ #include "nm-setting.h" -#include "nm-setting-private.h" -#include "nm-utils.h" #include "libnm-core-intern/nm-core-internal.h" -#include "nm-utils-private.h" +#include "libnm-glib-aux/nm-secret-utils.h" #include "nm-property-compare.h" +#include "nm-setting-private.h" +#include "nm-utils-private.h" +#include "nm-utils.h" /** * SECTION:nm-setting @@ -672,9 +673,13 @@ _property_direct_set_string(const NMSettInfoProperty *property_info, char **dst, goto out_take; } + if (NM_FLAGS_HAS(property_info->param_spec->flags, NM_SETTING_PARAM_SECRET)) + return nm_strdup_reset_secret(dst, src); + return nm_strdup_reset(dst, src); out_take: + nm_assert(!NM_FLAGS_HAS(property_info->param_spec->flags, NM_SETTING_PARAM_SECRET)); return nm_strdup_reset_take(dst, s); } @@ -1039,7 +1044,10 @@ _finalize_direct(NMSetting *setting) char **p_val = _nm_setting_get_private(setting, sett_info, property_info->direct_offset); - nm_clear_g_free(p_val); + if (NM_FLAGS_HAS(property_info->param_spec->flags, NM_SETTING_PARAM_SECRET)) + nm_clear_pointer(p_val, nm_free_secret); + else + nm_clear_g_free(p_val); break; } case NM_VALUE_TYPE_BYTES: @@ -1458,6 +1466,7 @@ _nm_setting_property_from_dbus_fcn_direct(_NM_SETT_INFO_PROP_FROM_DBUS_FCN_ARGS gs_free char *v_free = NULL; char **p_val; const char *v; + gboolean changed; if (g_variant_is_of_type(value, G_VARIANT_TYPE_STRING)) { v = g_variant_get_string(value, NULL); @@ -1472,9 +1481,14 @@ _nm_setting_property_from_dbus_fcn_direct(_NM_SETT_INFO_PROP_FROM_DBUS_FCN_ARGS } p_val = _nm_setting_get_private(setting, sett_info, property_info->direct_offset); - if (!_property_direct_set_string(property_info, p_val, v)) - goto out_unchanged; + changed = _property_direct_set_string(property_info, p_val, v); + + if (NM_FLAGS_HAS(property_info->param_spec->flags, NM_SETTING_PARAM_SECRET)) + nm_clear_pointer(&v_free, nm_free_secret); + + if (!changed) + goto out_unchanged; goto out_notify; } case NM_VALUE_TYPE_BYTES: diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c index 4fbddc4823..b4c67079ef 100644 --- a/src/libnm-core-impl/tests/test-setting.c +++ b/src/libnm-core-impl/tests/test-setting.c @@ -4430,6 +4430,7 @@ test_setting_metadata(void) GArray *property_types_data; guint prop_idx_val; gboolean can_set_including_default = FALSE; + int n_special_options; g_assert(sip->name); @@ -4579,10 +4580,24 @@ test_setting_metadata(void) g_assert(sip->property_type->direct_type == NM_VALUE_TYPE_STRING); } - g_assert(((sip->direct_set_string_mac_address_len != 0) - + (!!sip->direct_set_string_strip) + (!!sip->direct_set_string_ascii_strdown) - + (sip->direct_set_string_ip_address_addr_family != 0)) - <= 1); + n_special_options = (sip->direct_set_string_mac_address_len != 0) + + (!!sip->direct_set_string_strip) + + (!!sip->direct_set_string_ascii_strdown) + + (sip->direct_set_string_ip_address_addr_family != 0); + + /* currently, we have no cases where special options are mixed. There is no problem to support + * that, but as it's not needed, don't do it for now. */ + g_assert_cmpint(n_special_options, <=, 1); + + if (n_special_options > 0) { + /* currently, special options are only relevant for string properties. */ + g_assert(sip->property_type->direct_type == NM_VALUE_TYPE_STRING); + } + + if (sip->param_spec && NM_FLAGS_HAS(sip->param_spec->flags, NM_SETTING_PARAM_SECRET)) { + /* Currently, special options are not supported for secrets. */ + g_assert_cmpint(n_special_options, ==, 0); + } if (!sip->property_type->to_dbus_fcn) { /* it's allowed to have no to_dbus_fcn(), to ignore a property. But such