| Message ID | 20240215231414.3857320-1-stefanb@linux.ibm.com |
|---|---|
| Headers | show |
| Series | Add support for NIST P521 to ecdsa and ecdh | expand |
On 2/16/24 13:48, Elliott, Robert (Servers) wrote: >> -----Original Message----- >> From: Stefan Berger <stefanb@linux.ibm.com> >> Sent: Thursday, February 15, 2024 5:14 PM >> Subject: [PATCH v2 06/14] crypto: ecc - Add NIST P521 curve parameters >> >> Add the parameters for the NIST P521 curve and define a new curve ID >> for it. Make the curve available in ecc_get_curve. >> > ... >> diff --git a/crypto/ecc_curve_defs.h b/crypto/ecc_curve_defs.h > ... >> +static struct ecc_curve nist_p521 = { >> + .name = "nist_521", > > Are the name fields in the ecc_curve structures used anywhere or > exposed to userspace? > > It'd be nice if the strings for the nist_p192, nist_p256, and nist_p384 > structures and this new nist_p521 structure included "p" before > the number, better matching all the code and the NIST FIPS 186-4 names: > .name = "nist_p192" > .name = "nist_p256" > .name = "nist_p384" > .name = "nist_p521" > > This is what is exposed: $ cat /proc/crypto | grep nist name : ecdh-nist-p384 driver : ecdh-nist-p384-generic name : ecdh-nist-p256 driver : ecdh-nist-p256-generic name : ecdh-nist-p192 driver : ecdh-nist-p192-generic name : ecdsa-nist-p384 driver : ecdsa-nist-p384-generic name : ecdsa-nist-p256 driver : ecdsa-nist-p256-generic name : ecdsa-nist-p192 driver : ecdsa-nist-p192-generic
On Thu, 2024-02-15 at 18:13 -0500, Stefan Berger wrote: > This series of patches adds support for the NIST P521 curve to ecdsa and > ecdh. Test cases for NIST P521 are added to both modules. > > An issue with the current code in ecdsa and ecdh is that it assumes that > input arrays providing key coordinates for example, are arrays of digits > (a 'digit' is a 'u64'). This works well for all currently supported > curves, such as NIST P192/256/384, but does not work for NIST P521 where > coordinates are 8 digits + 2 bytes long. So some of the changes deal with > converting byte arrays to digits and digits to byte arrays. > > > Regards, > Stefan > > v2: > - Reformulated some patch descriptions > - Fixed issue detected by krobot > - Some other small changes to the code > > Stefan Berger (14): > crypto: ecdsa - Convert byte arrays with key coordinates to digits > crypto: ecdsa - Adjust tests on length of key parameters > crypto: ecdsa - Extend res.x mod n calculation for NIST P521 > crypto: ecc - Implement vli_mmod_fast_521 for NIST p521 > crypto: ecc - For NIST P521 use vli_num_bits to get number of bits > crypto: ecc - Add NIST P521 curve parameters > crypto: ecdsa - Register NIST P521 and extend test suite > x509: Add OID for NIST P521 and extend parser for it > crypto: ecdh - Use properly formatted digits to check for valid key > crypto: ecc - Implement ecc_digits_to_bytes to convert digits to byte > array > crypto: Add nbits field to ecc_curve structure > crypto: ecc - Implement and use ecc_curve_get_nbytes to get curve's > nbytes > crypto: ecdh - Use functions to copy digits from and to byte array > crypto: ecdh - Add support for NIST P521 and add test case > > crypto/asymmetric_keys/x509_cert_parser.c | 3 + > crypto/ecc.c | 71 +++++-- > crypto/ecc_curve_defs.h | 45 +++++ > crypto/ecdh.c | 59 +++++- > crypto/ecdsa.c | 48 ++++- > crypto/testmgr.c | 14 ++ > crypto/testmgr.h | 225 ++++++++++++++++++++++ > include/crypto/ecc_curve.h | 3 + > include/crypto/ecdh.h | 1 + > include/crypto/internal/ecc.h | 61 +++++- > include/linux/oid_registry.h | 1 + > 11 files changed, 495 insertions(+), 36 deletions(-) Hi Stefan, what kind of side-channel testing was performed on this code? And what is the use case you are adding it for? Thanks, Simo.
On 2/16/24 14:27, Simo Sorce wrote: > On Thu, 2024-02-15 at 18:13 -0500, Stefan Berger wrote: >> This series of patches adds support for the NIST P521 curve to ecdsa and >> ecdh. Test cases for NIST P521 are added to both modules. >> >> An issue with the current code in ecdsa and ecdh is that it assumes that >> input arrays providing key coordinates for example, are arrays of digits >> (a 'digit' is a 'u64'). This works well for all currently supported >> curves, such as NIST P192/256/384, but does not work for NIST P521 where >> coordinates are 8 digits + 2 bytes long. So some of the changes deal with >> converting byte arrays to digits and digits to byte arrays. >> >> >> Regards, >> Stefan >> >> v2: >> - Reformulated some patch descriptions >> - Fixed issue detected by krobot >> - Some other small changes to the code >> >> Stefan Berger (14): >> crypto: ecdsa - Convert byte arrays with key coordinates to digits >> crypto: ecdsa - Adjust tests on length of key parameters >> crypto: ecdsa - Extend res.x mod n calculation for NIST P521 >> crypto: ecc - Implement vli_mmod_fast_521 for NIST p521 >> crypto: ecc - For NIST P521 use vli_num_bits to get number of bits >> crypto: ecc - Add NIST P521 curve parameters >> crypto: ecdsa - Register NIST P521 and extend test suite >> x509: Add OID for NIST P521 and extend parser for it >> crypto: ecdh - Use properly formatted digits to check for valid key >> crypto: ecc - Implement ecc_digits_to_bytes to convert digits to byte >> array >> crypto: Add nbits field to ecc_curve structure >> crypto: ecc - Implement and use ecc_curve_get_nbytes to get curve's >> nbytes >> crypto: ecdh - Use functions to copy digits from and to byte array >> crypto: ecdh - Add support for NIST P521 and add test case >> >> crypto/asymmetric_keys/x509_cert_parser.c | 3 + >> crypto/ecc.c | 71 +++++-- >> crypto/ecc_curve_defs.h | 45 +++++ >> crypto/ecdh.c | 59 +++++- >> crypto/ecdsa.c | 48 ++++- >> crypto/testmgr.c | 14 ++ >> crypto/testmgr.h | 225 ++++++++++++++++++++++ >> include/crypto/ecc_curve.h | 3 + >> include/crypto/ecdh.h | 1 + >> include/crypto/internal/ecc.h | 61 +++++- >> include/linux/oid_registry.h | 1 + >> 11 files changed, 495 insertions(+), 36 deletions(-) > > Hi Stefan, > what kind of side-channel testing was performed on this code? > And what is the use case you are adding it for? We're using public keys for signature verification. I am not aware that public key usage is critical to side channels. The use case for adding it is primarily driven by closing a gap to complete the support for the common ECDSA NIST curves. Stefan > > Thanks, > Simo. >
On Fri, 2024-02-16 at 14:32 -0500, Stefan Berger wrote: > > On 2/16/24 14:27, Simo Sorce wrote: > > On Thu, 2024-02-15 at 18:13 -0500, Stefan Berger wrote: > > > This series of patches adds support for the NIST P521 curve to ecdsa and > > > ecdh. Test cases for NIST P521 are added to both modules. > > > > > > An issue with the current code in ecdsa and ecdh is that it assumes that > > > input arrays providing key coordinates for example, are arrays of digits > > > (a 'digit' is a 'u64'). This works well for all currently supported > > > curves, such as NIST P192/256/384, but does not work for NIST P521 where > > > coordinates are 8 digits + 2 bytes long. So some of the changes deal with > > > converting byte arrays to digits and digits to byte arrays. > > > > > > > > > Regards, > > > Stefan > > > > > > v2: > > > - Reformulated some patch descriptions > > > - Fixed issue detected by krobot > > > - Some other small changes to the code > > > > > > Stefan Berger (14): > > > crypto: ecdsa - Convert byte arrays with key coordinates to digits > > > crypto: ecdsa - Adjust tests on length of key parameters > > > crypto: ecdsa - Extend res.x mod n calculation for NIST P521 > > > crypto: ecc - Implement vli_mmod_fast_521 for NIST p521 > > > crypto: ecc - For NIST P521 use vli_num_bits to get number of bits > > > crypto: ecc - Add NIST P521 curve parameters > > > crypto: ecdsa - Register NIST P521 and extend test suite > > > x509: Add OID for NIST P521 and extend parser for it > > > crypto: ecdh - Use properly formatted digits to check for valid key > > > crypto: ecc - Implement ecc_digits_to_bytes to convert digits to byte > > > array > > > crypto: Add nbits field to ecc_curve structure > > > crypto: ecc - Implement and use ecc_curve_get_nbytes to get curve's > > > nbytes > > > crypto: ecdh - Use functions to copy digits from and to byte array > > > crypto: ecdh - Add support for NIST P521 and add test case > > > > > > crypto/asymmetric_keys/x509_cert_parser.c | 3 + > > > crypto/ecc.c | 71 +++++-- > > > crypto/ecc_curve_defs.h | 45 +++++ > > > crypto/ecdh.c | 59 +++++- > > > crypto/ecdsa.c | 48 ++++- > > > crypto/testmgr.c | 14 ++ > > > crypto/testmgr.h | 225 ++++++++++++++++++++++ > > > include/crypto/ecc_curve.h | 3 + > > > include/crypto/ecdh.h | 1 + > > > include/crypto/internal/ecc.h | 61 +++++- > > > include/linux/oid_registry.h | 1 + > > > 11 files changed, 495 insertions(+), 36 deletions(-) > > > > Hi Stefan, > > what kind of side-channel testing was performed on this code? > > And what is the use case you are adding it for? > > We're using public keys for signature verification. I am not aware that > public key usage is critical to side channels. > > The use case for adding it is primarily driven by closing a gap to > complete the support for the common ECDSA NIST curves. Is there an assumption the ECDH code uses exclusively ephemeral keys? Simo.
On 2/16/24 14:40, Simo Sorce wrote: > On Fri, 2024-02-16 at 14:32 -0500, Stefan Berger wrote: >> >> On 2/16/24 14:27, Simo Sorce wrote: >>> On Thu, 2024-02-15 at 18:13 -0500, Stefan Berger wrote: >>>> This series of patches adds support for the NIST P521 curve to ecdsa and >>>> ecdh. Test cases for NIST P521 are added to both modules. >>>> >>>> An issue with the current code in ecdsa and ecdh is that it assumes that >>>> input arrays providing key coordinates for example, are arrays of digits >>>> (a 'digit' is a 'u64'). This works well for all currently supported >>>> curves, such as NIST P192/256/384, but does not work for NIST P521 where >>>> coordinates are 8 digits + 2 bytes long. So some of the changes deal with >>>> converting byte arrays to digits and digits to byte arrays. >>>> >>>> >>>> Regards, >>>> Stefan >>>> >>>> v2: >>>> - Reformulated some patch descriptions >>>> - Fixed issue detected by krobot >>>> - Some other small changes to the code >>>> >>>> Stefan Berger (14): >>>> crypto: ecdsa - Convert byte arrays with key coordinates to digits >>>> crypto: ecdsa - Adjust tests on length of key parameters >>>> crypto: ecdsa - Extend res.x mod n calculation for NIST P521 >>>> crypto: ecc - Implement vli_mmod_fast_521 for NIST p521 >>>> crypto: ecc - For NIST P521 use vli_num_bits to get number of bits >>>> crypto: ecc - Add NIST P521 curve parameters >>>> crypto: ecdsa - Register NIST P521 and extend test suite >>>> x509: Add OID for NIST P521 and extend parser for it >>>> crypto: ecdh - Use properly formatted digits to check for valid key >>>> crypto: ecc - Implement ecc_digits_to_bytes to convert digits to byte >>>> array >>>> crypto: Add nbits field to ecc_curve structure >>>> crypto: ecc - Implement and use ecc_curve_get_nbytes to get curve's >>>> nbytes >>>> crypto: ecdh - Use functions to copy digits from and to byte array >>>> crypto: ecdh - Add support for NIST P521 and add test case >>>> >>>> crypto/asymmetric_keys/x509_cert_parser.c | 3 + >>>> crypto/ecc.c | 71 +++++-- >>>> crypto/ecc_curve_defs.h | 45 +++++ >>>> crypto/ecdh.c | 59 +++++- >>>> crypto/ecdsa.c | 48 ++++- >>>> crypto/testmgr.c | 14 ++ >>>> crypto/testmgr.h | 225 ++++++++++++++++++++++ >>>> include/crypto/ecc_curve.h | 3 + >>>> include/crypto/ecdh.h | 1 + >>>> include/crypto/internal/ecc.h | 61 +++++- >>>> include/linux/oid_registry.h | 1 + >>>> 11 files changed, 495 insertions(+), 36 deletions(-) >>> >>> Hi Stefan, >>> what kind of side-channel testing was performed on this code? >>> And what is the use case you are adding it for? >> >> We're using public keys for signature verification. I am not aware that >> public key usage is critical to side channels. >> >> The use case for adding it is primarily driven by closing a gap to >> complete the support for the common ECDSA NIST curves. > > Is there an assumption the ECDH code uses exclusively ephemeral keys? > It can use both, provided keys and ephemeral keys. I think at this point it's best to drop ecdh support from this series. Stefan > Simo. >
This series of patches adds support for the NIST P521 curve to ecdsa and ecdh. Test cases for NIST P521 are added to both modules. An issue with the current code in ecdsa and ecdh is that it assumes that input arrays providing key coordinates for example, are arrays of digits (a 'digit' is a 'u64'). This works well for all currently supported curves, such as NIST P192/256/384, but does not work for NIST P521 where coordinates are 8 digits + 2 bytes long. So some of the changes deal with converting byte arrays to digits and digits to byte arrays. Regards, Stefan v2: - Reformulated some patch descriptions - Fixed issue detected by krobot - Some other small changes to the code Stefan Berger (14): crypto: ecdsa - Convert byte arrays with key coordinates to digits crypto: ecdsa - Adjust tests on length of key parameters crypto: ecdsa - Extend res.x mod n calculation for NIST P521 crypto: ecc - Implement vli_mmod_fast_521 for NIST p521 crypto: ecc - For NIST P521 use vli_num_bits to get number of bits crypto: ecc - Add NIST P521 curve parameters crypto: ecdsa - Register NIST P521 and extend test suite x509: Add OID for NIST P521 and extend parser for it crypto: ecdh - Use properly formatted digits to check for valid key crypto: ecc - Implement ecc_digits_to_bytes to convert digits to byte array crypto: Add nbits field to ecc_curve structure crypto: ecc - Implement and use ecc_curve_get_nbytes to get curve's nbytes crypto: ecdh - Use functions to copy digits from and to byte array crypto: ecdh - Add support for NIST P521 and add test case crypto/asymmetric_keys/x509_cert_parser.c | 3 + crypto/ecc.c | 71 +++++-- crypto/ecc_curve_defs.h | 45 +++++ crypto/ecdh.c | 59 +++++- crypto/ecdsa.c | 48 ++++- crypto/testmgr.c | 14 ++ crypto/testmgr.h | 225 ++++++++++++++++++++++ include/crypto/ecc_curve.h | 3 + include/crypto/ecdh.h | 1 + include/crypto/internal/ecc.h | 61 +++++- include/linux/oid_registry.h | 1 + 11 files changed, 495 insertions(+), 36 deletions(-)