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.

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

EVP_PKEY_todata exporting compressed EC public key #16595

Closed
jogme opened this issue Sep 13, 2021 · 16 comments
Closed

EVP_PKEY_todata exporting compressed EC public key #16595

jogme opened this issue Sep 13, 2021 · 16 comments
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug

Comments

@jogme
Copy link

jogme commented Sep 13, 2021

OpenSSL 3.0

When using EVP_PKEY_todata, the public key is exported in compressed mode even when the key itself was imported by EVP_PKEY_fromdata uncompressed. (even when explicitly setting

OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT,
                                                   "uncompressed",
                                                   12);

before EVP_PKEY_fromdata - which is the default anyway)

The bug seems to be at the function openssl/providers/implementations/keymgmt/key_to_params where the compressed format is HARDCODED.
That means, the export will be compressed anytime. Is there a workaround for that?

The possible solution can be to use the EC_KEY->conv_form instead of the hardcoded format, so the export would be dependent on the format of the key.

@jogme jogme added the issue: bug report The issue was opened to report a bug label Sep 13, 2021
@mattcaswell mattcaswell added triaged: bug The issue/pr is/fixes a bug and removed issue: bug report The issue was opened to report a bug labels Sep 13, 2021
@mattcaswell
Copy link
Member

Hmmm. It is definitely surprising that we are defaulting to the compressed format.

@t8m t8m added branch: 3.0 Merge to openssl-3.0 branch branch: master Merge to master branch labels Sep 13, 2021
@jogme
Copy link
Author

jogme commented Sep 15, 2021

Do you agree with the proposed solution? If yes I can create a PR

@mattcaswell
Copy link
Member

I think we have two options: (1) always do uncompressed or (2) respect EC_KEY->conf_form

Either way I would expect setting the OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT before EVP_PKEY_fromdata to be respected - so that is another bug IMO.

I have a slight preference for (1) above. It means you will always get predictable output.

A PR would be great!!

@davidben
Copy link
Contributor

+1 to doing (1). Having random bits of hidden state on keys seems unnecessarily fragile. Though I guess I'd phrase it as "do whatever OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT said, default to uncompressed if unspecified".

@davidben
Copy link
Contributor

davidben commented Sep 15, 2021

...oh, I misunderstood. EVP_PKEY_todata only takes output params, so I guess OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT is state you can set on the key? I guess this means this API is stuck with weird stateful encoding bits on keys.

