Fix explicit Elliptic Curve parsing #705

Open
Marandil wants to merge 6 commits from Marandil/fix/mbedtls-4314-parse-explicit-curve into development
Marandil commented 2026-03-04 08:20:41 -06:00 (Migrated from github.com)

Description

Fixes https://github.com/Mbed-TLS/mbedtls/issues/4314.

  • Fix pk_group_from_specified self-referencing group generator G while loading G.
  • Fix pk_group_from_specified not accounting for the a = p-3 internal representation.
  • Fix mbedtls_pk_parse_public_key failing to load keys with explicit curve parameters.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

  • Multiple simple commits
    • please structure your PR into a series of small commits, each of which does one thing
  • Avoid force-push
    • please do not force-push to update your PR - just add new commit(s)
  • See our Guidelines for Contributors for more details about the review process.
## Description Fixes https://github.com/Mbed-TLS/mbedtls/issues/4314. * Fix `pk_group_from_specified` self-referencing group generator G while loading G. * Fix `pk_group_from_specified` not accounting for the a = p-3 internal representation. * Fix `mbedtls_pk_parse_public_key` failing to load keys with explicit curve parameters. ## PR checklist Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line. If the provided content is part of the present PR remove the # symbol. - [ ] **changelog** provided - [ ] **framework PR** provided https://github.com/Mbed-TLS/mbedtls-framework/pull/285 - [ ] **mbedtls development PR** not required because: not required - [ ] **mbedtls 3.6 PR** provided https://github.com/Mbed-TLS/mbedtls/pull/10630 - **tests** provided ## Notes for the submitter Please refer to the [contributing guidelines](https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/development/CONTRIBUTING.md), especially the checklist for PR contributors. Help make review efficient: * Multiple simple commits - please structure your PR into a series of small commits, each of which does one thing * Avoid force-push - please do not force-push to update your PR - just add new commit(s) * See our [Guidelines for Contributors](https://mbed-tls.readthedocs.io/en/latest/reviews/review-for-contributors/) for more details about the review process.
gilles-peskine-arm commented 2026-03-04 09:03:51 -06:00 (Migrated from github.com)

Thanks for the patch! Please note that due to review bandwidth limitations, it may take a while before we can review it.

Before that, please do the following:

Thanks for the patch! Please note that due to review bandwidth limitations, it may take a while before we can review it. Before that, please do the following: * [ ] Make sure that all your commits have a signoff line (`git commit --amend --signoff`). * [ ] Raise a pull request in https://github.com/Mbed-TLS/mbedtls-framework for the framework part. * [ ] Write a [changelog entry file](https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/development/ChangeLog.d/00README.md) for the ~new feature~ bug fix (thanks Manuel).
mpg commented 2026-03-05 03:27:03 -06:00 (Migrated from github.com)

Write a changelog entry file for the new feature.

For reference, as just discussed elsewhere, I don't think it's a new feature: we have an option that's supposed to enable support for that except we only have tests for parsing private keys encoded this way, so quite unsurprisingly we have bugs in the part that's not tested: parsing public keys encoded this way.

> Write a [changelog entry file](https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/development/ChangeLog.d/00README.md) for the new feature. For reference, as just discussed elsewhere, I don't think it's a new feature: we have an option that's [supposed to enable support for that](https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/development/include/psa/crypto_config.h#L837) except we only have tests for parsing private keys encoded this way, so quite unsurprisingly we have bugs in the part that's not tested: parsing public keys encoded this way.
mpg commented 2026-03-05 03:28:38 -06:00 (Migrated from github.com)
  • Fix pk_group_from_specified self-referencing group generator G while loading G.

    • Fix pk_group_from_specified not accounting for the a = p-3 internal representation.

By the way, do you understand why these problems only manifest when parsing public keys, and are somehow avoided when parsing private keys? I haven't had the time to investigate yet, but that's a bit surprising to me.

