On Mon, Feb 07, 2022 at 09:24:48PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> It is insecure to allow arbitrary hash algorithms and signature
> encodings to be used with arbitrary signature algorithms. Notably,
> ECDSA, ECRDSA, and SM2 all sign/verify raw hash values and don't
> disambiguate between different hash algorithms like RSA PKCS#1 v1.5
> padding does. Therefore, they need to be restricted to certain sets of
> hash algorithms (ideally just one, but in practice small sets are used).
> Additionally, the encoding is an integral part of modern signature
> algorithms, and is not supposed to vary.
>
> Therefore, tighten the checks of hash_algo and encoding done by
> software_key_determine_akcipher().
>
> Also rearrange the parameters to software_key_determine_akcipher() to
> put the public_key first, as this is the most important parameter and it
> often determines everything else.
>
> Fixes: 299f561a6693 ("x509: Add support for parsing x509 certs with ECDSA keys")
> Fixes: 215525639631 ("X.509: support OSCCA SM2-with-SM3 certificate verification")
> Fixes: 0d7a78643f69 ("crypto: ecrdsa - add EC-RDSA (GOST 34.10) algorithm")
> Cc: stable@xxxxxxxxxxxxxxx
> Tested-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> Tested-by: Tianjia Zhang <tianjia.zhang@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
Reviewed-by: Vitaly Chikunov <vt@xxxxxxxxxxxx>
Small question below.
> ---
> crypto/asymmetric_keys/public_key.c | 111 +++++++++++++++++++---------
> 1 file changed, 76 insertions(+), 35 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index e36213945686..7c9e6be35c30 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -60,39 +60,83 @@ static void public_key_destroy(void *payload0, void *payload3)
> }
>
> /*
> - * Determine the crypto algorithm name.
> + * Given a public_key, and an encoding and hash_algo to be used for signing
> + * and/or verification with that key, determine the name of the corresponding
> + * akcipher algorithm. Also check that encoding and hash_algo are allowed.
> */
> -static
> -int software_key_determine_akcipher(const char *encoding,
> - const char *hash_algo,
> - const struct public_key *pkey,
> - char alg_name[CRYPTO_MAX_ALG_NAME])
> +static int
> +software_key_determine_akcipher(const struct public_key *pkey,
> + const char *encoding, const char *hash_algo,
> + char alg_name[CRYPTO_MAX_ALG_NAME])
> {
> int n;
>
> - if (strcmp(encoding, "pkcs1") == 0) {
> - /* The data wangled by the RSA algorithm is typically padded
> - * and encoded in some manner, such as EMSA-PKCS1-1_5 [RFC3447
> - * sec 8.2].
> + if (!encoding)
> + return -EINVAL;
> +
> + if (strcmp(pkey->pkey_algo, "rsa") == 0) {
> + /*
> + * RSA signatures usually use EMSA-PKCS1-1_5 [RFC3447 sec 8.2].
> + */
> + if (strcmp(encoding, "pkcs1") == 0) {
> + if (!hash_algo)
> + n = snprintf(alg_name, CRYPTO_MAX_ALG_NAME,
> + "pkcs1pad(%s)",
> + pkey->pkey_algo);
> + else
> + n = snprintf(alg_name, CRYPTO_MAX_ALG_NAME,
> + "pkcs1pad(%s,%s)",
> + pkey->pkey_algo, hash_algo);
> + return n >= CRYPTO_MAX_ALG_NAME ? -EINVAL : 0;
> + }
> + if (strcmp(encoding, "raw") != 0)
> + return -EINVAL;
Should raw RSA be allowed at all? I understand it was there before, but
I wonder what is the use case.
Thanks,
> + /*
> + * Raw RSA cannot differentiate between different hash
> + * algorithms.
> + */
> + if (hash_algo)
> + return -EINVAL;
> + } else if (strncmp(pkey->pkey_algo, "ecdsa", 5) == 0) {
> + if (strcmp(encoding, "x962") != 0)
> + return -EINVAL;
> + /*
> + * ECDSA signatures are taken over a raw hash, so they don't
> + * differentiate between different hash algorithms. That means
> + * that the verifier should hard-code a specific hash algorithm.
> + * Unfortunately, in practice ECDSA is used with multiple SHAs,
> + * so we have to allow all of them and not just one.
> */
> if (!hash_algo)
> - n = snprintf(alg_name, CRYPTO_MAX_ALG_NAME,
> - "pkcs1pad(%s)",
> - pkey->pkey_algo);
> - else
> - n = snprintf(alg_name, CRYPTO_MAX_ALG_NAME,
> - "pkcs1pad(%s,%s)",
> - pkey->pkey_algo, hash_algo);
> - return n >= CRYPTO_MAX_ALG_NAME ? -EINVAL : 0;
> - }
> -
> - if (strcmp(encoding, "raw") == 0 ||
> - strcmp(encoding, "x962") == 0) {
> - strcpy(alg_name, pkey->pkey_algo);
> - return 0;
> + return -EINVAL;
> + if (strcmp(hash_algo, "sha1") != 0 &&
> + strcmp(hash_algo, "sha224") != 0 &&
> + strcmp(hash_algo, "sha256") != 0 &&
> + strcmp(hash_algo, "sha384") != 0 &&
> + strcmp(hash_algo, "sha512") != 0)
> + return -EINVAL;
> + } else if (strcmp(pkey->pkey_algo, "sm2") == 0) {
> + if (strcmp(encoding, "raw") != 0)
> + return -EINVAL;
> + if (!hash_algo)
> + return -EINVAL;
> + if (strcmp(hash_algo, "sm3") != 0)
> + return -EINVAL;
> + } else if (strcmp(pkey->pkey_algo, "ecrdsa") == 0) {
> + if (strcmp(encoding, "raw") != 0)
> + return -EINVAL;
> + if (!hash_algo)
> + return -EINVAL;
> + if (strcmp(hash_algo, "streebog256") != 0 &&
> + strcmp(hash_algo, "streebog512") != 0)
> + return -EINVAL;
> + } else {
> + /* Unknown public key algorithm */
> + return -ENOPKG;
> }
> -
> - return -ENOPKG;
> + if (strscpy(alg_name, pkey->pkey_algo, CRYPTO_MAX_ALG_NAME) < 0)
> + return -EINVAL;
> + return 0;
> }
>
> static u8 *pkey_pack_u32(u8 *dst, u32 val)
> @@ -113,9 +157,8 @@ static int software_key_query(const struct kernel_pkey_params *params,
> u8 *key, *ptr;
> int ret, len;
>
> - ret = software_key_determine_akcipher(params->encoding,
> - params->hash_algo,
> - pkey, alg_name);
> + ret = software_key_determine_akcipher(pkey, params->encoding,
> + params->hash_algo, alg_name);
> if (ret < 0)
> return ret;
>
> @@ -179,9 +222,8 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
>
> pr_devel("==>%s()\n", __func__);
>
> - ret = software_key_determine_akcipher(params->encoding,
> - params->hash_algo,
> - pkey, alg_name);
> + ret = software_key_determine_akcipher(pkey, params->encoding,
> + params->hash_algo, alg_name);
> if (ret < 0)
> return ret;
>
> @@ -340,9 +382,8 @@ int public_key_verify_signature(const struct public_key *pkey,
> return -EKEYREJECTED;
> }
>
> - ret = software_key_determine_akcipher(sig->encoding,
> - sig->hash_algo,
> - pkey, alg_name);
> + ret = software_key_determine_akcipher(pkey, sig->encoding,
> + sig->hash_algo, alg_name);
> if (ret < 0)
> return ret;
>
> --
> 2.35.1