I don't think that's a great design. Suppose two bits of code share a key object (common, and safe if they don't mutate it), but codepath needs uncompressed and the other needs compressed. They would need to coordinate (potentially across threads!) mutations to the key's internal byte form.

One option for a more thread-safe API would be to have two different output OSSL_PARAM types, one for uncompressed and the other for compressed. So if the caller wants a particular one, they specify the corresponding type. (And if they specify a generic one, probably document that it's uncompressed as that's the more common one.)

@mattcaswell
Copy link
Member

I guess this means this API is stuck with weird stateful encoding bits on keys.

Or we just default to uncompressed always. As a future feature we can add the capability to get it in other formats if you want.

@slontis
Copy link
Member

slontis commented Sep 15, 2021

@romen

@romen
Copy link
Member

romen commented Sep 16, 2021

I tend to prefer Matt's suggestion to use uncompressed always: I just checked the git blame and noticed I did hardcode this to compressed, but this was a typo on my part, I intended to use uncompressed as it's by far more common.

@romen
Copy link
Member

romen commented Sep 16, 2021

As a side-note on API usage, how much should an application rely on the internal format of exported params?
The original intention when this code was written was for the output of the to_params call to be consumed by the EVP layer when converting keys across the provider boundaries and let the providers decide what they would support about import/export options for each param.
In later developments the same API also became more and more exposed to applications for direct consumption, which was not a direct concern in the original design.
I see a risk of harmful ossification now that applications starting to hardcode assumptions on the way exported params are formatted.
This was supposed to be provider specific, but it seems like what the default provider does becomes what every provider must do to not break applications consuming the output of exported params.
@levitte what do you think about it?

@beldmit
Copy link
Member

beldmit commented Sep 16, 2021

Anyway the API should allow export in some set of supported formats (e.g. compressed and uncompressed for EC)

@mattcaswell
Copy link
Member

I did hardcode this to compressed, but this was a typo on my part, I intended to use uncompressed as it's by far more common.

i.e. not working as intended == bug. Let's fix it.

@t8m
Copy link
Member

t8m commented Sep 16, 2021

IMO the value from the internal EC_KEY should be respected as the EVP_PKEY_set_utf8_string_param(eckey, OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT, point_format) is used in apps to implement existing option. The EVP_PKEYs are not immutable in 3.0. That is 4.0 or later work.

And yes, the default should be uncompressed.

@davidben
Copy link
Contributor

The EVP_PKEYs are not immutable in 3.0. That is 4.0 or later work.

That is somewhat missing the point. This is a thread-safety requirement.

Even if EVP_PKEYs are mutable in 3.0, 3.0 cannot escape the long-standing fact that applications expect to share an EVP_PKEY across threads. This is how every non-trivial TLS application works. You load the EVP_PKEY into an SSL_CTX, make SSLs off the SSL_CTX (which then reference the original EVP_PKEY), and the various SSLs are allowed to live in different threads from each other. Provided no one mutates the EVP_PKEY, this has worked since at least the 1.0.x days, probably 0.9.x too.

Of course, this is much easier if EVP_PKEY simply had no mutable operations. But since 3.0 doesn't get this right, we have to categorize functions into mutating and non-mutating. The requirement then is that using a key must not require a mutating function.

If the way to select the EC point encoding is mutation, this rule is broken. Suppose some part of TLS says "this is encoded in uncompressed form", but it doesn't know what encoding the EVP_PKEY uses. Now every SSL object must mutate the EVP_PKEY. That is not thread-safe. It's also just bad API because, like the BN_FLG_CONSTTIME debacle, it leaves behavior up to some hidden state.

Fortunately, from @romen, it sounds like this isn't actually the recommended EC encoding API anyway, and there's another one? So it sounds like this can just pick some well-defined form and selecting formats can be left to whatever the recommended API is. (I hope you all made that one not require mutation!)

@t8m
Copy link
Member

t8m commented Sep 17, 2021

I do not think it makes sense to talk about some hypothetical solutions. We can totally agree that this should have been designed in a different way but what we have as an API in 3.0 must work in 3.0.x as expected as the 3.0 is already a released version.

As for the thread safety and mutability - you can always do an EVP_PKEY_dup to avoid mutating a key or, if you for example know that the key will be used for an application/protocol where the uncompressed format is required, you can set the format before sharing the key among multiple threads.

I completely agree the situation is not nice but that is what we have now. We certainly would like to make EVP_PKEYs immutable at some point.

@davidben
Copy link
Contributor

This isn't a hypothetical. Keys are shared across threads in applications today, including in libssl, and every well-specified protocol will write down what format it wants (including TLS). Setting the format before sharing the key among multiple threads is also not always an option depending on API shape. E.g. the libssl APIs are passed in an EVP_PKEY that may well already be shared across threads.

The old APIs (EC_POINT_point2oct, etc.) allowed picking a format without mutation. Indeed if we want to talk about what we have now, we currently have an API that doesn't use extra state. It just always uses compressed form. That's a perfectly well-defined, stateless API. It's just not very useful because uncompressed is more common.

As we're, either way, talking about introducing new behavior, there's no sense in making the new behavior stateful. You could imagine having two properties you can get out of a key, one for uncompressed and one for compressed. Or perhaps, as @romen says, these APIs aren't the main serialization APIs, and folks use other (stateless!) APIs.

romen added a commit to romen/openssl that referenced this issue Sep 18, 2021
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with openssl#13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in openssl#14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Fixes openssl#16595
@romen
Copy link
Member

romen commented Sep 18, 2021

#16624 attempts to solve the issue.

I just checked the git blame and noticed I did hardcode this to compressed, but this was a typo on my part, I intended to use uncompressed as it's by far more common.

I went spelunking in my notes and found that my previous comment was wrong, the choice of COMPRESSED was deliberate and not a mistake.
But I still consider this a bug, in the end, as not working as intended, because of the evolution of that code and how it was exposed to users outside the provider API, as detailed in #16624.

Given this history and what our released documentation says regarding OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT, I went towards @t8m suggestion of making the encoding of OSSL_PKEY_PARAM_PUB_KEY reflect the internal state associated with OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT.

I am aware of @davidben comments, and concur that having all this extra data as part of an EVP_PKEY state is not ideal for concurrency: but given this is the state of the finally released API I believe this approach is closer to a bugfix, while a change to unconditionally export always as uncompressed would be closer, in the grey area, to a new feature.

For what concerns libssl, #16624 does not change much, because AFAICT we use OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY (used to be OSSL_PKEY_PARAM_TLS_ENCODED_PT) which was always unconditionally in UNCOMPRESSED format.
For users that needs to convert to different formats but share the EVP_PKEY with other threads, it seems like a call to EVP_PKEY_dup() is inevitable, but at least @t8m suggestion does not preclude completely a way to extract the other formats without passing through deprecated APIs.

I am concerned that EVP_PKEY_todata() is exposing details that were supposed to be contained in the provider API and provider-specific directly to the users, bringing huge ossification risks that I am not sure were fully discussed within the OTC.
It seems it's too late now to fix that, but I will raise the issue for consideration at the next OTC meeting.

romen added a commit to romen/openssl that referenced this issue Oct 8, 2022
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with openssl#13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in openssl#14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Fixes openssl#16595
romen added a commit to romen/openssl that referenced this issue Oct 9, 2022
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with openssl#13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in openssl#14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Fixes openssl#16595
romen added a commit to romen/openssl that referenced this issue Oct 9, 2022
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with openssl#13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in openssl#14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Fixes openssl#16595
romen added a commit to romen/openssl that referenced this issue Nov 15, 2022
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with openssl#13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in openssl#14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Fixes openssl#16595
romen added a commit to romen/openssl that referenced this issue Nov 27, 2022
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with openssl#13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in openssl#14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Fixes openssl#16595
romen added a commit to romen/openssl that referenced this issue Nov 28, 2022
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with openssl#13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in openssl#14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Fixes openssl#16595
openssl-machine pushed a commit that referenced this issue Nov 29, 2022
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with #13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in #14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Expand documentation about `OSSL_PKEY_PARAM_PUB_KEY` and mention the
3.1 change in behavior for our providers.

Fixes #16595

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19681)
romen added a commit to romen/openssl that referenced this issue Dec 13, 2022
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with openssl#13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in openssl#14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Expand documentation about `OSSL_PKEY_PARAM_PUB_KEY` and mention the
change in behavior for our providers.

