Fix explicit Elliptic Curve parsing #705
Open
Marandil wants to merge 6 commits from
Marandil/fix/mbedtls-4314-parse-explicit-curve into development
pull from: Marandil/fix/mbedtls-4314-parse-explicit-curve
merge into: gh-Mbed-TLS:development
gh-Mbed-TLS:development
gh-Mbed-TLS:tf-psa-crypto-1.1
gh-Mbed-TLS:archive/20261005T153337Z-force/pull/927
gh-Mbed-TLS:archive/20261001T141055Z-force/pull/927
gh-Mbed-TLS:archive/20261005T153337Z-force/pull/931
gh-Mbed-TLS:archive/20261001T083616Z-force/pull/927
gh-Mbed-TLS:archive/20260930T232327Z-force/pull/927
gh-Mbed-TLS:archive/20260930T154618Z-force/pull/927
gh-Mbed-TLS:archive/20260929T231919Z-rescue/rohankl-arm/mldsa-pubkey-899
gh-Mbed-TLS:archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9342-public-key-import-export
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ManoharPaturi/fix-pem-ct-padding
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ManoharPaturi/fix-asn1write-null-guard
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ManoharPaturi/fix-pbes2-params-validation
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/blake2
gh-Mbed-TLS:archive/20260929T231919Z-rescue/jfdev001/v1.1.1_bump-cmake-minimum-version
gh-Mbed-TLS:archive/20260929T231919Z-rescue/jfdev001/bump-cmake-minimum-version
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ronald-cron-arm/sm3-base
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/blake2-hmac
gh-Mbed-TLS:archive/20260929T231919Z-rescue/v1rtu3x/enforce-hash-length-sign-verify
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/file-backed-keystore-doc
gh-Mbed-TLS:archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9344-set-context
gh-Mbed-TLS:archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9343-key-pair-import-export-depends-on-9342
gh-Mbed-TLS:archive/20260929T231919Z-rescue/romart/development
gh-Mbed-TLS:archive/20260929T231919Z-rescue/sweetlilmre/constant-time-embedded-perf
gh-Mbed-TLS:archive/20260929T231919Z-rescue/JulioLoayzaM/add-wycheproof-rsa-pss
gh-Mbed-TLS:archive/20260929T231919Z-rescue/JulioLoayzaM/add-wycheproof-ecdsa
gh-Mbed-TLS:archive/20260929T231919Z-rescue/JulioLoayzaM/add-wycheproof-tests
gh-Mbed-TLS:archive/20260929T231919Z-rescue/oubaid-a/dev_psa_driver_wrapper
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/keystoreless-prototype-1
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/issue9580
gh-Mbed-TLS:archive/20260929T231919Z-rescue/LeonieReichertABB/hpke-functionality
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/psa-hash-no-buffer-copy-1.1
gh-Mbed-TLS:archive/20260929T231919Z-rescue/vanhauser-thc/development
gh-Mbed-TLS:archive/20260929T231919Z-rescue/tpambor/key-wrap
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ruiliio/psa_xts
gh-Mbed-TLS:archive/20261011T063515Z-force/pull/538
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/fix-broken-links-1.1
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/issue669
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/prepare-release-test
gh-Mbed-TLS:archive/20260929T231919Z-rescue/krish2718/wpa3-sae-design-note
gh-Mbed-TLS:archive/20260929T231919Z-rescue/nzzlinh/fix_iar_endianness_1.1
gh-Mbed-TLS:archive/20260929T231919Z-rescue/nzzlinh/fix-iar-endianness
gh-Mbed-TLS:archive/20260929T231919Z-rescue/abtom87/fix-pbkdf2-iter-count
gh-Mbed-TLS:archive/20260929T231919Z-rescue/minosgalanakis/tools/prepare_release_lite
gh-Mbed-TLS:archive/20260929T231919Z-rescue/Harshal5/fix/aes-xts-in-place-cts
gh-Mbed-TLS:archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9347-prover-depends-on-9342-9343-9344
gh-Mbed-TLS:archive/20260929T231919Z-rescue/crabel99/async-hardware-support-c35df962
gh-Mbed-TLS:tf-psa-crypto-1.2.0_mergeback
gh-Mbed-TLS:tf-psa-crypto-1.1.1_mergeback
gh-Mbed-TLS:archive/20260929T231919Z-rescue/asmellby/feature/dispatch-library
gh-Mbed-TLS:tf-psa-crypto-release-sync
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ChakshuGupta13/fix/ecdsa-zeroize-stack-key
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/security-md-option-names
gh-Mbed-TLS:archive/20260929T231919Z-rescue/parmi93/patch-2
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/remove-mldsa-sign-from-build
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/tf-m-build-issues
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/test-context-population
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/iar-1.1.0
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/fork-helper-unit-tests-create-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/redundant-declarations
gh-Mbed-TLS:archive/20260929T231919Z-rescue/hasheddan/docs/fix-broken-links
gh-Mbed-TLS:main
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/reintroduce-dh
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/cipher-update-incorrect-error
gh-Mbed-TLS:archive/20260929T231919Z-rescue/Marandil/fix/mbedtls-4314-parse-explicit-curve
gh-Mbed-TLS:archive/20260929T231919Z-rescue/schneider-electric/dev/plaj-se/parse-rsassa-pss
gh-Mbed-TLS:archive/20260929T231919Z-rescue/minosgalanakis/bugfix/program-fuzz-mscv
gh-Mbed-TLS:archive/20260929T231919Z-rescue/daverodgman/aesgcm
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/sha3-coverity-20260224
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bensze01/undef
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/issue523
gh-Mbed-TLS:archive/20260929T231919Z-rescue/dannytsen/ppc-dev
gh-Mbed-TLS:archive/20260929T231919Z-rescue/daverodgman/aes12
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/ascon-prototype
gh-Mbed-TLS:archive/20260929T231919Z-rescue/daverodgman/chacha10
gh-Mbed-TLS:archive/20260929T231919Z-rescue/kusumitg/implement_psa_pake_set_context_for_spake2p
gh-Mbed-TLS:archive/20260929T231919Z-rescue/davidhorstmann-arm/add-code-size-target
gh-Mbed-TLS:archive/20260929T231919Z-rescue/bjwtaylor/simplify-sign_func
gh-Mbed-TLS:archive/20260929T231919Z-rescue/mfil/feature/aes_ni_parallel
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ivv19041994/patch-1
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/psa-verify-policy-relax
gh-Mbed-TLS:public_2/main
gh-Mbed-TLS:archive/20260929T231919Z-rescue/mpg/pk-sign-proto
gh-Mbed-TLS:release/tf-psa-crypto1.0.0-documentation
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/0e-plans-july-updates
gh-Mbed-TLS:archive/20260929T231919Z-rescue/davidhorstmann-arm/guard-context-types-in-client-only
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/psa-driver-interface-spec-remove
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/ecp-self-test-race-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/KiruthikPrakash/feature/9342_add-support-for-spake2p-public-key-import-export
gh-Mbed-TLS:archive/20260929T231919Z-rescue/ruiliio/chacha20_16
gh-Mbed-TLS:archive/20260929T231919Z-rescue/irwir/winsock2
gh-Mbed-TLS:archive/20260929T231919Z-rescue/aphroteus/development
gh-Mbed-TLS:archive/20260929T231919Z-rescue/amjoul01/amjoul01/psa-mac-out-of-bound-fix
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/everest-submodule-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/yamt/xtensa-ccount
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/all.sh-generate-only-before-checks-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/Harshal5/feat/builtin_cmac_driver_use_psa_crypto_interface
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/test-data_t-empty_is_empty-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/jean-baptisteboric-eaton/oids-pkcs7-authenticated-attributes
gh-Mbed-TLS:dev/bensze01/public-only-commit
gh-Mbed-TLS:archive/20260929T231919Z-rescue/waleed-elmelegy-arm/psa-key-wrap-aes-kw
gh-Mbed-TLS:archive/20260929T231919Z-rescue/tfloch/psa_add_xts_algorithm
gh-Mbed-TLS:archive/20260929T231919Z-rescue/gilles-peskine-arm/msancf-clang16-crypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/DemiMarie/document-unexpected-tags
gh-Mbed-TLS:archive/20260929T231919Z-rescue/DemiMarie/strict-len
gh-Mbed-TLS:archive/20260929T231919Z-rescue/valeriosetti/issue9255-tfpsacrypto
gh-Mbed-TLS:archive/20260929T231919Z-rescue/daantimmer/use-compiler-frontend
gh-Mbed-TLS:archive/20260929T231919Z-rescue/benmcollins/enable-rsassa-pss-pkparse
gh-Mbed-TLS:dev/paul-elliott-arm/add_iop_key_agreement_negative_tests
gh-Mbed-TLS:add-iop-export-pub-key-get-num-ops
gh-Mbed-TLS:dev/rc/new-2-d635ea1
gh-Mbed-TLS:dev/bensze01/jenkins
No reviewers
Labels
Clear labels
api-break
This issue/PR breaks the API and must wait for a new major version
approved
Design and code approved - may be waiting for CI or backports
bug
Something isn't working
component-docs
Docs / website issues filed here for tracking
component-platform
Portability layer and build scripts
component-test
Test framework and CI scripts
dependencies
Pull requests that update a dependency file
deprecation
needs deprecation announcement
DO-NOT-MERGE
enhancement
New feature or request
good first issue
Good for newcomers
help wanted
This issue is not being actively worked on, but PRs welcome.
needs-adoption
stalled PR that someone should pick up and complete
needs-backports
Backports are missing or are pending review and approval.
needs-ci
Needs to pass CI tests
needs-design-approval
Needs design discussion / approval
needs-info
An issue or PR which needs further info from the reporter / author
needs-preceding-pr
Requires another PR to be merged first
needs-review
Every commit must be reviewed by at least two team members
needs-reviewer
This PR needs someone to pick it up for review
needs-work
priority-high
High priority - will be reviewed soon
priority-low
Low priority - this may not receive review soon
priority-medium
Medium priority - this can be reviewed as time permits
priority-scheduled
This PR is big - it will require time to be scheduled for review
priority-very-high
Highest priority - prioritise this over other review work
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.
single-reviewer
This PR qualifies for having only one reviewer
size-l
Estimated task size: large (2w+)
size-m
Estimated task size: medium (~1w)
size-optimisation
size-s
Estimated task size: small (~2d)
size-xs
Estimated task size: extra small (a few hours at most)
No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
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
Loading…
Reference in a new issue
No description provided.
Delete branch "Marandil/fix/mbedtls-4314-parse-explicit-curve"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
Fixes https://github.com/Mbed-TLS/mbedtls/issues/4314.
pk_group_from_specifiedself-referencing group generator G while loading G.pk_group_from_specifiednot accounting for the a = p-3 internal representation.mbedtls_pk_parse_public_keyfailing 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:
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:
git commit --amend --signoff).new featurebug fix (thanks Manuel).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.
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 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: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.
Minor: the ChangeLog is plain text, so no backticks. Also, please use the full name of the funciton:
mbedtls_pk_parse_public_key().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_xxxifcomponent_test_xxxis failing.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.
I checked the failing pipelines, I think it has something to do with missing flags on tests as the following failed:
I'm assuming these tests should not be ran in these configs. I'll look into it.
Ah, you want to mark the new test cases as depending on
MBEDTLS_PK_PARSE_EC_EXTENDED.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:
While the same file converted to PEM using
openssl ec -in framework/data_files/ec_prv.specdom.der: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.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:
Both bugs affect both
mbedtls_pk_parse_public_key()andmbedtls_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.
Out of curiosity, I checked if we had any record of how the existing test file
ec_prv.specdom.derwas 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.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?
Hi! I was going to say we're just waiting for review capacity now, but I see there are conflicts this morning:
pkparse.chas been moved toextras/pkparse.cby this PR merged yesterdayWe 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.
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.
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?
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.
The CI is almost happy but has the following feedback:
BugfixnotBugfix:ifand opening parenthesis:if (Specifically, our code style checker suggests the following patch:
You can either address this now, or just wait for review feedback and address both at the same time, whatever you prefer.
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":0Parse Public EC Key #4d (RFC 5480, secp256r1, explicit, DER)depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_PK_PARSE_EC_EXTENDEDMBEDTLS_PEM_PARSE_Cbut the file is in.derformat.@ -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_EXTENDEDpk_parse_public_keyfile_ec:"../framework/data_files/ec_bp384_pub.expl.pem":0MBEDTLS_PEM_PARSE_C but the file is in
.derformat.MBEDTLS_PEM_PARSE_C but the file is in
.derformat.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.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.