Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add translation for ECX group parameter #19348

Closed
wants to merge 1 commit into from
Closed

Add translation for ECX group parameter #19348

wants to merge 1 commit into from

Conversation

juergenchrist
Copy link
Contributor

Legacy EVP_PKEY_CTX objects did not support the "group" parameter for X25519 and X448. The translation of this parameter resulted in an error. This caused errors for legacy keys and engines.

Fix this situation by adding a translation that simply checks that the correct parameter is to be set, but does not actually set anything. This is correct since the group name is anyway optional for these two curves.

Fixes #19313

Signed-off-by: Juergen Christ jchrist@linux.ibm.com

I could not find any tests for the translation. Please provide me a hint where to search for these tests.

Checklist
  • tests are added or updated

@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels Oct 5, 2022
@t8m
Copy link
Member

t8m commented Oct 5, 2022

I do not think it would be easy to add the test case. It would require a legacy ECX to be set up. There are some legacy EC tests in evp_extra_test.c but IMO none for ECX.

t8m
t8m reviewed Oct 5, 2022
View reviewed changes
crypto/evp/ctrl_params_translate.c Outdated
* ECX
* ===
*/
{ SET, -1, -1, EVP_PKEY_OP_KEYGEN, -1, NULL, NULL,
Copy link
Member

Choose a reason for hiding this comment

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

I am not quite sure it is right to use the wildcard -1 key type here. It will probably work, but just by accident it won't break the EC keygen group support because it is last in the table so the

    { SET, EVP_PKEY_EC, 0, EVP_PKEY_OP_PARAMGEN | EVP_PKEY_OP_KEYGEN,
      EVP_PKEY_CTRL_EC_PARAMGEN_CURVE_NID, "ec_paramgen_curve", NULL,
      OSSL_PKEY_PARAM_GROUP_NAME, OSSL_PARAM_UTF8_STRING,
      fix_ec_paramgen_curve_nid },

entry is before. I'd suggest using the EVP_PKEY_X25519, EVP_PKEY_ED25519 as the key types. And add another entry for the X448, ED448 types.

Copy link
Member

Choose a reason for hiding this comment

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

@levite might also comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this just for X25519 and X448? At least the fixing function will not work for ED25519 and ED448. At least looking at https://www.openssl.org/docs/manmaster/man7/EVP_PKEY-X25519.html it seems to be the X variants and not the ED variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use the X25519 and X448 key types.

@juergenchrist
Add translation for ECX group parameter
b00f2ca 
Legacy EVP_PKEY_CTX objects did not support the "group" parameter for X25519
and X448.  The translation of this parameter resulted in an error.  This
caused errors for legacy keys and engines.

Fix this situation by adding a translation that simply checks that the correct
parameter is to be set, but does not actually set anything.  This is correct
since the group name is anyway optional for these two curves.

Fixes openssl#19313

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Oct 7, 2022
@t8m t8m requested a review from levitte October 7, 2022 18:48
@openssl openssl deleted a comment Oct 8, 2022
@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 Oct 10, 2022
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Oct 11, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Oct 11, 2022
@hlandau
Copy link
Member

hlandau commented Oct 13, 2022

Merged to master and 3.0. Thank you.

@hlandau hlandau closed this Oct 13, 2022
openssl-machine pushed a commit that referenced this pull request Oct 13, 2022
@juergenchrist @hlandau
Add translation for ECX group parameter
e18186a 
Legacy EVP_PKEY_CTX objects did not support the "group" parameter for X25519
and X448.  The translation of this parameter resulted in an error.  This
caused errors for legacy keys and engines.

Fix this situation by adding a translation that simply checks that the correct
parameter is to be set, but does not actually set anything.  This is correct
since the group name is anyway optional for these two curves.

Fixes #19313

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #19348)

(cherry picked from commit c048779)
openssl-machine pushed a commit that referenced this pull request Oct 13, 2022
@juergenchrist @hlandau
Add translation for ECX group parameter
c048779 
Legacy EVP_PKEY_CTX objects did not support the "group" parameter for X25519
and X448.  The translation of this parameter resulted in an error.  This
caused errors for legacy keys and engines.

Fix this situation by adding a translation that simply checks that the correct
parameter is to be set, but does not actually set anything.  This is correct
since the group name is anyway optional for these two curves.

Fixes #19313

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #19348)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@juergenchrist @beldmit
Add translation for ECX group parameter
e83dafa 
Legacy EVP_PKEY_CTX objects did not support the "group" parameter for X25519
and X448.  The translation of this parameter resulted in an error.  This
caused errors for legacy keys and engines.

Fix this situation by adding a translation that simply checks that the correct
parameter is to be set, but does not actually set anything.  This is correct
since the group name is anyway optional for these two curves.

Fixes openssl#19313

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#19348)
InfoHunter pushed a commit to InfoHunter/Tongsuo that referenced this pull request Nov 28, 2023
@juergenchrist @InfoHunter
Add translation for ECX group parameter
93b310c 
Legacy EVP_PKEY_CTX objects did not support the "group" parameter for X25519
and X448.  The translation of this parameter resulted in an error.  This
caused errors for legacy keys and engines.

Fix this situation by adding a translation that simply checks that the correct
parameter is to be set, but does not actually set anything.  This is correct
since the group name is anyway optional for these two curves.

Fixes #19313

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#19348)
@InfoHunter InfoHunter mentioned this pull request Nov 28, 2023
Merged
5 tasks
InfoHunter added a commit to InfoHunter/Tongsuo that referenced this pull request Nov 30, 2023
@InfoHunter
Add translation for ECX group parameter
60ae61b 
Legacy EVP_PKEY_CTX objects did not support the "group" parameter for X25519
and X448.  The translation of this parameter resulted in an error.  This
caused errors for legacy keys and engines.

Fix this situation by adding a translation that simply checks that the correct
parameter is to be set, but does not actually set anything.  This is correct
since the group name is anyway optional for these two curves.

Fixes #19313

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#19348)
dongbeiouba pushed a commit to Tongsuo-Project/Tongsuo that referenced this pull request Nov 30, 2023
@InfoHunter
Add translation for ECX group parameter (#534)
daee105 
Legacy EVP_PKEY_CTX objects did not support the "group" parameter for X25519
and X448.  The translation of this parameter resulted in an error.  This
caused errors for legacy keys and engines.

Fix this situation by adding a translation that simply checks that the correct
parameter is to be set, but does not actually set anything.  This is correct
since the group name is anyway optional for these two curves.

Fixes #19313

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#19348)
dongbeiouba pushed a commit to Tongsuo-Project/Tongsuo that referenced this pull request Nov 30, 2023
@InfoHunter
Address issue #540 (#544)
02e9a2e 
* Add translation for ECX group parameter

Legacy EVP_PKEY_CTX objects did not support the "group" parameter for X25519
and X448.  The translation of this parameter resulted in an error.  This
caused errors for legacy keys and engines.

Fix this situation by adding a translation that simply checks that the correct
parameter is to be set, but does not actually set anything.  This is correct
since the group name is anyway optional for these two curves.

Fixes #19313

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl/openssl#19348)

* Address issue #540
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

@levitte levitte Awaiting requested review from levitte

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Legacy X25519 PKEY_CTX fails in SSL handshake for parameter "group"
5 participants
@juergenchrist @t8m @openssl-machine @hlandau @paulidale