> * Fix `pk_group_from_specified` self-referencing group generator G while loading G. > > * Fix `pk_group_from_specified` not accounting for the a = p-3 internal representation. By the way, do you understand why these problems only manifest when parsing public keys, and are somehow avoided when parsing private keys? I haven't had the time to investigate yet, but that's a bit surprising to me.
mpg (Migrated from github.com) reviewed 2026-03-05 03:39:11 -06:00
mpg (Migrated from github.com) left a comment

I have not done a full review, but the general shape of the PR is looking good.

I'm interested in reviewing this fully, but unfortunately it comes at a time where our load is unusually high. I want to review this before the end of the month, but I'm afraid I can't promise anything better at this point. (I realise this is not ideal as you are very responsive, but I think the second best thing we can do, other than reviewing immediately, is setting realistic expectations about timelines.)

I have not done a full review, but the general shape of the PR is looking good. I'm interested in reviewing this fully, but unfortunately it comes at a time where our load is unusually high. I want to review this before the end of the month, but I'm afraid I can't promise anything better at this point. (I realise this is not ideal as you are very responsive, but I think the second best thing we can do, other than reviewing immediately, is setting realistic expectations about timelines.)
@ -0,0 +1,4 @@
Bugfix:
mpg (Migrated from github.com) commented 2026-03-05 03:30:57 -06:00

This is an internal function, if should not be mentioned in the ChangeLog. I think those two lines are helpful in the PR description which is for developers, but the ChangeLog is for normal users.

This is an internal function, if should not be mentioned in the ChangeLog. I think those two lines are helpful in the PR description which is for developers, but the ChangeLog is for normal users.
mpg (Migrated from github.com) commented 2026-03-05 03:31:42 -06:00

Minor: the ChangeLog is plain text, so no backticks. Also, please use the full name of the funciton: mbedtls_pk_parse_public_key().

Minor: the ChangeLog is plain text, so no backticks. Also, please use the full name of the funciton: `mbedtls_pk_parse_public_key()`.
gilles-peskine-arm commented 2026-03-05 05:40:04 -06:00 (Migrated from github.com)

The CI has run and a few jobs are failing. Some of them are in driver tests, which are a bit weird, let us know if you need help figuring them out. Generally, to reporoduce a job, run tests/scripts/all.sh -k test_xxx if component_test_xxx is failing.

The CI has run and a few jobs are failing. Some of them are in driver tests, which are a bit weird, let us know if you need help figuring them out. Generally, to reporoduce a job, run `tests/scripts/all.sh -k test_xxx` if `component_test_xxx` is failing.
mpg commented 2026-03-05 05:50:37 -06:00 (Migrated from github.com)

