EC keys written with as DER could be incorrectly formatted #268

2019-09-19T04:44:04-05:00

joelpalsson74 commented

2019-09-19 04:44:04 -05:00

(Migrated from github.com)

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:
https://github.com/ARMmbed/mbedtls/issues/1028

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: https://github.com/ARMmbed/mbedtls/issues/1028

gilles-peskine-arm (Migrated from github.com) requested changes

2019-10-29 10:17:44 -05:00

gilles-peskine-arm (Migrated from github.com) left a 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.

Thank you for your contribution! Unfortunately, this is still not correct. [RFC 5915](https://tools.ietf.org/html/rfc5915#section-3) 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.

joelpalsson74 commented

2019-11-01 08:07:01 -05:00

(Migrated from github.com)

Gilles, thanks for you comments.
Unfortunately I got no idea how to make the unit tests.

/Joel

Gilles, thanks for you comments. Unfortunately I got no idea how to make the unit tests. /Joel

joelpalsson74 commented

2019-11-01 09:23:28 -05:00

(Migrated from github.com)

Gilles,
It seems as one of the unit test fails:
Failed test suite: MBEDTLS_ECP_DP_SECP192R1_ENABLED

I don't understand how to check if the test is correct or not.
Can you please help out?

Best regards,
Joel

Gilles, It seems as one of the unit test fails: Failed test suite: MBEDTLS_ECP_DP_SECP192R1_ENABLED I don't understand how to check if the test is correct or not. Can you please help out? Best regards, Joel

gilles-peskine-arm (Migrated from github.com) requested changes

2019-11-05 08:41:20 -06:00

include/mbedtls/asn1write.h

					
				@ -108,1 +108,4 @@

				                            const mbedtls_mpi *X );

				/**

				 * \brief           Write a arbitrary-precision number as an

“Write a arbitrary-precision number” + “in binary in big-endian order”

include/mbedtls/asn1write.h

					
				@ -109,0 +118,4 @@

				 * \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

“leading zeros is” → “leading zeros are”

library/asn1write.c

					
				@ -164,0 +188,4 @@

				    {

				       (*p)--;

				       **p = 0;

				    }

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) );

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) ); ```

library/pkwrite.c

					
				@ -430,3 +428,4 @@

				        MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_mpi_to_octet_string( &c, buf, &ec->d, ec->d.n * 4 ) );

				        /* version */

				        MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_int( &c, buf, 1 ) );

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

`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`

gilles-peskine-arm commented

2019-11-05 08:46:30 -06:00

(Migrated from github.com)

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: https://github.com/ARMmbed/mbed-crypto/pull/314 . It fixes ARMmbed/mbedtls#1028, but does not add mbedtls_asn1_write_mpi_to_octet_string.

Do you have a need for mbedtls_asn1_write_mpi_to_octet_string beyond fixing pkwrite on EC keys? If so, please rebase this pull request on top of the new code after https://github.com/ARMmbed/mbed-crypto/pull/314 is merged. If not, I think this pull request can be closed.

Please accept our apologies for the inconvenience.

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: https://github.com/ARMmbed/mbed-crypto/pull/314 . It fixes ARMmbed/mbedtls#1028, but does not add `mbedtls_asn1_write_mpi_to_octet_string`. Do you have a need for `mbedtls_asn1_write_mpi_to_octet_string` beyond fixing pkwrite on EC keys? If so, please rebase this pull request on top of the new code after https://github.com/ARMmbed/mbed-crypto/pull/314 is merged. If not, I think this pull request can be closed. Please accept our apologies for the inconvenience.

joelpalsson74 commented

2019-11-06 01:50:45 -06:00

(Migrated from github.com)