EC keys written with as DER could be incorrectly formatted #268
Conversation
When writing EC keys using mbedtls_pk_write_key_der() the private part of the key got incorrectly pre-pended with 0 if MSB of the key was 1.
There was a problem hiding this comment.
Thank you for your contribution! Unfortunately, this is still not correct. RFC 5915 states
privateKey is the private key. It is an octet string of length ceiling (log2(n)/8) (where n is the order of the curve)
The length of the octet string only depends on the curve, not on the numerical value of the private key. But your implementation writes the number with no leading zeros, so it fixes the cases where the most significant bit is 1 but not the cases where the most significant octet is 0.
For a proper fix, mbedtls_asn1_write_mpi_to_octet_string would need to take the size of the octet string as an argument.
Also we need unit tests for the ASN.1 module these days.
gilles-peskine-arm
added
bug
According to RFC 3447 (chapter 4.1, I2OSP) MPIs shall be prepended with zero data to its full size when written as an octet string.
|
Gilles, thanks for you comments. /Joel |
Missed to add one file in previous commit
|
Gilles, I don't understand how to check if the test is correct or not. Best regards, |
| @@ -106,6 +106,26 @@ int mbedtls_asn1_write_raw_buffer( unsigned char **p, unsigned char *start, | |||
| */ | |||
| int mbedtls_asn1_write_mpi( unsigned char **p, unsigned char *start, | |||
| const mbedtls_mpi *X ); | |||
|
|
|||
| /** | |||
| * \brief Write a arbitrary-precision number as an | |||
There was a problem hiding this comment.
“Write a arbitrary-precision number” + “in binary in big-endian order”
| * \param start The start of the buffer, for bounds-checking. | ||
| * \param X The MPI to write. | ||
| * \param len The intended number of octets of the resulting string. | ||
| * If the MPI results in less octets, leading zeros is |
There was a problem hiding this comment.
“leading zeros is” → “leading zeros are”
| if( mpi_size > len ) | ||
| return( MBEDTLS_ERR_ASN1_INVALID_LENGTH ); | ||
|
|
||
| (*p) -= mpi_size; | ||
| MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary( X, *p, mpi_size ) ); | ||
|
|
||
| for( i = 0; i < (int)(len - mpi_size); i++ ) | ||
| { | ||
| (*p)--; | ||
| **p = 0; | ||
| } |
There was a problem hiding this comment.
You don't need the check on mpi_size and the padding. Just
*p -= len;
MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary( X, *p, len) );
| MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_mpi( &c, buf, &ec->d ) ); | ||
| *c = MBEDTLS_ASN1_OCTET_STRING; | ||
| /* privateKey */ | ||
| MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_mpi_to_octet_string( &c, buf, &ec->d, ec->d.n * 4 ) ); |
There was a problem hiding this comment.
ec->d.n * 4 is (almost) never correct on 64-bit machines and not always correct on 32-bit machines. That's the size of the buffer in memory and it may have leading zeros or have fewer limbs if the number has leading enough zero bits (about 0.05% of P521 keys are like that).
The correct size is the nominal key size. That's the value of the nbits field in the group structure, converted to bytes: ( ec->grp->nbits + 7 ) / 8
|
So… this is embarrassing… it turns out that we had already fixed that bug, but the fix slipped through the net. It was applied to the long-time support branches, but it was never released in the development branch of Mbed TLS. We missed this and one other bug fix when we split the crypto code between Mbed TLS and Mbed Crypto. In the interest of simplifying our maintenance, we're going to apply the same patch to Mbed Crypto that we made to the Mbed TLS LTS branches: #314 . It fixes Mbed-TLS/mbedtls#1028, but does not add Do you have a need for Please accept our apologies for the inconvenience. |
|
Gilles, |
When writing EC keys using mbedtls_pk_write_key_der() the private
part of the key got incorrectly pre-pended with 0 if MSB of the
key was 1.
See issue:
Mbed-TLS/mbedtls#1028