mbox series

[0/3] ecdsa KEYCTL_PKEY_QUERY fixes

Message ID cover.1735236227.git.lukas@wunner.de
Headers show
Series ecdsa KEYCTL_PKEY_QUERY fixes | expand

Message

Lukas Wunner Dec. 26, 2024, 6:08 p.m. UTC
For ecdsa, KEYCTL_PKEY_QUERY reports nonsensical values for
enc/dec size and (for P521 keys) also the key size.

Since these are mostly just cosmetic issues, I recommend
not applying for v6.13, but rather let the patches soak
in linux-next.

Thanks!

Lukas Wunner (3):
  crypto: sig - Prepare for algorithms with variable signature size
  crypto: ecdsa - Fix enc/dec size reported by KEYCTL_PKEY_QUERY
  crypto: ecdsa - Fix NIST P521 key size reported by KEYCTL_PKEY_QUERY

 crypto/asymmetric_keys/public_key.c | 22 +++++++++++-----------
 crypto/ecdsa-p1363.c                |  4 ++--
 crypto/ecdsa-x962.c                 |  4 ++--
 crypto/ecdsa.c                      |  2 +-
 crypto/ecrdsa.c                     |  2 +-
 crypto/rsassa-pkcs1.c               |  4 ++--
 crypto/sig.c                        |  9 +++++++--
 crypto/testmgr.c                    |  7 ++++---
 include/crypto/sig.h                |  7 ++++---
 9 files changed, 34 insertions(+), 27 deletions(-)

Comments

Stefan Berger Jan. 2, 2025, 5:45 p.m. UTC | #1
On 12/26/24 1:08 PM, Lukas Wunner wrote:
> When user space issues a KEYCTL_PKEY_QUERY system call for a NIST P521
> key, the key_size is incorrectly reported as 528 bits instead of 521.

Is there a way to query this with keyctl pkey_query?

 > > That's because the key size obtained through crypto_sig_keysize() is in
> bytes and software_key_query() multiplies by 8 to yield the size in bits.
> The underlying assumption is that the key size is always a multiple of 8.
> With the recent addition of NIST P521, that's no longer the case.
> 
> Fix by returning the key_size in bits from crypto_sig_keysize() and
> adjusting the calculations in software_key_query().
> 
> The ->key_size() callbacks of sig_alg algorithms now return the size in
> bits, whereas the ->digest_size() and ->max_size() callbacks return the
> size in bytes.  This matches with the units in struct keyctl_pkey_query.
> 
> Fixes: a7d45ba77d3d ("crypto: ecdsa - Register NIST P521 and extend test suite")
 > Signed-off-by: Lukas Wunner <lukas@wunner.de>> ---
