libnm/bond: remove validation from nm_setting_bond_add_option() and explicitly validate

For historic reasons is NMSettingBond implemented differently from other
settings. It uses a strdict, and adds some validation on top of that.
The idea was probably to be able to treat bond options more generically.
But in practice we cannot treat them as opaque values, but need to know,
validate and understand all the options. Thus, this implementation with a
strdict is not nice.

The user can set the GObject property NM_SETTING_BOND_OPTIONS to any
strdict, and the setter performs no validation or normalization. That
is probably good, because g_object_set() cannot return an error to
signalize invalid settings. As often, we have corresponding C API like
nm_setting_bond_add_option() and nm_setting_bond_remove_option(). It
should be possible to get the same result both with the C API and with
the GObject property setting. Since there is already a way to set
certain invalid values, it does not help if the C API tries to prevent
that. That implies, that also add-option does not perform additional
validation and sets whatever the user asks.

Remove all validation from nm_setting_bond_add_option() and
nm_setting_bond_remove_option(). This validation was anyway only very
basic. It was calling nm_setting_bond_validate_option(), which can check
whether the string is (for example) and integer, but it cannot do
validation beyond one option. In most cases, the validation needs to
take into account the bond mode or other options, so validating one
option in isolation is not very useful.

Proper validation should instead be done via nm_connection_verify().
However, due to another historic oddity, that verification is very
forgiving too and doesn't reject many invalid settings when it should.
That is hard to fix, because making validation more strict can break
existing (and working) configurations. However, verify() already contains
basic validation via nm_setting_bond_validate_option(). So in the previous
behavior nm_setting_bond_add_option() would silently do nothing (only
returning %FALSE) for invalid options, while now it would add the
invalid options to the dictionary -- only to have it later fail validation
during nm_connection_verify(). That is a slight change in behavior, however it
seems preferable.

It seems preferable and acceptable because most users that call
nm_setting_bond_add_option() already understand the meaning and valid
values. Keyfile and ifcfg-rh readers are the few exceptions, which really just
parse a string dictionary, without need to understand them. But nmtui
or nmstate already know the option they want to set. They don't expect
a failure there, nor do they need the validation.

Note that this change in behavior could be dangerous for example for the
keyfile/ifcfg-rh readers, which silently ignored errors before. We
don't want them to start failing if they read invalid options from a
file, so instead let those callers explicitly pre-validate the value
and log an warning.

https://bugzilla.redhat.com/show_bug.cgi?id=1887523

7 files changed, 89 insertions, 53 deletions

