mbox series

[v3,00/10] Add support for NIST P521 to ecdsa

Message ID 20240223204149.4055630-1-stefanb@linux.ibm.com (mailing list archive)
Headers show
Series Add support for NIST P521 to ecdsa | expand

Message

Stefan Berger Feb. 23, 2024, 8:41 p.m. UTC
This series adds support for the NIST P521 curve to the ecdsa module.

An issue with the current code in ecdsa 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 adjusting tests on input byte
array lengths to tolerate arrays not providing multiples of 8 bytes.

Regards,
   Stefan

v3:
 - Dropped ecdh support
 - Use ecc_get_curve_nbits for getting number of bits in NIST P521 curve
   in ecc_point_mult (7/10)

v2:
 - Reformulated some patch descriptions
 - Fixed issue detected by krobot
 - Some other small changes to the code


Stefan Berger (10):
  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 - Add nbits field to ecc_curve structure
  crypte: ecc - Implement ecc_curve_get_nbits to get number of bits
  crypto: ecc - Use ecc_get_curve_nbits to get number of bits for NIST
    P521
  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/asymmetric_keys/x509_cert_parser.c |   3 +
 crypto/ecc.c                              |  38 +++++-
 crypto/ecc_curve_defs.h                   |  45 +++++++
 crypto/ecdsa.c                            |  48 +++++--
 crypto/testmgr.c                          |   7 ++
 crypto/testmgr.h                          | 146 ++++++++++++++++++++++
 include/crypto/ecc_curve.h                |   3 +
 include/crypto/ecdh.h                     |   1 +
 include/crypto/internal/ecc.h             |  32 ++++-
 include/linux/oid_registry.h              |   1 +
 10 files changed, 315 insertions(+), 9 deletions(-)

Comments

Lukas Wunner Feb. 29, 2024, 9:34 a.m. UTC | #1
On Fri, Feb 23, 2024 at 03:41:39PM -0500, Stefan Berger wrote:
> This series adds support for the NIST P521 curve to the ecdsa module.
> 
> An issue with the current code in ecdsa 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 adjusting tests on input byte
> array lengths to tolerate arrays not providing multiples of 8 bytes.

Don't you also need to amend software_key_query()?  In the "issig" case,
it calculates len = crypto_sig_maxsize(sig), which is 72 bytes for P521,
then further below calculates "info->max_sig_size = 2 * (len + 3) + 2;"

I believe the ASN.1 encoded integers are just 66 bytes instead of 72,
so info->max_sig_size is 6 bytes too large.  Am I missing something?

Thanks,

Lukas
Stefan Berger Feb. 29, 2024, 6:45 p.m. UTC | #2
On 2/29/24 04:34, Lukas Wunner wrote:
> On Fri, Feb 23, 2024 at 03:41:39PM -0500, Stefan Berger wrote:
>> This series adds support for the NIST P521 curve to the ecdsa module.
>>
>> An issue with the current code in ecdsa 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 adjusting tests on input byte
>> array lengths to tolerate arrays not providing multiples of 8 bytes.
> 
> Don't you also need to amend software_key_query()?  In the "issig" case,
> it calculates len = crypto_sig_maxsize(sig), which is 72 bytes for P521,
> then further below calculates "info->max_sig_size = 2 * (len + 3) + 2;"
> 
> I believe the ASN.1 encoded integers are just 66 bytes instead of 72,
> so info->max_sig_size is 6 bytes too large.  Am I missing something?

Right! Good catch. While the 'keyctl pkey_verify' interface was already 
working the space was too generous with 72 bytes. So I adjusted 
ecdsa_max_size now to base the size calculations on nbits rather than 
ndigits and we now get 66 bytes.

For so-far supported curves the max_sig_size is:

2 bytes for sequence (0x30) + following length as single byte
Each coordinate may have a 0 prepended to make a possibly negative 
number positive:

  => 2 + 2 * (2 + 1 + len)

In case of NIST P521 the max signature length is calculated as follows:

3 bytes for sequence (0x30) + following length as 2 bytes
The coordinates won't have a preprended 0 byte since only 1 bit is used 
in the highest bit, so only 2 bytes for

  => 3 + 2 * (2 + len)

We would have to adjust the math there as well. The max. signature size 
for NIST P521 is 139 rather than 140 with the first formula.

    Stefan



> 
> Thanks,
> 
> Lukas