>   crypto/asymmetric_keys/public_key.c | 8 ++++----
>   crypto/ecdsa-p1363.c                | 4 ++--
>   crypto/ecdsa-x962.c                 | 4 ++--
>   crypto/ecdsa.c                      | 2 +-
>   crypto/ecrdsa.c                     | 2 +-
>   crypto/rsassa-pkcs1.c               | 2 +-
>   crypto/sig.c                        | 9 +++++++--
>   include/crypto/sig.h                | 2 +-
>   8 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index dd44a966947f..6cafb2b8aa22 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -205,6 +205,7 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   			goto error_free_tfm;
>   
>   		len = crypto_sig_keysize(sig);
> +		info->key_size = len;
>   		info->max_sig_size = crypto_sig_maxsize(sig);
>   		info->max_data_size = crypto_sig_digestsize(sig);
>   
> @@ -213,8 +214,8 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   			info->supported_ops |= KEYCTL_SUPPORTS_SIGN;
>   
>   		if (strcmp(params->encoding, "pkcs1") == 0) {
> -			info->max_enc_size = len;
> -			info->max_dec_size = len;
> +			info->max_enc_size = len / 8;
> +			info->max_dec_size = len / 8;
>   
>   			info->supported_ops |= KEYCTL_SUPPORTS_ENCRYPT;
>   			if (pkey->key_is_private)
> @@ -235,6 +236,7 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   			goto error_free_tfm;
>   
>   		len = crypto_akcipher_maxsize(tfm);
> +		info->key_size = len * 8;
>   		info->max_sig_size = len;
>   		info->max_data_size = len;
>   		info->max_enc_size = len;
> @@ -245,8 +247,6 @@ static int software_key_query(const struct kernel_pkey_params *params,
>   			info->supported_ops |= KEYCTL_SUPPORTS_DECRYPT;
>   	}
>   
> -	info->key_size = len * 8;
> -
>   	ret = 0;
>   
>   error_free_tfm:
> diff --git a/crypto/ecdsa-p1363.c b/crypto/ecdsa-p1363.c
> index eaae7214d69b..c4f458df18ed 100644
> --- a/crypto/ecdsa-p1363.c
> +++ b/crypto/ecdsa-p1363.c
> @@ -21,7 +21,7 @@ static int ecdsa_p1363_verify(struct crypto_sig *tfm,
>   			      const void *digest, unsigned int dlen)
>   {
>   	struct ecdsa_p1363_ctx *ctx = crypto_sig_ctx(tfm);
> -	unsigned int keylen = crypto_sig_keysize(ctx->child);
> +	unsigned int keylen = DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);
>   	unsigned int ndigits = DIV_ROUND_UP(keylen, sizeof(u64));
>   	struct ecdsa_raw_sig sig;
>   
> @@ -45,7 +45,7 @@ static unsigned int ecdsa_p1363_max_size(struct crypto_sig *tfm)
>   {
>   	struct ecdsa_p1363_ctx *ctx = crypto_sig_ctx(tfm);
>   
> -	return 2 * crypto_sig_keysize(ctx->child);
> +	return 2 * DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);
>   }
>   
>   static unsigned int ecdsa_p1363_digest_size(struct crypto_sig *tfm)
> diff --git a/crypto/ecdsa-x962.c b/crypto/ecdsa-x962.c
> index 6a77c13e192b..0327e1441374 100644
> --- a/crypto/ecdsa-x962.c
> +++ b/crypto/ecdsa-x962.c
> @@ -82,7 +82,7 @@ static int ecdsa_x962_verify(struct crypto_sig *tfm,
>   	int err;
>   
>   	sig_ctx.ndigits = DIV_ROUND_UP(crypto_sig_keysize(ctx->child),
> -				       sizeof(u64));
> +				       sizeof(u64) * 8);
>   
>   	err = asn1_ber_decoder(&ecdsasignature_decoder, &sig_ctx, src, slen);
>   	if (err < 0)
> @@ -103,7 +103,7 @@ static unsigned int ecdsa_x962_max_size(struct crypto_sig *tfm)
>   {
>   	struct ecdsa_x962_ctx *ctx = crypto_sig_ctx(tfm);
>   	struct sig_alg *alg = crypto_sig_alg(ctx->child);
> -	int slen = crypto_sig_keysize(ctx->child);
> +	int slen = DIV_ROUND_UP(crypto_sig_keysize(ctx->child), 8);
>   
>   	/*
>   	 * Verify takes ECDSA-Sig-Value (described in RFC 5480) as input,
> diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c
> index 117526d15dde..a70b60a90a3c 100644
> --- a/crypto/ecdsa.c
> +++ b/crypto/ecdsa.c
> @@ -167,7 +167,7 @@ static unsigned int ecdsa_key_size(struct crypto_sig *tfm)
>   {
>   	struct ecc_ctx *ctx = crypto_sig_ctx(tfm);
>   
> -	return DIV_ROUND_UP(ctx->curve->nbits, 8);
> +	return ctx->curve->nbits;
>   }
>   
>   static unsigned int ecdsa_digest_size(struct crypto_sig *tfm)
> diff --git a/crypto/ecrdsa.c b/crypto/ecrdsa.c
> index b3dd8a3ddeb7..53c9fd9f807f 100644
> --- a/crypto/ecrdsa.c
> +++ b/crypto/ecrdsa.c
> @@ -249,7 +249,7 @@ static unsigned int ecrdsa_key_size(struct crypto_sig *tfm)
>   	 * Verify doesn't need any output, so it's just informational
>   	 * for keyctl to determine the key bit size.
>   	 */
> -	return ctx->pub_key.ndigits * sizeof(u64);
> +	return ctx->pub_key.ndigits * sizeof(u64) * 8;
>   }
>   
>   static unsigned int ecrdsa_max_size(struct crypto_sig *tfm)
> diff --git a/crypto/rsassa-pkcs1.c b/crypto/rsassa-pkcs1.c
> index d01ac75635e0..299b2512cc95 100644
> --- a/crypto/rsassa-pkcs1.c
> +++ b/crypto/rsassa-pkcs1.c
> @@ -301,7 +301,7 @@ static unsigned int rsassa_pkcs1_key_size(struct crypto_sig *tfm)
>   {
>   	struct rsassa_pkcs1_ctx *ctx = crypto_sig_ctx(tfm);
>   
> -	return ctx->key_size;
> +	return ctx->key_size * 8;
>   }
>   
>   static int rsassa_pkcs1_set_pub_key(struct crypto_sig *tfm,
> diff --git a/crypto/sig.c b/crypto/sig.c
> index dfc7cae90802..7399e67c6f12 100644
> --- a/crypto/sig.c
> +++ b/crypto/sig.c
> @@ -102,6 +102,11 @@ static int sig_default_set_key(struct crypto_sig *tfm,
>   	return -ENOSYS;
>   }
>   
> +static unsigned int sig_default_size(struct crypto_sig *tfm)
> +{
> +	return DIV_ROUND_UP(crypto_sig_keysize(tfm), 8);
> +}
> +
>   static int sig_prepare_alg(struct sig_alg *alg)
>   {
>   	struct crypto_alg *base = &alg->base;
> @@ -117,9 +122,9 @@ static int sig_prepare_alg(struct sig_alg *alg)
>   	if (!alg->key_size)
>   		return -EINVAL;
>   	if (!alg->max_size)
> -		alg->max_size = alg->key_size;
> +		alg->max_size = sig_default_size;
>   	if (!alg->digest_size)
> -		alg->digest_size = alg->key_size;
> +		alg->digest_size = sig_default_size;
>   
>   	base->cra_type = &crypto_sig_type;
>   	base->cra_flags &= ~CRYPTO_ALG_TYPE_MASK;
> diff --git a/include/crypto/sig.h b/include/crypto/sig.h
> index 11024708c069..fa6dafafab3f 100644
> --- a/include/crypto/sig.h
> +++ b/include/crypto/sig.h
> @@ -128,7 +128,7 @@ static inline void crypto_free_sig(struct crypto_sig *tfm)
>   /**
>    * crypto_sig_keysize() - Get key size
>    *
> - * Function returns the key size in bytes.
> + * Function returns the key size in bits.
>    * Function assumes that the key is already set in the transformation. If this
>    * function is called without a setkey or with a failed setkey, you may end up
>    * in a NULL dereference.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Stefan Berger Jan. 3, 2025, 6:06 p.m. UTC | #2
On 1/3/25 12:38 PM, Lukas Wunner wrote:
> On Thu, Jan 02, 2025 at 12:45:47PM -0500, Stefan Berger wrote:
>> On 12/26/24 1:08 PM, Lukas Wunner wrote:
>>> When user space issues a KEYCTL_PKEY_QUERY system call for a NIST P521
>>> key, the key_size is incorrectly reported as 528 bits instead of 521.
>>
>> Is there a way to query this with keyctl pkey_query?
> 
> Yes, these are the commands I've used for testing:
> 
>    id=`keyctl padd asymmetric "" %:_uid.0 < end_responder.cert.der`
>    keyctl pkey_query $id 0 enc=x962 hash=sha256
> 

I had tried with these here as root:

# keyctl show %keyring:.ima
Keyring
  461728044 ---lswrv      0     0  keyring: .ima
  579203092 ---lswrv      0     0   \_ asymmetric: Fedora kernel signing 
key: 50e9f2a484a5b9e7279e7bf7f3ad54b0572c2f1e
  774765589 --als--v      0     0   \_ asymmetric: my rsa signing key: 
69f518ae20dbb4a412f33b8950b2fd1e2b850fd1
   15381609 --als--v      0     0   \_ asymmetric: my ecc signing key: 
0ab4280f3df700f2cb6711b930748e1224eae40d
   72176491 --als--v      0     0   \_ asymmetric: Fedora 42 IMA 
Code-signing cert: a1a5c4c8d90554e0ce5c07c9e127f20362f02aa4
  612838334 --als--v      0     0   \_ asymmetric: Fedora 41 IMA 
Code-signing cert: 158befb98fc2ee070833d1a2a46669e7876d7435
   51623090 --als--v      0     0   \_ asymmetric: Fedora 40 IMA 
Code-signing cert: 2defa2e1d528db308d3e1ca28274aa40a3204a9e
   85986135 --als--v      0     0   \_ asymmetric: Fedora 39 IMA 
Code-signing cert: 155266a4a3ea7bdddc9e38ddb192c2d2388b603e
# keyctl pkey_query 612838334 0 enc=x962
keyctl_pkey_query: Permission denied
# keyctl pkey_query 612838334 0 enc=x962 hash=sha256
keyctl_pkey_query: Permission denied
# keyctl pkey_query 579203092 0 enc=x962 hash=sha256
keyctl_pkey_query: Permission denied
# keyctl pkey_query 774765589 0 enc=x962 hash=sha256
keyctl_pkey_query: Permission denied


> This is the certificate I've used:
> 
>    https://github.com/DMTF/libspdm/raw/refs/heads/main/unit_test/sample_key/ecp521/end_responder.cert.der

# keyctl show
Session Keyring
  377868180 --alswrv      0     0  keyring: _ses
1014059943 --alswrv      0 65534   \_ keyring: _uid.0
  138203159 --als--v      0     0       \_ asymmetric: DMTF libspdm 
ECP521 responder cert: e4bcd74895d3a7bd230ad2a46941c3be6d5c91cc

# keyctl pkey_query $id 0 enc=x962 hash=sha256
key_size=528
max_data_size=64
max_sig_size=139
max_enc_size=66
max_dec_size=66
encrypt=n
decrypt=n
sign=n
verify=y

more favorable permissions - obviously

Thanks!

   Stefan

 > > Before:
> 
>    key_size=528
>    max_data_size=64
>    max_sig_size=139
>    max_enc_size=66
>    max_dec_size=66
>    encrypt=n
>    decrypt=n
>    sign=n
>    verify=y
> 
> After:
> 
>    key_size=521
>    max_data_size=64
>    max_sig_size=139
>    max_enc_size=0
>    max_dec_size=0
>    encrypt=n
>    decrypt=n
>    sign=n
>    verify=y
> 
> Thanks,
> 
> Lukas
>