diff --git a/clients/common/nm-meta-setting-desc.c b/clients/common/nm-meta-setting-desc.c
index cdb3084a07..115f65591a 100644
--- a/clients/common/nm-meta-setting-desc.c
+++ b/clients/common/nm-meta-setting-desc.c
@@ -2400,13 +2400,9 @@ _nm_meta_setting_bond_add_option(NMSetting * setting,
     char *         p;
 
     if (!value || !value[0]) {
-        if (!nm_setting_bond_remove_option(s_bond, name)) {
-            nm_utils_error_set(error,
-                               NM_UTILS_ERROR_INVALID_ARGUMENT,
-                               _("failed to unset bond option \"%s\""),
-                               name);
-            return FALSE;
-        }
+        /* This call only fails if name is currently not tracked. It's not an
+         * error to remove an option that is not set. */
+        nm_setting_bond_remove_option(s_bond, name);
         return TRUE;
     }
 
@@ -2421,7 +2417,7 @@ _nm_meta_setting_bond_add_option(NMSetting * setting,
                 *p = ',';
     }
 
-    if (!nm_setting_bond_add_option(s_bond, name, value)) {
+    if (!nm_setting_bond_validate_option(name, value)) {
         nm_utils_error_set(error,
                            NM_UTILS_ERROR_INVALID_ARGUMENT,
                            _("failed to set bond option \"%s\""),
@@ -2429,6 +2425,8 @@ _nm_meta_setting_bond_add_option(NMSetting * setting,
         return FALSE;
     }
 
+    nm_setting_bond_add_option(s_bond, name, value);
+
     if (nm_streq(name, NM_SETTING_BOND_OPTION_ARP_INTERVAL)) {
         if (_nm_utils_ascii_str_to_int64(value, 10, 0, G_MAXINT, 0) > 0)
             _nm_setting_bond_remove_options_miimon(s_bond);
diff --git a/libnm-core/nm-keyfile/nm-keyfile.c b/libnm-core/nm-keyfile/nm-keyfile.c
index 395af1db09..a8aefe6349 100644
--- a/libnm-core/nm-keyfile/nm-keyfile.c
+++ b/libnm-core/nm-keyfile/nm-keyfile.c
@@ -1141,7 +1141,10 @@ mac_address_parser_INFINIBAND(KeyfileReaderInfo *info, NMSetting *setting, const
 }
 
 static void
-read_hash_of_string(GKeyFile *file, NMSetting *setting, const char *key)
+read_hash_of_string(KeyfileReaderInfo *info,
+                    GKeyFile *         file,
+                    NMSetting *        setting,
+                    const char *       kf_group)
 {
     gs_strfreev char **keys = NULL;
     const char *const *iter;
@@ -1149,10 +1152,10 @@ read_hash_of_string(GKeyFile *file, NMSetting *setting, const char *key)
     gboolean           is_vpn;
     gsize              n_keys;
 
-    nm_assert((NM_IS_SETTING_VPN(setting) && nm_streq(key, NM_SETTING_VPN_DATA))
-              || (NM_IS_SETTING_VPN(setting) && nm_streq(key, NM_SETTING_VPN_SECRETS))
-              || (NM_IS_SETTING_BOND(setting) && nm_streq(key, NM_SETTING_BOND_OPTIONS))
-              || (NM_IS_SETTING_USER(setting) && nm_streq(key, NM_SETTING_USER_DATA)));
+    nm_assert((NM_IS_SETTING_VPN(setting) && nm_streq(kf_group, NM_SETTING_VPN_DATA))
+              || (NM_IS_SETTING_VPN(setting) && nm_streq(kf_group, NM_SETTING_VPN_SECRETS))
+              || (NM_IS_SETTING_BOND(setting) && nm_streq(kf_group, NM_SETTING_BOND_OPTIONS))
+              || (NM_IS_SETTING_USER(setting) && nm_streq(kf_group, NM_SETTING_USER_DATA)));
 
     keys = nm_keyfile_plugin_kf_get_keys(file, setting_name, &n_keys, NULL);
     if (n_keys == 0)
@@ -1160,23 +1163,38 @@ read_hash_of_string(GKeyFile *file, NMSetting *setting, const char *key)
 
     if ((is_vpn = NM_IS_SETTING_VPN(setting)) || NM_IS_SETTING_BOND(setting)) {
         for (iter = (const char *const *) keys; *iter; iter++) {
+            const char *  kf_key  = *iter;
             gs_free char *to_free = NULL;
             gs_free char *value   = NULL;
             const char *  name;
 
-            value = nm_keyfile_plugin_kf_get_string(file, setting_name, *iter, NULL);
+            value = nm_keyfile_plugin_kf_get_string(file, setting_name, kf_key, NULL);
             if (!value)
                 continue;
 
-            name = nm_keyfile_key_decode(*iter, &to_free);
+            name = nm_keyfile_key_decode(kf_key, &to_free);
 
             if (is_vpn) {
                 /* Add any item that's not a class property to the data hash */
                 if (!g_object_class_find_property(G_OBJECT_GET_CLASS(setting), name))
                     nm_setting_vpn_add_data_item(NM_SETTING_VPN(setting), name, value);
             } else {
-                if (!nm_streq(name, "interface-name"))
-                    nm_setting_bond_add_option(NM_SETTING_BOND(setting), name, value);
+                if (!nm_streq(name, "interface-name")) {
+                    gs_free_error GError *error = NULL;
+
+                    if (!_nm_setting_bond_validate_option(name, value, &error)) {
+                        if (!handle_warn(info,
+                                         kf_key,
+                                         name,
+                                         NM_KEYFILE_WARN_SEVERITY_WARN,
+                                         _("ignoring invalid bond option %s%s%s = %s%s%s: %s"),
+                                         NM_PRINT_FMT_QUOTE_STRING(name),
+                                         NM_PRINT_FMT_QUOTE_STRING(value),
+                                         error->message))
+                            return;
+                    } else
+                        nm_setting_bond_add_option(NM_SETTING_BOND(setting), name, value);
+                }
             }
         }
         openconnect_fix_secret_flags(setting);
@@ -3170,7 +3188,7 @@ read_one_setting_value(KeyfileReaderInfo *       info,
                                                   NULL);
         g_object_set(setting, key, sa, NULL);
     } else if (type == G_TYPE_HASH_TABLE) {
-        read_hash_of_string(keyfile, setting, key);
+        read_hash_of_string(info, keyfile, setting, key);
     } else if (type == G_TYPE_ARRAY) {
         read_array_of_uint(keyfile, setting, key);
     } else if (G_TYPE_IS_FLAGS(type)) {
diff --git a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h
index 025d684776..ca29eade24 100644
--- a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h
+++ b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h
@@ -73,6 +73,8 @@ NMBondMode _nm_setting_bond_mode_from_string(const char *str);
 
 const char *_nm_setting_bond_mode_to_string(int mode);
 
+gboolean _nm_setting_bond_validate_option(const char *name, const char *value, GError **error);
+
 /*****************************************************************************/
 
 static inline guint32
diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c
index f355734bfa..2155815e06 100644
--- a/libnm-core/nm-setting-bond.c
+++ b/libnm-core/nm-setting-bond.c
@@ -515,8 +515,8 @@ validate_ifname(const char *name, const char *value)
     return nm_utils_ifname_valid_kernel(value, NULL);
 }
 
-static gboolean
-_setting_bond_validate_option(const char *name, const char *value, GError **error)
+gboolean
+_nm_setting_bond_validate_option(const char *name, const char *value, GError **error)
 {
     const OptionMeta *option_meta;
     gboolean          success;
@@ -589,7 +589,7 @@ handle_error:
 gboolean
 nm_setting_bond_validate_option(const char *name, const char *value)
 {
-    return _setting_bond_validate_option(name, value, NULL);
+    return _nm_setting_bond_validate_option(name, value, NULL);
 }
 
 /**
@@ -608,9 +608,6 @@ nm_setting_bond_get_option_by_name(NMSettingBond *setting, const char *name)
 {
     g_return_val_if_fail(NM_IS_SETTING_BOND(setting), NULL);
 
-    if (!nm_setting_bond_validate_option(name, NULL))
-        return NULL;
-
     return _bond_get_option(setting, name);
 }
 
@@ -620,16 +617,18 @@ nm_setting_bond_get_option_by_name(NMSettingBond *setting, const char *name)
  * @name: name for the option
  * @value: value for the option
  *
- * Add an option to the table.  The option is compared to an internal list
- * of allowed options.  Option names may contain only alphanumeric characters
- * (ie [a-zA-Z0-9]).  Adding a new name replaces any existing name/value pair
+ * Add an option to the table. Adding a new name replaces any existing name/value pair
  * that may already exist.
  *
- * The order of how to set several options is relevant because there are options
- * that conflict with each other.
+ * Returns: returns %FALSE if either @name or @value is %NULL, in that case
+ * the option is not set. Otherwise, the function does not fail and does not validate
+ * the arguments. All validation happens via nm_connection_verify() or do basic validation
+ * yourself with nm_setting_bond_validate_option().
  *
- * Returns: %TRUE if the option was valid and was added to the internal option
- * list, %FALSE if it was not.
+ * Note: Before 1.30, libnm would perform basic validation of the name and the value
+ * via nm_setting_bond_validate_option() and reject the request by returning FALSE.
+ * Since 1.30, libnm no longer rejects any values as the setter is not supposed
+ * to perform validation.
  **/
 gboolean
 nm_setting_bond_add_option(NMSettingBond *setting, const char *name, const char *value)
@@ -638,16 +637,16 @@ nm_setting_bond_add_option(NMSettingBond *setting, const char *name, const char
 
     g_return_val_if_fail(NM_IS_SETTING_BOND(setting), FALSE);
 
-    if (!value || !nm_setting_bond_validate_option(name, value))
+    if (!name)
+        return FALSE;
+    if (!value)
         return FALSE;
 
     priv = NM_SETTING_BOND_GET_PRIVATE(setting);
 
     nm_clear_g_free(&priv->options_idx_cache);
     g_hash_table_insert(priv->options, g_strdup(name), g_strdup(value));
-
     _notify(setting, PROP_OPTIONS);
-
     return TRUE;
 }
 
@@ -666,20 +665,17 @@ gboolean
 nm_setting_bond_remove_option(NMSettingBond *setting, const char *name)
 {
     NMSettingBondPrivate *priv;
-    gboolean              found;
 
     g_return_val_if_fail(NM_IS_SETTING_BOND(setting), FALSE);
 
-    if (!nm_setting_bond_validate_option(name, NULL))
-        return FALSE;
-
     priv = NM_SETTING_BOND_GET_PRIVATE(setting);
 
+    if (!g_hash_table_remove(priv->options, name))
+        return FALSE;
+
     nm_clear_g_free(&priv->options_idx_cache);
-    found = g_hash_table_remove(priv->options, name);
-    if (found)
-        _notify(setting, PROP_OPTIONS);
-    return found;
+    _notify(setting, PROP_OPTIONS);
+    return TRUE;
 }
 
 /**
@@ -783,7 +779,7 @@ verify(NMSetting *setting, NMConnection *connection, GError **error)
         for (i = 0; priv->options_idx_cache[i].name; i++) {
             n = &priv->options_idx_cache[i];
 
-            if (!n->value_str || !_setting_bond_validate_option(n->name, n->value_str, error)) {
+            if (!n->value_str || !_nm_setting_bond_validate_option(n->name, n->value_str, error)) {
                 g_prefix_error(error,
                                "%s.%s: ",
                                NM_SETTING_BOND_SETTING_NAME,
diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c
index a54c9b47c4..43f2a92ef4 100644
--- a/src/devices/nm-device-bond.c
+++ b/src/devices/nm-device-bond.c
@@ -149,6 +149,7 @@ ignore_option(NMSettingBond *s_bond, const char *option, const char *value)
 static void
 update_connection(NMDevice *device, NMConnection *connection)
 {
+    NMDeviceBond * self    = NM_DEVICE_BOND(device);
     NMSettingBond *s_bond  = nm_connection_get_setting_bond(connection);
     int            ifindex = nm_device_get_ifindex(device);
     NMBondMode     mode    = NM_BOND_MODE_UNKNOWN;
@@ -197,7 +198,10 @@ update_connection(NMDevice *device, NMConnection *connection)
                 }
             }
 
-            nm_setting_bond_add_option(s_bond, option, value);
+            if (!_nm_setting_bond_validate_option(option, value, NULL))
+                _LOGT(LOGD_BOND, "cannot set invalid bond option '%s' = '%s'", option, value);
+            else
+                nm_setting_bond_add_option(s_bond, option, value);
         }
     }
 }
diff --git a/src/initrd/nmi-cmdline-reader.c b/src/initrd/nmi-cmdline-reader.c
index b9432d4a62..ead9053683 100644
--- a/src/initrd/nmi-cmdline-reader.c
+++ b/src/initrd/nmi-cmdline-reader.c
@@ -682,8 +682,6 @@ reader_parse_master(Reader *reader, char *argument, const char *type_name, const
     char *               slaves;
     const char *         slave;
     char *               opts;
-    char *               opt;
-    const char *         opt_name;
     const char *         mtu = NULL;
 
     master = get_word(&argument, ':');
@@ -705,8 +703,21 @@ reader_parse_master(Reader *reader, char *argument, const char *type_name, const
 
         opts = get_word(&argument, ':');
         while (opts && *opts) {
+            gs_free_error GError *error = NULL;
+            char *                opt;
+            const char *          opt_name;
+
             opt      = get_word(&opts, ',');
             opt_name = get_word(&opt, '=');
+
+            if (!_nm_setting_bond_validate_option(opt_name, opt, &error)) {
+                _LOGW(LOGD_CORE,
+                      "Ignoring invalid bond option: %s%s%s = %s%s%s: %s",
+                      NM_PRINT_FMT_QUOTE_STRING(opt_name),
+                      NM_PRINT_FMT_QUOTE_STRING(opt),
+                      error->message);
+                continue;
+            }
             nm_setting_bond_add_option(s_bond, opt_name, opt);
         }
 
diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c
index 5a021a8c37..3389b2a0aa 100644
--- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c
+++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c
@@ -5339,24 +5339,31 @@ infiniband_connection_from_ifcfg(const char *file, shvarFile *ifcfg, GError **er
 static void
 handle_bond_option(NMSettingBond *s_bond, const char *key, const char *value)
 {
-    char *      sanitized = NULL, *j;
-    const char *p         = value;
+    gs_free char *sanitized = NULL;
+    const char *  p         = value;
 
     /* Remove any quotes or +/- from arp_ip_target */
-    if (!g_strcmp0(key, NM_SETTING_BOND_OPTION_ARP_IP_TARGET) && value && value[0]) {
+    if (nm_streq0(key, NM_SETTING_BOND_OPTION_ARP_IP_TARGET) && value && value[0]) {
+        char *j;
+
         if (*p == '\'' || *p == '"')
             p++;
-        j = sanitized = g_malloc0(strlen(p) + 1);
+        j = sanitized = g_malloc(strlen(p) + 1);
         while (*p) {
             if (*p != '+' && *p != '-' && *p != '\'' && *p != '"')
                 *j++ = *p;
             p++;
         }
+        *j++  = '\0';
+        value = sanitized;
+    }
+
+    if (!_nm_setting_bond_validate_option(key, value, NULL)) {
+        PARSE_WARNING("invalid bonding option '%s' = %s", key, value);
+        return;
     }
 
-    if (!nm_setting_bond_add_option(s_bond, key, sanitized ?: value))
-        PARSE_WARNING("invalid bonding option '%s' = %s", key, sanitized ?: value);
-    g_free(sanitized);
+    nm_setting_bond_add_option(s_bond, key, value);
 }
 
 static NMSetting *