libnm-core: tweak crypto.c APIs

Update crypto_verify_private_key() and
crypto_verify_private_key_data() to indicate whether the key was
encrypted or not.

Rename crypto_decrypt_private_key() and
crypto_decrypt_private_key_data() to
crypto_decrypt_openssl_private_key*, since that's the only private key
format they deal with, and the old names made them sound more generic
than they were. Also, update the openssl private key parsing code to
recognize unencrypted private keys as well. (Previously we accepted
unencrypted PKCS#8 keys, but not unencrypted openssl-style keys.)

5 files changed, 88 insertions, 78 deletions

diff --git a/libnm-core/crypto.c b/libnm-core/crypto.c
index a695747109..766c6513c5 100644
--- a/libnm-core/crypto.c
+++ b/libnm-core/crypto.c
@@ -78,7 +78,7 @@ find_tag (const char *tag,
 static GByteArray *
 parse_old_openssl_key_file (const guint8 *data,
                             gsize data_len,
-                            int key_type,
+                            NMCryptoKeyType *out_key_type,
                             char **out_cipher,
                             char **out_iv,
                             GError **error)
@@ -89,6 +89,7 @@ parse_old_openssl_key_file (const guint8 *data,
 	gsize start = 0, end = 0;
 	GString *str = NULL;
 	int enc_tags = 0;
+	NMCryptoKeyType key_type;
 	char *iv = NULL;
 	char *cipher = NULL;
 	unsigned char *tmp = NULL;
@@ -97,20 +98,15 @@ parse_old_openssl_key_file (const guint8 *data,
 	const char *end_tag;
 	guint8 save_end = 0;
 
-	switch (key_type) {
-	case NM_CRYPTO_KEY_TYPE_RSA:
+	if (find_tag (PEM_RSA_KEY_BEGIN, data, data_len, 0, &start)) {
+		key_type = NM_CRYPTO_KEY_TYPE_RSA;
 		start_tag = PEM_RSA_KEY_BEGIN;
 		end_tag = PEM_RSA_KEY_END;
-		break;
-	case NM_CRYPTO_KEY_TYPE_DSA:
+	} else if (find_tag (PEM_DSA_KEY_BEGIN, data, data_len, 0, &start)) {
+		key_type = NM_CRYPTO_KEY_TYPE_DSA;
 		start_tag = PEM_DSA_KEY_BEGIN;
 		end_tag = PEM_DSA_KEY_END;
-		break;
-	default:
-		g_assert_not_reached ();
-	}
-
-	if (!find_tag (start_tag, data, data_len, 0, &start))
+	} else
 		goto parse_error;
 
 	start += strlen (start_tag);
@@ -144,7 +140,7 @@ parse_old_openssl_key_file (const guint8 *data,
 			continue;
 
 		if (!strncmp (p, PROC_TYPE_TAG, strlen (PROC_TYPE_TAG))) {
-			if (enc_tags++ != 0) {
+			if (enc_tags++ != 0 || str->len != 0) {
 				g_set_error (error, NM_CRYPTO_ERROR,
 				             NM_CRYPTO_ERROR_INVALID_DATA,
 				             _("Malformed PEM file: Proc-Type was not first tag."));
@@ -162,7 +158,7 @@ parse_old_openssl_key_file (const guint8 *data,
 		} else if (!strncmp (p, DEK_INFO_TAG, strlen (DEK_INFO_TAG))) {
 			char *comma;
 
-			if (enc_tags++ != 1) {
+			if (enc_tags++ != 1 || str->len != 0) {
 				g_set_error (error, NM_CRYPTO_ERROR,
 				             NM_CRYPTO_ERROR_INVALID_DATA,
 				             _("Malformed PEM file: DEK-Info was not the second tag."));
@@ -203,7 +199,7 @@ parse_old_openssl_key_file (const guint8 *data,
 				goto parse_error;
 			}
 		} else {
-			if ((enc_tags != 0) && (enc_tags != 2)) {
+			if (enc_tags == 1) {
 				g_set_error (error, NM_CRYPTO_ERROR,
 				             NM_CRYPTO_ERROR_INVALID_DATA,
 				             "Malformed PEM file: both Proc-Type and DEK-Info tags are required.");
@@ -229,6 +225,7 @@ parse_old_openssl_key_file (const guint8 *data,
 	g_byte_array_append (bindata, tmp, tmp_len);
 	g_free (tmp);
 
+	*out_key_type = key_type;
 	*out_iv = iv;
 	*out_cipher = cipher;
 	return bindata;
@@ -475,14 +472,14 @@ out:
 }
 
 GByteArray *
-crypto_decrypt_private_key_data (const guint8 *data,
-                                 gsize data_len,
-                                 const char *password,
-                                 NMCryptoKeyType *out_key_type,
-                                 GError **error)
+crypto_decrypt_openssl_private_key_data (const guint8 *data,
+                                         gsize data_len,
+                                         const char *password,
+                                         NMCryptoKeyType *out_key_type,
+                                         GError **error)
 {
 	GByteArray *decrypted = NULL;
-	NMCryptoKeyType key_type = NM_CRYPTO_KEY_TYPE_RSA;
+	NMCryptoKeyType key_type = NM_CRYPTO_KEY_TYPE_UNKNOWN;
 	GByteArray *parsed;
 	char *iv = NULL;
 	char *cipher = NULL;
@@ -491,30 +488,24 @@ crypto_decrypt_private_key_data (const guint8 *data,
 	if (out_key_type)
 		g_return_val_if_fail (*out_key_type == NM_CRYPTO_KEY_TYPE_UNKNOWN, NULL);
 
-	/* OpenSSL non-standard legacy PEM files */
+	parsed = parse_old_openssl_key_file (data, data_len, &key_type, &cipher, &iv, NULL);
+	/* return the key type even if decryption failed */
+	if (out_key_type)
+		*out_key_type = key_type;
 
-	/* Try RSA keys first */
-	parsed = parse_old_openssl_key_file (data, data_len, key_type, &cipher, &iv, error);
 	if (!parsed) {
-		g_clear_error (error);
-
-		/* DSA next */
-		key_type = NM_CRYPTO_KEY_TYPE_DSA;
-		parsed = parse_old_openssl_key_file (data, data_len, key_type, &cipher, &iv, error);
-		if (!parsed) {
-			g_clear_error (error);
-			g_set_error (error, NM_CRYPTO_ERROR,
-			             NM_CRYPTO_ERROR_INVALID_DATA,
-			             _("Unable to determine private key type."));
-		}
+		g_set_error (error, NM_CRYPTO_ERROR,
+		             NM_CRYPTO_ERROR_INVALID_DATA,
+		             _("Unable to determine private key type."));
+		return NULL;
 	}
 
-	if (parsed) {
-		/* return the key type even if decryption failed */
-		if (out_key_type)
-			*out_key_type = key_type;
-
-		if (password) {
+	if (password) {
+		if (!cipher || !iv) {
+			g_set_error (error, NM_CRYPTO_ERROR,
+			             NM_CRYPTO_ERROR_INVALID_PASSWORD,
+			             _("Password provided, but key was not encrypted."));
+		} else {
 			decrypted = decrypt_key (cipher,
 			                         key_type,
 			                         parsed->data,
@@ -523,9 +514,10 @@ crypto_decrypt_private_key_data (const guint8 *data,
 			                         password,
 			                         error);
 		}
-		g_byte_array_free (parsed, TRUE);
-	}
+	} else if (!cipher && !iv)
+		decrypted = g_byte_array_ref (parsed);
 
+	g_byte_array_unref (parsed);
 	g_free (cipher);
 	g_free (iv);
 
@@ -533,18 +525,18 @@ crypto_decrypt_private_key_data (const guint8 *data,
 }
 
 GByteArray *
-crypto_decrypt_private_key (const char *file,
-                            const char *password,
-                            NMCryptoKeyType *out_key_type,
-                            GError **error)
+crypto_decrypt_openssl_private_key (const char *file,
+                                    const char *password,
+                                    NMCryptoKeyType *out_key_type,
+                                    GError **error)
 {
 	GByteArray *contents;
 	GByteArray *key = NULL;
 
 	contents = file_to_g_byte_array (file, error);
 	if (contents) {
-		key = crypto_decrypt_private_key_data (contents->data, contents->len,
-		                                       password, out_key_type, error);
+		key = crypto_decrypt_openssl_private_key_data (contents->data, contents->len,
+		                                               password, out_key_type, error);
 		g_byte_array_free (contents, TRUE);
 	}
 	return key;
@@ -684,6 +676,7 @@ NMCryptoFileFormat
 crypto_verify_private_key_data (const guint8 *data,
                                 gsize data_len,
                                 const char *password,
+                                gboolean *out_is_encrypted,
                                 GError **error)
 {
 	GByteArray *tmp;
@@ -691,42 +684,50 @@ crypto_verify_private_key_data (const guint8 *data,
 	NMCryptoKeyType ktype = NM_CRYPTO_KEY_TYPE_UNKNOWN;
 	gboolean is_encrypted = FALSE;
 
-	g_return_val_if_fail (data != NULL, FALSE);
+	g_return_val_if_fail (data != NULL, NM_CRYPTO_FILE_FORMAT_UNKNOWN);
+	g_return_val_if_fail (out_is_encrypted == NULL || *out_is_encrypted == FALSE, NM_CRYPTO_FILE_FORMAT_UNKNOWN);
 
 	/* Check for PKCS#12 first */
 	if (crypto_is_pkcs12_data (data, data_len)) {
+		is_encrypted = TRUE;
 		if (!password || crypto_verify_pkcs12 (data, data_len, password, error))
 			format = NM_CRYPTO_FILE_FORMAT_PKCS12;
 	} else {
 		/* Maybe it's PKCS#8 */
-		tmp = parse_pkcs8_key_file (data, data_len, &is_encrypted, error);
+		tmp = parse_pkcs8_key_file (data, data_len, &is_encrypted, NULL);
 		if (tmp) {
 			if (crypto_verify_pkcs8 (tmp->data, tmp->len, is_encrypted, password, error))
 				format = NM_CRYPTO_FILE_FORMAT_RAW_KEY;
 		} else {
-			g_clear_error (error);
+			char *cipher, *iv;
 
 			/* Or it's old-style OpenSSL */
-			tmp = crypto_decrypt_private_key_data (data, data_len, password, &ktype, error);
-			if (tmp)
-				format = NM_CRYPTO_FILE_FORMAT_RAW_KEY;
-			else if (!password && (ktype != NM_CRYPTO_KEY_TYPE_UNKNOWN))
+			tmp = parse_old_openssl_key_file (data, data_len, &ktype,
+			                                  &cipher, &iv, NULL);
+			if (tmp) {
 				format = NM_CRYPTO_FILE_FORMAT_RAW_KEY;
+				is_encrypted = (cipher && iv);
+				g_free (cipher);
+				g_free (iv);
+			}
 		}
 
 		if (tmp) {
-			/* Don't leave decrypted key data around */
+			/* Don't leave key data around */
 			memset (tmp->data, 0, tmp->len);
 			g_byte_array_free (tmp, TRUE);
 		}
 	}
 
+	if (out_is_encrypted)
+		*out_is_encrypted = is_encrypted;
 	return format;
 }
 
 NMCryptoFileFormat
 crypto_verify_private_key (const char *filename,
                            const char *password,
+                           gboolean *out_is_encrypted,
                            GError **error)
 {
 	GByteArray *contents;
@@ -736,7 +737,7 @@ crypto_verify_private_key (const char *filename,
 
 	contents = file_to_g_byte_array (filename, error);
 	if (contents) {
-		format = crypto_verify_private_key_data (contents->data, contents->len, password, error);
+		format = crypto_verify_private_key_data (contents->data, contents->len, password, out_is_encrypted, error);
 		g_byte_array_free (contents, TRUE);
 	}
 	return format;
diff --git a/libnm-core/crypto.h b/libnm-core/crypto.h
index 9173ba2203..b7b7dc4ae7 100644
--- a/libnm-core/crypto.h
+++ b/libnm-core/crypto.h
@@ -48,16 +48,16 @@ gboolean crypto_init (GError **error);
 
 void crypto_deinit (void);
 
-GByteArray *crypto_decrypt_private_key_data (const guint8 *data,
-                                             gsize data_len,
-                                             const char *password,
-                                             NMCryptoKeyType *out_key_type,
-                                             GError **error);
-
-GByteArray *crypto_decrypt_private_key (const char *file,
-                                        const char *password,
-                                        NMCryptoKeyType *out_key_type,
-                                        GError **error);
+GByteArray *crypto_decrypt_openssl_private_key_data (const guint8 *data,
+                                                     gsize data_len,
+                                                     const char *password,
+                                                     NMCryptoKeyType *out_key_type,
+                                                     GError **error);
+
+GByteArray *crypto_decrypt_openssl_private_key (const char *file,
+                                                const char *password,
+                                                NMCryptoKeyType *out_key_type,
+                                                GError **error);
 
 GByteArray *crypto_load_and_verify_certificate (const char *file,
                                                 NMCryptoFileFormat *out_file_format,
@@ -70,10 +70,12 @@ gboolean crypto_is_pkcs12_data (const guint8 *data, gsize len);
 NMCryptoFileFormat crypto_verify_private_key_data (const guint8 *data,
                                                    gsize data_len,
                                                    const char *password,
+                                                   gboolean *out_is_encrypted,
                                                    GError **error);
 
 NMCryptoFileFormat crypto_verify_private_key (const char *file,
                                               const char *password,
+                                              gboolean *out_is_encrypted,
                                               GError **error);
 
 /* Internal utils API bits for crypto providers */
diff --git a/libnm-core/nm-setting-8021x.c b/libnm-core/nm-setting-8021x.c
index a1019060d1..4bf03a153a 100644
--- a/libnm-core/nm-setting-8021x.c
+++ b/libnm-core/nm-setting-8021x.c
@@ -1750,7 +1750,7 @@ nm_setting_802_1x_set_private_key (NMSetting8021x *setting,
 	 * given, that it decrypts the private key.
 	 */
 	if (key_path) {
-		format = crypto_verify_private_key (key_path, password, &local_err);
+		format = crypto_verify_private_key (key_path, password, NULL, &local_err);
 		if (format == NM_CRYPTO_FILE_FORMAT_UNKNOWN) {
 			g_set_error_literal (error,
 			                     NM_CONNECTION_ERROR,
@@ -2060,7 +2060,7 @@ nm_setting_802_1x_set_phase2_private_key (NMSetting8021x *setting,
 	 * given, that it decrypts the private key.
 	 */
 	if (key_path) {
-		format = crypto_verify_private_key (key_path, password, &local_err);
+		format = crypto_verify_private_key (key_path, password, NULL, &local_err);
 		if (format == NM_CRYPTO_FILE_FORMAT_UNKNOWN) {
 			g_set_error_literal (error,
 			                     NM_CONNECTION_ERROR,
@@ -2204,11 +2204,11 @@ need_private_key_password (GBytes *blob,
 	/* Private key password is required */
 	if (password) {
 		if (path)
-			format = crypto_verify_private_key (path, password, NULL);
+			format = crypto_verify_private_key (path, password, NULL, NULL);
 		else if (blob)
 			format = crypto_verify_private_key_data (g_bytes_get_data (blob, NULL),
 			                                         g_bytes_get_size (blob),
-			                                         password, NULL);
+			                                         password, NULL, NULL);
 		else
 			g_warning ("%s: unknown private key password scheme", __func__);
 	}
diff --git a/libnm-core/tests/Makefile.am b/libnm-core/tests/Makefile.am
index dc8c40080b..a21c131cb5 100644
--- a/libnm-core/tests/Makefile.am
+++ b/libnm-core/tests/Makefile.am
@@ -48,6 +48,7 @@ EXTRA_DIST =                              \
 	certs/ca-no-ending-newline.pem    \
 	certs/test-key-only.pem           \
 	certs/test-key-only-decrypted.der \
+	certs/test-key-only-decrypted.pem \
 	certs/pkcs8-enc-key.pem           \
 	certs/pkcs8-noenc-key.pem         \
 	certs/pkcs8-decrypted.der         \
diff --git a/libnm-core/tests/test-crypto.c b/libnm-core/tests/test-crypto.c
index 2ce7a35a0a..cbca52c801 100644
--- a/libnm-core/tests/test-crypto.c
+++ b/libnm-core/tests/test-crypto.c
@@ -135,7 +135,7 @@ test_load_private_key (const char *path,
 	GByteArray *array, *decrypted;
 	GError *error = NULL;
 
-	array = crypto_decrypt_private_key (path, password, &key_type, &error);
+	array = crypto_decrypt_openssl_private_key (path, password, &key_type, &error);
 	/* Even if the password is wrong, we should determine the key type */
 	g_assert_cmpint (key_type, ==, NM_CRYPTO_KEY_TYPE_RSA);
 
@@ -173,9 +173,10 @@ test_load_pkcs12 (const char *path,
                   int expected_error)
 {
 	NMCryptoFileFormat format = NM_CRYPTO_FILE_FORMAT_UNKNOWN;
+	gboolean is_encrypted = FALSE;
 	GError *error = NULL;
 
-	format = crypto_verify_private_key (path, password, &error);
+	format = crypto_verify_private_key (path, password, &is_encrypted, &error);
 	if (expected_error != -1) {
 		g_assert_error (error, NM_CRYPTO_ERROR, expected_error);
 		g_assert_cmpint (format, ==, NM_CRYPTO_FILE_FORMAT_UNKNOWN);
@@ -183,6 +184,7 @@ test_load_pkcs12 (const char *path,
 	} else {
 		g_assert_no_error (error);
 		g_assert_cmpint (format, ==, NM_CRYPTO_FILE_FORMAT_PKCS12);
+		g_assert (is_encrypted);
 	}
 }
 
@@ -190,12 +192,14 @@ static void
 test_load_pkcs12_no_password (const char *path)
 {
 	NMCryptoFileFormat format = NM_CRYPTO_FILE_FORMAT_UNKNOWN;
+	gboolean is_encrypted = FALSE;
 	GError *error = NULL;
 
 	/* We should still get a valid returned crypto file format */
-	format = crypto_verify_private_key (path, NULL, &error);
+	format = crypto_verify_private_key (path, NULL, &is_encrypted, &error);
 	g_assert_no_error (error);
 	g_assert_cmpint (format, ==, NM_CRYPTO_FILE_FORMAT_PKCS12);
+	g_assert (is_encrypted);
 }
 
 static void
@@ -222,9 +226,10 @@ test_load_pkcs8 (const char *path,
                  int expected_error)
 {
 	NMCryptoFileFormat format = NM_CRYPTO_FILE_FORMAT_UNKNOWN;
+	gboolean is_encrypted = FALSE;
 	GError *error = NULL;
 
-	format = crypto_verify_private_key (path, password, &error);
+	format = crypto_verify_private_key (path, password, &is_encrypted, &error);
 	if (expected_error != -1) {
 		g_assert_error (error, NM_CRYPTO_ERROR, expected_error);
 		g_assert_cmpint (format, ==, NM_CRYPTO_FILE_FORMAT_UNKNOWN);
@@ -232,6 +237,7 @@ test_load_pkcs8 (const char *path,
 	} else {
 		g_assert_no_error (error);
 		g_assert_cmpint (format, ==, NM_CRYPTO_FILE_FORMAT_RAW_KEY);
+		g_assert (is_encrypted);
 	}
 }
 
@@ -265,7 +271,7 @@ test_encrypt_private_key (const char *path,
 	GByteArray *array, *encrypted, *re_decrypted;
 	GError *error = NULL;
 
-	array = crypto_decrypt_private_key (path, password, &key_type, &error);
+	array = crypto_decrypt_openssl_private_key (path, password, &key_type, &error);
 	g_assert_no_error (error);
 	g_assert (array != NULL);
 	g_assert_cmpint (key_type, ==, NM_CRYPTO_KEY_TYPE_RSA);
@@ -280,8 +286,8 @@ test_encrypt_private_key (const char *path,
 
 	/* Then re-decrypt the private key */
 	key_type = NM_CRYPTO_KEY_TYPE_UNKNOWN;
-	re_decrypted = crypto_decrypt_private_key_data (encrypted->data, encrypted->len,
-	                                                password, &key_type, &error);
+	re_decrypted = crypto_decrypt_openssl_private_key_data (encrypted->data, encrypted->len,
+	                                                        password, &key_type, &error);
 	g_assert_no_error (error);
 	g_assert (re_decrypted != NULL);
 	g_assert_cmpint (key_type, ==, NM_CRYPTO_KEY_TYPE_RSA);