Skip to content

Fix pk_write with EC key to use a constant size for the private value #314

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

Conversation

gilles-peskine-arm
Copy link
Collaborator

@gilles-peskine-arm gilles-peskine-arm commented Nov 5, 2019

When writing a private EC key, use a constant size for the private value, as specified in RFC 5915. Previously, the value was written as an ASN.1 INTEGER, which caused the size of the key to leak about 1 bit of information on average and could cause the value to be 1 byte too large for the output buffer.

These changes were previously applied to Mbed TLS 2.7, 2.16 and development (released in 2.19), but got lost during the split of Mbed TLS and Mbed Crypto. This is a straightforward side-port of https://github.com/ARMmbed/mbedtls-restricted/pull/503 and https://github.com/ARMmbed/mbedtls-restricted/pull/508 (private links: these were pull requests for a security fix which was quarantined until release).

Fix Mbed-TLS/mbedtls#1028

Internal ref: IOTCRYPT-969

Add pk_write test cases where the ASN.1 INTEGER encoding of the
private value would not have the mandatory size for the OCTET STRING
that contains the value.

ec_256_long_prv.pem is a random secp256r1 private key, selected so
that the private value is >= 2^255, i.e. the top bit of the first byte
is set (which would cause the INTEGER encoding to have an extra
leading 0 byte).

ec_521_short_prv.pem is a random secp521r1 private key, selected so
that the private value is < 2^519, i.e. the first byte is 0 and the
top bit of the second byte is 0 (which would cause the INTEGER
encoding to have one less 0 byte at the start).
When writing a private EC key, use a constant size for the private
value, as specified in RFC 5915. Previously, the value was written
as an ASN.1 INTEGER, which caused the size of the key to leak
about 1 bit of information on average, and could cause the value to be
1 byte too large for the output buffer.
@gilles-peskine-arm gilles-peskine-arm added bug Something isn't working needs: review The pull request is ready for review. This generally means that it has no known issues. labels Nov 5, 2019
Copy link
Contributor

@dgreen-arm dgreen-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@AndrzejKurek AndrzejKurek self-requested a review November 8, 2019 10:29
Copy link
Contributor

@AndrzejKurek AndrzejKurek left a comment

Choose a reason for hiding this comment

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

This is consistent with PR 503.

@gilles-peskine-arm gilles-peskine-arm added needs: ci Needs a passing full CI run and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Nov 8, 2019
@gilles-peskine-arm
Copy link
Collaborator Author

gilles-peskine-arm commented Nov 8, 2019

New CI run with repaired CI: https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-314-merge/2/ → passed. Good to merge.

@gilles-peskine-arm gilles-peskine-arm removed the needs: ci Needs a passing full CI run label Nov 8, 2019
@gilles-peskine-arm gilles-peskine-arm merged commit 2ad5e45 into ARMmbed:development Nov 8, 2019
gilles-peskine-arm added a commit to gilles-peskine-arm/mbed-crypto that referenced this pull request Nov 15, 2019
* ARMmbed#292: Make psa_close_key(0) and psa_destroy_key(0) succeed
* ARMmbed#299: Allow xxx_drbg_set_entropy_len before xxx_drbg_seed
* ARMmbed#259: Check `len` against buffers size upper bound in PSA tests
* ARMmbed#288: Add ECDSA tests with hash and key of different lengths
* ARMmbed#305: CTR_DRBG: grab a nonce from the entropy source if needed
* ARMmbed#316: Stop transactions from being reentrant
* ARMmbed#317: getting_started: Make it clear that keys are passed in
* ARMmbed#314: Fix pk_write with EC key to use a constant size for the private value
* ARMmbed#298: Test a build without any asymmetric cryptography
* ARMmbed#284: Fix some possibly-undefined variable warnings
* ARMmbed#315: Define MBEDTLS_PK_SIGNATURE_MAX_SIZE
* ARMmbed#318: Finish side-porting commits from mbedtls-restricted that missed the split
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ECPrivateKey wrong format with MSB=1
3 participants