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

jethrogb · 2018-02-16T21:32:52Z

Description

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
}

Because of the two optional fields, there are 4 possible variants that need to
be parsed: no optional fields, only parameters, only public key, and both
optional fields. Previously mbedTLS was unable to parse keys with "only
parameters". Also, only "only public key" was tested. There was a test for "no
optional fields", but it was labelled incorrectly as SEC.1 and not run because
of a great renaming mixup.

This contribution is submitted under CLA with Fortanix, Inc.

Status

READY

Requires Backporting

YES

Migrations

NO

Todos

Tests
Documentation
Changelog updated
Backported - 2.7 Backport 2.7: Fix parsing of PKCS#8 encoded Elliptic Curve keys. #1529 - 2.1 Backport 2.1: Fix parsing of PKCS#8 encoded Elliptic Curve keys. #1494

Steps to test or reproduce

Run the test suite from this PR without the changes in pkparse.c

mbed-ci · 2018-02-16T21:32:54Z

--none--

RonEld

No major comments.
However, as in #1135 PR, I would like to ask you to add the key generation commands in tests/data_files/Makefile for proper documentations

RonEld · 2018-02-20T16:37:00Z

tests/suites/test_suite_pkparse.data

@@ -406,10 +406,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)


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

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

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

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.

jethrogb · 2018-02-20T18:13:08Z

I created the keys by hand. If you know of a tool that can manipulate ASN.1 based on command line input I'll happily figure out the commands needed to generate these files.

RonEld · 2018-02-22T17:01:05Z

@jethrogb I don't understand your comment:

If you know of a tool that can manipulate ASN.1 based on command line input I'll happily figure out the commands needed to generate these files.

Maybe I am missing something.
How did you create the keys by hand? tests/data_files/Makefile is only used to document how the keys were created, not as a tool to create the tool every build.

jethrogb · 2018-02-22T17:19:19Z

How did you create the keys by hand?

I took an existing key and manipulated the DER in a hex editor.

mpg · 2018-03-22T11:01:55Z

Hi @jethrogb,

Thanks for your contribution and for working with us to improve your PR in response to Ron's comments. I just had a look at it and I think it's a good improvement to our current codebase and tests. However, I have a few more requests for you:

Can you please add a ChangeLog entry under the "BugFix" section explaining the bug from a user's perspective and crediting yourself for finding and fixing it? (With a reference to this PR.) Please create a new heading:

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

and put your entry under that version.

As Ron said, we're trying to track how files are created better than we did in the past. I understand those files had to be created manually (I don't know of a way to programmatically manipulate ASN.1 either, though I would be very interested), but could you please add comments to tests/data_files/Makefile with the file names in the comment (to make it easy to find) explaining (1) what you used as the base file for your modifications and (2) what modifications you made (eg "remove Parameters", "remove PublicKey"). You can imagine the following scenario: someone wants to make a version of the library that uses only P-384 and they want to create test files similar to yours but using that curve. Your comments should give them enough information to do that efficiently, assuming they're already familiar with Mbed TLS and editing ASN.1 with a hex editor in general, just not with how these particular files were created. I realise you already picked meaningful file names and explicit test description (thanks for that), but I think a comment in test/data_files/Makefile would still be a worthwile addition so that we don't have to hunt around for the information.

Once those steps are done, can you please ping @RonEld and I to give your PR another round of reviews? Based on my preliminary review today, I expect mine should be an approval at this stage.

Ideally if you could prepare a backport for the 2.1 branch that would be great (this branch looks can it can be merge both in development and 2.7, so we don't need a 2.7 backport), otherwise someone from the team can do it - just let us know.

Once the main PR and backport have been reviewed and approved by two members of the team, your PR will be ready to merge.

Thanks again for your interest in Mbed TLS, and I'm looking forward to working with you on finalizing this PR!

jethrogb · 2018-03-22T16:16:38Z

@mpg @RonEld Items 1 and 2 done. I'm not sure how to do the backport given I can only target one branch in my PR.

mpg · 2018-03-22T20:27:00Z

Thanks for the rework!

For the backports, the way we do this is create a new branch based on the target branch (here mbedtls-2.1), then usually cherry-pick commit(s) from the main branch (or any other method that suits you), then open a new PR with this branch, selecting the appropriate target branch (github should show just the commits you added).

As github doesn't offer any special support for marking a PR as a backport of another, what we usually do is give the backport PR the same title as the original, with a "Backport 2.1: " prefix, and include links in the descriptions from one branch to the other. See for example #1486 #1487 #1488

mpg

The code (including tests and documentation in the Makefile) looks great to me, but there are minor things to fix in the ChangeLog entry.

I notice you didn't credit yourself in the ChangeLog. If that's a conscious decision, of course no problem. Otherwise, don't be shy about adding "Found and fixed by #1379" :)

mpg · 2018-03-22T20:28:58Z

ChangeLog

+Bugfix
+   * Fix parsing of PKCS#8 encoded Elliptic Curve keys. Previously mbedTLS was
+     unable to parse keys with only the optional parameters field of the
+     ECPrivateKey strucutre.


Typo: structure.

mpg · 2018-03-22T20:31:19Z

ChangeLog

+= mbed TLS x.x.x branch released xxx-xx-xx
+
+Bugfix
+   * Fix parsing of PKCS#8 encoded Elliptic Curve keys. Previously mbedTLS was


Please use the the official spelling "Mbed TLS", thanks!

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 } Because of the two optional fields, there are 4 possible variants that need to be parsed: no optional fields, only parameters, only public key, and both optional fields. Previously mbedTLS was unable to parse keys with "only parameters". Also, only "only public key" was tested. There was a test for "no optional fields", but it was labelled incorrectly as SEC.1 and not run because of a great renaming mixup.

