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.

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

Jump to bottom

Fix parsing of PKCS#8 encoded Elliptic Curve keys. #1379

Merged
merged 1 commit into from Mar 29, 2018
Merged

Fix parsing of PKCS#8 encoded Elliptic Curve keys. #1379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions ChangeLog
View file
Open in desktop
@@ -1,5 +1,12 @@
mbed TLS ChangeLog (Sorted per branch, date)

= mbed TLS x.x.x branch released xxx-xx-xx

Bugfix
* Fix parsing of PKCS#8 encoded Elliptic Curve keys. Previously Mbed TLS was
unable to parse keys with only the optional parameters field of the
ECPrivateKey structure. Found by jethrogb, fixed in #1379.

= mbed TLS 2.8.0 branch released 2018-03-16

Default behavior changes
Expand Down
3 changes: 3 additions & 0 deletions library/pkparse.c
View file
Open in desktop
Expand Up @@ -861,7 +861,10 @@ static int pk_parse_key_sec1_der( mbedtls_ecp_keypair *eck,
mbedtls_ecp_keypair_free( eck );
return( MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + ret );
}
}

if( p != end )
{
/*
* Is 'publickey' present? If not, or if we can't read it (eg because it
* is compressed), create it from the private key.
Expand Down
79 changes: 79 additions & 0 deletions tests/data_files/Makefile
View file
Open in desktop
Expand Up @@ -578,7 +578,86 @@ keys_rsa_enc_pkcs8_v2: keys_rsa_enc_pkcs8_v2_1024 keys_rsa_enc_pkcs8_v2_2048 key
### Generate all RSA keys
keys_rsa_all: keys_rsa_unenc keys_rsa_enc_basic keys_rsa_enc_pkcs8_v1 keys_rsa_enc_pkcs8_v2

################################################################
#### Generate various EC keys
################################################################

###
### PKCS8 encoded
###

ec_prv.pk8.der:
$(OPENSSL) genpkey -algorithm EC -pkeyopt ec_paramgen_curve:prime192v1 -pkeyopt ec_param_enc:named_curve -out $@ -outform DER
all_final += ec_prv.pk8.der

# ### Instructions for creating `ec_prv.pk8nopub.der`,
# ### `ec_prv.pk8nopubparam.der`, and `ec_prv.pk8param.der` by hand from
# ### `ec_prv.pk8.der`.
#
# These instructions assume you are familiar with ASN.1 DER encoding and can
# use a hex editor to manipulate DER.
#
# The relevant ASN.1 definitions for a PKCS#8 encoded Elliptic Curve key are:
#
# PrivateKeyInfo ::= SEQUENCE {
# version Version,
# privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
# privateKey PrivateKey,
# attributes [0] IMPLICIT Attributes OPTIONAL
# }
#
# AlgorithmIdentifier ::= SEQUENCE {
# algorithm OBJECT IDENTIFIER,
# parameters ANY DEFINED BY algorithm OPTIONAL
# }
#
# ECParameters ::= CHOICE {
# namedCurve OBJECT IDENTIFIER
# -- implicitCurve NULL
# -- specifiedCurve SpecifiedECDomain
# }
#
# ECPrivateKey ::= SEQUENCE {
# version INTEGER { ecPrivkeyVer1(1) } (ecPrivkeyVer1),
# privateKey OCTET STRING,
# parameters [0] ECParameters {{ NamedCurve }} OPTIONAL,
# publicKey [1] BIT STRING OPTIONAL
# }
#
# `ec_prv.pk8.der` as generatde above by OpenSSL should have the following
# fields:
#
# * privateKeyAlgorithm namedCurve
# * privateKey.parameters NOT PRESENT
# * privateKey.publicKey PRESENT
# * attributes NOT PRESENT
#
# # ec_prv.pk8nopub.der
#
# Take `ec_prv.pk8.der` and remove `privateKey.publicKey`.
#
# # ec_prv.pk8nopubparam.der
#
# Take `ec_prv.pk8nopub.der` and add `privateKey.parameters`, the same value as
# `privateKeyAlgorithm.namedCurve`. Don't forget to add the explicit tag.
#
# # ec_prv.pk8param.der
#
# Take `ec_prv.pk8.der` and add `privateKey.parameters`, the same value as
# `privateKeyAlgorithm.namedCurve`. Don't forget to add the explicit tag.

ec_prv.pk8.pem: ec_prv.pk8.der
$(OPENSSL) pkey -in $< -inform DER -out $@
all_final += ec_prv.pk8.pem
ec_prv.pk8nopub.pem: ec_prv.pk8nopub.der
$(OPENSSL) pkey -in $< -inform DER -out $@
all_final += ec_prv.pk8nopub.pem
ec_prv.pk8nopubparam.pem: ec_prv.pk8nopubparam.der
$(OPENSSL) pkey -in $< -inform DER -out $@
all_final += ec_prv.pk8nopubparam.pem
ec_prv.pk8param.pem: ec_prv.pk8param.der
$(OPENSSL) pkey -in $< -inform DER -out $@
all_final += ec_prv.pk8param.pem

################################################################
### Generate certificates for CRT write check tests
Expand Down
0 tests/data_files/ec_prv.noopt.der → tests/data_files/ec_prv.pk8nopub.der
View file
Open in desktop
File renamed without changes.
4 changes: 4 additions & 0 deletions tests/data_files/ec_prv.pk8nopub.pem
View file
Open in desktop
@@ -0,0 +1,4 @@
-----BEGIN PRIVATE KEY-----
MEECAQAwEwYHKoZIzj0CAQYIKoZIzj0DAQcEJzAlAgEBBCDH78XUX+cxmTPQ1hVkYbu3VvBc9c82
EyGKaGvkAo1Pkw==
-----END PRIVATE KEY-----
Binary file added tests/data_files/ec_prv.pk8nopubparam.der
View file
Open in desktop
Binary file not shown.
4 changes: 4 additions & 0 deletions tests/data_files/ec_prv.pk8nopubparam.pem
View file
Open in desktop
@@ -0,0 +1,4 @@
-----BEGIN PRIVATE KEY-----
ME0CAQAwEwYHKoZIzj0CAQYIKoZIzj0DAQcEMzAxAgEBBCDH78XUX+cxmTPQ1hVkYbu3VvBc9c82
EyGKaGvkAo1Pk6AKBggqhkjOPQMBBw==
-----END PRIVATE KEY-----
Binary file added tests/data_files/ec_prv.pk8param.der
View file
Open in desktop
Binary file not shown.
5 changes: 5 additions & 0 deletions tests/data_files/ec_prv.pk8param.pem
View file
Open in desktop
@@ -0,0 +1,5 @@
-----BEGIN PRIVATE KEY-----
MIGTAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBHkwdwIBAQQgx+/F1F/nMZkz0NYVZGG7t1bwXPXP
NhMhimhr5AKNT5OgCgYIKoZIzj0DAQehRANCAARkJXH1LofHesYJwJkoZQ0ijCVrxDFEi8e/fc1d
6DS2Hsk55TWpL953QEIDN8RmW01lejceK3jQWs0uGDenGCcM
-----END PRIVATE KEY-----
28 changes: 24 additions & 4 deletions tests/suites/test_suite_pkparse.data
View file
Open in desktop
Expand Up @@ -992,10 +992,6 @@ Parse EC Key #1 (SEC1 DER)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.sec1.der":"NULL":0

Parse EC Key #1a (SEC1 DER, no optional part)
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed? Wouldn't it be better to fix the renaming error?

Copy link
Author

Choose a reason for hiding this comment

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

The file is in the same format as test 5a (except PEM vs. DER). I only found out about this after I had already created my test case files. Having both is superfluous, choose one to keep.

# ec_prv.noopt.der
    0:d=0  hl=2 l=  65 cons: SEQUENCE          
    2:d=1  hl=2 l=   1 prim:  INTEGER           :00
    5:d=1  hl=2 l=  19 cons:  SEQUENCE          
    7:d=2  hl=2 l=   7 prim:   OBJECT            :id-ecPublicKey
   16:d=2  hl=2 l=   8 prim:   OBJECT            :prime256v1
   26:d=1  hl=2 l=  39 prim:  OCTET STRING      [HEX DUMP]:30250201010420EBCB4887A86D06F43418A298FCAE9CFD81EA7FF7FB27D45FDCBD902AB10D4F37

# ec_prv.pk8nopub.pem
    0:d=0  hl=2 l=  65 cons: SEQUENCE          
    2:d=1  hl=2 l=   1 prim:  INTEGER           :00
    5:d=1  hl=2 l=  19 cons:  SEQUENCE          
    7:d=2  hl=2 l=   7 prim:   OBJECT            :id-ecPublicKey
   16:d=2  hl=2 l=   8 prim:   OBJECT            :prime256v1
   26:d=1  hl=2 l=  39 prim:  OCTET STRING      [HEX DUMP]:30250201010420C7EFC5D45FE7319933D0D6156461BBB756F05CF5CF3613218A686BE4028D4F93

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarification. I would like to have both DER and PEM formats tested though

Copy link
Author

Choose a reason for hiding this comment

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

The PEM to DER conversion is already being tested, but I added the DER versions anyway (one is renamed from the original mislabeled test), as requested.

depends_on:POLARSSL_PEM_PARSE_C:POLARSSL_ECP_C:POLARSSL_ECP_DP_SECP256R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.noopt.der":"NULL":0

Parse EC Key #2 (SEC1 PEM)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.sec1.pem":"NULL":0
Expand All @@ -1008,10 +1004,34 @@ Parse EC Key #4 (PKCS8 DER)
depends_on:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.pk8.der":"NULL":0

Parse EC Key #4a (PKCS8 DER, no public key)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED
Copy link

Choose a reason for hiding this comment

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

MBEDTLS_ECP_C is already a dependency of test function pk_parse_keyfile_ec in tests/suites/test_suite_pkparse.function. Hence not required here and at other places.

pk_parse_keyfile_ec:"data_files/ec_prv.pk8nopub.der":"NULL":0

Parse EC Key #4b (PKCS8 DER, no public key, with parameters)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.pk8nopubparam.der":"NULL":0

Parse EC Key #4c (PKCS8 DER, with parameters)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.pk8param.der":"NULL":0

Parse EC Key #5 (PKCS8 PEM)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.pk8.pem":"NULL":0

Parse EC Key #5a (PKCS8 PEM, no public key)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.pk8nopub.pem":"NULL":0

Parse EC Key #5b (PKCS8 PEM, no public key, with parameters)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.pk8nopubparam.pem":"NULL":0

Parse EC Key #5c (PKCS8 PEM, with parameters)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.pk8param.pem":"NULL":0

Parse EC Key #6 (PKCS8 encrypted DER)
depends_on:MBEDTLS_DES_C:MBEDTLS_SHA1_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED
pk_parse_keyfile_ec:"data_files/ec_prv.pk8.pw.der":"polar":0
Expand Down