From d5ee67981c16c7bcb464b92f5981d101f254b4e1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 19 Mar 2022 00:40:20 +0100 Subject: [PATCH 1/7] libnm/802-1x: simplify verify_tls() for phase1 and phase2 The checks are duplicated and verbose. Combine them. --- src/libnm-core-impl/nm-setting-8021x.c | 170 ++++++++----------------- 1 file changed, 52 insertions(+), 118 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-8021x.c b/src/libnm-core-impl/nm-setting-8021x.c index 884f883056..1f2db8fc33 100644 --- a/src/libnm-core-impl/nm-setting-8021x.c +++ b/src/libnm-core-impl/nm-setting-8021x.c @@ -2618,132 +2618,66 @@ static gboolean verify_tls(NMSetting8021x *self, gboolean phase2, GError **error) { NMSetting8021xPrivate *priv = NM_SETTING_802_1X_GET_PRIVATE(self); + GBytes *client_cert; + GBytes *private_key; + const char *prop_client_cert; + const char *prop_private_key; - if (phase2) { - if (!priv->phase2_client_cert) { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_MISSING_PROPERTY, - _("property is missing")); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_PHASE2_CLIENT_CERT); - return FALSE; - } else if (!g_bytes_get_size(priv->phase2_client_cert)) { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("property is empty")); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_PHASE2_CLIENT_CERT); - return FALSE; - } + client_cert = phase2 ? priv->phase2_client_cert : priv->client_cert; + private_key = phase2 ? priv->phase2_private_key : priv->private_key; + prop_client_cert = + phase2 ? NM_SETTING_802_1X_PHASE2_CLIENT_CERT : NM_SETTING_802_1X_CLIENT_CERT; + prop_private_key = + phase2 ? NM_SETTING_802_1X_PHASE2_PRIVATE_KEY : NM_SETTING_802_1X_PRIVATE_KEY; - /* Private key is required for TLS */ - if (!priv->phase2_private_key) { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_MISSING_PROPERTY, - _("property is missing")); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_PHASE2_PRIVATE_KEY); - return FALSE; - } else if (!g_bytes_get_size(priv->phase2_private_key)) { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("property is empty")); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_PHASE2_PRIVATE_KEY); - return FALSE; - } - - /* If the private key is PKCS#12, check that it matches the client cert */ - if (nm_crypto_is_pkcs12_data(g_bytes_get_data(priv->phase2_private_key, NULL), - g_bytes_get_size(priv->phase2_private_key), - NULL)) { - if (!g_bytes_equal(priv->phase2_private_key, priv->phase2_client_cert)) { - g_set_error(error, + if (!client_cert) { + g_set_error_literal(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_MISSING_PROPERTY, + _("property is missing")); + g_prefix_error(error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, prop_client_cert); + return FALSE; + } + if (g_bytes_get_size(client_cert) == 0) { + g_set_error_literal(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("has to match '%s' property for PKCS#12"), - NM_SETTING_802_1X_PHASE2_PRIVATE_KEY); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_PHASE2_CLIENT_CERT); - return FALSE; - } - } - } else { - if (!priv->client_cert) { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_MISSING_PROPERTY, - _("property is missing")); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_CLIENT_CERT); - return FALSE; - } else if (!g_bytes_get_size(priv->client_cert)) { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("property is empty")); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_CLIENT_CERT); - return FALSE; - } + _("property is empty")); + g_prefix_error(error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, prop_client_cert); + return FALSE; + } - /* Private key is required for TLS */ - if (!priv->private_key) { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_MISSING_PROPERTY, - _("property is missing")); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_PRIVATE_KEY); - return FALSE; - } else if (!g_bytes_get_size(priv->private_key)) { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("property is empty")); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_PRIVATE_KEY); - return FALSE; - } + /* Private key is required for TLS */ + if (!private_key) { + g_set_error_literal(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_MISSING_PROPERTY, + _("property is missing")); + g_prefix_error(error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, prop_private_key); + return FALSE; + } - /* If the private key is PKCS#12, check that it matches the client cert */ - if (nm_crypto_is_pkcs12_data(g_bytes_get_data(priv->private_key, NULL), - g_bytes_get_size(priv->private_key), - NULL)) { - if (!g_bytes_equal(priv->private_key, priv->client_cert)) { - g_set_error(error, + if (g_bytes_get_size(private_key) == 0) { + g_set_error_literal(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("has to match '%s' property for PKCS#12"), - NM_SETTING_802_1X_PRIVATE_KEY); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_CLIENT_CERT); - return FALSE; - } + _("property is empty")); + g_prefix_error(error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, prop_private_key); + return FALSE; + } + + /* If the private key is PKCS#12, check that it matches the client cert */ + if (nm_crypto_is_pkcs12_data(g_bytes_get_data(private_key, NULL), + g_bytes_get_size(private_key), + NULL)) { + if (!g_bytes_equal(private_key, client_cert)) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("has to match '%s' property for PKCS#12"), + prop_private_key); + g_prefix_error(error, "%s.%s: ", NM_SETTING_802_1X_SETTING_NAME, prop_client_cert); + return FALSE; } } From a3aec9dc5ca1a69043a2b60522595dc31127a4f1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 19 Mar 2022 00:49:09 +0100 Subject: [PATCH 2/7] libnm/802-1x: reuse verify_identity() in verify_ttls() implementation --- src/libnm-core-impl/nm-setting-8021x.c | 72 ++++++++++---------------- 1 file changed, 28 insertions(+), 44 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-8021x.c b/src/libnm-core-impl/nm-setting-8021x.c index 1f2db8fc33..a9cafde1c2 100644 --- a/src/libnm-core-impl/nm-setting-8021x.c +++ b/src/libnm-core-impl/nm-setting-8021x.c @@ -2614,6 +2614,33 @@ need_secrets_tls(NMSetting8021x *self, GPtrArray *secrets, gboolean phase2) } } +static gboolean +verify_identity(NMSetting8021x *self, gboolean phase2, GError **error) +{ + NMSetting8021xPrivate *priv = NM_SETTING_802_1X_GET_PRIVATE(self); + + if (nm_str_is_empty(priv->identity)) { + if (!priv->identity) { + g_set_error_literal(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_MISSING_PROPERTY, + _("property is missing")); + } else { + g_set_error_literal(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("property is empty")); + } + g_prefix_error(error, + "%s.%s: ", + NM_SETTING_802_1X_SETTING_NAME, + NM_SETTING_802_1X_IDENTITY); + return FALSE; + } + + return TRUE; +} + static gboolean verify_tls(NMSetting8021x *self, gboolean phase2, GError **error) { @@ -2689,24 +2716,8 @@ verify_ttls(NMSetting8021x *self, gboolean phase2, GError **error) { NMSetting8021xPrivate *priv = NM_SETTING_802_1X_GET_PRIVATE(self); - if (nm_str_is_empty(priv->identity)) { - if (!priv->identity) { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_MISSING_PROPERTY, - _("property is missing")); - } else { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("property is empty")); - } - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_IDENTITY); + if (!verify_identity(self, phase2, error)) return FALSE; - } if ((!priv->phase2_auth && !priv->phase2_autheap) || (priv->phase2_auth && priv->phase2_autheap)) { @@ -2726,33 +2737,6 @@ verify_ttls(NMSetting8021x *self, gboolean phase2, GError **error) return TRUE; } -static gboolean -verify_identity(NMSetting8021x *self, gboolean phase2, GError **error) -{ - NMSetting8021xPrivate *priv = NM_SETTING_802_1X_GET_PRIVATE(self); - - if (nm_str_is_empty(priv->identity)) { - if (!priv->identity) { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_MISSING_PROPERTY, - _("property is missing")); - } else { - g_set_error_literal(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("property is empty")); - } - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_802_1X_SETTING_NAME, - NM_SETTING_802_1X_IDENTITY); - return FALSE; - } - - return TRUE; -} - static void need_secrets_phase2(NMSetting8021x *self, GPtrArray *secrets, gboolean phase2) { From 47f2c5e5dbd4fe894921362f4982c21a4ddc0423 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 19 Mar 2022 00:53:16 +0100 Subject: [PATCH 3/7] libnm/802-1x: cleanup need_secrets_phase2() --- src/libnm-core-impl/nm-setting-8021x.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-8021x.c b/src/libnm-core-impl/nm-setting-8021x.c index a9cafde1c2..b744ea635f 100644 --- a/src/libnm-core-impl/nm-setting-8021x.c +++ b/src/libnm-core-impl/nm-setting-8021x.c @@ -2751,17 +2751,14 @@ need_secrets_phase2(NMSetting8021x *self, GPtrArray *secrets, gboolean phase2) if (!method) method = priv->phase2_autheap; - if (!method) { - g_warning("Couldn't find EAP method."); - g_assert_not_reached(); - return; - } + if (!method) + g_return_if_reached(); /* Ask the configured phase2 method if it needs secrets */ for (i = 0; eap_methods_table[i].method; i++) { - if (eap_methods_table[i].ns_func == NULL) + if (!eap_methods_table[i].ns_func) continue; - if (!strcmp(eap_methods_table[i].method, method)) { + if (nm_streq(eap_methods_table[i].method, method)) { (*eap_methods_table[i].ns_func)(self, secrets, TRUE); break; } From e4a7b671d6250d27010deb71af421338d2b325f8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 21 Mar 2022 18:41:13 +0100 Subject: [PATCH 4/7] libnm/802-1x: cleanup duplicate code paths in need_secrets_tls() I think code is easier to understand, if the difference (between phase1 and phase2) is pushed to the bottom. Having one large "if(phase2){}else{}" at the top makes it harder to compare the two branches and see where they differ. --- src/libnm-core-impl/nm-setting-8021x.c | 93 ++++++++++++-------------- 1 file changed, 41 insertions(+), 52 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-8021x.c b/src/libnm-core-impl/nm-setting-8021x.c index b744ea635f..a9d430873e 100644 --- a/src/libnm-core-impl/nm-setting-8021x.c +++ b/src/libnm-core-impl/nm-setting-8021x.c @@ -2557,60 +2557,49 @@ need_secrets_tls(NMSetting8021x *self, GPtrArray *secrets, gboolean phase2) GBytes *blob = NULL; const char *path = NULL; - if (phase2) { - scheme = nm_setting_802_1x_get_phase2_private_key_scheme(self); - if (scheme == NM_SETTING_802_1X_CK_SCHEME_PATH) - path = nm_setting_802_1x_get_phase2_private_key_path(self); - else if (scheme == NM_SETTING_802_1X_CK_SCHEME_BLOB) - blob = nm_setting_802_1x_get_phase2_private_key_blob(self); - else if (scheme != NM_SETTING_802_1X_CK_SCHEME_PKCS11) - g_warning("%s: unknown phase2 private key scheme %d", __func__, scheme); + scheme = phase2 ? nm_setting_802_1x_get_phase2_private_key_scheme(self) + : nm_setting_802_1x_get_private_key_scheme(self); + if (scheme == NM_SETTING_802_1X_CK_SCHEME_PATH) + path = phase2 ? nm_setting_802_1x_get_phase2_private_key_path(self) + : nm_setting_802_1x_get_private_key_path(self); + else if (scheme == NM_SETTING_802_1X_CK_SCHEME_BLOB) + blob = phase2 ? nm_setting_802_1x_get_phase2_private_key_blob(self) + : nm_setting_802_1x_get_private_key_blob(self); + else if (scheme != NM_SETTING_802_1X_CK_SCHEME_PKCS11) + g_warning("%s: unknown %sprivate key scheme %d", __func__, phase2 ? "phase2 " : "", scheme); + if (need_private_key_password( + blob, + scheme, + path, + phase2 ? priv->phase2_private_key_password : priv->private_key_password, + phase2 ? priv->phase2_private_key_password_flags : priv->private_key_password_flags)) { + g_ptr_array_add(secrets, + phase2 ? NM_SETTING_802_1X_PHASE2_PRIVATE_KEY_PASSWORD + : NM_SETTING_802_1X_PRIVATE_KEY_PASSWORD); + } - if (need_private_key_password(blob, - scheme, - path, - priv->phase2_private_key_password, - priv->phase2_private_key_password_flags)) - g_ptr_array_add(secrets, NM_SETTING_802_1X_PHASE2_PRIVATE_KEY_PASSWORD); + scheme = phase2 ? nm_setting_802_1x_get_phase2_ca_cert_scheme(self) + : nm_setting_802_1x_get_ca_cert_scheme(self); + if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11 + && !NM_FLAGS_HAS(phase2 ? priv->phase2_ca_cert_password_flags + : priv->ca_cert_password_flags, + NM_SETTING_SECRET_FLAG_NOT_REQUIRED) + && !(phase2 ? priv->phase2_ca_cert_password : priv->ca_cert_password)) { + g_ptr_array_add(secrets, + phase2 ? NM_SETTING_802_1X_PHASE2_CA_CERT_PASSWORD + : NM_SETTING_802_1X_CA_CERT_PASSWORD); + } - scheme = nm_setting_802_1x_get_phase2_ca_cert_scheme(self); - if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11 - && !(priv->phase2_ca_cert_password_flags & NM_SETTING_SECRET_FLAG_NOT_REQUIRED) - && !priv->phase2_ca_cert_password) - g_ptr_array_add(secrets, NM_SETTING_802_1X_PHASE2_CA_CERT_PASSWORD); - - scheme = nm_setting_802_1x_get_phase2_client_cert_scheme(self); - if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11 - && !(priv->phase2_client_cert_password_flags & NM_SETTING_SECRET_FLAG_NOT_REQUIRED) - && !priv->phase2_client_cert_password) - g_ptr_array_add(secrets, NM_SETTING_802_1X_PHASE2_CLIENT_CERT_PASSWORD); - } else { - scheme = nm_setting_802_1x_get_private_key_scheme(self); - if (scheme == NM_SETTING_802_1X_CK_SCHEME_PATH) - path = nm_setting_802_1x_get_private_key_path(self); - else if (scheme == NM_SETTING_802_1X_CK_SCHEME_BLOB) - blob = nm_setting_802_1x_get_private_key_blob(self); - else if (scheme != NM_SETTING_802_1X_CK_SCHEME_PKCS11) - g_warning("%s: unknown private key scheme %d", __func__, scheme); - - if (need_private_key_password(blob, - scheme, - path, - priv->private_key_password, - priv->private_key_password_flags)) - g_ptr_array_add(secrets, NM_SETTING_802_1X_PRIVATE_KEY_PASSWORD); - - scheme = nm_setting_802_1x_get_ca_cert_scheme(self); - if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11 - && !(priv->ca_cert_password_flags & NM_SETTING_SECRET_FLAG_NOT_REQUIRED) - && !priv->ca_cert_password) - g_ptr_array_add(secrets, NM_SETTING_802_1X_CA_CERT_PASSWORD); - - scheme = nm_setting_802_1x_get_client_cert_scheme(self); - if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11 - && !(priv->client_cert_password_flags & NM_SETTING_SECRET_FLAG_NOT_REQUIRED) - && !priv->client_cert_password) - g_ptr_array_add(secrets, NM_SETTING_802_1X_CLIENT_CERT_PASSWORD); + scheme = phase2 ? nm_setting_802_1x_get_phase2_client_cert_scheme(self) + : nm_setting_802_1x_get_client_cert_scheme(self); + if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11 + && !NM_FLAGS_HAS(phase2 ? priv->phase2_client_cert_password_flags + : priv->client_cert_password_flags, + NM_SETTING_SECRET_FLAG_NOT_REQUIRED) + && !(phase2 ? priv->phase2_client_cert_password : priv->client_cert_password)) { + g_ptr_array_add(secrets, + phase2 ? NM_SETTING_802_1X_PHASE2_CLIENT_CERT_PASSWORD + : NM_SETTING_802_1X_CLIENT_CERT_PASSWORD); } } From bcb1ab9e1c54410f4048a46183bf1d0fca7e703d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 21 Mar 2022 18:51:26 +0100 Subject: [PATCH 5/7] libnm/802-1x: don't use g_warning() in need_secrets_tls() g_warning() for unexpected scheme is not right. Either, this should be an assertion (and never be hit), or the library should be silent about conditions that can happen regularly. --- src/libnm-core-impl/nm-setting-8021x.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-8021x.c b/src/libnm-core-impl/nm-setting-8021x.c index a9d430873e..de5c3a48ff 100644 --- a/src/libnm-core-impl/nm-setting-8021x.c +++ b/src/libnm-core-impl/nm-setting-8021x.c @@ -2565,8 +2565,6 @@ need_secrets_tls(NMSetting8021x *self, GPtrArray *secrets, gboolean phase2) else if (scheme == NM_SETTING_802_1X_CK_SCHEME_BLOB) blob = phase2 ? nm_setting_802_1x_get_phase2_private_key_blob(self) : nm_setting_802_1x_get_private_key_blob(self); - else if (scheme != NM_SETTING_802_1X_CK_SCHEME_PKCS11) - g_warning("%s: unknown %sprivate key scheme %d", __func__, phase2 ? "phase2 " : "", scheme); if (need_private_key_password( blob, scheme, From d3a6b9e7cc9a40ae2a0845639ccc77fce68f1d57 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 21 Mar 2022 20:49:05 +0100 Subject: [PATCH 6/7] libnm/802-1x: move need_private_key_password() to need_secrets_tls() When a static function only has one caller, it is often simpler to not have the code in a separate function. Drop need_private_key_password() and move it to need_secrets_tls(). --- src/libnm-core-impl/nm-setting-8021x.c | 129 ++++++++++++------------- 1 file changed, 61 insertions(+), 68 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-8021x.c b/src/libnm-core-impl/nm-setting-8021x.c index de5c3a48ff..d720bf7a59 100644 --- a/src/libnm-core-impl/nm-setting-8021x.c +++ b/src/libnm-core-impl/nm-setting-8021x.c @@ -2520,84 +2520,77 @@ need_secrets_sim(NMSetting8021x *self, GPtrArray *secrets, gboolean phase2) g_ptr_array_add(secrets, NM_SETTING_802_1X_PIN); } -static gboolean -need_private_key_password(GBytes *blob, - NMSetting8021xCKScheme scheme, - const char *path, - const char *password, - NMSettingSecretFlags flags) -{ - NMCryptoFileFormat format = NM_CRYPTO_FILE_FORMAT_UNKNOWN; - - if (flags & NM_SETTING_SECRET_FLAG_NOT_REQUIRED) - return FALSE; - - /* Private key password is required */ - if (password) { - if (path) - format = nm_crypto_verify_private_key(path, password, NULL, NULL); - else if (blob) - format = nm_crypto_verify_private_key_data(g_bytes_get_data(blob, NULL), - g_bytes_get_size(blob), - password, - NULL, - NULL); - else - return FALSE; - } - - return (format == NM_CRYPTO_FILE_FORMAT_UNKNOWN); -} - static void need_secrets_tls(NMSetting8021x *self, GPtrArray *secrets, gboolean phase2) { NMSetting8021xPrivate *priv = NM_SETTING_802_1X_GET_PRIVATE(self); NMSetting8021xCKScheme scheme; - GBytes *blob = NULL; - const char *path = NULL; - scheme = phase2 ? nm_setting_802_1x_get_phase2_private_key_scheme(self) - : nm_setting_802_1x_get_private_key_scheme(self); - if (scheme == NM_SETTING_802_1X_CK_SCHEME_PATH) - path = phase2 ? nm_setting_802_1x_get_phase2_private_key_path(self) - : nm_setting_802_1x_get_private_key_path(self); - else if (scheme == NM_SETTING_802_1X_CK_SCHEME_BLOB) - blob = phase2 ? nm_setting_802_1x_get_phase2_private_key_blob(self) - : nm_setting_802_1x_get_private_key_blob(self); - if (need_private_key_password( - blob, - scheme, - path, - phase2 ? priv->phase2_private_key_password : priv->private_key_password, - phase2 ? priv->phase2_private_key_password_flags : priv->private_key_password_flags)) { - g_ptr_array_add(secrets, - phase2 ? NM_SETTING_802_1X_PHASE2_PRIVATE_KEY_PASSWORD - : NM_SETTING_802_1X_PRIVATE_KEY_PASSWORD); + if (!NM_FLAGS_HAS(phase2 ? priv->phase2_private_key_password_flags + : priv->private_key_password_flags, + NM_SETTING_SECRET_FLAG_NOT_REQUIRED)) { + NMCryptoFileFormat format = NM_CRYPTO_FILE_FORMAT_UNKNOWN; + gboolean has_password = FALSE; + const char *password; + + password = phase2 ? priv->phase2_private_key_password : priv->private_key_password; + + /* Check whether the password works. */ + if (password) { + scheme = phase2 ? nm_setting_802_1x_get_phase2_private_key_scheme(self) + : nm_setting_802_1x_get_private_key_scheme(self); + + if (scheme == NM_SETTING_802_1X_CK_SCHEME_PATH) { + const char *path = phase2 ? nm_setting_802_1x_get_phase2_private_key_path(self) + : nm_setting_802_1x_get_private_key_path(self); + + if (path) + format = nm_crypto_verify_private_key(path, password, NULL, NULL); + } else if (scheme == NM_SETTING_802_1X_CK_SCHEME_BLOB) { + GBytes *blob = phase2 ? nm_setting_802_1x_get_phase2_private_key_blob(self) + : nm_setting_802_1x_get_private_key_blob(self); + + if (blob) + format = nm_crypto_verify_private_key_data(g_bytes_get_data(blob, NULL), + g_bytes_get_size(blob), + password, + NULL, + NULL); + } else { + /* For PKCS#11 URLS, we assume the password is correct. */ + has_password = TRUE; + } + } + if (!has_password && format == NM_CRYPTO_FILE_FORMAT_UNKNOWN) { + g_ptr_array_add(secrets, + phase2 ? NM_SETTING_802_1X_PHASE2_PRIVATE_KEY_PASSWORD + : NM_SETTING_802_1X_PRIVATE_KEY_PASSWORD); + } } - scheme = phase2 ? nm_setting_802_1x_get_phase2_ca_cert_scheme(self) - : nm_setting_802_1x_get_ca_cert_scheme(self); - if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11 - && !NM_FLAGS_HAS(phase2 ? priv->phase2_ca_cert_password_flags - : priv->ca_cert_password_flags, - NM_SETTING_SECRET_FLAG_NOT_REQUIRED) - && !(phase2 ? priv->phase2_ca_cert_password : priv->ca_cert_password)) { - g_ptr_array_add(secrets, - phase2 ? NM_SETTING_802_1X_PHASE2_CA_CERT_PASSWORD - : NM_SETTING_802_1X_CA_CERT_PASSWORD); + if (!NM_FLAGS_HAS(phase2 ? priv->phase2_ca_cert_password_flags : priv->ca_cert_password_flags, + NM_SETTING_SECRET_FLAG_NOT_REQUIRED)) { + scheme = phase2 ? nm_setting_802_1x_get_phase2_ca_cert_scheme(self) + : nm_setting_802_1x_get_ca_cert_scheme(self); + if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11 + && !(phase2 ? priv->phase2_ca_cert_password : priv->ca_cert_password)) { + g_ptr_array_add(secrets, + phase2 ? NM_SETTING_802_1X_PHASE2_CA_CERT_PASSWORD + : NM_SETTING_802_1X_CA_CERT_PASSWORD); + } } - scheme = phase2 ? nm_setting_802_1x_get_phase2_client_cert_scheme(self) - : nm_setting_802_1x_get_client_cert_scheme(self); - if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11 - && !NM_FLAGS_HAS(phase2 ? priv->phase2_client_cert_password_flags - : priv->client_cert_password_flags, - NM_SETTING_SECRET_FLAG_NOT_REQUIRED) - && !(phase2 ? priv->phase2_client_cert_password : priv->client_cert_password)) { - g_ptr_array_add(secrets, - phase2 ? NM_SETTING_802_1X_PHASE2_CLIENT_CERT_PASSWORD - : NM_SETTING_802_1X_CLIENT_CERT_PASSWORD); + if (!NM_FLAGS_HAS(phase2 ? priv->phase2_client_cert_password_flags + : priv->client_cert_password_flags, + NM_SETTING_SECRET_FLAG_NOT_REQUIRED)) { + scheme = phase2 ? nm_setting_802_1x_get_phase2_client_cert_scheme(self) + : nm_setting_802_1x_get_client_cert_scheme(self); + if (scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11 + && !(phase2 ? priv->phase2_client_cert_password : priv->client_cert_password)) { + g_ptr_array_add(secrets, + phase2 ? NM_SETTING_802_1X_PHASE2_CLIENT_CERT_PASSWORD + : NM_SETTING_802_1X_CLIENT_CERT_PASSWORD); + } } } From ee1467fcdba39733a96e265a4e630cc758bcce7a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 19 Mar 2022 00:42:02 +0100 Subject: [PATCH 7/7] libnm/802-1x: check is-pkcs12 only for blob certificates in verify_tls() If the certificate is not a blob, it makes no sense to call nm_crypto_is_pkcs12_data(). --- src/libnm-core-impl/nm-setting-8021x.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-8021x.c b/src/libnm-core-impl/nm-setting-8021x.c index d720bf7a59..fa0a3057fe 100644 --- a/src/libnm-core-impl/nm-setting-8021x.c +++ b/src/libnm-core-impl/nm-setting-8021x.c @@ -2673,10 +2673,11 @@ verify_tls(NMSetting8021x *self, gboolean phase2, GError **error) return FALSE; } - /* If the private key is PKCS#12, check that it matches the client cert */ - if (nm_crypto_is_pkcs12_data(g_bytes_get_data(private_key, NULL), - g_bytes_get_size(private_key), - NULL)) { + if (_nm_setting_802_1x_cert_get_scheme(private_key, NULL) == NM_SETTING_802_1X_CK_SCHEME_BLOB + && nm_crypto_is_pkcs12_data(g_bytes_get_data(private_key, NULL), + g_bytes_get_size(private_key), + NULL)) { + /* If the private key is PKCS#12, check that it matches the client cert */ if (!g_bytes_equal(private_key, client_cert)) { g_set_error(error, NM_CONNECTION_ERROR,