jethrogb · 2018-03-23T01:05:59Z

Updated ChangeLog as requested and filed backport PR #1494

mpg

Thanks for making those changes. I'm happy with the result!

mpg · 2018-03-23T17:21:01Z

@jethrogb Thanks for the changes and the backport! I took the liberty of editing the description in order to include a link to the backport PR (and vice-versa) and checking the todo boxes as you did everything :)

@RonEld Can you please give this (and the backport PR #1494) another round of review?

RonEld

LGTM

Patater · 2018-03-26T17:16:50Z

This needs a backport for 2.7. This PR as is doesn't apply cleanly to mbedtls-2.7.

mpg · 2018-03-26T18:43:07Z

Indeed, I guess it has been rebased on top of current development since the time I marked it as suitable for 2.7. @jethrogb would you like to create a 2.7 backport like you did for the 2.1 backport? Otherwise let us know and one of us will take care of it. Thanks!

mpg · 2018-03-28T09:40:29Z

2.7 backport created: #1529

mazimkhan · 2018-03-28T10:24:40Z

tests/suites/test_suite_pkparse.data

@@ -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


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.

RonEld added CLA valid enhancement labels Feb 20, 2018

RonEld suggested changes Feb 20, 2018

View reviewed changes

jethrogb force-pushed the ec_pkparse branch from 8c35a4f to ccf466e Compare February 22, 2018 17:25

mpg self-assigned this Mar 22, 2018

mpg added bug and removed enhancement labels Mar 22, 2018

jethrogb force-pushed the ec_pkparse branch from ccf466e to e6bdbda Compare March 22, 2018 16:15

mpg requested changes Mar 22, 2018

View reviewed changes

jethrogb force-pushed the ec_pkparse branch from e6bdbda to 6e2c609 Compare March 23, 2018 00:59

jethrogb force-pushed the ec_pkparse branch from 6e2c609 to d2df936 Compare March 23, 2018 01:01

jethrogb mentioned this pull request Mar 23, 2018

Backport 2.1: Fix parsing of PKCS#8 encoded Elliptic Curve keys. #1494

Merged

4 tasks

mpg approved these changes Mar 23, 2018

View reviewed changes

RonEld approved these changes Mar 25, 2018

View reviewed changes

gilles-peskine-arm added the approved Design and code approved - may be waiting for CI or backports label Mar 26, 2018

Patater added needs-backports Backports are missing or are pending review and approval. and removed approved Design and code approved - may be waiting for CI or backports labels Mar 26, 2018

mpg mentioned this pull request Mar 28, 2018

Backport 2.7: Fix parsing of PKCS#8 encoded Elliptic Curve keys. #1529

Merged

mazimkhan reviewed Mar 28, 2018

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports approved for design and removed needs-backports Backports are missing or are pending review and approval. labels Mar 29, 2018

Patater merged commit d2df936 into Mbed-TLS:development Mar 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jethrogb commented Feb 16, 2018 •

edited by mpg

mbed-ci commented Feb 16, 2018

RonEld left a comment

RonEld Feb 20, 2018

jethrogb Feb 20, 2018

RonEld Feb 22, 2018

jethrogb Feb 22, 2018

jethrogb commented Feb 20, 2018

RonEld commented Feb 22, 2018

jethrogb commented Feb 22, 2018

mpg commented Mar 22, 2018

jethrogb commented Mar 22, 2018

mpg commented Mar 22, 2018

mpg left a comment

mpg Mar 22, 2018

mpg Mar 22, 2018

jethrogb commented Mar 23, 2018

mpg left a comment

mpg commented Mar 23, 2018

RonEld left a comment

Patater commented Mar 26, 2018 •

edited

mpg commented Mar 26, 2018

mpg commented Mar 28, 2018

mazimkhan Mar 28, 2018

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

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

Conversation

jethrogb commented Feb 16, 2018 • edited by mpg

Description

Status

Requires Backporting

Migrations

Todos

Steps to test or reproduce

mbed-ci commented Feb 16, 2018

RonEld left a comment

Choose a reason for hiding this comment

RonEld Feb 20, 2018

Choose a reason for hiding this comment

jethrogb Feb 20, 2018

Choose a reason for hiding this comment

RonEld Feb 22, 2018

Choose a reason for hiding this comment

jethrogb Feb 22, 2018

Choose a reason for hiding this comment

jethrogb commented Feb 20, 2018

RonEld commented Feb 22, 2018

jethrogb commented Feb 22, 2018

mpg commented Mar 22, 2018

jethrogb commented Mar 22, 2018

mpg commented Mar 22, 2018

mpg left a comment

Choose a reason for hiding this comment

mpg Mar 22, 2018

Choose a reason for hiding this comment

mpg Mar 22, 2018

Choose a reason for hiding this comment

jethrogb commented Mar 23, 2018

mpg left a comment

Choose a reason for hiding this comment

mpg commented Mar 23, 2018

RonEld left a comment

Choose a reason for hiding this comment

Patater commented Mar 26, 2018 • edited

mpg commented Mar 26, 2018

mpg commented Mar 28, 2018

mazimkhan Mar 28, 2018

Choose a reason for hiding this comment

jethrogb commented Feb 16, 2018 •

edited by mpg

Patater commented Mar 26, 2018 •

edited