Some additional "fun" for reproducing locally: if the component that fails lives in the mbedtls repo (we haven't finished migrating components to the right repo yet, sorry), you'll need to check out mbedtls, locally checkout your your branch into the tf-psa-crypto submodule, and then you can run the failing component.

Or, since you have already prepared a 3.6 backport, I think reproducing in 3.6 will just be easier.

Some additional "fun" for reproducing locally: if the component that fails lives in the mbedtls repo (we haven't finished migrating components to the right repo yet, sorry), you'll need to check out mbedtls, locally checkout your your branch into the tf-psa-crypto submodule, and then you can run the failing component. Or, since you have already prepared a 3.6 backport, I think reproducing in 3.6 will just be easier.
Marandil commented 2026-03-05 07:21:37 -06:00 (Migrated from github.com)

The CI has run and a few jobs are failing. Some of them are in driver tests, which are a bit weird, let us know if you need help figuring them out. Generally, to reporoduce a job, run tests/scripts/all.sh -k test_xxx if component_test_xxx is failing.

I checked the failing pipelines, I think it has something to do with missing flags on tests as the following failed:

  • test_psa_crypto_config_reference_ecc_no_bignum
  • test_psa_crypto_config_reference_ecc_ffdh_no_bignum
  • test_psa_crypto_config_reference_ecc_no_ecp_at_all

I'm assuming these tests should not be ran in these configs. I'll look into it.

> The CI has run and a few jobs are failing. Some of them are in driver tests, which are a bit weird, let us know if you need help figuring them out. Generally, to reporoduce a job, run `tests/scripts/all.sh -k test_xxx` if `component_test_xxx` is failing. I checked the failing pipelines, I think it has something to do with missing flags on tests as the following failed: * test_psa_crypto_config_reference_ecc_no_bignum * test_psa_crypto_config_reference_ecc_ffdh_no_bignum * test_psa_crypto_config_reference_ecc_no_ecp_at_all I'm assuming these tests should not be ran in these configs. I'll look into it.
mpg commented 2026-03-05 07:32:12 -06:00 (Migrated from github.com)

Ah, you want to mark the new test cases as depending on MBEDTLS_PK_PARSE_EC_EXTENDED.

Ah, you want to mark the new test cases as depending on `MBEDTLS_PK_PARSE_EC_EXTENDED`.
Marandil commented 2026-03-05 10:09:53 -06:00 (Migrated from github.com)

By the way, do you understand why these problems only manifest when parsing public keys, and are somehow avoided when parsing private keys? I haven't had the time to investigate yet, but that's a bit surprising to me.

I also found an answer to this one. It's actually because the test data for private key actually does not use the canonical representation and instead uses a highly compressed variant.

This is the extracted SpecifiedECDomain from the test file:

SEQUENCE (6 elem)
  INTEGER 1
  SEQUENCE (2 elem)
    OBJECT IDENTIFIER 1.2.840.10045.1.1 prime-field (ANSI X9.62 field type)
    INTEGER (256 bit) 1157920892373161954235709850086879078532699846656405640394575840079088…
  SEQUENCE (2 elem)
    OCTET STRING (1 byte) 00
    OCTET STRING (1 byte) 07
  OCTET STRING (33 byte) 0279BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798
  INTEGER (256 bit) 1157920892373161954235709850086879078528375642790749043826051631415181…
  INTEGER 1

While the same file converted to PEM using openssl ec -in framework/data_files/ec_prv.specdom.der:

SEQUENCE (6 elem)
  INTEGER 1
  SEQUENCE (2 elem)
    OBJECT IDENTIFIER 1.2.840.10045.1.1 prime-field (ANSI X9.62 field type)
    INTEGER (256 bit) 1157920892373161954235709850086879078532699846656405640394575840079088…
  SEQUENCE (2 elem)
    OCTET STRING (32 byte) 0000000000000000000000000000000000000000000000000000000000000000
    OCTET STRING (32 byte) 0000000000000000000000000000000000000000000000000000000000000007
  OCTET STRING (65 byte) 0479BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798483A…
  INTEGER (256 bit) 1157920892373161954235709850086879078528375642790749043826051631415181…
  INTEGER 1

To answer specifically "why does this work" -- the first file contains a compressed EC point that already had a special case built-in and was not the case of a = p-3.

> By the way, do you understand why these problems only manifest when parsing public keys, and are somehow avoided when parsing private keys? I haven't had the time to investigate yet, but that's a bit surprising to me. I also found an answer to this one. It's actually because the test data for private key actually does not use the canonical representation and instead uses a highly compressed variant. This is the extracted SpecifiedECDomain from the test file: ``` SEQUENCE (6 elem) INTEGER 1 SEQUENCE (2 elem) OBJECT IDENTIFIER 1.2.840.10045.1.1 prime-field (ANSI X9.62 field type) INTEGER (256 bit) 1157920892373161954235709850086879078532699846656405640394575840079088… SEQUENCE (2 elem) OCTET STRING (1 byte) 00 OCTET STRING (1 byte) 07 OCTET STRING (33 byte) 0279BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798 INTEGER (256 bit) 1157920892373161954235709850086879078528375642790749043826051631415181… INTEGER 1 ``` While the same file converted to PEM using `openssl ec -in framework/data_files/ec_prv.specdom.der`: ``` SEQUENCE (6 elem) INTEGER 1 SEQUENCE (2 elem) OBJECT IDENTIFIER 1.2.840.10045.1.1 prime-field (ANSI X9.62 field type) INTEGER (256 bit) 1157920892373161954235709850086879078532699846656405640394575840079088… SEQUENCE (2 elem) OCTET STRING (32 byte) 0000000000000000000000000000000000000000000000000000000000000000 OCTET STRING (32 byte) 0000000000000000000000000000000000000000000000000000000000000007 OCTET STRING (65 byte) 0479BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798483A… INTEGER (256 bit) 1157920892373161954235709850086879078528375642790749043826051631415181… INTEGER 1 ``` To answer specifically "why does this work" -- the first file contains a compressed EC point that already had a special case built-in and was not the case of `a = p-3`.
mpg commented 2026-03-06 03:05:25 -06:00 (Migrated from github.com)

Ah, very nice, thanks! Ok, so unlike what we (or at least I) originally thought, it has nothing to do with public vs private. As mentioned in the PR description, there are two bugs here:

  • Self-referencing G while loading G: this affects all curves and only depends on whether the base point is encoded in compressed form or not - only compressed format works, because it already had a fallback in the code.
  • Special representation of a = -3: this affects secpNNNr1 curves, but secpNNNk1 curves and Brainpool curves work.

Both bugs affect both mbedtls_pk_parse_public_key() and mbedtls_pk_parse_key(). It's just random luck that our existing test file was for a private key on an "Koblitz" curve with compressed G, while the other keys reported not working where with a public key on an "a = -3" curve with uncompressed G. Public vs private was a distraction, it is the other two differences that matter.

Taking a quick look at the companion framework PR, I can see that you generated files for all curves, so that's good, thanks! I wonder if we can easily control whether G is encoded in compressed form or not. It would be great to be able to test both variants for each curve if we can.

Ah, very nice, thanks! Ok, so unlike what we (or at least I) originally thought, it has nothing to do with public vs private. As mentioned in the PR description, there are two bugs here: - Self-referencing G while loading G: this affects all curves and only depends on whether the base point is encoded in compressed form or not - only compressed format works, because it already had a fallback in the code. - Special representation of a = -3: this affects secpNNNr1 curves, but secpNNNk1 curves and Brainpool curves work. Both bugs affect both `mbedtls_pk_parse_public_key()` and `mbedtls_pk_parse_key()`. It's just random luck that our existing test file was for a private key on an "Koblitz" curve with compressed G, while the other keys reported not working where with a public key on an "a = -3" curve with uncompressed G. Public vs private was a distraction, it is the other two differences that matter. Taking a quick look at the companion framework PR, I can see that you generated files for all curves, so that's good, thanks! I wonder if we can easily control whether G is encoded in compressed form or not. It would be great to be able to test both variants for each curve if we can.
mpg commented 2026-03-06 03:10:34 -06:00 (Migrated from github.com)

Out of curiosity, I checked if we had any record of how the existing test file ec_prv.specdom.der was generated, and unfortunately, but unsurprisingly for a file added 10 years ago, we don't. I'm glad we improved our practices and now systematically document how test files are generated.

Out of curiosity, I checked if we had any record of how the existing test file `ec_prv.specdom.der` was generated, and unfortunately, but unsurprisingly for a file added 10 years ago, we don't. I'm glad we improved our practices and now systematically document how test files are generated.
Marandil commented 2026-03-11 03:29:22 -05:00 (Migrated from github.com)

Hey, just to clarify: is there any more work that must be done or my side, or are we just waiting for review capacity now?

Hey, just to clarify: is there any more work that must be done or my side, or are we just waiting for review capacity now?
mpg commented 2026-03-11 03:43:29 -05:00 (Migrated from github.com)

Hi! I was going to say we're just waiting for review capacity now, but I see there are conflicts this morning:

We usually resolve conflicts by rebasing when doable. If you ca do that in the coming days, then I'll kick the CI again (it has to be started manually on external contributions), then we'll be all set for when there's review capacity.

Hi! I was going to say we're just waiting for review capacity now, but I see there are conflicts this morning: - `pkparse.c` has been moved to `extras/pkparse.c` by [this PR](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/696) merged yesterday - the framework pointer unfortunately very often has conflicts, but in this PR you want to keep it pointing to the tip of https://github.com/Mbed-TLS/mbedtls-framework/pull/285 anyway We usually resolve conflicts by rebasing when doable. If you ca do that in the coming days, then I'll kick the CI again (it has to be started manually on external contributions), then we'll be all set for when there's review capacity.
mpg commented 2026-03-11 04:25:43 -05:00 (Migrated from github.com)

We usually resolve conflicts by rebasing when doable.

FWIW, I just rebased one of my own PRs due to the same conflicts (file reorg) and git resolved it automatically, so hopefully it will be the same for you.

> We usually resolve conflicts by rebasing when doable. FWIW, I just rebased one of my own PRs due to the same conflicts (file reorg) and git resolved it automatically, so hopefully it will be the same for you.
Marandil commented 2026-03-11 05:07:09 -05:00 (Migrated from github.com)

Thanks, rebase indeed worked, although framework updates are a bit of a headache. Could we maybe get https://github.com/Mbed-TLS/mbedtls-framework/pull/285 going first s.t. it's not a moving target?

Thanks, rebase indeed worked, although framework updates are a bit of a headache. Could we maybe get https://github.com/Mbed-TLS/mbedtls-framework/pull/285 going first s.t. it's not a moving target?
mpg commented 2026-03-12 04:23:06 -05:00 (Migrated from github.com)

Yes, that's the plan! Thanks for the rebase.

We're experiencing CI issues at the moment that should be fixed shortly, I'll kick the CI on this PR once it's functional again.

Yes, that's the plan! Thanks for the rebase. We're experiencing CI issues at the moment that should be fixed shortly, I'll kick the CI on this PR once it's functional again.
mpg commented 2026-03-16 05:41:53 -05:00 (Migrated from github.com)

The CI is almost happy but has the following feedback:

  • the ChangeLog entry should start with Bugfix not Bugfix:
  • our code style mandates a space between if and opening parenthesis: if (

Specifically, our code style checker suggests the following patch:

--- extras/pkparse.c	2026-03-13 10:25:28.000000000 +0000
+++ extras/pkparse.c.uncrustify	2026-03-13 11:11:58.965754598 +0000
@@ -191,11 +191,11 @@
      * see mbedtls_ecp_group_a_is_minus_3
      */
     mbedtls_mpi_init(&tmp);
-    if((ret = mbedtls_mpi_sub_int(&tmp, &grp->P, 3)) != 0) {
+    if ((ret = mbedtls_mpi_sub_int(&tmp, &grp->P, 3)) != 0) {
         mbedtls_mpi_free(&tmp);
         return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PK_KEY_INVALID_FORMAT, ret);
     }
-    if(mbedtls_mpi_cmp_mpi(&tmp, &grp->A) == 0) {
+    if (mbedtls_mpi_cmp_mpi(&tmp, &grp->A) == 0) {

You can either address this now, or just wait for review feedback and address both at the same time, whatever you prefer.

The CI is almost happy but has the following feedback: - the ChangeLog entry should start with `Bugfix` not `Bugfix:` - our code style mandates a space between `if` and opening parenthesis: `if (` Specifically, our code style checker suggests the following patch: ```diff --- extras/pkparse.c 2026-03-13 10:25:28.000000000 +0000 +++ extras/pkparse.c.uncrustify 2026-03-13 11:11:58.965754598 +0000 @@ -191,11 +191,11 @@ * see mbedtls_ecp_group_a_is_minus_3 */ mbedtls_mpi_init(&tmp); - if((ret = mbedtls_mpi_sub_int(&tmp, &grp->P, 3)) != 0) { + if ((ret = mbedtls_mpi_sub_int(&tmp, &grp->P, 3)) != 0) { mbedtls_mpi_free(&tmp); return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PK_KEY_INVALID_FORMAT, ret); } - if(mbedtls_mpi_cmp_mpi(&tmp, &grp->A) == 0) { + if (mbedtls_mpi_cmp_mpi(&tmp, &grp->A) == 0) { ``` You can either address this now, or just wait for review feedback and address both at the same time, whatever you prefer.
valeriosetti (Migrated from github.com) requested changes 2026-05-05 06:29:40 -05:00
valeriosetti (Migrated from github.com) left a comment

I might be wrong, but it seems to me that we're not testing the case of explicit + compressed. Am I wrong?

I might be wrong, but it seems to me that we're not testing the case of explicit + compressed. Am I wrong?
@ -1194,0 +1196,4 @@
pk_parse_public_keyfile_ec:"../framework/data_files/ec_256_pub.expl.pem":0
Parse Public EC Key #4d (RFC 5480, secp256r1, explicit, DER)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_PK_PARSE_EC_EXTENDED
valeriosetti (Migrated from github.com) commented 2026-05-05 05:16:30 -05:00

MBEDTLS_PEM_PARSE_C but the file is in .der format.

`MBEDTLS_PEM_PARSE_C` but the file is in `.der` format.
@ -1226,0 +1250,4 @@
Parse Public EC Key #8b (RFC 5480, brainpoolP384r1, explicit)
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_DP_BP384R1_ENABLED:MBEDTLS_PK_PARSE_EC_EXTENDED
pk_parse_public_keyfile_ec:"../framework/data_files/ec_bp384_pub.expl.pem":0
valeriosetti (Migrated from github.com) commented 2026-05-05 05:16:48 -05:00

MBEDTLS_PEM_PARSE_C but the file is in .der format.

MBEDTLS_PEM_PARSE_C but the file is in `.der` format.
valeriosetti (Migrated from github.com) commented 2026-05-05 05:17:00 -05:00

MBEDTLS_PEM_PARSE_C but the file is in .der format.

MBEDTLS_PEM_PARSE_C but the file is in `.der` format.
This pull request has changes conflicting with the target branch.
  • framework
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'Marandil/fix/mbedtls-4314-parse-explicit-curve:Marandil/fix/mbedtls-4314-parse-explicit-curve'
git switch 'Marandil/fix/mbedtls-4314-parse-explicit-curve'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'
git merge --no-ff 'Marandil/fix/mbedtls-4314-parse-explicit-curve'
git switch 'Marandil/fix/mbedtls-4314-parse-explicit-curve'
git rebase 'development'
git switch 'development'
git merge --ff-only 'Marandil/fix/mbedtls-4314-parse-explicit-curve'
git switch 'Marandil/fix/mbedtls-4314-parse-explicit-curve'
git rebase 'development'
git switch 'development'
git merge --no-ff 'Marandil/fix/mbedtls-4314-parse-explicit-curve'
git switch 'development'
git merge --squash 'Marandil/fix/mbedtls-4314-parse-explicit-curve'
git switch 'development'
git merge --ff-only 'Marandil/fix/mbedtls-4314-parse-explicit-curve'
git switch 'development'
git merge 'Marandil/fix/mbedtls-4314-parse-explicit-curve'
git push origin 'development'
Sign in to join this conversation.
No reviewers
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/TF-PSA-Crypto!705
No description provided.