Skip to content

Comments

Disabling explicit EC curves encoding#29639

Closed
beldmit wants to merge 2 commits intoopenssl:masterfrom
beldmit:disable-explicit-ec
Closed

Disabling explicit EC curves encoding#29639
beldmit wants to merge 2 commits intoopenssl:masterfrom
beldmit:disable-explicit-ec

Conversation

@beldmit
Copy link
Member

@beldmit beldmit commented Jan 14, 2026 •
edited
Loading

In case the parameters don't exactly match the well-known ones

Checklist
  • documentation is added or updated
  • tests are added or updated

@beldmit beldmit requested a review from quarckster as a code owner January 14, 2026 17:01
@beldmit beldmit added the branch: master Applies to master branch label Jan 14, 2026
@beldmit beldmit marked this pull request as draft January 14, 2026 17:02
@loci-dev loci-dev mentioned this pull request Jan 14, 2026
Open
2 tasks
@beldmit beldmit force-pushed the disable-explicit-ec branch from 14edbd0 to 58c669f Compare January 14, 2026 19:30
@davidben
Copy link
Contributor

davidben commented Jan 14, 2026

This is a good idea. Giving attacker control over EC domain parameters has been the source of countless problems, like CVE-2024-9143 and CVE-2022-0778. Limiting to well-known ones removes this risk and avoids allowing attackers to break the many many invariants that elliptic curve cryptography relies on.

@beldmit beldmit marked this pull request as ready for review January 14, 2026 20:09
@beldmit beldmit added approval: review pending This pull request needs review by a committer triaged: feature The issue/pr requests/adds a feature labels Jan 14, 2026
@beldmit
Copy link
Member Author

beldmit commented Jan 14, 2026

Ready for review

simo5
simo5 requested changes Jan 14, 2026
View reviewed changes
Copy link
Contributor

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, only a few nits

simo5
simo5 previously approved these changes Jan 14, 2026
View reviewed changes
Copy link
Contributor

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

LGTM

paulidale
paulidale reviewed Jan 14, 2026
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Some suggested wording changes in the changes entry.
The rest looks good.

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 14, 2026
@beldmit
Copy link
Member Author

beldmit commented Jan 14, 2026

After applying the change via github interface I got an CLA violation :(. Will re-push tomorrow via command line

paulidale
paulidale previously approved these changes Jan 14, 2026
View reviewed changes
@paulidale
Copy link
Contributor

paulidale commented Jan 14, 2026

Yeah, that doesn't work :( Approved in spirit.

@slontis
Copy link
Member

slontis commented Jan 15, 2026

This should be added into INSTALL.md.
Also the name is different/weird - remove the enable bit. It is already 'enable-XXX'

@slontis
Copy link
Member

slontis commented Jan 15, 2026

That is a strange email address :)

shahsb
shahsb reviewed Jan 15, 2026
View reviewed changes
Copy link
Contributor

@shahsb shahsb left a comment

Choose a reason for hiding this comment

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

LGTM. CLA seems to be missing, please check

@beldmit
Disabling explicit EC curves encoding
e7b8473 
In case the parameters don't exactly match the well-known ones
@beldmit beldmit force-pushed the disable-explicit-ec branch from af0b3e4 to e7b8473 Compare January 15, 2026 08:41
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 15, 2026
@beldmit beldmit marked this pull request as draft January 15, 2026 09:00
@beldmit
Copy link
Member Author

beldmit commented Jan 15, 2026

Taking back to draft, I need to adjust the naming, documentation, and got some ideas how to improve test skip.

@beldmit beldmit marked this pull request as ready for review January 15, 2026 11:50
@beldmit beldmit requested a review from simo5 January 15, 2026 11:50
@beldmit beldmit requested a review from paulidale January 15, 2026 11:50
@beldmit
Copy link
Member Author

beldmit commented Jan 15, 2026

@simo5 @paulidale could you please reapprove?

mattcaswell
mattcaswell reviewed Jan 15, 2026
View reviewed changes
@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 15, 2026
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 16, 2026
@openssl-machine
Copy link
Collaborator

openssl-machine commented Jan 16, 2026

This pull request is ready to merge

@beldmit
Copy link
Member Author

beldmit commented Jan 17, 2026

Merged. thanks for review!

@beldmit beldmit closed this Jan 17, 2026
openssl-machine pushed a commit that referenced this pull request Jan 17, 2026
@beldmit
Disabling explicit EC curves encoding
54d175c 
In case the parameters don't exactly match the well-known ones

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Simo Sorce <[email protected]>
(Merged from #29639)
esyr pushed a commit to esyr/openssl that referenced this pull request Jan 19, 2026
@beldmit @esyr
Disabling explicit EC curves encoding
7ec078d 
In case the parameters don't exactly match the well-known ones

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Simo Sorce <[email protected]>
(Merged from openssl#29639)
@beldmit beldmit deleted the disable-explicit-ec branch January 22, 2026 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattcaswell mattcaswell mattcaswell left review comments

@simo5 simo5 simo5 approved these changes

@paulidale paulidale paulidale approved these changes

@quarckster quarckster Awaiting requested review from quarckster quarckster is a code owner

+1 more reviewer

@shahsb shahsb shahsb left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch triaged: feature The issue/pr requests/adds a feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@beldmit @davidben @paulidale @slontis @openssl-machine @mattcaswell @simo5 @shahsb