Skip to content
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.

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

Add support for compressed format to ecp_point_read_binary #1608

Closed
wants to merge 1 commit into from

Conversation

meh
Copy link

@meh meh commented May 2, 2018

Since it's supported when writing it should also be supported when reading.

I sent a corporate signed CLA earlier.

@RonEld
Copy link
Contributor

RonEld commented May 10, 2018

resolves #1616

@meh Thank you for your contribution! please fix CI failure:

The following tests FAILED:
	 28 - ecp-suite (Failed)
	 51 - pkparse-suite (Failed)

@simonbutcher simonbutcher added the component-crypto Crypto primitives and low-level interfaces label May 18, 2018
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Hi @meh, and thanks for you contribution.

Unfortunately, this implementation does not conform to the standard, as way more computations are required. (See #521 for something more realistic.) Also, it doesn't come with any tests (which would have caught the issues).

MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( &pt->Y, buf + 1 + plen, plen ) );
MBEDTLS_MPI_CHK( mbedtls_mpi_lset( &pt->Z, 1 ) );
MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( &pt->X, buf + 1, plen ) );
MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( &pt->Y, 0, buf[0] & 1 ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct. The Y coordinate should be computed by using the curve equation to computer Y^2, then extracting a square root modulo P, and only then using that bit to select which of the two square roots we want.

@mpg
Copy link
Contributor

mpg commented Feb 7, 2019

Support for compressed format has been deprecated by RFC 8422 in the context of TLS, which reflects a more general sentiment in the ECC community to prefer uncompressed format. Also, implementing it correctly for all supported curves would require substantial code, impacting our footprint.

At this point, we're unlikely to want to add that amount of code for a feature that's formally deprecated in TLS and being abandoned more generally, so I'm closing this PR.

Thanks for your contribution and interest in Mbed TLS anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants