Fix explicit Elliptic Curve parsing

[Marandil]

Description

Fixes Mbed-TLS/mbedtls#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 Add explicit parameter variants to ec_*_pub data files mbedtls-framework#285
• mbedtls development PR not required because: not required
• mbedtls 3.6 PR provided Fix explicit Elliptic Curve parsing (3.6 backport) mbedtls#10630
• tests provided

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.

[gilles-peskine-arm]

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 for the new feature bug fix (thanks Manuel).

[mpg]

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.

[mpg]

• 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]

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.)

[gilles-peskine-arm]

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]

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]

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]

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

[Marandil]

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]

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]

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]

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]

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 merged yesterday
• the framework pointer unfortunately very often has conflicts, but in this PR you want to keep it pointing to the tip of Add explicit parameter variants to ec_*_pub data files mbedtls-framework#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]

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]

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

[mpg]

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]

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.