| Message ID | 20220202065906.2598366-1-vt@altlinux.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] KEYS: Double max_size to make keyctl pkey_verify work | expand |
On 2/2/22 01:59, Vitaly Chikunov wrote: > Rarely used `keyctl pkey_verify' can verify raw signatures, but was > failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which > does not pass in/out sizes check in keyctl_pkey_params_get_2. > This in turn because these values cannot be distinguished by a single > `max_size' callback return value. > Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these > algorithms. > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> How do you use pkey_query? $ keyctl padd asymmetric testkey %keyring:test < cert.der 385037223 $ keyctl pkey_query 385037223 '' Password passing is not yet supported $ keyctl pkey_query 385037223 Format: keyctl --version keyctl add <type> <desc> <data> <keyring> [...] $ keyctl unlink 385037223 1 links removed
Stefan, On Wed, Feb 02, 2022 at 07:55:43AM -0500, Stefan Berger wrote: > On 2/2/22 01:59, Vitaly Chikunov wrote: > > Rarely used `keyctl pkey_verify' can verify raw signatures, but was > > failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which > > does not pass in/out sizes check in keyctl_pkey_params_get_2. > > This in turn because these values cannot be distinguished by a single > > `max_size' callback return value. > > Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these > > algorithms. > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > > How do you use pkey_query? > > $ keyctl padd asymmetric testkey %keyring:test < cert.der > 385037223 It should be (for RSA key): keyctl pkey_query 385037223 0 enc=pkcs1 hash=sha256 `0` is placeholder for a password. For example, I generated keys with your eckey-testing/generate.sh, and pkey_query after this patch is applied: # keyctl padd asymmetric "" @u < ecdsa-ca/ca.crt.der 66509339 # keyctl pkey_query 66509339 0 enc=x962 hash=sha256 key_size=256 max_data_size=64 max_sig_size=64 max_enc_size=32 max_dec_size=32 encrypt=y decrypt=n sign=n verify=y W/o patch max_data_size= and max_sig_size= will be 32. Thanks, > $ keyctl pkey_query 385037223 '' > Password passing is not yet supported > $ keyctl pkey_query 385037223 > Format: > keyctl --version > keyctl add <type> <desc> <data> <keyring> > [...] > > $ keyctl unlink 385037223 > 1 links removed >
On Thu, Feb 03, 2022 at 12:24:37AM +0300, Vitaly Chikunov wrote: > Stefan, > > On Wed, Feb 02, 2022 at 07:55:43AM -0500, Stefan Berger wrote: > > On 2/2/22 01:59, Vitaly Chikunov wrote: > > > Rarely used `keyctl pkey_verify' can verify raw signatures, but was > > > failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which > > > does not pass in/out sizes check in keyctl_pkey_params_get_2. > > > This in turn because these values cannot be distinguished by a single > > > `max_size' callback return value. > > > Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these > > > algorithms. > > > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > > > > How do you use pkey_query? > > > > $ keyctl padd asymmetric testkey %keyring:test < cert.der > > 385037223 > > It should be (for RSA key): > > keyctl pkey_query 385037223 0 enc=pkcs1 hash=sha256 > > `0` is placeholder for a password. > > For example, I generated keys with your eckey-testing/generate.sh, and > pkey_query after this patch is applied: > > # keyctl padd asymmetric "" @u < ecdsa-ca/ca.crt.der > 66509339 > # keyctl pkey_query 66509339 0 enc=x962 hash=sha256 > key_size=256 > max_data_size=64 > max_sig_size=64 > max_enc_size=32 > max_dec_size=32 I just thought, we can also set these to 0 if encrypt/decrypt is not enabled. Currently, there is no way to detect that encrypt is not possible, except by extending that if (strcmp...), but if we going to have it, why not correct other info too? Thanks, > encrypt=y > decrypt=n > sign=n > verify=y > > W/o patch max_data_size= and max_sig_size= will be 32. > > Thanks, > > > $ keyctl pkey_query 385037223 '' > > Password passing is not yet supported > > $ keyctl pkey_query 385037223 > > Format: > > keyctl --version > > keyctl add <type> <desc> <data> <keyring> > > [...] > > > > $ keyctl unlink 385037223 > > 1 links removed > >
JFYI, On Thu, Feb 03, 2022 at 12:24:37AM +0300, Vitaly Chikunov wrote: > On Wed, Feb 02, 2022 at 07:55:43AM -0500, Stefan Berger wrote: > > On 2/2/22 01:59, Vitaly Chikunov wrote: > > > Rarely used `keyctl pkey_verify' can verify raw signatures, but was > > > failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which > > > does not pass in/out sizes check in keyctl_pkey_params_get_2. > > > This in turn because these values cannot be distinguished by a single > > > `max_size' callback return value. > > > Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these > > > algorithms. > > > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > > > > How do you use pkey_query? > > > > $ keyctl padd asymmetric testkey %keyring:test < cert.der > > 385037223 > > It should be (for RSA key): > > keyctl pkey_query 385037223 0 enc=pkcs1 hash=sha256 > > `0` is placeholder for a password. > > For example, I generated keys with your eckey-testing/generate.sh, and > pkey_query after this patch is applied: > > # keyctl padd asymmetric "" @u < ecdsa-ca/ca.crt.der > 66509339 > # keyctl pkey_query 66509339 0 enc=x962 hash=sha256 > key_size=256 > max_data_size=64 > max_sig_size=64 > max_enc_size=32 > max_dec_size=32 > encrypt=y > decrypt=n > sign=n > verify=y > > W/o patch max_data_size= and max_sig_size= will be 32. In case someone wants to reproduce `keyctl pkey_verify`, there are steps: 1. Keys are generated using ima-evm-utils tests suite, for example test-gost2012_512-A.cer: $ base64 test-gost2012_512-A.cer MIIB/DCCAWagAwIBAgIUK8+whWevr3FFkSdU9GLDAM7ure8wDAYIKoUDBwEBAwMFADARMQ8wDQYD VQQDDAZDQSBLZXkwIBcNMjIwMjAxMjIwOTQxWhgPMjA4MjEyMDUyMjA5NDFaMBExDzANBgNVBAMM BkNBIEtleTCBoDAXBggqhQMHAQEBAjALBgkqhQMHAQIBAgEDgYQABIGALXNrTJGgeErBUOov3Cfo IrHF9fcj8UjzwGeKCkbCcINzVUbdPmCopeJRHDJEvQBX1CQUPtlwDv6ANjTTRoq5nCk9L5PPFP1H z73JIXHT0eRBDVoWy0cWDRz1mmQlCnN2HThMtEloaQI81nTlKZOcEYDtDpi5WODmjEeRNQJMdqCj UDBOMAwGA1UdEwQFMAMBAf8wHQYDVR0OBBYEFCwfOITMbE9VisW1i2TYeu1tAo5QMB8GA1UdIwQY MBaAFCwfOITMbE9VisW1i2TYeu1tAo5QMAwGCCqFAwcBAQMDBQADgYEAmBfJCMTdC0/NSjz4BBiQ qDIEjomO7FEHYlkX5NGulcF8FaJW2jeyyXXtbpnub1IQ8af1KFIpwoS2e93LaaofxpWlpQLlju6m KYLOcO4xK3Whwa2hBAz9YbpUSFjvxnkS2/jpH2MsOSXuUEeCruG/RkHHB3ACef9umG6HCNQuAPY= 2. Hash and raw signature generated using openssl dgst: $ head -123c /dev/zero > foo.data $ openssl dgst -md_gost12_512 -out foo.hash512 -binary foo.data $ openssl dgst -md_gost12_512 -sign test-gost2012_512-A.key -out foo.sign512 -binary foo.data This may require configuring openssl to support engine, so there are resulting files: $ base64 foo.data AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA AAAAAAAAAAAA $ base64 foo.hash512 CZEpA3sP+swJIsahmsA2GX/GDp+4Ibt8c/Oli01NbTXmSyjbC0yivmouDW+LfqckewMKiKWCdNIE aY4cxfy4sQ== $ base64 foo.sign512 WNz9w7qmCvL/UG4uuCnj57C9udLRz1JXQLTOflpVa3fHPrp0qLBhoiLAdMtDr0AHPAsIlGS0vb9o vJIxtlGsimeqlAfffIpfmvu1oD/tqOT5NRa7xANT7tW2V9jiMRWt887dDSX+QBARcmXNwe07reoX Ko8xWMZ8xvOqWEuVPPw= 3. Then (with this patch applied, I run in virtme): # k=`keyctl padd asymmetric "" @u < test-gost2012_512-A.cer` # keyctl pkey_verify $k 0 foo.hash512 foo.sign512 enc=raw hash=streebog512 This gives exit 0. Test failure (due to wrong hash): # keyctl pkey_verify $k 0 foo.sign512 foo.sign512 enc=raw hash=streebog512 keyctl_pkey_verify: Bad message Thanks,
On 2/2/22 01:59, Vitaly Chikunov wrote: > Rarely used `keyctl pkey_verify' can verify raw signatures, but was > failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which > does not pass in/out sizes check in keyctl_pkey_params_get_2. > This in turn because these values cannot be distinguished by a single > `max_size' callback return value. > Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these > algorithms. > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > --- > crypto/asymmetric_keys/public_key.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c > index 4fefb219bfdc..3ffbab07ed2a 100644 > --- a/crypto/asymmetric_keys/public_key.c > +++ b/crypto/asymmetric_keys/public_key.c > @@ -143,8 +143,19 @@ static int software_key_query(const struct kernel_pkey_params *params, > > len = crypto_akcipher_maxsize(tfm); > info->key_size = len * 8; > - info->max_data_size = len; > - info->max_sig_size = len; > + if (strcmp(alg_name, "ecrdsa") == 0 || > + strncmp(alg_name, "ecdsa-", 6) == 0) { > + /* > + * For these algos sig size is twice key size. > + * keyctl uses max_sig_size as minimum input size, and > + * max_data_size as minimum output size for a signature. > + */ > + info->max_data_size = len * 2; > + info->max_sig_size = len * 2; I don't know about the data size but following my tests this is not enough for ECDSA signature size. In ECDSA case the r and s components of the signature are encode in asn.1, not 'raw'. So there are 2 bytes at the beginning for sequence identifier , 2 bytes asn.1 for the r component, 1 additional 0-byte to make the r component always a positive number, then the r component, then 2 bytes asn.1 for the s component, 1 addition 0-byte to make the s component a positive number, then the s component. Phew. info->max_sig_size = 2 + (2 + 1 + len) * 2; so for NIST P384 it's: 2 + (2+1+48) * 2 = 104 Then it works for me as well. > + } else { > + info->max_data_size = len; > + info->max_sig_size = len; > + } > info->max_enc_size = len; > info->max_dec_size = len; > info->supported_ops = (KEYCTL_SUPPORTS_ENCRYPT |
On Wed, Feb 02, 2022 at 10:15:24PM -0500, Stefan Berger wrote: > > On 2/2/22 01:59, Vitaly Chikunov wrote: > > Rarely used `keyctl pkey_verify' can verify raw signatures, but was > > failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which > > does not pass in/out sizes check in keyctl_pkey_params_get_2. > > This in turn because these values cannot be distinguished by a single > > `max_size' callback return value. > > Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these > > algorithms. > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > > --- > > crypto/asymmetric_keys/public_key.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c > > index 4fefb219bfdc..3ffbab07ed2a 100644 > > --- a/crypto/asymmetric_keys/public_key.c > > +++ b/crypto/asymmetric_keys/public_key.c > > @@ -143,8 +143,19 @@ static int software_key_query(const struct kernel_pkey_params *params, > > len = crypto_akcipher_maxsize(tfm); > > info->key_size = len * 8; > > - info->max_data_size = len; > > - info->max_sig_size = len; > > + if (strcmp(alg_name, "ecrdsa") == 0 || > > + strncmp(alg_name, "ecdsa-", 6) == 0) { > > + /* > > + * For these algos sig size is twice key size. > > + * keyctl uses max_sig_size as minimum input size, and > > + * max_data_size as minimum output size for a signature. > > + */ > > + info->max_data_size = len * 2; > > + info->max_sig_size = len * 2; > I don't know about the data size but following my tests this is not enough > for ECDSA signature size. In ECDSA case the r and s components of the > signature are encode in asn.1, not 'raw'. So there are 2 bytes at the > beginning for sequence identifier , 2 bytes asn.1 for the r component, 1 > additional 0-byte to make the r component always a positive number, then the > r component, then 2 bytes asn.1 for the s component, 1 addition 0-byte to > make the s component a positive number, then the s component. Phew. > > info->max_sig_size = 2 + (2 + 1 + len) * 2; > > so for NIST P384 it's: 2 + (2+1+48) * 2 = 104 > > Then it works for me as well. Well, another solution, without changing API, is that max_size() should return bigger size (to fit encoded signature), but in that case keyctl will think wrongly about key_size. Just for reference, keyctl_pkey_params_get_2 check that needs to be passed: case KEYCTL_PKEY_VERIFY: if (uparams.in_len > info.max_sig_size || uparams.out_len > info.max_data_size) return -EINVAL; So we can return arbitrarily big value, in theory. Thanks, > > > > + } else { > > + info->max_data_size = len; > > + info->max_sig_size = len; > > + } > > info->max_enc_size = len; > > info->max_dec_size = len; > > info->supported_ops = (KEYCTL_SUPPORTS_ENCRYPT |
On 2/2/22 17:38, Vitaly Chikunov wrote: > On Thu, Feb 03, 2022 at 12:24:37AM +0300, Vitaly Chikunov wrote: >> Stefan, >> >> On Wed, Feb 02, 2022 at 07:55:43AM -0500, Stefan Berger wrote: >>> On 2/2/22 01:59, Vitaly Chikunov wrote: >>>> Rarely used `keyctl pkey_verify' can verify raw signatures, but was >>>> failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which >>>> does not pass in/out sizes check in keyctl_pkey_params_get_2. >>>> This in turn because these values cannot be distinguished by a single >>>> `max_size' callback return value. >>>> Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these >>>> algorithms. >>>> >>>> Signed-off-by: Vitaly Chikunov <vt@altlinux.org> >>> How do you use pkey_query? >>> >>> $ keyctl padd asymmetric testkey %keyring:test < cert.der >>> 385037223 >> It should be (for RSA key): >> >> keyctl pkey_query 385037223 0 enc=pkcs1 hash=sha256 >> >> `0` is placeholder for a password. >> >> For example, I generated keys with your eckey-testing/generate.sh, and >> pkey_query after this patch is applied: >> >> # keyctl padd asymmetric "" @u < ecdsa-ca/ca.crt.der >> 66509339 >> # keyctl pkey_query 66509339 0 enc=x962 hash=sha256 >> key_size=256 >> max_data_size=64 >> max_sig_size=64 >> max_enc_size=32 >> max_dec_size=32 > I just thought, we can also set these to 0 if encrypt/decrypt is not > enabled. Currently, there is no way to detect that encrypt is not > possible, except by extending that if (strcmp...), but if we going to > have it, why not correct other info too? Sounds good. This here works for signature verification for ECDSA now. [max_data_size = len * 2 was not enough] diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c index 4fefb219bfdc..9e47a825b418 100644 --- a/crypto/asymmetric_keys/public_key.c +++ b/crypto/asymmetric_keys/public_key.c @@ -143,8 +143,26 @@ static int software_key_query(const struct kernel_pkey_params *params, len = crypto_akcipher_maxsize(tfm); info->key_size = len * 8; - info->max_data_size = len; - info->max_sig_size = len; + if (strcmp(alg_name, "ecrdsa") == 0) { + /* + * For these algos sig size is twice key size. + * keyctl uses max_sig_size as minimum input size, and + * max_data_size as minimum output size for a signature. + */ + info->max_data_size = len * 2; + info->max_sig_size = len * 2; + } else if (strncmp(alg_name, "ecdsa-", 6) == 0) { + /* + * ECDSA encodes the signature in ASN.1 sequence (2 bytes) + * with 2 bytes identifying each integer and 1 byte prefixed + * to each integer to make it a positive number. + */ + info->max_sig_size = 2 + (2 + 1 + len) * 2; + info->max_data_size = 2 + (2 + 1 + len) * 2; + } else { + info->max_data_size = len; + info->max_sig_size = len; + } info->max_enc_size = len; info->max_dec_size = len; info->supported_ops = (KEYCTL_SUPPORTS_ENCRYPT |
On Wed, Feb 02, 2022 at 09:59:06AM +0300, Vitaly Chikunov wrote: > Rarely used `keyctl pkey_verify' can verify raw signatures, but was > failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which > does not pass in/out sizes check in keyctl_pkey_params_get_2. > This in turn because these values cannot be distinguished by a single > `max_size' callback return value. > Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these > algorithms. > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> Please provide a fixes tag and describe your changes. BR, Jarkko
On Wed, Feb 02, 2022 at 10:15:24PM -0500, Stefan Berger wrote: > > On 2/2/22 01:59, Vitaly Chikunov wrote: > > Rarely used `keyctl pkey_verify' can verify raw signatures, but was > > failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which > > does not pass in/out sizes check in keyctl_pkey_params_get_2. > > This in turn because these values cannot be distinguished by a single > > `max_size' callback return value. > > Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these > > algorithms. > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > > --- > > crypto/asymmetric_keys/public_key.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c > > index 4fefb219bfdc..3ffbab07ed2a 100644 > > --- a/crypto/asymmetric_keys/public_key.c > > +++ b/crypto/asymmetric_keys/public_key.c > > @@ -143,8 +143,19 @@ static int software_key_query(const struct kernel_pkey_params *params, > > len = crypto_akcipher_maxsize(tfm); > > info->key_size = len * 8; > > - info->max_data_size = len; > > - info->max_sig_size = len; > > + if (strcmp(alg_name, "ecrdsa") == 0 || > > + strncmp(alg_name, "ecdsa-", 6) == 0) { > > + /* > > + * For these algos sig size is twice key size. > > + * keyctl uses max_sig_size as minimum input size, and > > + * max_data_size as minimum output size for a signature. > > + */ > > + info->max_data_size = len * 2; > > + info->max_sig_size = len * 2; > I don't know about the data size but following my tests this is not enough > for ECDSA signature size. In ECDSA case the r and s components of the > signature are encode in asn.1, not 'raw'. So there are 2 bytes at the > beginning for sequence identifier , 2 bytes asn.1 for the r component, 1 > additional 0-byte to make the r component always a positive number, then the > r component, then 2 bytes asn.1 for the s component, 1 addition 0-byte to > make the s component a positive number, then the s component. Phew. > > info->max_sig_size = 2 + (2 + 1 + len) * 2; > > so for NIST P384 it's: 2 + (2+1+48) * 2 = 104 > > Then it works for me as well. Thank you for the trouble of providing this great explanation. This reasoning should be included to the commit message for future reference. It would be also nice to encapsulate this calculation to an inline function. /Jarkko
On Wed, Feb 02, 2022 at 07:55:43AM -0500, Stefan Berger wrote: > > On 2/2/22 01:59, Vitaly Chikunov wrote: > > Rarely used `keyctl pkey_verify' can verify raw signatures, but was > > failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which > > does not pass in/out sizes check in keyctl_pkey_params_get_2. > > This in turn because these values cannot be distinguished by a single > > `max_size' callback return value. > > Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these > > algorithms. > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > > How do you use pkey_query? > > $ keyctl padd asymmetric testkey %keyring:test < cert.der > 385037223 > $ keyctl pkey_query 385037223 '' > Password passing is not yet supported > $ keyctl pkey_query 385037223 > Format: > keyctl --version > keyctl add <type> <desc> <data> <keyring> > [...] > > $ keyctl unlink 385037223 > 1 links removed A keyctl transcript of the failing case would be really educating addition to the commit message (low-barrier to test this patch). BR, Jarkko
On Mon, Feb 21, 2022 at 12:29:13AM +0100, Jarkko Sakkinen wrote: > On Wed, Feb 02, 2022 at 10:15:24PM -0500, Stefan Berger wrote: > > > > On 2/2/22 01:59, Vitaly Chikunov wrote: > > > Rarely used `keyctl pkey_verify' can verify raw signatures, but was > > > failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which > > > does not pass in/out sizes check in keyctl_pkey_params_get_2. > > > This in turn because these values cannot be distinguished by a single > > > `max_size' callback return value. > > > Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these > > > algorithms. > > > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > > > --- > > > crypto/asymmetric_keys/public_key.c | 15 +++++++++++++-- > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > > > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c > > > index 4fefb219bfdc..3ffbab07ed2a 100644 > > > --- a/crypto/asymmetric_keys/public_key.c > > > +++ b/crypto/asymmetric_keys/public_key.c > > > @@ -143,8 +143,19 @@ static int software_key_query(const struct kernel_pkey_params *params, > > > len = crypto_akcipher_maxsize(tfm); > > > info->key_size = len * 8; > > > - info->max_data_size = len; > > > - info->max_sig_size = len; > > > + if (strcmp(alg_name, "ecrdsa") == 0 || > > > + strncmp(alg_name, "ecdsa-", 6) == 0) { > > > + /* > > > + * For these algos sig size is twice key size. > > > + * keyctl uses max_sig_size as minimum input size, and > > > + * max_data_size as minimum output size for a signature. > > > + */ > > > + info->max_data_size = len * 2; > > > + info->max_sig_size = len * 2; > > I don't know about the data size but following my tests this is not enough > > for ECDSA signature size. In ECDSA case the r and s components of the > > signature are encode in asn.1, not 'raw'. So there are 2 bytes at the > > beginning for sequence identifier , 2 bytes asn.1 for the r component, 1 > > additional 0-byte to make the r component always a positive number, then the > > r component, then 2 bytes asn.1 for the s component, 1 addition 0-byte to > > make the s component a positive number, then the s component. Phew. > > > > info->max_sig_size = 2 + (2 + 1 + len) * 2; > > > > so for NIST P384 it's: 2 + (2+1+48) * 2 = 104 > > > > Then it works for me as well. > > Thank you for the trouble of providing this great explanation. This > reasoning should be included to the commit message for future reference. > > It would be also nice to encapsulate this calculation to an inline > function. I wanted to discuss if there's a better way to do it. For example, instead of calculating algorithm specific information in software_key_query maybe we should extend akcipher_alg API with a pkey_params request (just for keyctl)? Also, there possible other solution - instead of setting info in software_key_query depending on algo (as in this patch), it's possible (in a hackish way) just to return larger value from akcipher_alg::max_size. But this will possible somewhat confuse keyctl users, as, for example, they will see arbitrary value for a key_size. Currently, this patch is the simplest non-confusing solution, and it's in accord with how we calculate algorithm specific things all around the code base (outside of algorithm implementation itself). Thanks, > > /Jarkko
On 2/20/22 21:43, Vitaly Chikunov wrote: > On Mon, Feb 21, 2022 at 12:29:13AM +0100, Jarkko Sakkinen wrote: >> On Wed, Feb 02, 2022 at 10:15:24PM -0500, Stefan Berger wrote: >>> On 2/2/22 01:59, Vitaly Chikunov wrote: >>>> Rarely used `keyctl pkey_verify' can verify raw signatures, but was >>>> failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which >>>> does not pass in/out sizes check in keyctl_pkey_params_get_2. >>>> This in turn because these values cannot be distinguished by a single >>>> `max_size' callback return value. >>>> Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these >>>> algorithms. >>>> >>>> Signed-off-by: Vitaly Chikunov <vt@altlinux.org> >>>> --- >>>> crypto/asymmetric_keys/public_key.c | 15 +++++++++++++-- >>>> 1 file changed, 13 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c >>>> index 4fefb219bfdc..3ffbab07ed2a 100644 >>>> --- a/crypto/asymmetric_keys/public_key.c >>>> +++ b/crypto/asymmetric_keys/public_key.c >>>> @@ -143,8 +143,19 @@ static int software_key_query(const struct kernel_pkey_params *params, >>>> len = crypto_akcipher_maxsize(tfm); >>>> info->key_size = len * 8; >>>> - info->max_data_size = len; >>>> - info->max_sig_size = len; >>>> + if (strcmp(alg_name, "ecrdsa") == 0 || >>>> + strncmp(alg_name, "ecdsa-", 6) == 0) { >>>> + /* >>>> + * For these algos sig size is twice key size. >>>> + * keyctl uses max_sig_size as minimum input size, and >>>> + * max_data_size as minimum output size for a signature. >>>> + */ >>>> + info->max_data_size = len * 2; >>>> + info->max_sig_size = len * 2; >>> I don't know about the data size but following my tests this is not enough >>> for ECDSA signature size. In ECDSA case the r and s components of the >>> signature are encode in asn.1, not 'raw'. So there are 2 bytes at the >>> beginning for sequence identifier , 2 bytes asn.1 for the r component, 1 >>> additional 0-byte to make the r component always a positive number, then the >>> r component, then 2 bytes asn.1 for the s component, 1 addition 0-byte to >>> make the s component a positive number, then the s component. Phew. >>> >>> info->max_sig_size = 2 + (2 + 1 + len) * 2; >>> >>> so for NIST P384 it's: 2 + (2+1+48) * 2 = 104 >>> >>> Then it works for me as well. >> Thank you for the trouble of providing this great explanation. This >> reasoning should be included to the commit message for future reference. >> >> It would be also nice to encapsulate this calculation to an inline >> function. > I wanted to discuss if there's a better way to do it. For example, > instead of calculating algorithm specific information in > software_key_query maybe we should extend akcipher_alg API with a > pkey_params request (just for keyctl)? > > Also, there possible other solution - instead of setting info in > software_key_query depending on algo (as in this patch), it's possible > (in a hackish way) just to return larger value from > akcipher_alg::max_size. But this will possible somewhat confuse keyctl > users, as, for example, they will see arbitrary value for a key_size. > > Currently, this patch is the simplest non-confusing solution, and it's > in accord with how we calculate algorithm specific things all around > the code base (outside of algorithm implementation itself). I don't know the answer to the other questions, but I agree that your patch seem to be the simplest non-confusing solution. Are you going to repost it, possibly with my ECDSA modifications added? Stefan > > Thanks, > >> /Jarkko
diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c index 4fefb219bfdc..3ffbab07ed2a 100644 --- a/crypto/asymmetric_keys/public_key.c +++ b/crypto/asymmetric_keys/public_key.c @@ -143,8 +143,19 @@ static int software_key_query(const struct kernel_pkey_params *params, len = crypto_akcipher_maxsize(tfm); info->key_size = len * 8; - info->max_data_size = len; - info->max_sig_size = len; + if (strcmp(alg_name, "ecrdsa") == 0 || + strncmp(alg_name, "ecdsa-", 6) == 0) { + /* + * For these algos sig size is twice key size. + * keyctl uses max_sig_size as minimum input size, and + * max_data_size as minimum output size for a signature. + */ + info->max_data_size = len * 2; + info->max_sig_size = len * 2; + } else { + info->max_data_size = len; + info->max_sig_size = len; + } info->max_enc_size = len; info->max_dec_size = len; info->supported_ops = (KEYCTL_SUPPORTS_ENCRYPT |
Rarely used `keyctl pkey_verify' can verify raw signatures, but was failing, because ECDSA/EC-RDSA signature sizes are twice key sizes which does not pass in/out sizes check in keyctl_pkey_params_get_2. This in turn because these values cannot be distinguished by a single `max_size' callback return value. Also, `keyctl pkey_query` displays incorrect `max_sig_size' about these algorithms. Signed-off-by: Vitaly Chikunov <vt@altlinux.org> --- crypto/asymmetric_keys/public_key.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)