Fixes openssl#16595

(cherry picked from commit 926db47)
openssl-machine pushed a commit that referenced this issue Dec 22, 2022
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with #13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in #14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Expand documentation about `OSSL_PKEY_PARAM_PUB_KEY` and mention the
change in behavior for our providers.

Fixes #16595

(cherry picked from commit 926db47)

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19901)
beldmit pushed a commit to beldmit/openssl that referenced this issue Dec 26, 2022
…o UNCOMPRESSED

Originally the code to im/export the EC pubkey was meant to be consumed
only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made
sense to avoid memory waste, as it wasn't exposed outside the provider
API, and providers had all tools available to convert across the three
formats.

Later on, with openssl#13139 deprecating the `EC_KEY_*` functions, more state
was added among the params imported/exported on an EC provider-native
key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it
did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`).

Finally, in openssl#14800, `EVP_PKEY_todata()` was introduced and prominently
exposed directly to users outside the provider API, and the choice of
COMPRESSED over UNCOMPRESSED as the default became less sensible in
light of usability, given the latter is more often needed by
applications and protocols.

This commit fixes it, by using `EC_KEY_get_conv_form()` to get the
point format from the internal state (an `EC_KEY` under the hood) of the
provider-side object, and using it on
`EVP_PKEY_export()`/`EVP_PKEY_todata()` to format
`OSSL_PKEY_PARAM_PUB_KEY`.
The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered
if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via
`EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the
more specialized methods.

