Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2f47c0
EC keys written with as DER could be incorrectly formatted
joelpalsson74 Sep 19, 2019
e3c500b
Prepending MPI with zero data when written as octet string
joelpalsson74 Nov 1, 2019
7b68a37
Fixed compile error
joelpalsson74 Nov 1, 2019
549aa57
Added cast to avoid compilation error
joelpalsson74 Nov 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions include/mbedtls/asn1write.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* octet string (#MBEDTLS_ASN1_OCTET_STRING)
* in ASN.1 format.
*
* \note This function works backwards in data buffer.
*
* \param p The reference to the current position pointer.
* \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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“leading zeros is” → “leading zeros are”

* added to fill the string.
*
* \return The number of bytes written to \p p on success.
* \return A negative \c MBEDTLS_ERR_ASN1_XXX error code on failure.
*/
int mbedtls_asn1_write_mpi_to_octet_string( unsigned char **p, unsigned char *start,
const mbedtls_mpi *X, size_t len );
#endif /* MBEDTLS_BIGNUM_C */

/**
Expand Down
34 changes: 34 additions & 0 deletions library/asn1write.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,40 @@ int mbedtls_asn1_write_mpi( unsigned char **p, unsigned char *start, const mbedt

ret = (int) len;

cleanup:
return( ret );
}

int mbedtls_asn1_write_mpi_to_octet_string( unsigned char **p, unsigned char *start, const mbedtls_mpi *X, size_t len )
{
int ret;
size_t mpi_size;
int i;

// Write the MPI
//
mpi_size = mbedtls_mpi_size( X );

if( *p < start || (size_t)( *p - start ) < len )
return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL );

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;
}
Comment on lines +181 to +191
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_len( p, start, len ) );
MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( p, start, MBEDTLS_ASN1_OCTET_STRING ) );

ret = (int) len;

cleanup:
return( ret );
}
Expand Down
5 changes: 2 additions & 3 deletions library/pkwrite.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,9 +424,8 @@ int mbedtls_pk_write_key_der( mbedtls_pk_context *key, unsigned char *buf, size_
MBEDTLS_ASN1_CONTEXT_SPECIFIC | MBEDTLS_ASN1_CONSTRUCTED | 0 ) );
len += par_len;

/* privateKey: write as MPI then fix tag */
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 ) );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/* version */
MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_int( &c, buf, 1 ) );
Expand Down