From 313a5c1fec8d41da6b45f57530c145f9afdf80c2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 11 Aug 2018 00:42:21 +0200 Subject: [PATCH 1/4] pk_write test cases with short/long private key Add pk_write test cases where the ASN.1 INTEGER encoding of the private value does 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^518, i.e. the first byte is zero 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). --- tests/data_files/ec_256_long_prv.pem | 5 +++++ tests/data_files/ec_521_short_prv.pem | 7 +++++++ tests/suites/test_suite_pkwrite.data | 8 ++++++++ 3 files changed, 20 insertions(+) create mode 100644 tests/data_files/ec_256_long_prv.pem create mode 100644 tests/data_files/ec_521_short_prv.pem diff --git a/tests/data_files/ec_256_long_prv.pem b/tests/data_files/ec_256_long_prv.pem new file mode 100644 index 000000000..5141e30b4 --- /dev/null +++ b/tests/data_files/ec_256_long_prv.pem @@ -0,0 +1,5 @@ +-----BEGIN EC PRIVATE KEY----- +MHcCAQEEIIcex4mqXsQamUKTVf8vXmTAJrQvGjh5mXG8p9+OR4xAoAoGCCqGSM49 +AwEHoUQDQgAEqJ2HQjPpc6fDwE/vSa6U35USXawkTo98y4U6NsAl+rOGuqMPEFXf +P1Srm/Jrzwa/RuppRL5kgyAsGJTUmwZEzQ== +-----END EC PRIVATE KEY----- diff --git a/tests/data_files/ec_521_short_prv.pem b/tests/data_files/ec_521_short_prv.pem new file mode 100644 index 000000000..427b7ad47 --- /dev/null +++ b/tests/data_files/ec_521_short_prv.pem @@ -0,0 +1,7 @@ +-----BEGIN EC PRIVATE KEY----- +MIHcAgEBBEIAOXdk7W+Hf5L7Hc9fKe44wmpaRNs5ERFTkv5CrlXv/Bu3y28M673q +vBNo7a/UE/6NNQHu2pQODEYFpMg6R34b5SigBwYFK4EEACOhgYkDgYYABAFUMHXV +KPA4vkMgq+pFgDoH96XoM517gF2GJFV6h2gLhykzIHL/otAyEpAStw7MBvbU0V21 +ixB+hjqzO7Snxaj9mwB8g87OKxm5eGfsqvJNPdJ0RZ/EKy06Ukg6KThlhQeyrtIk +g5PTCrPnNszlffAy6/jCOe3Moi59g15H13sSzwfX6g== +-----END EC PRIVATE KEY----- diff --git a/tests/suites/test_suite_pkwrite.data b/tests/suites/test_suite_pkwrite.data index c8ff1773c..16d0dd627 100644 --- a/tests/suites/test_suite_pkwrite.data +++ b/tests/suites/test_suite_pkwrite.data @@ -30,10 +30,18 @@ Private key write check EC 192 bits depends_on:MBEDTLS_ECP_C:MBEDTLS_BASE64_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED pk_write_key_check:"data_files/ec_prv.sec1.pem" +Private key write check EC 256 bits (top bit set) +depends_on:MBEDTLS_ECP_C:MBEDTLS_BASE64_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED +pk_write_key_check:"data_files/ec_256_long_prv.pem" + Private key write check EC 521 bits depends_on:MBEDTLS_ECP_C:MBEDTLS_BASE64_C:MBEDTLS_ECP_DP_SECP521R1_ENABLED pk_write_key_check:"data_files/ec_521_prv.pem" +Private key write check EC 521 bits (top byte is 0) +depends_on:MBEDTLS_ECP_C:MBEDTLS_BASE64_C:MBEDTLS_ECP_DP_SECP521R1_ENABLED +pk_write_key_check:"data_files/ec_521_short_prv.pem" + Private key write check EC Brainpool 512 bits depends_on:MBEDTLS_ECP_C:MBEDTLS_BASE64_C:MBEDTLS_ECP_DP_BP512R1_ENABLED pk_write_key_check:"data_files/ec_bp512_prv.pem" From 98add4fd5a66bd4ac32c5e3079993316c00fc8bb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 11 Aug 2018 00:48:44 +0200 Subject: [PATCH 2/4] Fix pk_write with an EC key to write a constant-length private value 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. --- library/pkwrite.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/library/pkwrite.c b/library/pkwrite.c index 8eabd889b..fa934703a 100644 --- a/library/pkwrite.c +++ b/library/pkwrite.c @@ -37,6 +37,7 @@ #include "mbedtls/rsa.h" #endif #if defined(MBEDTLS_ECP_C) +#include "mbedtls/bignum.h" #include "mbedtls/ecp.h" #endif #if defined(MBEDTLS_ECDSA_C) @@ -54,6 +55,13 @@ #define mbedtls_free free #endif +#if defined(MBEDTLS_ECP_C) +/* Implementation that should never be optimized out by the compiler */ +static void mbedtls_zeroize( void *v, size_t n ) { + volatile unsigned char *p = (unsigned char*)v; while( n-- ) *p++ = 0; +} +#endif /* MBEDTLS_ECP_C */ + #if defined(MBEDTLS_RSA_C) /* * RSAPublicKey ::= SEQUENCE { @@ -143,6 +151,26 @@ static int pk_write_ec_param( unsigned char **p, unsigned char *start, return( (int) len ); } + +/* + * privateKey OCTET STRING -- always of length ceil(log2(n)/8) + */ +static int pk_write_ec_private( unsigned char **p, unsigned char *start, + mbedtls_ecp_keypair *ec ) +{ + int ret; + size_t byte_length = ( ec->grp.pbits + 7 ) / 8; + unsigned char tmp[MBEDTLS_ECP_MAX_BYTES]; + + ret = mbedtls_mpi_write_binary( &ec->d, tmp, byte_length ); + if( ret != 0 ) + goto exit; + ret = mbedtls_asn1_write_octet_string( p, start, tmp, byte_length ); + +exit: + mbedtls_zeroize( tmp, byte_length ); + return( ret ); +} #endif /* MBEDTLS_ECP_C */ int mbedtls_pk_write_pubkey( unsigned char **p, unsigned char *start, @@ -340,9 +368,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, pk_write_ec_private( &c, buf, ec ) ); /* version */ MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_int( &c, buf, 1 ) ); From 938845484cf67722bcab70aa134a48aadaf554c1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 11 Aug 2018 00:51:04 +0200 Subject: [PATCH 3/4] Add ChangeLog entry --- ChangeLog | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ChangeLog b/ChangeLog index 0634d763a..20be15d61 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,14 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS x.x.x branch released xxxx-xx-xx + +Security + * 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. + = mbed TLS 2.7.6 branch released 2018-08-31 Security From 3faac3e255db1f8f0ca8001521698aef1576f44d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 6 Nov 2018 14:38:06 +0100 Subject: [PATCH 4/4] Fix copypasta in test dependency --- tests/suites/test_suite_pkwrite.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_pkwrite.data b/tests/suites/test_suite_pkwrite.data index 16d0dd627..e0101ccdf 100644 --- a/tests/suites/test_suite_pkwrite.data +++ b/tests/suites/test_suite_pkwrite.data @@ -31,7 +31,7 @@ depends_on:MBEDTLS_ECP_C:MBEDTLS_BASE64_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED pk_write_key_check:"data_files/ec_prv.sec1.pem" Private key write check EC 256 bits (top bit set) -depends_on:MBEDTLS_ECP_C:MBEDTLS_BASE64_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED +depends_on:MBEDTLS_ECP_C:MBEDTLS_BASE64_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED pk_write_key_check:"data_files/ec_256_long_prv.pem" Private key write check EC 521 bits