For symmetry, this commit also alters `ec_pkey_export_to()` in
`crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC
keys: it exclusively used COMPRESSED format, and now it honors the
conversion format specified in the EC_KEY object being exported to a
provider when this function is called.

Expand documentation about `OSSL_PKEY_PARAM_PUB_KEY` and mention the
3.1 change in behavior for our providers.

Fixes openssl#16595

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

(cherry picked from commit 926db47)
space88man added a commit to space88man/pkcs11-provider that referenced this issue Feb 22, 2024
openssl/openssl#16595
OpenSSL <= 3.0.7 unconditionally exports EC public keys
in compressed format. Workaround this by creating the
uncompressed format for correct import.

Addresses latchset#348

Signed-off-by: S-P Chan <shihping.chan@gmail.com>
space88man added a commit to space88man/pkcs11-provider that referenced this issue Feb 22, 2024
openssl/openssl#16595
OpenSSL <= 3.0.7 unconditionally exports EC public keys
in compressed format - create the uncompressed
format for correct import

Addresses latchset#348

Signed-off-by: S-P Chan <shihping.chan@gmail.com>
space88man added a commit to space88man/pkcs11-provider that referenced this issue Feb 23, 2024
openssl/openssl#16595
OpenSSL <= 3.0.7 unconditionally exports EC public keys
in compressed format - create the uncompressed
format for correct import

Addresses latchset#348

Signed-off-by: S-P Chan <shihping.chan@gmail.com>
space88man added a commit to space88man/pkcs11-provider that referenced this issue Feb 23, 2024
openssl/openssl#16595
OpenSSL <= 3.0.7 unconditionally exports EC public keys
in compressed format - create the uncompressed
format for correct import

Addresses latchset#348

Signed-off-by: S-P Chan <shihping.chan@gmail.com>
space88man added a commit to space88man/pkcs11-provider that referenced this issue Feb 23, 2024
openssl/openssl#16595
OpenSSL <= 3.0.7 unconditionally exports EC public keys
in compressed format - create the uncompressed
format for correct import

Addresses latchset#348

Signed-off-by: S-P Chan <shihping.chan@gmail.com>
space88man added a commit to space88man/pkcs11-provider that referenced this issue Feb 23, 2024
openssl/openssl#16595

Providers may export EC points in compressed format, e.g.,
OpenSSL < 3.0.8, and this format may not actually match
the OSSL_PARAM "point-format". Use heuristics to
detect compressed points and convert to uncompressed.

Addresses latchset#348

Signed-off-by: S-P Chan <shihping.chan@gmail.com>
simo5 pushed a commit to latchset/pkcs11-provider that referenced this issue Feb 26, 2024
openssl/openssl#16595

Providers may export EC points in compressed format, e.g.,
OpenSSL < 3.0.8, and this format may not actually match
the OSSL_PARAM "point-format". Use heuristics to
detect compressed points and convert to uncompressed.

Addresses #348

Signed-off-by: S-P Chan <shihping.chan@gmail.com>
The-Mule pushed a commit to The-Mule/pkcs11-provider that referenced this issue Mar 12, 2024
openssl/openssl#16595

Providers may export EC points in compressed format, e.g.,
OpenSSL < 3.0.8, and this format may not actually match
the OSSL_PARAM "point-format". Use heuristics to
detect compressed points and convert to uncompressed.

Addresses latchset#348

Signed-off-by: S-P Chan <shihping.chan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
7 participants